[sonar] Short-circuit logic should be used in boolean contexts

[strehle]

Rule: java:S2178, e.g. https://rules.sonarsource.com/java/RSPEC-2178

https://sonarcloud.io/project/issues?resolved=false&severities=BLOCKER&id=cloudfoundry-identity-parent&open=AYBVU2mG8w5rn7tLse0R&tab=why

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/184821669

The labels on this github issue will be updated when the story is started.

[hsinn0]

Actually, let me review if not executing the add operation is OK here.

[hsinn0]

It does appear that changing | to || affects the outcome of the executing this line. In previous code, new LdapAuthroity is always added to authorities. In the changed code, new LdapAuthroity is not added if circular is true.

[strehle]

hi @hsinn0 i rotated, but it is not wanted to get this merged.

This code is not covered with junit or integration tests yet, so I wont touch it.
I wanted highlight the sonar findings and that sonars marks it as dangerous to use bit-wise OR .

There is another finding where we working on a fix because there is a test available

[hsinn0]

My understanding of the intention of the existing code is that it wants to always execute authorities.add(new LdapAuthority(role,dn,record)) and combine the resulting boolean value with circular. So it is proper use of bit-wise OR in my opinion. Of course, it can be written as:
circular |= !authorities.add(new LdapAuthority(role,dn,record)); // maybe the intention is clearer this way
or
circular = !authorities.add(new LdapAuthority(role,dn,record)) || circular; // to accomplish the same with ||

[hsinn0]

I made another comment before seeing this change, so you can ignore that another comment. This change looks good except the commit message, which just says "rotate". Approving, assuming that you will make the commit message better when merging.

[strehle]

I made another comment before seeing this change, so you can ignore that another comment. This change looks good except the commit message, which just says "rotate". Approving, assuming that you will make the commit message better when merging.

@hsinn0 About commit messages. I always merge to UAA with squashed commit, so finally you dont see the seperate commits not in git history, only in the single commit they are visible in the commit text, therefore I dont care really about the intermediate commits.....
You can do your merged with git history but I prefere the squash since this is our working mode in company and it helps if you have changes which are many years old,.... here you dont have to check the many small commits in between but only the story is relevant
Here is an example from past:
#2161 only a single commit to uaa develop 4f5443e and the changed file has only one entry for git log https://github.com/cloudfoundry/uaa/commits/develop/.github/workflows/unit-tests.yml