Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve devonfw Java quality profile #93

Closed
hohwille opened this issue Aug 14, 2020 · 2 comments · Fixed by #103
Closed

Improve devonfw Java quality profile #93

hohwille opened this issue Aug 14, 2020 · 2 comments · Fixed by #103
Assignees

Comments

@hohwille
Copy link
Member

With issue #16 implemented by PR #56 we now automatically provide a standard devonfw Java quality profile to SonarQube.
This comes from here:https://github.com/devonfw/sonar-devon4j-plugin/blob/master/src/main/resources/com/devonfw/ide/sonarqube/common/rules/devon4j/devon4j.xml

We need feedback from projects if this is a suitable default.
Obviously you can have endless discussions about this, but everyone is invited to provide feedback and request for change if a rationale is given. Examples are

  • we might miss out a cool new rule that you suggest to add by default telling what the rule archives and linking to details. The rule should not produce many false-positives if the suggested default severity is higher than info
  • we might use a rule that is deprecated or otherwise replaced by a newer and better rule.
  • we might include a rule with a too high or too low severity. Please explain in detail why you want to raise or lower the severity. Give examples whenever possible.
@hohwille
Copy link
Member Author

hohwille commented Aug 18, 2020

My first feedback (read squid as synonym to java as the feedback was from an older installation and the new feature is not yet available):

  • squid:S2699 (Add at least one assertion to this test case.) - This rule is creating issues of severity Blocker. While the goal and idea of the rule is valid its implementation is so naive and stupid that it is creating tons of false-positives and it is therefore inacceptable to have these as Blocker. It does not support soft-assertions. It does not support when a test method delegates to a reused check-method that does the actual assertions, etc. So in the end it is better to entirely disable this rule (or at least put it down to Info severity).
  • squid:S3369 (Add "security-constraint" elements to this descriptor.) - This rule is checking if security-constraint element is present in a web.xml. As we are using spring-boot and esp. spring-security this is typically not done via web.xml so in most cases this is also a false-positive. However, as we suggest not to have a web.xml at all and as there is at maximum only one web.xml per project, the rule is still valid and projects can set this to false-positive or not-an-issue if they properly address the problem on other levels. Otherwise it is still good to point projects to the potential problem.
  • squid:S2063 (Comparators should be "Serializable") - Java Serialization is about dead. Do not use JEE App servers that serialize your objects silently into a session storage. Use stateless servers after all. Use reasonable formats such as JSON for safe and secure data-exchange and never ever use serialization for external interfaces (HTTP Invoker, RMI, etc.). Then you can stop wasting your time with serialVersionUID.
  • squid:S00121 (Missing curly brace.) - This is rule is valid after all and it really is bad code style to create if statements or the like without curly braces. However, I only see this with stupid equals methods generated by Eclipse. Is the serverity Critical then? Either we need to get our common tools fixed to avoid producing such nonsense, or we might consider lowering the severity (to Minor or at max. Major) if that happens due to stupid tools. Manually refactoring all these methods not always leads to a good balance of effort/quaility gain. IMHO the best approach is still to make our tools to properly format the code so things like this never happen and then the rule is just fine as no issues will be raised.

RSOKOLSK added a commit to RSOKOLSK/sonar-devon4j-plugin that referenced this issue Sep 9, 2020
… assertions" to INFO

2. changed priority of rule "s3369 - Security constraints should be defined" to INFO
3. removed rule "s2063 - "Security constraints should be "Serializable""
4. changed priority of rule "s121 - Control structures should use curly braces" to MINOR
RSOKOLSK added a commit to RSOKOLSK/sonar-devon4j-plugin that referenced this issue Sep 9, 2020
1. changed priority of rule "s2699 - Tests should include assertions" to INFO
2. changed priority of rule "s3369 - Security constraints should be defined" to INFO
3. removed rule "s2063 - "Security constraints should be "Serializable""
4. changed priority of rule "s121 - Control structures should use curly braces" to MINOR
RSOKOLSK added a commit to RSOKOLSK/sonar-devon4j-plugin that referenced this issue Sep 11, 2020
1. Added HttpClient to pom.xml
2. Parsed tag value from devon4j.xml to String
RSOKOLSK added a commit to RSOKOLSK/sonar-devon4j-plugin that referenced this issue Sep 11, 2020
…everities

1. Added HttpClient to pom.xml
2. Parsed tag value from devon4j.xml to String
3. Implemented method to sent POST request to SonarQube API "api/rules/update"
4. Added negation of "severity.isEmpty() 127 line"
5. Changed S3369 rule priority back to "BLOCKER"
RSOKOLSK added a commit to RSOKOLSK/sonar-devon4j-plugin that referenced this issue Sep 11, 2020
@RSOKOLSK RSOKOLSK self-assigned this Sep 17, 2020
RSOKOLSK added a commit to RSOKOLSK/sonar-devon4j-plugin that referenced this issue Sep 21, 2020
RSOKOLSK added a commit to RSOKOLSK/sonar-devon4j-plugin that referenced this issue Sep 21, 2020
This reverts commit 18c9993.

Need to correct formatting
RSOKOLSK added a commit to RSOKOLSK/sonar-devon4j-plugin that referenced this issue Sep 21, 2020
RSOKOLSK added a commit to RSOKOLSK/sonar-devon4j-plugin that referenced this issue Sep 21, 2020
hohwille pushed a commit that referenced this issue Nov 12, 2020
Added missing negation of method severity.isEmpty()
@hohwille hohwille added this to the release:2020.12.001 milestone Dec 8, 2020
@hohwille hohwille linked a pull request Dec 8, 2020 that will close this issue
@hohwille
Copy link
Member Author

Status:

  • S2699 - severity lowered from blocker to info
  • S3369 - no problem: devon4j projects do not have web.xml anymore. Projects that do have it might take value from the rule or mark the single finding as not an issue/false positive.
  • S2063 - has been removed
  • S00121 - no problem: rule is fine. We need to fix devonfw-ide settings for eclipse.

Therefore considering done and closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants