Sonarqube ignores to be defined in source code

[vmassol]

Hi devs,

Right now, we often use the “false positive” action in the SonarCloud UI to mark an issue as a false positive. The problem with this is the following:

• If we ever loose or change our sonarqube instance (as we did when we moved from sonar.xwiki.org to sonarcloud years ago), we loose all these configs.
• A local developers having sonarlint installed but not connected to sonarcloud cannot have the info about what should be ignored.

The proposal is to bring this inside the code as much as possible, using the following rules:

• Use the @SuppressWarnings(<rule id>) annotation whenever it’s valid to use it, ie everywhere except inside methods (except for variable declarations). Note that <rule id> is the full id, e.g. java:S1133
• For the places where the @SuppressWarnings annotation is not allowed, continue using th SonarCloud UI for now. An alternative would be to use // NOSONAR but it disables all rules which is not nice.

Sometimes we also need to disable rules on several classes. For example right now we ignore the java:S1133 rule (see RSPEC) for all classes in **/xwiki-platform-legacy-*/**/*.java.

Right now this is configured in SonarCloud

I’ll research this to see if we can configure it in our pom.xml, see sonarqube - Configure Sonar sonar.issue.ignore.multicriteria through maven - Stack Overflow

WDYT?

Thanks

PS: See Checkstyle ignores to be defined in source code for the same idea for Checkstyle.

[vmassol]

@vmassol

Use the @SuppressWarnings(<rule id>) annotation whenever it’s valid to use it, ie everywhere except inside methods (except for variable declarations). Note that <rule id> is the full id, e.g. java:S1133

I forgot one thing: In addition to the annotation we also need to add a comment to explain WHY it’s ignored, i.e. why we consider it a false positive.

I’m proposing the format:

// Explanation here
@SuppressWarnings("java:S1133")
public final class ABC
{
...

[mleduc]

+1 thanks

[vmassol]

@vmassol

Right now, we often use the “false positive” action in the SonarCloud UI to mark an issue as a false positive.

For example in platform we have 15 issues marked as false positives on sonarcloud: SonarCloud

[vmassol]

Note that the following:

    @Override
    // The token is generated and not user-controlled
    @SuppressWarnings("javasecurity:S5145")
    public boolean isTokenValid(String token)
    {
        if (!this.configuration.isEnabled()) {
            return true;
        }
        String storedToken = getToken();
        if (token == null || token.equals("") || !storedToken.equals(token)) {
            this.logger.warn("CSRFToken: Secret token verification failed, token: \"" + token
                + "\", stored token: \"" + storedToken + "\"");
            return false;
        }
        return true;
    }

Is a lot less easy to understand than the following:

We need to check what happens when we use the @SuppressWarnings annotation and where Sonarcloud still allow us to see an issue. I’ll test that.

EDIT: I’ve reopened the issue in Sonarcloud and pushed [Misc] Try using a @SuppressWarnings annotation for SonarCloud to see… · xwiki/xwiki-platform@684eedd · GitHub to try it, now waiting for the next quality build to update sonarcloud

[tmortagne]

@vmassol

Use the @SuppressWarnings(<rule id>) annotation whenever it’s valid to use it, ie everywhere except inside methods (except for variable declarations). Note that <rule id> is the full id, e.g. java:S1133

+1

@vmassol

For the places where the @SuppressWarnings annotation is not allowed, continue using th SonarCloud UI for now. An alternative would be to use // SONAR:ON/OFF but it disables all rules which is not nice.

+1 for now, let’s hope they add support for more specific rules eventually (How do I exclude certain code from certain rules? - #7 by denis.troller - SonarQube - Sonar Community seems to suggest they are thinking about it)

@vmassol

I’ll research this to see if we can configure it in our pom.xml, see sonarqube - Configure Sonar sonar.issue.ignore.multicriteria through maven - Stack Overflow

https://www.baeldung.com/sonar-exclude-violations#disable-using-maven is another source which seems interesting.

@vmassol

• If we ever loose or change our sonarqube instance (as we did when we moved from sonar.xwiki.org to sonarcloud years ago), we loose all these configs.
• A local developers having sonarlint installed but not connected to sonarcloud cannot have the info about what should be ignored.

Another important benefit is that others see those ignores and can comment when they think it’s actually wrong.

[vmassol]

@vmassol

Sometimes we also need to disable rules on several classes. For example right now we ignore the java:S1133 rule (see RSPEC) for all classes in **/xwiki-platform-legacy-*/**/*.java.

Right now this is configured in SonarCloud

I’ll research this to see if we can configure it in our pom.xml, see sonarqube - Configure Sonar sonar.issue.ignore.multicriteria through maven - Stack Overflow

I’ve now committed [Misc] Exclude legacy code from SonarQube's "java:S1133" rule using p… · xwiki/xwiki-commons@6fd9a26 · GitHub and removed the config from SonarCloud’s UI. Let’s see how it goes now!

[vmassol]

@vmassol

I’ve now committed [Misc] Exclude legacy code from SonarQube’s “java:S1133” rule using p… · xwiki/xwiki-commons@6fd9a26 · GitHub and removed the config from SonarCloud’s UI. Let’s see how it goes now!

I forgot that we now need to do the same for ALL branches…

Just got the following sonarcloud notif:

Project: XWiki Platform - Parent POM
Branch: stable-15.10.x
Version: 15.10.13-SNAPSHOT

11 new issues

   Rules
       Deprecated code should be removed (java): 11

   Tags
       obsolete: 11

   Most impacted files
       OrderClause.java: 1
       XWikiNotificationInterface.java: 1
       CompatibilityMailStorageConfiguration.java: 1
       XWikiNotificationRule.java: 1
       XWikiDocChangeNotificationInterface.java: 1

More details at: https://sonarcloud.io/project/issues?id=org.xwiki.platform%3Axwiki-platform&assignees=vmassol%40github&branch=stable-15.10.x&createdAt=2024-10-15T11%3A50%3A38%2B0000

^^ that’s for the 15.10.x branch…

Merging to all branches…

[vmassol]

@vmassol

I’ve now committed [Misc] Exclude legacy code from SonarQube’s “java:S1133” rule using p… · xwiki/xwiki-commons@6fd9a26 · GitHub and removed the config from SonarCloud’s UI. Let’s see how it goes now!

It works! I’m now waiting for 1 or 2 more agreement before proceeding and documenting it.

Thx

[vmassol]

@vmassol

We need to check what happens when we use the @SuppressWarnings annotation and where Sonarcloud still allow us to see an issue. I’ll test that.

So I’ve tested it and the issue is gone and thus there’s no way to see what was the issue. That’s a bit painful. When we mark issues as false positives in the UI, we can still see the issue and get the explanation for it, and thus it’s easy to review it and undo the false positive if need be. Here with the annotation it’s much harder to understand the problem…

[mflorea]

+1

Thanks,
Marius

[tmortagne]

@vmassol

So I’ve tested it and the issue is gone and thus there’s no way to see what was the issue. That’s a bit painful. When we mark issues as false positives in the UI, we can still see the issue and get the explanation for it, and thus it’s easy to review it and undo the false positive if need be. Here with the annotation it’s much harder to understand the problem…

Another reason to comment very well the ignore, I guess.

[vmassol]

It’s now been accepted and documented at https://dev.xwiki.org/xwiki/bin/view/Community/CodeStyle/JavaCodeStyle/#HSonarQube

Please use this starting now!

Thx