fix: Migrate importer & workers off of Bug entities #4658
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 is a significant refactoring to migrate from using Bug entities to Vulnerability entities as the source of truth. The changes are extensive and touch the importer, worker, and recoverer components, as well as associated tests and datafix tools.
Key changes include:
- Removing
Bug-dependent logic from the importer and worker. - Updating the worker to operate directly on
Vulnerabilityentities and their corresponding protobufs in GCS. - Modifying the recoverer to trigger a forced re-import for missing GCS files instead of regenerating from
Bugentities. - Updating data models and helper functions to reflect the new data structure.
- Archiving old datafix tools that operated on
Bugentities.
The changes appear to be well-executed and consistent with the goal of the migration. The test suite has been updated to reflect the new implementation, which is great. My review focuses on a minor logging improvement for better debuggability.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
michaelkedar
requested review from
Ly-Joey,
another-rex,
cuixq and
jess-lowe
and removed request for
Ly-Joey and
cuixq
another-rex
left a comment
another-rex
left a comment
There was a problem hiding this comment.
basically LGTM first try!
Just confirming that we want to delete all affected information on withdraw?
| ds_vuln.is_withdrawn = True | ||
| ds_vuln.modified = proto_vuln.modified.ToDatetime(datetime.UTC) | ||
| osv.models.put_entities(ds_vuln, proto_vuln) | ||
| osv.update_affected_commits(vuln_id, [], False) |
There was a problem hiding this comment.
Should this be deleting all affected commits information?
It does not look like the existing implementation is deleting affected commits.
|
|
||
| # NB: Only OSS-Fuzz is editable - all other sources are read-only. | ||
| # This should not be reachable by this worker. | ||
| output_path = os.path.join(osv.repo_path(repo), path) |
There was a problem hiding this comment.
Should this add a warning log if it gets to this point.
| else: | ||
| to_put.append(ListedVulnerability.from_vulnerability(vuln_pb)) | ||
| new_affected = affected_from_proto(vuln_pb) | ||
| added, removed = diff_affected_versions(old_affected, new_affected) |
There was a problem hiding this comment.
Ah I see, all affected versions are deleted when a record is withdrawn.
2 participants
#3850
This change makes OSV finally stop writing new Bug entities, moving the source of truth to solely be the Vulnerability entity and proto in GCS (for non-oss-fuzz stuff).
old/directory for reference.Also added some missing type hints in some places.