Bundle new sarif output format by default

[chao2zhang]

It is great to see that Detekt is adding support for SARIF #3045. As I was testing sarif with Github code scanning, I found that there is a decent amount of working code but we cannot generate SARIF output yet.

This PR should be the final step to fully integrate with SARIF. I tested locally in this repo and verified that the generated SARIF output passed the validation.

I tried my best to update all places where report types appear but I may still miss a few.
This PR can also wait if we are planning to roll out SARIF in future versions of Detekt.

[codecov]

Codecov Report

Merging #3268 (6820cc6) into master (b3245be) will decrease coverage by 0.06%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##             master    #3268      +/-   ##
============================================
- Coverage     80.07%   80.01%   -0.07%     
- Complexity     2653     2655       +2     
============================================
  Files           443      443              
  Lines          8096     8106      +10     
  Branches       1540     1541       +1     
============================================
+ Hits           6483     6486       +3     
- Misses          789      795       +6     
- Partials        824      825       +1     

Impacted Files	Coverage Δ	Complexity Δ
.../kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt	100.00% <ø> (ø)	3.00 <0.00> (ø)
...tlab/arturbosch/detekt/core/reporting/Reporting.kt	94.11% <0.00%> (-2.86%)	0.00 <0.00> (ø)
.../main/kotlin/io/gitlab/arturbosch/detekt/Detekt.kt	28.08% <0.00%> (-0.98%)	18.00 <0.00> (ø)
.../io/gitlab/arturbosch/detekt/internal/DetektJvm.kt	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
...tlab/arturbosch/detekt/extensions/DetektReports.kt	35.29% <33.33%> (-0.43%)	5.00 <1.00> (+1.00)	⬇️
...b/arturbosch/detekt/extensions/DetektReportType.kt	83.33% <100.00%> (+3.33%)	2.00 <0.00> (ø)
...gitlab/arturbosch/detekt/internal/DetektAndroid.kt	71.42% <100.00%> (+0.46%)	12.00 <0.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 b3245be...5f9488c. Read the comment docs.

[cortinico]

The change looks good to me.

This PR can also wait if we are planning to roll out SARIF in future versions of Detekt.

@arturbosch has most context on the Sarif front. I'd rather wait for a review from him

[BraisGabin]

I think that artur added this as a first party plugin and not as a core feature. But I have no idea about the reason. But I agree that we need documentation about this feature in both cases.

[chao2zhang]

It would also be nice if we can run Github checks to display the detekt violations embedded in the code panel of any PR, don't you agree :)
Eventually when most analysis tool have adopted SARIF, having SARIF as the core support definitely makes more sense.

I am doing this for my company since we are recently moving to Github to host our code.

[schalkms]

The changes look good! Thank you for trying out and testing the SARIF support of detekt! 🙂
Please let me reply to the questions.

I think that artur added this as a first party plugin and not as a core feature. But I have no idea about the reason. But I agree that we need documentation about this feature in both cases.

The idea behind the feature was that users can try it out with --plugins detekt-report-sarif.jar. Please consider SARIF support as beta state at the moment.
Please also take a look at issue #3132. Some details have been discussed there.

It would also be nice if we can run Github checks to display the detekt violations embedded in the code panel of any PR, don't you agree :)
Eventually when most analysis tool have adopted SARIF, having SARIF as the core support definitely makes more sense.

I completely agree. This is planned to be part of detekt's core.

[chao2zhang]

The changes look good! Thank you for trying out and testing the SARIF support of detekt! 🙂
Please let me reply to the questions.

I think that artur added this as a first party plugin and not as a core feature. But I have no idea about the reason. But I agree that we need documentation about this feature in both cases.

The idea behind the feature was that users can try it out with --plugins detekt-report-sarif.jar. Please consider SARIF support as beta state at the moment.
Please also take a look at issue #3132. Some details have been discussed there.

It would also be nice if we can run Github checks to display the detekt violations embedded in the code panel of any PR, don't you agree :)
Eventually when most analysis tool have adopted SARIF, having SARIF as the core support definitely makes more sense.

I completely agree. This is planned to be part of detekt's core.

Thank you for the response.
I publish this PR because I do want to start testing SARIF output with Github checks for my repo. And fix any bugs that may come. For example, I see level = Result.Level.WARNING where I do want to make output severity levels configurable. This would let Github checks to block PR merge if there are any detekt violations.

[chao2zhang]

@schalkms To keep it as "beta" or experimental, do you think it is a good idea set to enabled = false by default unless the user turned it on specifically?

[schalkms]

I publish this PR because I do want to start testing SARIF output with Github checks for my repo. And fix any bugs that may come. For example, I see level = Result.Level.WARNING where I do want to make output severity levels configurable. This would let Github checks to block PR merge if there are any detekt violations.

Yep, that's one of the common use cases. 🙂

@schalkms To keep it as "beta" or experimental, do you think it is a good idea set to enabled = false by default unless the user turned it on specifically?

I don't have a strong preference here. Having it disabled for now is fine, as you have done.

[chao2zhang]

It looks like this PR is waiting for a review from @arturbosch, is that correct?

[schalkms]

@chao2zhang yes, indeed.

[arturbosch]

Code looks good, thx!

Yes, @BraisGabin is right, I intentionally did not bundle the plugin in core as this adds two more dependencies with 2.2mb for all users.

Adding plugins is as easy as specifing --plugins or detektPlugins("..."detekt-sarif:...").
But I also see the point in having it bundled for convenience.
I'm also okay with bundling it now if nobody of the maintainers is against it. We can release a RC2 with this PR and see if everything is alright.

[schalkms]

I'm also okay with bundling it now if nobody of the maintainers is against it.

+1

[chao2zhang]

I believe with the future SARIF adoption and popularity, this argument of putting it into core should be stronger.

Usually, the app size matters more, and detekt is not bundled into the artifact of an application. But I do agree that we should be mindful of network usage and disk usage. For network with pay-by-usage, or big companies where they have their own clone of the artifactory, this is a good concern. Fun fact: I have been running out of disk space almost once a month on my laptop.

I created this PR to softly unblocks me from implementing #3274. This PR is not a hard blocker.

[BraisGabin]

I'm also okay with bundling it now if nobody of the maintainers is against it.

+1. I don't get why we should sort the output of a json. It goes against the format. If we could ignore that we could use moshi instead of jackson and reduce the bunlde size a lot.

[arturbosch]

I'm also okay with bundling it now if nobody of the maintainers is against it.

+1. I don't get why we should sort the output of a json. It goes against the format. If we could ignore that we could use moshi instead of jackson and reduce the bunlde size a lot.

Image big projects with a lot of issues -> large json files.
Sniffing/streaming the output for the right version is the reason mentioned by the sarif guys.
The sarif report is also not only about dumping issue data but can be used to interlink different reports and even create issue baselines.
I used moshi at the start but missed the possibility to exclude default values based on the json scheme.
This is also the prefered way to reduce the report size. The sarif report specifies much more additional stuff than just our findings :).

[chao2zhang]

Well technically we don't need to bundle with any json encoding libraries. I just realized that the SARIF reporter by android lint is handcrafted json https://cs.android.com/android-studio/platform/tools/base/+/mirror-goog-studio-master-dev:lint/cli/src/main/java/com/android/tools/lint/SarifReporter.kt