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

#93: changed rules priorities and removed one #103

Merged
merged 3 commits into from Nov 12, 2020

Conversation

RSOKOLSK
Copy link
Contributor

@RSOKOLSK RSOKOLSK commented Sep 21, 2020

PR addressing issue #93

  1. changed priority of rule "s2699 - Tests should include assertions" to INFO
  2. changed priority of rule "s121 - Control structures should use curly braces" to MINOR
  3. removed rule "s2063 - "Comparators should be "Serializable""

Copy link
Member

@lmarniazman lmarniazman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

Copy link
Member

@lmarniazman lmarniazman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, there is one change request. You included this in the other PR for #93, and probably forgot to put it here too. There is a bug in line 102 of DevonfwJavaProfile. The if-condition there should be negated, otherwise the new priority will not be set. Could you include this in this PR? Thanks a lot.

@RSOKOLSK
Copy link
Contributor Author

Done, thanks for reminder.

@lmarniazman
Copy link
Member

lmarniazman commented Sep 24, 2020

@hohwille
From the issue text, I also understood it in the way that S2063 should be removed because of this line:

Java Serialization is about dead. Do not use JEE App servers that serialize your objects silently into a session storage.

Or should this rule be replaced by our own implementation? We could create a rule that detects if java.io.Serializable is used and point the user to an appropriate alternative.

What might be confusing is that the title of the rule is slightly off here. Its real name is Comparators should be "Serializable" instead of Security Constraints should be "Serializable".

@hohwille
Copy link
Member

  1. For s2699 on INFO I am also fine and this is what I intended.

  2. For s121 I am also not entirely sure.
    https://rules.sonarsource.com/java/tag/serialization/RSPEC-121
    Actually I agree with the rule and its severity in general and think the main problem comes from tools like Eclipse that generate code violating this rule. I will first check what we can do with the latest Eclipse version that IMHO has improved here.

  3. @lmarniazman thanks for clarification. So we are talking about this rule:
    https://rules.sonarsource.com/java/tag/serialization/RSPEC-2063

Regarding the serialization I was more talking about this one:
https://rules.sonarsource.com/java/tag/serialization/RSPEC-2057

I am still thinking if we should keep these rules but on INFO level.
Further, I am more keen on removing the according warning from Eclipse default config in devonfw-ide.

So to summarize: With #93 I wanted to collect feedback and start a discussion. Implementing all as one PR was maybe to quick. But I try to clarify the above points so we can finalize this PR and merge it to make progress.

@hohwille
Copy link
Member

devonfw/ide#473

@lmarniazman
Copy link
Member

Thanks for the input @hohwille

So for S121 and S2063, the severity should be kept at CRITICAL for now, and S2057 should be at INFO? Or should the severities of all three rules be set to INFO?

@lmarniazman lmarniazman added this to the release:2020.12.001 milestone Nov 10, 2020
@hohwille
Copy link
Member

IMHO only 2. was in question and S121 should remain on CRITICAL. Change 1. and 3. are fine as well as the fixed IF-statement. Could you find a way to revert the change of 2. so we can merge?

@hohwille
Copy link
Member

I could edit directly on github. BTW: cool feature... So new we can merge.

@hohwille hohwille merged commit 1ee05b6 into devonfw:master Nov 12, 2020
@hohwille hohwille linked an issue Dec 8, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve devonfw Java quality profile
3 participants