Re: [keycloak-dev] Code quality & bugs

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 ?

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); Example 2: LDAPObject ldapUser = ldapQuery.getFirstResult(); if (ldapUser == null) { return null; } return ldapUser; Can become: return ldapQuery.getFirstResult(); Example 3: if (responderURIs.size() == 0) { if (responderURIs.isEmpty()) { Example 4: Set<Annotation> set = new HashSet<Annotation>(); Set<Annotation> set = new HashSet<>(); Other examples : https://github.com/fperot74/keycloak/pull/1/files

_______________________________________________ keycloak-dev mailing list keycloak-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/keycloak-dev