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

Add last_affected event type. #38

Merged
merged 3 commits into from
Mar 24, 2022
Merged

Add last_affected event type. #38

merged 3 commits into from
Mar 24, 2022

Conversation

oliverchang
Copy link
Contributor

Part of #35.

Copy link
Collaborator

@chrisbloom7 chrisbloom7 left a comment

Choose a reason for hiding this comment

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

LGTM! I left a couple suggestions that aren't blockers

docs/schema.md Outdated Show resolved Hide resolved
}
},
"required": [
"last_affected"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also add validation to assert that a fixed event cannot occur alongside a last_effected event and vice versa?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that's a good idea! I'll look at doing this.

That said, I'm wondering how this would look like for GHSA specifically when an entry wants to encode both <= and < (the patched version)? Would you just be using "fixed" here to capture the < and then database_specific to capture the <= ?

Copy link
Collaborator

@chrisbloom7 chrisbloom7 Mar 18, 2022

Choose a reason for hiding this comment

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

Currently if an advisory has a fix we replace the upper bound with < f.i.x when we rehydrate and consider it an acceptable change. It's only for advisories that don't have a fix that we need to add the rehydration hints.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And same thing for the other direction when transforming to OSV - if an advisory has a fix, we add the fixed event and don't add the rehydration hint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also added the JSON validation.

Copy link

@rsc rsc left a comment

Choose a reason for hiding this comment

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

Looks great, thanks.

@oliverchang oliverchang merged commit ef3f219 into next Mar 24, 2022
@oliverchang oliverchang deleted the last_affected branch March 24, 2022 02:27
oliverchang added a commit that referenced this pull request Mar 28, 2022
* Add `last_affected` event type. (#38)

* Add `last_affected` event type.

Part of #35.

* Update docs/schema.md

Co-authored-by: Chris Bloom <[email protected]>

* JSON validation

Co-authored-by: Chris Bloom <[email protected]>

* Add database_specific to `affected[].ranges[]`. (#37)

* Add database_specific to `affected[].ranges[]`.

This is intended only for metadata that enables databases to losslessly
convert OSV entries back into their original representation.

Part of #35.

* Update docs/schema.md

Co-authored-by: Chris Bloom <[email protected]>

Co-authored-by: Chris Bloom <[email protected]>

* Bump version and add change log.

Co-authored-by: Chris Bloom <[email protected]>
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.

3 participants