[keycloak-dev] Code quality & bugs
Marek Posolda
mposolda at redhat.com
Thu Nov 28 04:43:41 EST 2019
Hi Francis,
I see. In shortcut, it may be good to discuss individual changes here
first on this ML and then eventually send PR for individual changes
based on it. It is question of priorities though, as it seems that lots
of the reported issues are more or less cosmetic changes, which don't
have any bigger impact than warning in the IDE. But improving those
never hurts :) On the other hand we should be careful to not introduce
regressions - like AFAIK broke the compilation on JDK7 by adding some
optimization specific to JDK 8 etc.
Feel free to send PRs for the issues, which we already discuss here and
agree on. Then feel free to send mail for other possible optimizations.
Thanks,
Marek
On 26. 11. 19 17:23, Perot Francis wrote:
> Hi Marek,
>
> The link to a PR I sent was only for information of suggestions. As Tomas said, it would be hard to review (and it is a PR against an old master version of my Keycloak fork).
> I created it quickly with find/replace and without tests so my aim was not to open a PR on your repo but just to provide some examples: we might define a scope of which recommendations should be followed and one which parts of the code (as you said, some recommendations are not valid with JDK 7).
>
> I edited my PR (https://github.com/fperot74/keycloak/pull/1) with recommendations : there is almost 100 types of recommendations and not all are relevant (Add a default case to this switch, Return an empty array instead of null, Rename field "VERSION", ... and many others).
>
> Francis
>
>
> -----Original Message-----
> From: Marek Posolda <mposolda at redhat.com>
> Sent: mardi, 26 novembre 2019 12:02
> To: Tomas Kyjovsky <tkyjovsk at redhat.com>; Perot Francis <francis.perot at elca.ch>
> Cc: keycloak-dev at lists.jboss.org
> Subject: Re: [keycloak-dev] Code quality & bugs
>
> On 25. 11. 19 21:29, Tomas Kyjovsky wrote:
>> Hi Francis,
>>
>> See my comments inline..
>>
>> ----- Original Message -----
>>> Hi all,
>>>
>>> I recently had to start working with Keycloak and, as I’m using a
>>> static code analyzer in my IDE (Sonar Lint), I got lots of
>>> recommendations about Keycloak code.
>>> These recommendations are not always very relevant but can help to
>>> increase the code quality, the readability, can help to detect bugs
>>> (I actually found
>>> some) and this might be an important point for the stability of the
>>> product, especially for a product managing authentication.
>>>
>>> First, in MultiValuedHashMap.equalsIgnoreValueOrder(), there is a bug
>>> but it can be considered as assumed if we suppose that it is not
>>> possible to have duplicated values in this map (which is often the case).
>>> For a given key, if we compare associated lists of 2
>>> MultivaluedHashMap instances [1, 2, 2] and [1, 2, 3],
>>> equalsIgnoreValueOrder will return true because lengths are equal and
>>> all elements from the first list exist in the second list. Do you think it is necessary to fix this ?
>> I created JIRA and posted PR for this:
>> https://github.com/keycloak/keycloak/pull/6544
>>
>>> Static code analysis revealed issues described in the following
>>> examples and show some inconsistencies : In
>>> FileTruststoreProviderFactory.init(), if pass is null, we “return” at
>>> the beginning of the method… but pass is still compared to null twice after that.
>>> Similary, in FreeMarkerUtil.processTemplate(), we set cache=null;
>>> then we compare cache to null (if (cache!=null)) Don’t you think it
>>> could be better to follow some recommendations ? I could work on this
>>> : as it should impact lots of code but maybe you should decide which
>>> type of recommendation you want to follow and how you prefer to do
>>> this : all changes at once, one PR per type of recommendation, on PR
>>> per maven module, …
>>>
>>> Francis Pérot
>>>
>>> (about me: I’m currently working on the multi-factor feature and we
>>> had a meeting two weeks ago with Marek P. and Peter S. : the left ear
>>> you saw was mine 😊)
>>>
>>>
>>> Example 1:
>>> byte[] signature = null;
>>> try {
>>> signature =
>>> HMACProvider.sign(buffer.toString().getBytes("UTF-8"),
>>> Algorithm.HS512, sharedSecret);
>>> } catch (UnsupportedEncodingException e) {
>>> throw new RuntimeException(e);
>>> }
>>> Can become:
>>> byte[] signature =
>>> HMACProvider.sign(buffer.toString().getBytes(StandardCharsets.UTF_8),
>>> Algorithm.HS512, sharedSecret);
> +1 to fix, but I am not sure if it is not fixed already in the meantime
> in latest Keycloak master? I recall seeing some recent PR related to this. It can be related to quarkus related work.
>>> Example 2:
>>> LDAPObject ldapUser = ldapQuery.getFirstResult();
>>> if (ldapUser == null) {
>>> return null;
>>> }
>>>
>>> return ldapUser;
>>> Can become:
>>> return ldapQuery.getFirstResult();
> +1 to fix. BTV. It's quite possible I wrote this non-sense code :)
>>> Example 3:
>>>
>>>
>>> if (responderURIs.size() == 0) {
>>>
>>>
>>> if (responderURIs.isEmpty()) {
> +1 to fix.
>>>
>>>
>>> Example 4:
>>>
>>>
>>> Set<Annotation> set = new HashSet<Annotation>();
>>>
>>>
>>> Set<Annotation> set = new HashSet<>();
> This is bit tricky. AFAIK this can be done just on Java 8? But some of the adapter modules still rely on JDK 7 and I think we still want them to be "compilable" on JDK 7 as well.
>
> If it's the case, it may be good to either post-pone this optimization or do it just in the java modules, which can run only on server (server-spi, server-spi-private, services, some federation modules etc)
>
> Marek
>
>>>
>>>
>>> Other examples : https://github.com/fperot74/keycloak/pull/1/files
>> I had a brief look at these changes and I agree, this would clean up the code and improve readability.
>> It's a lot of changes though (445 files) which might take a long to
>> review and I'm not sure about the priority of this compared to some other tasks like wrapping up the multi-factor stuff (KEYCLOAK-7159). I'll discuss this with colleagues and get back to you.
> From very brief look, there are lots of changes for example 4 in the adapter module. Not sure if those can be done or not. I vote for not ATM unless you're sure that this stuff works on all JDK 7 versions.
>
> Marek
>
>>> _______________________________________________
>>> keycloak-dev mailing list
>>> keycloak-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>> Regards,
>> Tomas
>>
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>
More information about the keycloak-dev
mailing list