[keycloak-dev] Code quality & bugs

Tue Nov 26 11:23:16 EST 2019

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