feat: begin implementing ecosystem helpers in Go#4926
Hidden character warning
feat: begin implementing ecosystem helpers in Go#4926michaelkedar wants to merge 2 commits intogoogle:masterfrom
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request lays the groundwork for ecosystem helpers in Go by re-implementing version comparison logic, delegating to osv-scalibr. It introduces many new files for different ecosystems and a wrapper to handle special version "0" cases. The overall structure is solid. I've found a critical issue in the root ecosystem version parsing logic that could lead to incorrect version comparisons, and a recurring issue where unimplemented methods panic instead of returning an error. My detailed comments provide suggestions for fixes.
| if m := rootIOPlusRegexp.FindStringSubmatch(version); m != nil { | ||
| upstreamVersion = m[1] | ||
| rootPatch, err = strconv.Atoi(m[2]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| // Generic Format | ||
| if m := rootIOGenericRegexp.FindStringSubmatch(version); m != nil { | ||
| upstreamVersion = m[1] | ||
| rootPatch, err = strconv.Atoi(m[2]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| // Alpine Format | ||
| if m := rootAlpineRegexp.FindStringSubmatch(upstreamVersion); m != nil { | ||
| rootPatch, err = strconv.Atoi(m[2]) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
The version parsing logic for the Root ecosystem has a few issues that could lead to incorrect behavior:
- Lack of mutual exclusion: The
root.iosuffix parsers (rootIOPlusRegexpandrootIOGenericRegexp) are not in anif-else ifchain, meaning if a version string could match both, the second would overwrite the first. - Patch overwrite: The
rootAlpineRegexpmatch overwrites anyrootPatchvalue found fromroot.iosuffixes. This is likely a bug, as a version like1.2.3-r4+root.io.5would lose theroot.iopatch information. - Incomplete version stripping: The
rootAlpineRegexpdoes not strip the matched suffix (e.g.,-r4) fromupstreamVersion. This will cause parsing errors in sub-ecosystem parsers that don't expect this format, such asdpkgEcosystem.
I suggest chaining the regex checks with else if to establish precedence and fix the overwrite issue, and ensuring the suffix is stripped in all cases.
if m := rootIOPlusRegexp.FindStringSubmatch(version); m != nil {
upstreamVersion = m[1]
rootPatch, err = strconv.Atoi(m[2])
if err != nil {
return nil, err
}
} else if m := rootIOGenericRegexp.FindStringSubmatch(version); m != nil {
upstreamVersion = m[1]
rootPatch, err = strconv.Atoi(m[2])
if err != nil {
return nil, err
}
} else if m := rootAlpineRegexp.FindStringSubmatch(version); m != nil {
upstreamVersion = m[1]
rootPatch, err = strconv.Atoi(m[2])
if err != nil {
return nil, err
}
}There was a problem hiding this comment.
This is exactly what the python implementation was doing:
osv.dev/osv/ecosystems/root.py
Lines 56 to 73 in 0439c0d
There was a problem hiding this comment.
Maybe add a TODO here to keep track of it? Having "if else" probably is a good idea.
| func (e bioconductorEcosystem) GetVersions(_ string) ([]string, error) { | ||
| panic("not yet implemented") | ||
| } |
There was a problem hiding this comment.
Using panic for unimplemented functions can cause the entire application to crash, which is not ideal for library code. It's better to return an error, for example errors.New("not yet implemented"). You will need to add import "errors" to the file.
This comment applies to all other GetVersions methods in this pull request that currently use panic.
| func (e bioconductorEcosystem) GetVersions(_ string) ([]string, error) { | |
| panic("not yet implemented") | |
| } | |
| func (e bioconductorEcosystem) GetVersions(_ string) ([]string, error) { | |
| return nil, errors.New("not yet implemented") | |
| } |
There was a problem hiding this comment.
This aren't going to be used before these are implemented.
2 participants
Setting up the groundwork for re-implementing the python ecosystems stuff (so we can migrate the worker and API)
Right now, this is just the version comparison stuff, without coarse versions and enumeration. It also just directly delegates to the osv-scalibr (née osv-scanner) implementation of them.
The only interesting one is Root, which is not (currently?) covered by the semantic package.
I don't know if I should be porting the python tests over for comparison logic - this stuff should already be tested in scalibr, and I'm barely adding extra logic on top (and the extra logic of "0" handling I have added tests for)