Discuss static analysis options

[jimschubert]

Description

I raised the question in #26 about how we could verify standard usage of slf4j loggers at compile time, and whether there is a static analysis tool we could use. We could look into SonarQube, but I've also found a Findbugs plugin at https://github.com/KengoTODA/findbugs-slf4j which would do exactly this.

Should we consider incorporating this type of static analysis now? With so many open source contributors, it would be helpful to standardize on some coding patterns to reduce maintenance overhead.

openapi-generator version

all

OpenAPI declaration file content or url

n/a

Command line used for generation

n/a

Steps to reproduce

n/a

Related issues/PRs

• Code clean-up: Add own private static final LOGGER in each class #26

Suggest a fix/enhancement

Incorporate FindBugs or similar into the build.

[jmini]

This is a great idea. Thank you a lot for opening this issue.

---

I thought Findbugs is no longer developed and was forked to Spotbugs.
https://mailman.cs.umd.edu/pipermail/findbugs-discuss/2016-November/004321.html

---

One thing we need to consider is the integration of this with the build and/or with the PR workflow...
Do we want the build to fail if one of the rule is broken? This is hard, but it will bring us quality.

---

SonarQube: they have an online version and it is free for OpenSource project:
https://about.sonarcloud.io/sq/

I have heard they are working on an integration with the PR workflow, I am not sure if the feature was released yet or not.

[wing328]

UPDATE: I've used sonarcloud.io to generate the report

Ref: #788

[tomasbjerre]

I'm missing this as well. I would propose doing this with PMD and Spotbugs, using maven-plugins.

• Add maven-plugins to parent pom
  • https://spotbugs.readthedocs.io/en/latest/maven.html (and in the future with Gradle https://spotbugs.readthedocs.io/en/latest/gradle.html)
  • https://maven.apache.org/plugins/maven-pmd-plugin/ (and in the future with Gradle https://docs.gradle.org/current/userguide/pmd_plugin.html)
• Configure plugins to not fail the build on errors.
• Add Violations Maven Plugin (and in the future we can use Violations Gradle Plugin)

See #5040

With these changes we would have builds failing if new issues are found from static code analysis. There is a configuration option maxViolations that we can set to whatever when introducing this and decrease as issues are fixed.

This would naturally work both when working locally and on CI servers like Travis or CircleCI. When doing mvn verify it would fail the build if issues found.

...
[INFO] --- maven-jar-plugin:2.4:jar (default-jar) @ plugin-example ---
[WARNING] JAR will be empty - no content was marked for inclusion!
[INFO] Building jar: /home/travis/build/tomasbjerre/violations-maven-plugin/violations-maven-plugin-example/target/plugin-example-1.0-SNAPSHOT.jar
[INFO] 
[INFO] --- violations-maven-plugin:1.33-SNAPSHOT:violations (default) @ plugin-example ---
[WARNING] The POM for se.bjurr.violations:violations-git-lib:jar:1.32 is missing, no dependency information available
[WARNING] The POM for se.bjurr.violations:violations-lib:jar:1.108 is missing, no dependency information available
[INFO] 
Violations in repo
/home/bjerre/workspace/violations-test/src/main/java/se/bjurr/violations/lib/example/CopyOfMyClass.java
|            |                                                                    |          |      |                              |
| Reporter   | Rule                                                               | Severity | Line | Message                      |
|            |                                                                    |          |      |                              |
+------------+--------------------------------------------------------------------+----------+------+------------------------------+
|            |                                                                    |          |      |                              |
| Checkstyle | com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck      | INFO     | 9    | Must  have  at   least   one |
|            |                                                                    |          |      | statement.                   |
|            |                                                                    |          |      |                              |
+------------+--------------------------------------------------------------------+----------+------+------------------------------+
|            |                                                                    |          |      |                              |
| Checkstyle | com.puppycrawl.tools.checkstyle.checks.javadoc.JavadocPackageCheck | ERROR    | 0    | Missing    package-info.java |
|            |                                                                    |          |      | file.                        |
|            |                                                                    |          |      |                              |
+------------+--------------------------------------------------------------------+----------+------+------------------------------+
Summary of /home/bjerre/workspace/violations-test/src/main/java/se/bjurr/violations/lib/example/CopyOfMyClass.java
|            |      |      |       |       |
| Reporter   | INFO | WARN | ERROR | Total |
|            |      |      |       |       |
+------------+------+------+-------+-------+
|            |      |      |       |       |
| Checkstyle | 1    | 0    | 1     | 2     |
|            |      |      |       |       |
+------------+------+------+-------+-------+
|            |      |      |       |       |
|            | 1    | 0    | 1     | 2     |
|            |      |      |       |       |
+------------+------+------+-------+-------+

...

se/bjurr/violations/lib/example/OtherClass.java
|          |                            |          |      |                              |
| Reporter | Rule                       | Severity | Line | Message                      |
|          |                            |          |      |                              |
+----------+----------------------------+----------+------+------------------------------+
|          |                            |          |      |                              |
| Findbugs | MS_SHOULD_BE_FINAL         | INFO     | 7    | Field isn't final but should |
|          |                            |          |      | be  <p>  This  static  field |
|          |                            |          |      | public but  not  final,  and |
|          |                            |          |      | could    be    changed    by |
|          |                            |          |      | malicious   code    or    by |
|          |                            |          |      | accident    from     another |
|          |                            |          |      | package. The field could  be |
|          |                            |          |      | made  final  to  avoid  this |
|          |                            |          |      | vulnerability.</p>           |
|          |                            |          |      |                              |
+----------+----------------------------+----------+------+------------------------------+
|          |                            |          |      |                              |
| Findbugs | NM_FIELD_NAMING_CONVENTION | INFO     | 6    | Field  names  should   start |
|          |                            |          |      | with a lower case letter <p> |
|          |                            |          |      | Names of fields that are not |
|          |                            |          |      | final  should  be  in  mixed |
|          |                            |          |      | case with a lowercase  first |
|          |                            |          |      | letter and the first letters |
|          |                            |          |      | of     subsequent      words |
|          |                            |          |      | capitalized. </p>            |
|          |                            |          |      |                              |
+----------+----------------------------+----------+------+------------------------------+
Summary of se/bjurr/violations/lib/example/OtherClass.java
|          |      |      |       |       |
| Reporter | INFO | WARN | ERROR | Total |
|          |      |      |       |       |
+----------+------+------+-------+-------+
|          |      |      |       |       |
| Findbugs | 2    | 0    | 0     | 2     |
|          |      |      |       |       |
+----------+------+------+-------+-------+
|          |      |      |       |       |
|          | 2    | 0    | 0     | 2     |
|          |      |      |       |       |
+----------+------+------+-------+-------+
Summary
|            |      |      |       |       |
| Reporter   | INFO | WARN | ERROR | Total |
|            |      |      |       |       |
+------------+------+------+-------+-------+
|            |      |      |       |       |
| Checkstyle | 4    | 1    | 1     | 6     |
|            |      |      |       |       |
+------------+------+------+-------+-------+
|            |      |      |       |       |
| Findbugs   | 2    | 2    | 5     | 9     |
|            |      |      |       |       |
+------------+------+------+-------+-------+
|            |      |      |       |       |
|            | 6    | 3    | 6     | 15    |
|            |      |      |       |       |
+------------+------+------+-------+-------+

[jimschubert]

Excellent contribution in #5040.

I've created the Sonar CI GitHub Action as well to perform analysis via SonarCloud, which will run on pushes to master or feature branches.

I put the contribution from #5040 under a new profile in #5116. I want to get this into master, and then evaluate where is the best place to run the additional checks. I'm thinking the Sonar CI action could just run PMD/SpotBugs as part of it's initial build and then push all those details to SonarCloud for UI inspection, but also cache the xml reports in the GitHub Action.