Re: [keycloak-dev] Code quality & bugs

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(a)redhat.com&gt; Sent: mardi, 26 novembre 2019 12:02 To: Tomas Kyjovsky <tkyjovsk(a)redhat.com>; Perot Francis <francis.perot(a)elca.ch&gt; Cc: keycloak-dev(a)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(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/keycloak-dev > Regards, > Tomas > > _______________________________________________ > keycloak-dev mailing list > keycloak-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/keycloak-dev