SONARJAVA-5909 FN on S3752 when @RequestMapping in class

[asya-vorobeva]

Fixed FN in case when we mix safe and unsafe methods on class and method. See SONARJAVA-5909.

[hashicorp-vault-sonar-prod]

SONARJAVA-5909

[dorian-burihabwa-sonarsource]

Nitpick: Could we rename the method to have a more interrogative name such as mixesSafeAndUnsafeMethods or isMixingSafeAndUnsafeMethods?

[dorian-burihabwa-sonarsource]

We introduced the isClassVisited flag so that visiting the method=<VERBS> on a class or a method behaves slightly differently. Could we get rid of the field and use 2 separate methods instead?

[asya-vorobeva]

We can of course, but it will duplicate a code. I try to minimize such things. But if you think it will be better from maintainability view, we can. Another approach is to extract

if (isClassVisited) {
      classHasSafeMethods = visitor.hasSafeMethods;
      classHasUnsafeMethods = visitor.hasUnsafeMethods;
    } else {
      methodHasSafeMethods = visitor.hasSafeMethods;
      methodHasUnsafeMethods = visitor.hasUnsafeMethods;
    }

to a separate method.

[dorian-burihabwa-sonarsource]

In the spirit of minimizing state of the check, I would prefer that the isClassVisited flag be passed as a parameter or replaced by 2 methods. I have no particular opinion over which is best but it make maintanability of the rule a little easier

[dorian-burihabwa-sonarsource]

We initialize these 4 new fields to false when an instance of the check is created (which should be once per module visited). But it looks like we don't reset them when moving from one file/class to another. Could we ensure to reset them when visiting a new class or entering a new file?

[asya-vorobeva]

For each new class (as well as for each new method) we recalculate these fields. So it shouldn't be a problem. (and as we see, corresponding sample works pretty well)

[dorian-burihabwa-sonarsource]

Indeed, it looks like we properly reset he values everytime in the current implementation. However, we have had issues in the past where after rework some checks no longer reset their state properly (we no longer passed through the reset path in some cases or the check would fail with a NPE on a file and then proceed to the next file with a invalid state). So we generally would explicitly reset the state on entry of the check. If you don't mind, I would prefer that reseting the state when entering visitNode to ease debugging

[dorian-burihabwa-sonarsource]

Thank your for the changes, I added some more comments about patterns in managing the state of checks

[dorian-burihabwa-sonarsource]

Thanks for the quick turn aroudn, the PR LGTM!

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
97.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube