-
Notifications
You must be signed in to change notification settings - Fork 363
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
Update the model to better match schema and add YAML tags. #417
Conversation
- YAML tags added to support YAML serialization - omitempty added to optional fields - Package changed to a pointer in Affected - Bitnami added as an ecosystem Signed-off-by: Caleb Brown <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks Caleb!
pkg/models/vulnerability.go
Outdated
Versions []string `json:"versions,omitempty"` | ||
DatabaseSpecific map[string]interface{} `json:"database_specific,omitempty"` | ||
EcosystemSpecific map[string]interface{} `json:"ecosystem_specific,omitempty"` | ||
Package *Package `json:"package,omitempty" yaml:"package,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a required change? It may break things subtlely for some users expecting copy semantics instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it back. It does change the output to always include the package, even if it has been omitted.
The workaround for this would be to re-add omitempty
to name
and ecosystem
in Package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public dependents appear to be only Guac and Scorecard.
Actually, there are more: https://github.com/search?q=osv-scanner%2Fpkg%2Fmodels&type=code
Most notably https://github.com/grafana/plugin-validator is the only code I can find using this library, and it doesn't reference this struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the question is whether this is important to get right early even if a few dependents break, or whether it should be done later with a v2 release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just maintain compatibility for now, and re-add omitempty to name
and ecosystem
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative would be to add a custom MarshalYAML/MarshalJSON that uses reflection to remove the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a bit of an overkill at this point. Is there much benefit to not having omitempty
on the name
and ecosystem
fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I may have a bit of an misunderstanding about how this all works in Go here. If we just revert back to what we had before (with omitempty
on the fields within the Package
), would we just end up with:
"package": {}
Or would package
itself be omitted too?
- Restores the Package type in Affected to avoid breaking other users. - Hides the Package field in JSON output. Signed-off-by: Caleb Brown <[email protected]>
Signed-off-by: Caleb Brown <[email protected]>
Signed-off-by: Caleb Brown <[email protected]>
Signed-off-by: Caleb Brown <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for fixing the serialization issue!!
This PR is primarily to support the replacement of the same structs in google/osv.dev/vulnfeeds/vulns.