Refactor codebase and tests to treat Advisory class mutable #363
Conversation
Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
sbs2001
force-pushed
the
remove_advisory_hash_method
branch
from
697e97e to
321f0c3
Compare
pombredanne
left a comment
pombredanne
left a comment
There was a problem hiding this comment.
Looking good, see a few nits for your consideration. And merge!
| resolved_package_urls=resolved_purls, | ||
| vuln_references=references, | ||
| vulnerability_id=vuln_ids[0] if vuln_ids[0] != "CVE-????-?????" else None, | ||
| vulnerability_id=vuln_ids[0] if vuln_ids[0] != "CVE-????-?????" else "", |
There was a problem hiding this comment.
Add TODO or ticket to revisit this cryptic code
| while advisories: | ||
| b, advisories = advisories[: self.batch_size], advisories[self.batch_size:] | ||
| yield set(b) | ||
| yield b |
There was a problem hiding this comment.
Add TODO or ticket to revisit this cryptic code
There was a problem hiding this comment.
Also do not use incorrect type hints as in Set[Advisory]... nothing is better than wrong.
| summary: str | ||
| impacted_package_urls: Iterable[PackageURL] | ||
| vulnerability_id: Optional[str] = None | ||
| impacted_package_urls: Iterable[PackageURL] = dataclasses.field(default_factory=list) |
There was a problem hiding this comment.
IMHO you should have a way to compare and sort Advisory.
This would make the tests super simpler. See https://docs.python.org/3/library/functools.html?highlight=functools#functools.total_ordering
vulnerabilities/tests/utils.py
Outdated
| return advisory.vulnerability_id | ||
|
|
||
|
|
||
| def advisories_are_equal(expected_advisories, found_advisories): |
There was a problem hiding this comment.
See above. You should implmenent https://docs.python.org/3/library/functools.html?highlight=functools#functools.total_ordering for an Advisory for simpler and clearer code.
With that, the advisories_are_equal would become:
sorted(expected_advisories) == sorted(found_advisories)
and could be removed
vulnerabilities/tests/utils.py
Outdated
| from vulnerabilities.data_source import Reference | ||
|
|
||
|
|
||
| def normalized_reference(reference): |
There was a problem hiding this comment.
IMHO make this a method of Reference
vulnerabilities/tests/utils.py
Outdated
| return Reference(reference_id=reference.reference_id, url=reference.url, severities=severities) | ||
|
|
||
|
|
||
| def normalized_advisory(advisory): |
There was a problem hiding this comment.
IMHO move as a method of Advisory
vulnerabilities/tests/utils.py
Outdated
|
|
||
| # This is not entirely correct, but enough for testing purpose. | ||
| def advisory_sort_key(advisory): | ||
| return advisory.vulnerability_id |
vulnerabilities/tests/utils.py
Outdated
| def advisories_are_equal(expected_advisories, found_advisories): | ||
|
|
||
| expected_advisories = list(filter(lambda x: isinstance(x, Advisory), expected_advisories)) | ||
| expected_advisories.sort(key=advisory_sort_key) |
There was a problem hiding this comment.
Using from operator import attrgetter you could use:
| expected_advisories.sort(key=advisory_sort_key) | |
| expected_advisories.sort(key=attrgetter('vulnerability_id`)) |
Or...
| expected_advisories.sort(key=advisory_sort_key) | |
| expected_advisories.sort(key=lambda advisory: advisory.vulnerability_id) |
But IMHO it is better to have that in the Advisory class as suggested here.
Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
2 participants
Signed-off-by: Shivam Sandbhor shivam.sandbhor@gmail.com
Fixes #334