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

Support sarif as a report type - #3045 #3132

Merged
merged 6 commits into from
Nov 16, 2020
Merged

Support sarif as a report type - #3045 #3132

merged 6 commits into from
Nov 16, 2020

Conversation

arturbosch
Copy link
Member

@arturbosch arturbosch commented Oct 6, 2020

New optional report format which can be plugged via --plugins detekt-report-sarif.jar:

Output preview via the VSCode extension:
2020-10-06-153635_927x499_scrot
2020-10-06-153629_932x514_scrot

CI will fail due to not a published version of sarif4j. We need to wait until #3045 gets an answer from the sarif authors.

@schalkms
Copy link
Member

I just skimmed through the code change list. Hence, I only have some general questions.
Is there a roadmap for the Java support? I'm not sure whether reinventing the wheel by implementing the SARIF support in Kotlin is the right approach here?

@arturbosch arturbosch mentioned this pull request Oct 17, 2020
@arturbosch arturbosch marked this pull request as ready for review October 17, 2020 19:23
@arturbosch
Copy link
Member Author

arturbosch commented Oct 17, 2020

I just skimmed through the code change list. Hence, I only have some general questions.
Is there a roadmap for the Java support? I'm not sure whether reinventing the wheel by implementing the SARIF support in Kotlin is the right approach here?

When the official MS java support will be out, we can and should migrate to it.
We do not reinvent any wheels here, just generating POJOs :).
Please also see my comment in #3045 (comment).

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Good to know. Thanks for the explanation. I really appreciate your work for supporting this feature. For me this PR looks fine.

@arturbosch arturbosch added this to the 1.15.0 milestone Oct 19, 2020
Run().apply {
tool = tool {
driver = component {
guid = "022ca8c2-f6a2-4c95-b107-bb72c43263f3"
Copy link
Member

Choose a reason for hiding this comment

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

is this randomly generated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I generated it randomly once with UUID.getXXX().
As I've understand it, the guid needs to be the same between all detekt 1.x.y runs.
Correct me if I'm wrong here @michaelcfanning

@michaelcfanning
Copy link

@arturbosch, just checking in on this, as there's strong interest internally at Microsoft for consuming this new report format. Any idea when it might be merged/available?

@arturbosch
Copy link
Member Author

arturbosch commented Nov 12, 2020

@arturbosch, just checking in on this, as there's strong interest internally at Microsoft for consuming this new report format. Any idea when it might be merged/available?

Hey @michaelcfanning , thanks for interest and the ping.
I've worked on this further locally. I have one last question regarding default -1 values in the scheme which should not appear in the report. I will keep you updated in #3045 later this week.
There are so many good rule improvements in the main branch so I really should get up to speed to finish this and release 1.15.0 xD.

@michaelcfanning
Copy link

@arturbosch, glad to help w/any SARIF advice. The large # of optional properties/defaults is definitely a concern depending on your JSON mechanism. The C# SARIF SDK uses a library (Newtonsoft JSON.NET) that automatically elides/hydrates these on save/open. We autogenerated the C# OM that has appropriate attributes for this by writing a custom tool, 'JsonSchema.ToDotNet' that parses the SARIF JSON schema. We could consider a similar trick in your scenario, if your JSON serializer supports similar features.

I took Larry's suggestion and have opened an issue to write a validator that flags all unnecessarily rendered default values. There's a lot of surface here to examine...

Something to keep in mind, though: SARIF which contains this data is valid. You might consider hard-coding eliminating obvious things for now, while considering a more systematic/comprehensive solution.

@codecov
Copy link

codecov bot commented Nov 15, 2020

Codecov Report

Merging #3132 (d6bc64c) into master (d309424) will decrease coverage by 0.01%.
The diff coverage is 78.26%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3132      +/-   ##
============================================
- Coverage     80.09%   80.07%   -0.02%     
- Complexity     2648     2651       +3     
============================================
  Files           440      443       +3     
  Lines          8013     8082      +69     
  Branches       1528     1536       +8     
============================================
+ Hits           6418     6472      +54     
- Misses          775      789      +14     
- Partials        820      821       +1     
Impacted Files Coverage Δ Complexity Δ
...n/io/github/detekt/report/sarif/RuleDescriptors.kt 21.05% <21.05%> (ø) 0.00 <0.00> (?)
...n/kotlin/io/github/detekt/report/sarif/SarifDsl.kt 100.00% <100.00%> (ø) 0.00 <0.00> (?)
...io/github/detekt/report/sarif/SarifOutputReport.kt 100.00% <100.00%> (ø) 3.00 <3.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 d309424...d6bc64c. Read the comment docs.

@arturbosch
Copy link
Member Author

arturbosch commented Nov 16, 2020

@arturbosch, glad to help w/any SARIF advice. The large # of optional properties/defaults is definitely a concern depending on your JSON mechanism. The C# SARIF SDK uses a library (Newtonsoft JSON.NET) that automatically elides/hydrates these on save/open. We autogenerated the C# OM that has appropriate attributes for this by writing a custom tool, 'JsonSchema.ToDotNet' that parses the SARIF JSON schema. We could consider a similar trick in your scenario, if your JSON serializer supports similar features.

I took Larry's suggestion and have opened an issue to write a validator that flags all unnecessarily rendered default values. There's a lot of surface here to examine...

Something to keep in mind, though: SARIF which contains this data is valid. You might consider hard-coding eliminating obvious things for now, while considering a more systematic/comprehensive solution.

@michaelcfanning thanks for the advice.
I've switched to Jackson which allowed to preserve the order of properties and eliminate default values from the report.

@arturbosch arturbosch merged commit 415ca77 into master Nov 16, 2020
@arturbosch arturbosch deleted the sarif-support branch November 16, 2020 16:38
arturbosch added a commit that referenced this pull request Dec 21, 2020
* Support sarif as a report type - #3045

* Integrate sarif feedback from @lgolding

* Test the whole sarif report instead of some json paths

* Provide only the short description; we do not have access to the long one programmatically

* Use the plain rule id as sarif rule name; the rule set id is encoded in the sarif rule id

* Remove need for casting by using a when expression
arturbosch added a commit that referenced this pull request Jan 16, 2021
* Support sarif as a report type - #3045

* Integrate sarif feedback from @lgolding

* Test the whole sarif report instead of some json paths

* Provide only the short description; we do not have access to the long one programmatically

* Use the plain rule id as sarif rule name; the rule set id is encoded in the sarif rule id

* Remove need for casting by using a when expression
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.

5 participants