[New Release] codehawk Plugin v1.0

Hi, I have released the first version of the SonarQube plugin to analyze Java code:

This is our first release, I was told by the guide to mention that the plugin should be added to the Plugin Library page :slight_smile:

Hello!

I’m not the gatekeeper for new releases to the Marketplace here, but I took a glance at your plugin and… I have some feedback. :slight_smile:

You mention that a complete list of rules are described in the README, which is… accurate. And looking at the list of rules, none of them are particularly clear to me.

  • What is a “lazy” class? How do I avoid writing one so that I don’t trigger an issue? Same question for “Shotgun Surgery” and “Refused Bequest”?

  • Lots of your rules use the word “large” and “long”. How do you define “large” or “long” for each rule?

I thought there might be some more clarity once I actually installed the plugin and checked out rule descriptions. Unfortunately not.

Descriptions should answer questions “why is there an issue?”, “what could be its impact?”. Compliant / Non-compliant code examples are also missing for all rules. If there are really TO DO items, maybe they should be done before your first release!

I executed analysis against a sample project (an old copy of sonar-php I had around) and while analysis ultimately succeeded, I got a huge number of errors like this:

[ **ERROR** ] Unable to run check class org.codehawk.plugin.java.checks.RefusedBequest - refused bequest on file 'php-checks/src/main/java/org/sonar/php/checks/utils/type/NewObjectCall.java', To help improve SonarJava, please report this problem to SonarSource : see https://www.sonarqube.org/community/

java.lang.NullPointerException: null
at org.codehawk.plugin.java.checks.RefusedBequest.visitNode(RefusedBequest.java:100)
at org.sonar.java.model.VisitorsBridge$IssuableSubsciptionVisitorsRunner.lambda$visit$6(VisitorsBridge.java:307)
at org.sonar.java.model.VisitorsBridge$IssuableSubsciptionVisitorsRunner.lambda$forEach$9(VisitorsBridge.java:322)
at org.sonar.java.model.VisitorsBridge.runScanner(VisitorsBridge.java:173)
at org.sonar.java.model.VisitorsBridge.access$100(VisitorsBridge.java:68)
at org.sonar.java.model.VisitorsBridge$IssuableSubsciptionVisitorsRunner.forEach(VisitorsBridge.java:322)
at org.sonar.java.model.VisitorsBridge$IssuableSubsciptionVisitorsRunner.visit(VisitorsBridge.java:309)
at org.sonar.java.model.VisitorsBridge$IssuableSubsciptionVisitorsRunner.visitChildren(VisitorsBridge.java:293)
at org.sonar.java.model.VisitorsBridge$IssuableSubsciptionVisitorsRunner.visit(VisitorsBridge.java:313)
at org.sonar.java.model.VisitorsBridge$IssuableSubsciptionVisitorsRunner.run(VisitorsBridge.java:284)
at org.sonar.java.model.VisitorsBridge.visitFile(VisitorsBridge.java:155)
at org.sonar.java.ast.JavaAstScanner.simpleScan(JavaAstScanner.java:99)
at org.sonar.java.ast.JavaAstScanner.scan(JavaAstScanner.java:65)
at org.sonar.java.JavaSquid.scanSources(JavaSquid.java:111)
at org.sonar.java.JavaSquid.scan(JavaSquid.java:105)
at org.sonar.plugins.java.JavaSquidSensor.execute(JavaSquidSensor.java:88)

Not sure if it matters, but this was on a SonarQube v7.9.2 instance with SonarJava v6.0 installed!

For the issues that were raised, there were some formatting issues in the issue titles.

Finally, there’s overlap between the rules in your plugin and rules already provided by SonarQube’s Java analysis. Some examples:

RSPEC-107: Methods should not have too many parameters
RSPEC-1479: “switch” statements should not have too many “case” clauses
RSPEC-138: Methods should not have too many lines

What do your implementation of the rules bring to the table, so that a user should disable existing rules and add the rules from your plugin?

While maybe an interesting exercise with the SonarJava API (and seriously, kudos on the work you’ve done, rule development is tough!) it might be good to step back and ask yourself if your plugin is really ready for the Marketplace with this release.

Best regards,

Colin

Hi,

I guess I am the gatekeeper, and I’m going to work off of Colin’s testing. When we do this initial testing, we’re looking at a couple different levels. Does the plugin

  1. prevent the server from starting up?
  2. crash analysis?
  3. give a minimally acceptable user experience?

Clearly you got past points 1 and 2 and Colin’s feedback is around point #3. I’m going to divide his points into 2 categories:

Blocker

  • NPEs are raised during analysis of a random project. This needs to be fixed before we can add the plugin to the Marketplace. I’m sure @Colin can help you out with the right version of the project in question to reproduce the errors.

Optional

  • You’ve listed your rule titles in your repo’s README… I guess because you felt compelled to provide some “documentation” to get past the requirements? If I’m guessing correctly, then you - and your users! - would be better off if you kept it simple. Something like:

    This SonarQube plugin adds additional Java rules. It can be installed via the Marketplace (pending) or by downloading the plugin, and…

  • Your rule documentation within the plugin appears a little… sparse. As Colin said, you should use the rule description and code samples to help the user understand why the issue was raised and what to do about it. It would be nice if future versions fleshed this out.

  • Issue message formatting

  • Duplicate rules. I’ll be honest, this one teeters between being a blocker and not, partially because such a large percentage of your rules seems to overlap. Can you please articulate the difference between your size-based rules and SonarQube’s built-in ones?


Also, I’ve done a quick scan of the bureaucratic requirements. They look pretty good. However your PR is incomplete: you need to register your plugin/file in the master list. You’ll find the details in the docs. Look under “Register file”.

 
Ann

Hi, @Colin_SonarSource and @ganncamp, we really appreciate your feedback. They are all on point and posted few questions that we haven’t think of.

We will fix raised issues as soon as possible, and will let you know when we’ve done so.

Our team is rather new to this criteria, and the list of rules was given to us as a start off project. We will further discuss on how we want to change up some rules so they won’t overlap.

Thanks again for your patience and advice,
Jack

1 Like

Hi, @Colin_SonarSource, may we ask for the source file which raised the error in your example? It would help us a lot! :smiley:

Jack

Hey Jack,

I checked, and the errors are all over the master branch of SonarSource/sonar-php!

Here’s debug level logs of the analysis:

sonarphpnpe2.txt (3.4 MB)

Heads-up: I also noticed your SonarCloud Analysis is only analyzing the HTML and XML code in your repo. As most of the logic is going to be Java, you should definitely fix that up.

Colin

2 Likes

Hi, we fixed the issues listed in the comments and decided to leave the overlapping rules out of the rule list.
New sonarcloud link: https://sonarcloud.io/dashboard?id=SDPMLab_CodeHawk

Hi,

Where is the new jar to test? At a minimum, you should update your PR to point to it. Ideally, you’d make it easy for us & also post the link here.

 
:smiley:
Ann

Hi Ann,
Thanks for your reminder, I’ll list the required details below as our new release note.

We’ve updated ReadMe file, rule documentaion, issue title format, and removed overlapping rules from rule list.

We found the problem to colin’s case and tested the solution on our test project and we believe the issue is resolved. However, we can’t get the sonar-php project of colin’s to analyze correctly. Is it possible for @Colin_SonarSource to give it another go and see if the error still pops up? Thank you very much! :smiley:

Best regards,
Jack

Hi Jack,

Using the download link you just provided, I re-analyzed the sonar-php project and still got NPEs:

[ERROR] Unable to run check class org.codehawk.plugin.java.checks.AvoidShotgunSurgery - AvoidShotgunSurgery on file 'php-checks/src/main/java/org/sonar/php/checks/AliasFunctionUsageCheck.java', To help improve SonarJava, please report this problem to SonarSource : see https://www.sonarqube.org/community/
java.lang.NullPointerException: null
	at org.codehawk.plugin.java.checks.AvoidShotgunSurgery.isNewClass(AvoidShotgunSurgery.java:360)
	at org.codehawk.plugin.java.checks.AvoidShotgunSurgery.comparedWithUsedInMethod(AvoidShotgunSurgery.java:326)
	at org.codehawk.plugin.java.checks.AvoidShotgunSurgery.visitNode(AvoidShotgunSurgery.java:62)
	at org.sonar.java.model.VisitorsBridge$IssuableSubsciptionVisitorsRunner.lambda$visit$6(VisitorsBridge.java:307)

Did you mean to re-send the link to the 1.0 jar? (Since you’ve made changes, you should bump the version number anyway…) Is it possible I’m using a stale jar? I see this in the manifest, but it’s still worth asking: Plugin-BuildDate: 2020-03-10T11:58:30+0800

 
While we’re at it, this is what I see when I look at Installed plugins in my instance’s Marketplace:

Selection_959

Looks like you need to update the project pom some more. :smile:

 
Ann

Hi Ann,

Yes, that was one of the error from the previous version.
We’ve updated our plugin, pom and posted
New release note: https://github.com/SDPMLab/CodeHawk/releases/tag/1.1
New jar download link: https://github.com/SDPMLab/CodeHawk/releases/download/1.1/codehawk-1.1.jar

Thanks for your notice,
Jack

Hi Jack,

I’ve tripple-checked that I’m running the right version of your plugin Selection_962

And I still get

[ERROR] Unable to run check class org.codehawk.plugin.java.checks.AvoidShotgunSurgery - AvoidShotgunSurgery on file 'php-checks/src/main/java/org/sonar/php/checks/security/QueryUsageCheck.java', To help improve SonarJava, please report this problem to SonarSource : see https://www.sonarqube.org/community/
java.lang.ClassCastException: class org.sonar.java.model.JavaTree$ParameterizedTypeTreeImpl cannot be cast to class org.sonar.plugins.java.api.tree.IdentifierTree (org.sonar.java.model.JavaTree$ParameterizedTypeTreeImpl and org.sonar.plugins.java.api.tree.IdentifierTree are in unnamed module of loader org.sonar.classloader.ClassRealm @238291d4)
	at org.codehawk.plugin.java.checks.AvoidShotgunSurgery.checkVariableTree(AvoidShotgunSurgery.java:231)
	at org.codehawk.plugin.java.checks.AvoidShotgunSurgery.checkStatementTree(AvoidShotgunSurgery.java:142)
	at org.codehawk.plugin.java.checks.AvoidShotgunSurgery.checkInnerClassTree(AvoidShotgunSurgery.java:106)
	at org.codehawk.plugin.java.checks.AvoidShotgunSurgery.visitNode(AvoidShotgunSurgery.java:61)
	at org.sonar.java.model.VisitorsBridge$IssuableSubsciptionVisitorsRunner.lambda$visit$6(VisitorsBridge.java:307)

 
:woman_shrugging:
Ann

Hi Ann,

We would like to analyze sonar-php ourselves and fix errors that might come up so it won’t cause you more trouble. :sweat_smile:
However, when we built the project with maven clean package and tried to analyze it using maven scanner, the following error was raised.

[ERROR] Failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:3.6.0.1398:sonar (default-cli) on project php: Not inside a Git work tree: D:\專題\sonarqube\sonar-php-master\sonar-php-plugin

Jack

Hi Jack,

You’re trying too hard. :smile:

You don’t analyze from from the sonar-php-plugin directory, but from the top of the project, sonar-php (where the .git directory is located :wink:)

 
HTH,
Ann

Hi Ann,

I did analyze from the top directory, the execution just stopped at the sonar-php-plugin directory somehow. Sonar-scanner for Maven was not working as expected for us for some reason.

I switched to vanilla sonar-scanner instead and the execution succeeded though. Now we just have to fix the errors from our plugin.

Thanks for your help!
Jack

Hi Ann,

We’ve resolved the NPEs and fixed some other issues. Below is the sonar-php analysis result.

INFO: Analysis report generated in 58000ms, dir size=33 MB
INFO: Analysis reports compressed in 96788ms, zip size=14 MB
INFO: Analysis report uploaded in 1042ms
INFO: ANALYSIS SUCCESSFUL, you can browse http://localhost:9000/dashboard?id=sonar-php
INFO: Note that you will be able to access the updated dashboard once the server has processed the submitted analysis report
INFO: More about the report processing at http://localhost:9000/api/ce/task?id=AXEMzAvWPR60rxagJ1YF
INFO: Executing post-job 'Display issues'
INFO: Task total time: 6:48.328 s
INFO: ------------------------------------------------------------------------
INFO: EXECUTION SUCCESS
INFO: ------------------------------------------------------------------------
INFO: Total time: 6:53.558s
INFO: Final Memory: 28M/1566M
INFO: ------------------------------------------------------------------------

Release note: https://github.com/SDPMLab/CodeHawk/releases/tag/v1.2
jar: https://github.com/SDPMLab/CodeHawk/releases/download/v1.2/codehawk-1.2.jar
Sonarcloud: https://sonarcloud.io/dashboard?id=SDPMLab_CodeHawk

Please let us know if there are any problems.

Best,
Jack

Hi Jack,

I’ve just re-tested, and …
No NPEs! :fireworks:

Before I can pull the trigger on adding your plugin to the Marketplace, I’ll need you to update your PR. It still lists 1.0 as the version to go in, rather than 1.2.

Additionally, I have some non-blocker feedback:

  • I notice that your SonarCloud analysis shows 0% code coverage. You’re currently passing your Quality Gate and that’s the requirement. Still, you should fix that.

  • The dataClump rule could benefit by adding secondary locations on the other methods with the same clumps.

  • I would raise the Large Class issues on the class declaration, rather than on all the lines in the class (my eyes! :scream:)

  • Ditto Lazy Class

 
Ann

Hi Ann,

The PR has been updated.

We will continue to improve our plugin’s user experience and work on the code coverage as well as adding new rules. We hope to meet higher standards upon next release.

Thanks a lot for your feedback! :smile:
Jack

Hi Jack,

I’ve asked for changes in your PR.

 
Ann

You’re in!

 
:smile:
Ann