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

fix: ensure the sarif output has a stable order #938

Merged
merged 6 commits into from
May 3, 2024

Conversation

G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Apr 23, 2024

This stabilizes the order in the SARIF output so that it is deterministic, as #937 proved it was not.

Comment on lines +25 to +30
for _, fn := range []func() int{
func() int { return strings.Compare(a.Source.Path, b.Source.Path) },
func() int { return strings.Compare(a.Package.Name, b.Package.Name) },
func() int { return strings.Compare(a.Package.Version, b.Package.Version) },
} {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm super on the fence about this: on the one hand its clever, but on the other maybe it's too clever?

I've got a commit locally that unwinds this into if statements which I can push up if folks agree it's too clever

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's easier to parse than if statements once you understand what it's doing, can you just add a comment above it basically describing it's just sorting by each field in descending priority.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sweet done - one other annoying downside though is that we can't cover the return 0 😞

Copy link
Collaborator Author

@G-Rath G-Rath May 1, 2024

Choose a reason for hiding this comment

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

Ok turns out there's a cmp.Or for exactly this - it's technically less efficient because it's not lazy (though there's a proposal for that), but these compares should be pretty inexpensive and its nice to remove the unreachable code path nwm its go 1.22 only

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +25 to +30
for _, fn := range []func() int{
func() int { return strings.Compare(a.Source.Path, b.Source.Path) },
func() int { return strings.Compare(a.Package.Name, b.Package.Name) },
func() int { return strings.Compare(a.Package.Version, b.Package.Version) },
} {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's easier to parse than if statements once you understand what it's doing, can you just add a comment above it basically describing it's just sorting by each field in descending priority.

@G-Rath G-Rath force-pushed the fix-sarif branch 3 times, most recently from 6e3020f to 35ea85d Compare May 1, 2024 00:00
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2024

Codecov Report

Attention: Patch coverage is 38.88889% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 63.57%. Comparing base (559d0c9) to head (c4ffa47).
Report is 6 commits behind head on main.

Files Patch % Lines
internal/output/result.go 26.66% 10 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #938      +/-   ##
==========================================
- Coverage   63.58%   63.57%   -0.02%     
==========================================
  Files         146      146              
  Lines       11922    11933      +11     
==========================================
+ Hits         7581     7586       +5     
- Misses       3875     3881       +6     
  Partials      466      466              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@G-Rath
Copy link
Collaborator Author

G-Rath commented May 1, 2024

The coverage drop is fine - we were not covering the original code anyway, and it will be covered by #937, after which just the final return 0 will be uncovered as I can't seem to hit that :/

@oliverchang oliverchang merged commit c2bd45e into google:main May 3, 2024
13 checks passed
@G-Rath G-Rath deleted the fix-sarif branch May 3, 2024 03:32
another-rex pushed a commit that referenced this pull request May 6, 2024
These got included in #938 even though they're for #937 and it seems
that `go-snaps` does not error about this even though it will clean them
up if run with `UPDATE_SNAPS=clean`.
josieang pushed a commit to josieang/osv-scanner that referenced this pull request Jun 6, 2024
This stabilizes the order in the SARIF output so that it is
deterministic, as google#937 proved it was not.
josieang pushed a commit to josieang/osv-scanner that referenced this pull request Jun 6, 2024
These got included in google#938 even though they're for google#937 and it seems
that `go-snaps` does not error about this even though it will clean them
up if run with `UPDATE_SNAPS=clean`.
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.

4 participants