Support configurable severity per ruleset/rule in XML and Sarif output #3310

chao2zhang · 2020-12-20T03:48:18Z

Summary

This PR implements configuring the severity in the XML and Sarif output, which is the first step to implement #3274. The implementation is also different from #3306, because the severity in the configuration can only be a simplified version and does not allow custom strings.

Implementation

Adding the severity as a property of Issue does not work: Each rule initializes the issue first then uses the issue.id to initialize ruleConfig. If we want to use ruleConfig to parse severity, it has to be done after issue is created.
We can break such cycling dependency by refactoring, but I would prefer doing so in the next PR where we can remove the existing Severity and replace it with the new SeverityLevel.

The choice here is to add the severity as a property of Finding, which is appended at the end to mitigate incompatibility. However, since Finding and its children are not data class, I need to create copyWithSeverity to override the given severity.

Followup

I did not deprecate or remove Severity, maxIssues, warningsAsErrors in this PR. If this is a good time to do so, I would be happy to do those in a follow-up PR.

Test

In addition to the unit test, I also performed an integration test in a different repo. Users can configure severity at ruleset level and rule level.

style:
  severity: error
  MagicNumber:
    severity: ignore

codecov · 2020-12-20T04:01:06Z

Codecov Report

Merging #3310 (afcab04) into master (ff655d2) will decrease coverage by 0.07%.
The diff coverage is 70.37%.

@@             Coverage Diff              @@
##             master    #3310      +/-   ##
============================================
- Coverage     80.34%   80.27%   -0.08%     
+ Complexity     2724     2723       -1     
============================================
  Files           445      445              
  Lines          8177     8193      +16     
  Branches       1555     1558       +3     
============================================
+ Hits           6570     6577       +7     
- Misses          774      778       +4     
- Partials        833      838       +5

Impacted Files	Coverage Δ	Complexity Δ
...n/kotlin/io/gitlab/arturbosch/detekt/api/Config.kt	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...lin/io/gitlab/arturbosch/detekt/api/ConfigAware.kt	`66.66% <ø> (ø)`	`0.00 <0.00> (ø)`
...kotlin/io/gitlab/arturbosch/detekt/api/Findings.kt	`62.50% <0.00%> (-8.93%)`	`0.00 <0.00> (ø)`
...lin/io/github/detekt/report/xml/XmlOutputReport.kt	`90.90% <0.00%> (-5.25%)`	`6.00 <1.00> (-3.00)`
...otlin/io/gitlab/arturbosch/detekt/api/CodeSmell.kt	`59.57% <50.00%> (-0.90%)`	`13.00 <2.00> (+2.00)`	⬇️
...ain/kotlin/io/gitlab/arturbosch/detekt/api/Rule.kt	`83.33% <60.00%> (-16.67%)`	`16.00 <3.00> (+1.00)`	⬇️
...sch/detekt/core/config/ValidatableConfiguration.kt	`96.96% <100.00%> (+0.09%)`	`0.00 <0.00> (ø)`
...io/github/detekt/report/sarif/SarifOutputReport.kt	`100.00% <100.00%> (ø)`	`4.00 <0.00> (ø)`
...in/kotlin/io/gitlab/arturbosch/detekt/api/Issue.kt	`75.00% <0.00%> (-12.50%)`	`4.00% <0.00%> (-1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff655d2...afcab04. Read the comment docs.

chao2zhang · 2020-12-20T05:59:20Z

I also realized that the severity, for now, is only in XML + Sarif. We are not differentiating severities in HTML or console output. If this is a good idea, please react to this comment so I will open a new issue for supporting them.

For txt, the output should include the severity level.
For console, we should color-code each finding in addition to showing the severity.
For HTML, we should both display the severity and use a different background.

BraisGabin · 2020-12-20T09:21:34Z

I agree with all except html. I don't think that we should change all the background. We could use a label and only change that label color.

schalkms

Looks good!
Anyhow, a refactoring would be great in separate PR in order to implement the severity levels properly and keep the architecture tidy. Then there's no need for copying stuff around and no risk to introduce cyclic dependencies with future refactorings.

We can break such cycling dependency by refactoring, but I would prefer doing so in the next PR where we can remove the existing Severity and replace it with the new SeverityLevel.

schalkms · 2020-12-20T17:30:41Z

If this is a good idea, please react to this comment so I will open a new issue for supporting them.

For txt, the output should include the severity level.

For console, we should color-code each finding in addition to showing the severity.

For HTML, we should both display the severity and use a different background.

+1 great
I agree with this suggestion. I wouldn't want to build it overly complicated, so simplicity is the key here.

chao2zhang · 2020-12-20T18:04:36Z

Looks good!
Anyhow, a refactoring would be great in separate PR in order to implement the severity levels properly and keep the architecture tidy. Then there's no need for copying stuff around and no risk to introduce cyclic dependencies with future refactorings.

We can break such cycling dependency by refactoring, but I would prefer doing so in the next PR where we can remove the existing Severity and replace it with the new SeverityLevel.

#3127 prototyped a new Rule API, which is much cleaner, but also not backward compatible. In order to keep the severity within every Issue, it will be a breaking change and I would prefer to wait till Detekt 2.0 for making such changes.

I have seen Detekt 2.0 mentioned in a few issues. Do we know when and how we are planning this? I imagine there will be a separate 2.0 branch where we can merge all breaking changes.

schalkms · 2020-12-20T18:35:51Z

#3127 prototyped a new Rule API, which is much cleaner, but also not backward compatible. In order to keep the severity within every Issue, it will be a breaking change and I would prefer to wait till Detekt 2.0 for making such changes.

I have seen Detekt 2.0 mentioned in a few issues. Do we know when and how we are planning this? I imagine there will be a separate 2.0 branch where we can merge all breaking changes.

At the moment those PRs are tagged in the milestone field with the corresponding detekt version.
Nevertheless, with the number of those PRs growing I suggest to create a new v2.0 branch for simplicity reasons.

BraisGabin · 2020-12-20T20:29:48Z

We talk a lot about 2.0 but we don't have the list of things that we want to change. So I'm not sure about creating a new branch. #3127 pretends to be retrocompatible. I mean, the idea is to deprecate the old api and keep it until 2.0.

arturbosch

Implementation looks good!
I think this could be backwards compatible for 1.x, please see my two comments below.

I also added a comment to our 2.0 ticket - #2680 (comment)

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Rule.kt

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Findings.kt

BraisGabin

Great PR! I'm just a bit concerned about the compatibility.

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/CodeSmell.kt

chao2zhang · 2020-12-27T02:38:33Z

This PR also depends on #3320

detekt-report-sarif/src/test/kotlin/io/github/detekt/report/sarif/SarifOutputReportSpec.kt

#3310) * Support configurable severity per rule * Change default severity to warning and add default impl for Findings API * Add secondary constructor to maintain binary compatibility * Implement severity with backing properties with internal set * Revert some of the changes in CodeSmell * Fix detekt failure * Fix another detekt failure * Rebase onto master * Remove SeverityLevel.ignore * Dump API

schalkms approved these changes Dec 20, 2020

View reviewed changes

arturbosch requested changes Dec 21, 2020

View reviewed changes

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Rule.kt Show resolved Hide resolved

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Findings.kt Outdated Show resolved Hide resolved

chao2zhang force-pushed the severity branch from 8d03d9f to 0fa3206 Compare December 21, 2020 17:12

arturbosch added this to the 1.16.0 milestone Dec 21, 2020

arturbosch approved these changes Dec 21, 2020

View reviewed changes

BraisGabin reviewed Dec 21, 2020

View reviewed changes

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/CodeSmell.kt Outdated Show resolved Hide resolved

BraisGabin reviewed Dec 21, 2020

View reviewed changes

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/CodeSmell.kt Outdated Show resolved Hide resolved

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/CodeSmell.kt Outdated Show resolved Hide resolved

BraisGabin approved these changes Dec 21, 2020

View reviewed changes

This was referenced Dec 25, 2020

Apply binary compatibility plugin to Detekt #3320

Merged

Support relative output paths #3319

Merged

chao2zhang force-pushed the severity branch from 4eef8de to bf51683 Compare December 27, 2020 02:36

BraisGabin reviewed Dec 27, 2020

View reviewed changes

detekt-report-sarif/src/test/kotlin/io/github/detekt/report/sarif/SarifOutputReportSpec.kt Outdated Show resolved Hide resolved

Chao Zhang added 10 commits December 30, 2020 14:12

Support configurable severity per rule

9ed606d

Change default severity to warning and add default impl for Findings API

020315a

Add secondary constructor to maintain binary compatibility

5c96b1d

Implement severity with backing properties with internal set

eaf5828

Revert some of the changes in CodeSmell

0bf23f4

Fix detekt failure

d7381f8

Fix another detekt failure

a90d9fa

Rebase onto master

7a8ef8d

Remove SeverityLevel.ignore

d18a6b1

Dump API

afcab04

chao2zhang force-pushed the severity branch from 568dc98 to afcab04 Compare December 30, 2020 22:13

chao2zhang mentioned this pull request Dec 30, 2020

Reorganize docs for the configuration file #3337

Merged

arturbosch merged commit 8467796 into detekt:master Jan 4, 2021

chao2zhang deleted the severity branch January 6, 2021 16:38

chao2zhang mentioned this pull request Jan 8, 2021

Continuous improvement on SARIF support #3360

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support configurable severity per ruleset/rule in XML and Sarif output #3310

Support configurable severity per ruleset/rule in XML and Sarif output #3310

chao2zhang commented Dec 20, 2020

codecov bot commented Dec 20, 2020 •

edited

Loading

chao2zhang commented Dec 20, 2020

BraisGabin commented Dec 20, 2020

schalkms left a comment

schalkms commented Dec 20, 2020

chao2zhang commented Dec 20, 2020

schalkms commented Dec 20, 2020

BraisGabin commented Dec 20, 2020

arturbosch left a comment •

edited

Loading

BraisGabin left a comment

chao2zhang commented Dec 27, 2020

Support configurable severity per ruleset/rule in XML and Sarif output #3310

Support configurable severity per ruleset/rule in XML and Sarif output #3310

Conversation

chao2zhang commented Dec 20, 2020

Summary

Implementation

Followup

Test

codecov bot commented Dec 20, 2020 • edited Loading

Codecov Report

chao2zhang commented Dec 20, 2020

BraisGabin commented Dec 20, 2020

schalkms left a comment

Choose a reason for hiding this comment

schalkms commented Dec 20, 2020

chao2zhang commented Dec 20, 2020

schalkms commented Dec 20, 2020

BraisGabin commented Dec 20, 2020

arturbosch left a comment • edited Loading

Choose a reason for hiding this comment

BraisGabin left a comment

Choose a reason for hiding this comment

chao2zhang commented Dec 27, 2020

codecov bot commented Dec 20, 2020 •

edited

Loading

arturbosch left a comment •

edited

Loading