-
Notifications
You must be signed in to change notification settings - Fork 273
feat: implement coarse versioning for future matching against #4590
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
base: master
Are you sure you want to change the base?
Conversation
|
/gemini review |
|
|
||
| # Matches RPM versions: optional epoch, alternating alphanumeric segments. | ||
| rpm_version_strategy = st.from_regex( | ||
| re.compile(r'^([0-9]+:)?(([0-9]+|[A-Za-z]+)((?![0-9A-Za-z])[ -~])*)+$', |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
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.
hypothesis uses the regex to generate patterns. It doesn't match with them afaik.
|
|
||
| # Matches RPM versions: optional epoch, alternating alphanumeric segments. | ||
| rpm_version_strategy = st.from_regex( | ||
| re.compile(r'^([0-9]+:)?(([0-9]+|[A-Za-z]+)((?![0-9A-Za-z])[ -~])*)+$', |
Check failure
Code scanning / CodeQL
Inefficient regular expression High
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.
hypothesis uses the regex to generate patterns. It doesn't match with them afaik.
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.
Code Review
This is a substantial and well-executed pull request that introduces a coarse versioning scheme to optimize database queries. The implementation is thorough, covering a wide range of ecosystems, and the addition of property-based testing with hypothesis is a fantastic way to ensure the correctness of the version ordering logic. The refactoring of _sort_key methods to raise ValueError on invalid versions is a solid design improvement. I've identified one potential issue in the Debian coarse versioning logic and a minor opportunity for optimization in osv/models.py. Overall, this is an excellent contribution.
| parts = re.findall(r'^$|\d+|\D+', v) | ||
| int_parts = [] | ||
| for couple in batched(parts, 2): | ||
| if not couple[0].isdecimal(): | ||
| # This is probably handled by is_valid | ||
| break | ||
| int_parts.append(int(couple[0])) | ||
| if len(couple) == 1: | ||
| break | ||
| sep = couple[1] | ||
| if sep == '.': | ||
| continue | ||
| if sep[0] == '.': | ||
| # Bigger than the max int, so we overflow | ||
| int_parts.append(9999999999) | ||
| break |
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.
The logic for parsing Debian versions in coarse_version appears to be incorrect for versions containing alphabetic characters. For an input like '10.20.a30', re.findall(r'^$|\d+|\D+', v) will produce ['10', '.', '20', '.', 'a', '30']. The loop processing these parts will break when it encounters the non-decimal part 'a'. This results in int_parts being [10, 20], which then produces a coarse version ending in .00000000.
However, the corresponding test expects a result ending in .99999999, implying that the alphabetic character should cause an 'overflow' to a maximal value for that component to preserve ordering. The current implementation doesn't seem to handle this case; it only considers special separators for overflow, not alphabetic components in the version string itself.
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.
For an input like
'10.20.a30',re.findall(r'^$|\d+|\D+', v)will produce['10', '.', '20', '.', 'a', '30'].
Um, no:
>>> re.findall(r'^$|\d+|\D+', '10.20.a30')
['10', '.', '20', '.a', '30']which would find its way to the int_parts.append(9999999999) line
| raise ValueError(f'Invalid version: {version}') | ||
| # is_valid uses a $ regex anchor (which can match a newline), | ||
| # so we need to remove the newline if one exists. | ||
| if version[-1] == '\n': |
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.
does $ also match \r\n? Might be worth it to call strip(), since I don't believe any versioning scheme have white spaces as part of the spec.
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.
does $ also match
\r\n?
No, at least not on Linux.
I would rather not call strip, just in case
osv/ecosystems/ecosystems_base.py
Outdated
| while len(components) < 3: | ||
| components.append(pad_value) | ||
|
|
||
| return f'00:{components[0]:08d}.{components[1]:08d}.{components[2]:08d}' |
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.
nit, let's make this construction a function that just takes in 3 int args. then we can replace 374, 337, and 300 with this func.
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've made coarse_version_from_ints be the generic construction function for this, and consolidated some of the logic into it.
osv/ecosystems/ecosystems_base.py
Outdated
| Args: | ||
| version: The version string to convert. | ||
| separators_regex: Regex for separators (default: r'[.]'). | ||
| trim_regex: Regex for characters to trim after (default: r'[-+]'). |
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.
rename to trim_suffix_regex?
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.
'suffix' is a bit misleading here, because it implies it'd match the whole end of the string rather than a point at which to truncate from.
I've renamed it to truncate_regex, which might be clearer?
osv/ecosystems/ecosystems_base.py
Outdated
| if not p.isdecimal(): | ||
| break | ||
| val = int(p) | ||
| if val > 99999999: |
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.
Create a const for MAX
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.
const'd
osv/ecosystems/alpine.py
Outdated
| separators_regex=r'[.]', | ||
| # in APK, 1.02.1 < 1.1.1, so we must treat everything after .0x as 0 | ||
| # also split off the _rc, _p, or -r suffixes | ||
| trim_regex=r'(?:\.0|[_-])', |
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.
does this also trim 1.0.1?
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.
Might need \.0\d
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.
It does trim 1.0.1, but that's intentional because 1.0.2 < 1.01.1 < 1.1.0
Will mention that in the comment.
| """ | ||
| # legacy versions are less than non-legacy versions, thus mapped to 0 | ||
| ver = packaging_legacy.version.parse(version) | ||
| if isinstance(ver, packaging_legacy.version.LegacyVersion): |
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.
Hmm how common are these, might it be worth it to bump all legacy version up by 1 so we can still have legacy versions? (probably not worth it unless it's really common)
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 don't think they're that common.
Plus, legacy versions can be arbitrary strings and I don't want to work out the comparison rules for them.
| # if not search: | ||
| # return False | ||
|
|
||
| # s = search.group(1) | ||
| # left, _, _ = s.partition(".") | ||
| # # handle the suffix case | ||
| # left, _, _ = left.partition("-") | ||
| # if not left.isdecimal(): | ||
| # return True | ||
| # i = int(left) | ||
| # return str(i) == left |
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.
Should this still be here?
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 kinda wanted to signpost the original third-party code that I've changed
osv/models.py
Outdated
|
|
||
| _MAX_GIT_VERSIONS_TO_INDEX = 5000 | ||
|
|
||
| MIN_COARSE_VERSION = '00:00000000.00000000.00000000' |
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.
this can use the helper function suggested in ecosystems.
osv/models.py
Outdated
|
|
||
| def affected_from_bug(entity: Bug) -> list[AffectedVersions]: | ||
| """Compute the AffectedVersions from a Bug entity.""" | ||
| def _get_coarse_min_max(events, e_helper, db_id): |
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.
nit: add types
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.
typified
|
|
||
| # Add the enumerated versions | ||
| # We need at least a package name to perform matching. | ||
| if pkg_name and affected.versions: |
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.
Should we be logging something if there is non pkg_name but got ranges or affected.versions?
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'm pretty sure this would be expected for GIT ranges, so probably not.
Implemented a string-comparable encoding of ecosystem versions to allow us to do filtering on the database queries.
This should help reduce the number of entities needed to fetch and compare in many ecosystems.
I've also slightly changed the behaviour of
_sort_key, which previously was returning a 'maximal' value for invalid versions. Now it should raise aValueError, and thesort_keywrapper (which was handling0values) converts those into maximal version objects.I've added
hypothesisto do some fuzzing tests to make sure the logic retains the ordering rules of the ecosystems. (Please let me know if after this is merged you run into more failure cases so I can fix them). Fuzzing also helped identify some potential uncaught errors in our version parsing (particularly, usingisdigitinstead ofisdecimal), which I've fixed where applicable.I need to write a script to populate the existing AffectedVersions entities with the newly generated values, then work on making the API use the coarse versions for querying.
We have some PRs open on adding new ecosystems - they don't necessarily need the coarse version method, but I'm happy to merge those first then add the code for this later.