[FOLIO-864] Managing Sonarqube exceptions and rule customization Created: 27/Sep/17  Updated: 15/Jan/19

Status: In Progress
Project: FOLIO
Components: None
Affects versions: None
Fix versions: None

Type: Task Priority: P3
Reporter: John Malconian Assignee: John Malconian
Resolution: Unresolved Votes: 0
Labels: ci, core, devops
Remaining Estimate: Not Specified
Time Spent: 1 hour
Original estimate: Not Specified

Issue links:
Relates
relates to FOLIO-1049 Reject pull request if new code doesn... Open
Sprint:
Development Team: Core: Platform

 Description   

With Sonarcloud, there are default Quality Profiles for each programming language. These profiles are referred to as the 'Sonar way' and are updated regularly as plugins. There will be instances when we need to override or customize the default Quality Profiles that are used to analyze FOLIO projects.



 Comments   
Comment by John Malconian [ 27/Sep/17 ]

I've created two Java Quality Profile called 'FOLIO Java' based on the default Sonar profile. It's not clear to me whether I should create a custom profile for each project or just one for FOLIO. In other words, if we make exceptions in one project, should we apply to all other similar projects? I'm not sure the correct answer to this, but for now, I will maintain one custom Quality Profile for all Java projects and do the same for Javascript projects, etc.

The 'FOLIO Java' profile configured as with the default Sonar profile as its parent. The advantage to this is that as Sonar updates rules in its default profile, those rules will trickle down into the custom profile. The disadvantage is that I cannot deactivate specific rules completely. I can only change their thresholds and priority.

Adam has a specific rule he wants to add as an exception to the Okapi project - squid:S1192 'String literals should not be duplicated'. I've changed the priority of the rule from 'critical' to 'info'.

Comment by John Malconian [ 27/Sep/17 ]

There is no way to change the default Quality Gate in Sonarcloud which consists of 'A' ratings for Reliability, Security, and Maintainability and 80% coverage on new code which is a bit of a bummer. We'd have to install a local instance of Sonarqube in order to customize Quality Gates.

Comment by Heikki Levanto [ 28/Sep/17 ]

Any way to reduce the required test coverage to 75% or 60% ? It could be increased slowly once we have reached that in most things. Some of the code it impossible to cover in tests, mostly error handling (catch the exception when the second database operation suddenly fails)

Comment by John Malconian [ 28/Sep/17 ]

I've changed the threshold for Sonarqube rule squid:S3776: Cognitive Complexity of methods should not be too high from '15' to '30' and downgraded priority from 'critical' to 'major' in the Sonarqube FOLIO Java quality profile. Should be enough not fail anything. Let's see how it works out.

Comment by John Malconian [ 28/Sep/17 ]

Heikki Levanto - Unfortunately, custom Quality Gates (where test coverage is defined) cannot be customized in the hosted version of Sonarqube (Sonarcloud) at this time.

Comment by Heikki Levanto [ 28/Sep/17 ]

The thing with cognitive complexity seems too strict. Vertx requires a more complex code. Could we up the limit from 15 to 25? And maybe reduce it to a warning, for now.

Comment by shale99 [ 19/Oct/17 ]

i would like the following also reduced to a warning:

Extract this nested try block into a separate method.
it seems legit to me to have nested try / catch clauses , especially in a non-blocking vertx type env. for example:

    try {
      vertxContext.runOnContext(v -> {
        try {
          PostgresClient postgresClient = PostgresClient.getInstance(vertxContext.owner(),
              TenantTool.calculateTenantId(tenantId));

Comment by shale99 [ 19/Oct/17 ]

also, when rmb instantiates a class to use in order to call an implemented meathod, it allows the class to implement a constructor for some minor bootstraping. the constructor contains a vertx and a tenantid parameter - but the implementing class can refer to either both or just one of the arguments - so the rule

Remove this unused method parameter "vertx".

is problematic as well

Comment by Julian Ladisch [ 02/Feb/18 ]

@shale99: "Extract this nested try block into a separate method." should not be reduced to a warning. This mainly pops up in org/folio/rest/impl/*API.java files. Use

@SuppressWarnings("squid:S1141")  // suppress "Extract this nested try block into a separate method."
public class MaterialTypeAPI implements MaterialTypesResource {

in those files so that this error can be dealt with on an individual basis in all other files.

Comment by Julian Ladisch [ 02/Feb/18 ]

shale99 The error/warning "Remove this unused method parameter" can be disabled on a case-by-case basis:

  @SuppressWarnings("squid:S1172")  // suppress "Remove these unused method parameters."
  public FixedDueDateSchedulesAPI(Vertx vertx, String tenantId) {
    if(schema == null){
      initCQLValidation();
    }
  }

The rule should remain enabled by default in sonar so that the likely programming error is flagged in all other places.

Generated at Thu Feb 08 23:08:53 UTC 2024 using Jira 1001.0.0-SNAPSHOT#100246-sha1:7a5c50119eb0633d306e14180817ddef5e80c75d.