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
Decompose safety.check into multiple functions to allow checking without fetching the database. #178
base: main
Are you sure you want to change the base?
Conversation
…out fetching the database.
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.
Incredible on how you were able to organize better the code and add a new interface function without changing the public interface. I still need to look this further to make sure we are not breaking anything, but looks good so far.
It would be awesome if you could rebase and check some of the suggested changes.
for cnd in candidates: | ||
for data in get_vulnerabilities(pkg=cnd.name, spec=cnd.spec, db=db_full): |
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.
To increase readability:
for current in candidates:
vulnerabilities = get_candidate_vulnerabilities(current)
for data in vulnerabilities:
vulnerable = [] | ||
for cnd in candidates: | ||
for data in get_vulnerabilities(pkg=cnd.name, spec=cnd.spec, db=db_full): | ||
vuln_id = data.get("id").replace("pyup.io-", "") |
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 for being faithful to original code. Can you fix this to cases where id
is not found here: data.get("id", "").replace("pyup.io-", "")
vulnerable.append( | ||
Vulnerability( | ||
name=cnd.name, | ||
spec=cnd.spec, | ||
version=cnd.version, | ||
advisory=data.get("advisory"), | ||
vuln_id=vuln_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.
advisory = data.get("advisory")
found_vulnerability = Vulnerability.from_candidate(advisory=advisory, vuln_id=vuln_id)
vulnerable.append(found_vulnerability)
return vulnerable | ||
|
||
|
||
def check(packages, key, db_mirror, cached, ignore_ids): |
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.
At least proxy
is missing here. Would need to rebase from master to check changes.
db = fetch_database(key=key, db=db_mirror, cached=cached) | ||
candidates = find_candidates(packages, db) | ||
if candidates: | ||
db_full = fetch_database(full=True, key=key, db=db_mirror) |
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.
Would need to pass cached
here as well.
I've tried to preserve existing behavior with this PR while also providing an interface that allows packages to be checked without fetching the database. This change allows a use case like: