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
| @@ -187,7 +187,7 @@ def _load_advisories( | |||
| impacted_package_urls=[], | |||
| 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
| @@ -209,7 +200,7 @@ def batch_advisories(self, advisories: List[Advisory]) -> Set[Advisory]: | |||
|
|
|||
| 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.
| @@ -78,19 +78,10 @@ class Advisory: | |||
| """ | |||
|
|
|||
| 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 |
There was a problem hiding this comment.
See below using attrgetter
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>
Signed-off-by: Shivam Sandbhor shivam.sandbhor@gmail.com
Fixes #334