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

Emit fixes local to the result, not as an aggregate #5425

Merged
merged 14 commits into from
Jun 11, 2022

Conversation

kristof-mattei
Copy link
Contributor

@kristof-mattei kristof-mattei commented Jun 3, 2022

Fix for #5421

Testing https://github.com/returntocorp/semgrep/blob/5dbf6dd47ab4fda6e9dc8dec42b90fce24d1fd82/semgrep/tests/e2e/snapshots/test_output/test_sarif_output_with_autofix/results.sarif against https://github.com/oasis-tcs/sarif-spec/blob/master/Documents/CommitteeSpecifications/2.1.0/sarif-schema-2.1.0.json validates 100%.

I don't want to use that url though as the $schema in the files we generate, as that is not the spec.

PR checklist:

@CLAassistant
Copy link

CLAassistant commented Jun 3, 2022

CLA assistant check
All committers have signed the CLA.

@kristof-mattei kristof-mattei changed the title [WIP] Emit fixes local to the result, not as an aggregate to the whole sari… Emit fixes local to the result, not as an aggregate Jun 3, 2022
@kristof-mattei kristof-mattei marked this pull request as ready for review June 3, 2022 21:47
@ajbt200128 ajbt200128 self-requested a review June 3, 2022 21:52
semgrep/semgrep/formatter/sarif.py Outdated Show resolved Hide resolved
semgrep/semgrep/formatter/sarif.py Outdated Show resolved Hide resolved
semgrep/tox.ini Outdated Show resolved Hide resolved
@kristof-mattei
Copy link
Contributor Author

@ajbt200128
Copy link
Collaborator

@ajbt200128 trying with https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/schemas/sarif-schema-2.1.0.json, which is the latest version.

Great, if it works, make sure to regenerate the tests so the output contains the new schema url :)

@kristof-mattei
Copy link
Contributor Author

@ajbt200128 had to join text into a single string as the scheme doesn't allow for []. Ran tests, merged in changes. LMK.

@ajbt200128
Copy link
Collaborator

@ajbt200128 had to join text into a single string as the scheme doesn't allow for []. Ran tests, merged in changes. LMK.

Good catch, can you escape the newlines though instead of removing them? That'd be in accordant w/ the json specs. After that we should be good

@kristof-mattei
Copy link
Contributor Author

@ajbt200128 I didn't remove them. text is an array when it comes from semgrep, and then they are joined with \n:
https://github.com/returntocorp/semgrep/pull/5425/files#diff-9a6ebaf633a3e083e4a7792fb44df39b96d443dc8641055c8cabc5f50bf8f3baR75

@ajbt200128
Copy link
Collaborator

@kristof-mattei my bad, I misread, but either way, they should be joined with escaped new lines, as per the json spec

@kristof-mattei
Copy link
Contributor Author

@ajbt200128 they are escaped in the rendered output:

❯ python3
Python 3.10.4 (main, Mar 23 2022, 20:25:24) [GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import json
>>> foo = ["hello", "world"]
>>> bar = { "foo": "\n".join(foo) }
>>> bar
{'foo': 'hello\nworld'}
>>> json.dumps(bar)
'{"foo": "hello\\nworld"}'
>>>

@ajbt200128
Copy link
Collaborator

great!

Copy link
Collaborator

@ajbt200128 ajbt200128 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!

@aryx
Copy link
Collaborator

aryx commented Jun 9, 2022

@ajbt200128 can you push this on the finish line?

@michael-schwarz
Copy link

michael-schwarz commented Jun 10, 2022

It would be nice if this could be merged soon, it's quite annoying to have the CI jobs fail on every commit because of this. 😄

@aryx aryx merged commit 070591d into semgrep:develop Jun 11, 2022
@kristof-mattei kristof-mattei deleted the fix-fixes branch June 12, 2022 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants