I would like to add the Sonar jQAssistant Plugin to your market place. It analyzes the reports generated by jQAssistant during a build and creates issues for failed constraints or non-applyable concepts in SonarQube.
Welcome to the community and congrats on your plugin!
You’re probably aware that plugins must be tested by SonarSource staff before initial entry into the Marketplace. Generally, “SonarSource staff” means me in this context. So you know, I’ll be out of pocket for the beginning of next week and probably catching up for the end, so it will be a while before I get to this. But it is on my list.
I’ve had a chance to start on this. Beginning with the Requirements:
I don’t see a license in your project. It needs to be a FLOSS one, and it needs to not be implicit. (And yes, to be fair the Requirement for explicitness isn’t explicit either )
I can find your releases in GH, but I don’t see the relevant changelogs. The general convention is that each release would list in its description the major changes and/or include a link to the issues handled in the version.
What you’ve attached to your releases is not the generally expected jar files, but source zips. From the URLs in your PR, I guess you’ve uploaded to Maven central…? At a minimum, it would be nice if your Release records in GH pointed to the jars there. Even better if they were attached directly.
Your link to your SC dashboard actually points to the project in GH.
Marketplace PR: I’ve requested some changes and suggested a couple more.
Testing:
I can’t get the test project you gave me to build. After adding -DskipTests=true (and BTW the failing test failed very messily) I got past the test failures, only to get stuck at this:
[ERROR] --[ Constraint Violation ]-----------------------------------------
[ERROR] Constraint: spring-injection:FieldsOfInjectablesMustNotBeManipulated
[ERROR] Severity: MAJOR
[ERROR] Number of rows: 1
[ERROR] Fields of injectable types must not be manipulated, except from constructors.
[ERROR] Message=com.piggymetrics.statistics.service.ExchangeRatesServiceImpl.getCurrentRates(…) writes field 'container' at line 35, Injectable=com.piggymetrics.statistics.service.ExchangeRatesServiceImpl, Method=com.piggymetrics.statistics.service.ExchangeRatesServiceImpl#java.util.Map getCurrentRates(), Field=com.piggymetrics.statistics.service.ExchangeRatesServiceImpl#com.piggymetrics.statistics.domain.ExchangeRatesContainer container, LineNumber=35
[ERROR] -------------------------------------------------------------------
[ERROR]
[ERROR] --[ Constraint Violation ]-----------------------------------------
[ERROR] Constraint: spring-injection:InjectablesMustNotBeInstantiated
[ERROR] Severity: MAJOR
[ERROR] Number of rows: 1
[ERROR] Injectables types must not be instantiated directly except by @Bean methods or test code.
[ERROR]
[ERROR] Type=com.piggymetrics.account.service.security.CustomUserInfoTokenServices, Method=com.piggymetrics.account.service.security.CustomUserInfoTokenServices#java.util.Map getMap(java.lang.String,java.lang.String), Injectable=org.springframework.security.oauth2.client.OAuth2RestTemplate, LineNumber=119
[ERROR] -------------------------------------------------------------------
[ERROR]
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] piggymetrics 1.0-SNAPSHOT .......................... SUCCESS [ 21.027 s]
[INFO] config 1.0.0-SNAPSHOT .............................. SUCCESS [ 5.468 s]
[INFO] monitoring 0.0.1-SNAPSHOT .......................... SUCCESS [ 1.599 s]
[INFO] registry 0.0.1-SNAPSHOT ............................ SUCCESS [ 1.303 s]
[INFO] gateway 1.0-SNAPSHOT ............................... SUCCESS [ 1.577 s]
[INFO] auth-service 1.0-SNAPSHOT .......................... SUCCESS [ 3.899 s]
[INFO] account-service 1.0-SNAPSHOT ....................... SUCCESS [ 3.281 s]
[INFO] statistics-service 1.0-SNAPSHOT .................... SUCCESS [ 1.457 s]
[INFO] notification-service 1.0.0-SNAPSHOT ................ SUCCESS [ 1.061 s]
[INFO] turbine-stream-service 0.0.1-SNAPSHOT .............. FAILURE [ 3.298 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44.938 s
[INFO] Finished at: 2019-11-19T15:15:56-05:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal com.buschmais.jqassistant:jqassistant-maven-plugin:1.7.0:analyze (default-cli) on project turbine-stream-service: Violations detected: 0 concepts, 2 constraints -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
[ERROR]
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR] mvn <goals> -rf :turbine-stream-service
[INFO] Closing store in directory 'file:/home/ganncamp/testProjects/java/piggymetrics/target/jqassistant/store/'.
$
The release notes are maintained in the file release-notes.adoc in the root directory of the GH project
The released artifacts are currently only pushed to Maven Central, the direct download link is part of the PR and mentioned in the installation instructions. Is this sufficient or do you require the artifacts and and changelog to be maintained as GH releases?
The link to the SC dashboard is fixed now (see below), sorry for this.
For the test project: This is a fork of a project that I chose for an jQA example setup because it’s not too big but already “complex” enough to show that jQA is not about “Hello World” applications.
I’ve updated it such that you can build it, you’ll see jQA warnings (by intention) but it will not fail. The tests are fine “on my machine”, but if they are causing problems while testing the jQA SQ plugin it’s IMHO ok to skip them.
This is good. And I should have mentioned to start with that this should be added to your pom as well, so that the Marketplace can pick it up and show it to potential users. Hopefully you’ll have it in there for the next release.
Technically, yes.
On the test project, I can now build it, but analysis fails:
[INFO] Sensor JQA [jqassistant]
[INFO] Using project path 'testProjects/java/piggymetrics'.
[INFO] Found jQAssistant report at 'testProjects/java/piggymetrics/target/jqassistant/jqassistant-report.xml'.
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] piggymetrics 1.0-SNAPSHOT .......................... FAILURE [ 14.003 s]
[INFO] config 1.0.0-SNAPSHOT .............................. SKIPPED
[INFO] monitoring 0.0.1-SNAPSHOT .......................... SKIPPED
[INFO] registry 0.0.1-SNAPSHOT ............................ SKIPPED
[INFO] gateway 1.0-SNAPSHOT ............................... SKIPPED
[INFO] auth-service 1.0-SNAPSHOT .......................... SKIPPED
[INFO] account-service 1.0-SNAPSHOT ....................... SKIPPED
[INFO] statistics-service 1.0-SNAPSHOT .................... SKIPPED
[INFO] notification-service 1.0.0-SNAPSHOT ................ SKIPPED
[INFO] turbine-stream-service 0.0.1-SNAPSHOT .............. SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 24.683 s
[INFO] Finished at: 2019-11-21T12:25:34-05:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.sonarsource.scanner.maven:sonar-maven-plugin:3.7.0.1746:sonar (default-cli) on project piggymetrics: Cannot create JAXB context for com.buschmais.jqassistant.core.report.schema.v1.JqassistantReport: Implementation of JAXB-API has not been found on module path or classpath. com.sun.xml.bind.v2.ContextFactory -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
I’m on Ubuntu Linux.
$ java -version
openjdk version "11.0.4" 2019-07-16
OpenJDK Runtime Environment (build 11.0.4+11-post-Ubuntu-1ubuntu218.04.3)
OpenJDK 64-Bit Server VM (build 11.0.4+11-post-Ubuntu-1ubuntu218.04.3, mixed mode, sharing)
Administrative note: I’m moving the posts in this thread about how to correctly structure the plugin to a new topic under Plugin Development. They won’t get the proper attention (i.e. answers) here.
I’ve finally been able to test this! At least, I’ve made a start. Here’s my analysis log end. Was it expected that all but the first module is skipped?
First, when I look at the jqassistant-report.xml, I think there are a lot more violated constraints than there are issues raised in SonarQube. If the user isn’t expecting that, then it could lead to a false sense of security which would be a Bad Thing. Can you help me understand what’s going on here? As an example, here are the issues I see in SonarQube:
Looking at what made it into SonarQube, I’m torn. I feel that what I’m seeing is not a good user experience. On the other hand, I recognize that you’re parsing a report that you did not generate (you didn’t, did you?) and so you have to work with what’s available.
At a minimum:
Make the rules as granular as possible. What I see in SonarQube is 3 issues from the same rule that look conceptually like they could be broken into 3 rules:
Methods in “Transactional” classes should be labled “Spring” and “Transactional”
“Controller” types should be labeled “Spring”, “Controller” and “Component”
And this one (honestly, I don’t understand what it’s trying to tell me to do): Labels a classes which are annotated with “@org.springframework.transaction.annotation.Transactional” and their declared methods with “Spring” and “Transactional”.
Is that possible? Or are the “rules” user-defined? If it is possible, ideally - and completely optionally - you’re going to give a description for each rule. (Why should I do this? What bad thing can happen if I don’t? What does Okay code look like?)
When the report gives you a source file, you raise the issue on that file (instead of jamming the file into the issue message and making the user find it). When it gives you file and line number, you raise the issue on that file line (instead of jamming both - twice?! - into the issue message).
As a side note, I’m off next week for Thanksgiving, so I’ll see you in December.
good that the technical hurdles are now passed. Regarding your feedback:
Missing issues
During my testing all the warnings and violations printed during the Maven build are as well available in SonarQube. Is it possible that you only looked at the issues created on project level? Try to filter the issues by rule types “jQAssistant Constraint Violation” and “jQAssistant Invalid Concept”:
What might be confusing at this time is why some violations are attached to the project and others to the classes. This leads directly to your second remark:
Bad UX
The idea of rules in jQA differs a little bit from the way SonarQube does: A user writes his architecture documentation in Asciidoc files that are placed directly into the project (i.e. under version control) and he is encouraged to add, change or remove rules at any time. A rule usually is represented by a database query (Cypher/Neo4j) and the user is totally free on how the results looks like (i.e. which columns he is returning). So I as the developer of jQAssistant define the schema of the XML report but its a generic representation of database query result and what actually goes in there is defined by the rules and these are in the hands of the user.
In that sense the rules in the PiggyMetrics example are of different quality:
“spring-injection:InjectablesMustNotBeInstantiated” returns a type in its first column (i.e. “primary report column”). This enables the jQA SQ plugin to attach it directly to the affected class.
At the time of writing the other rules they did not make use of that primary column. For this reason the SQ plugin cannot identify the affected code element (class, field, method, etc.) and attaches these messages at the project level (i.e. fallback).
To be honest I’m actually as well not really happy with the way of representing the jQA results in SQ. It would be much better if there were not the two generic “jQA Constraint Violation” and “jQA Invalid Concept” rules but the rules themselves (e.g. “spring-injection:InjectablesMustNotBeInstantiated”) represented in SQ.
This was indeed the first attempt of the SQ plugin we implemented years ago but UX in that case was an absolute nightmare. The reason for this: everytime a rule changed in the project documentation (and this happens frequently) theses rules needed to be bundled into the jQA SonarQube plugin and the latter needed to be deployed to the SQ server. This is an absolute no-go in enterprise environments. We tried to find a way to allow the user to customize rules in the SQ UI but that was not usable as well. Another attempt was to synchronize rules coming from jQA and SQ in a dynamic way (e.g. while running the sensor) but at least the API did not show any way to do this. So we decided to fallback to a simple but working solution which are the two generic rules which allow the required dynamics but for the sake of less beautyful representation in the SQ UI.
At this point there’s one thing where I see immediate potential of improvement from my/our side: The predefined rules that are delivered with jQA (e.g. the Spring related rules used in the Piggy Metrics example) can be changed in a way that issues can be attached to the affected code element (i.e. by making use of the primary report column mentioned above). But this is not directly related to the SQ plugin.
Furthermore I’ll add an example for user defined rule that creates an issues in the project which hopefully gives a better illustration.
I’ve updated the demo application with a very rough skeleton of a developer documentation containing executable rules. You can find it in the file “jqassistant/index.adoc”. It contains two constraints showing violations as well as a concept which creates a report as PlantUML component diagram.
During the build this documentation is rendered to “target/jqassistant/report/index.html”, the rule results are embedded:
I’m sorry it has taken me so long to come back to you on this. There’s been a lot going on.
I’m pretty sure I didn’t. But now I see 8 issues. Is that what I should see?
I was afraid of that, but it was worth asking for changes.
Previously I would have pushed back on this because you’re raising project-level issues. Generally we say that issues should be raised in/on the code (i.e. readily actionable). And yet we’re starting to raise project-level issues of our own. So you get a pass on that.
At this point I think you’re in good shape for initial entry into the Marketplace. So… you’re in!
(And yes, to be fair the Requirement for explicitness isn’t explicit either 
)
