-
Notifications
You must be signed in to change notification settings - Fork 194
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
feat: add support for an 'upstream' field in the models #3172
base: master
Are you sure you want to change the base?
Conversation
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.
Some minor code comments here, will discuss the rest in the design doc.
osv/models.py
Outdated
@@ -762,7 +780,9 @@ def to_vulnerability(self, include_source=False, include_alias=True): | |||
def to_vulnerability_async(self, include_source=False): |
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.
Do we want to add include_upstream and include_alias to the this function
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.
LGTM!!!
osv/models.py
Outdated
@@ -259,6 +259,8 @@ class Bug(ndb.Model): | |||
aliases: list[str] = ndb.StringProperty(repeated=True) | |||
# Related IDs. | |||
related: list[str] = ndb.StringProperty(repeated=True) | |||
# Upstream IDs. | |||
upstream: list[str] = ndb.StringProperty(repeated=True) |
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.
Please rename this to upstream_raw as described in the doc.
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.
Can you also expand the docstring above this to explain that this is upstream ids directly from the import and does not include any transitive upstreams.
osv/models.py
Outdated
@@ -905,6 +934,16 @@ class AliasDenyListEntry(ndb.Model): | |||
bug_id: str = ndb.StringProperty() | |||
|
|||
|
|||
class UpstreamGroup(ndb.Model): | |||
"""Upstream group for storing transitive upstreams of a Bug""" |
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.
Can you expand the description here to explain why this is in a separate table rather than directly on the bug entry itself?
I.e. to prevent additional race conditions, by having it in a separate table only worker will modify Bug's directly.
This PR aims to prepare the models.py file to support the 'upstream' field to be introduced by ossf/osv-schema#312. Part of addressing #3052
This will calculate all of the transitive upstream dependencies of a given CVE. Hierarchy will be calculated and determined by the frontend in a later PR.