-
-
Notifications
You must be signed in to change notification settings - Fork 264
Refactor codebase and tests to treat Advisory class mutable #363
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,13 +47,13 @@ | |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @dataclasses.dataclass | ||
| @dataclasses.dataclass(order=True) | ||
| class VulnerabilitySeverity: | ||
| system: ScoringSystem | ||
| value: str | ||
|
|
||
|
|
||
| @dataclasses.dataclass | ||
| @dataclasses.dataclass(order=True) | ||
| class Reference: | ||
|
|
||
| reference_id: str = "" | ||
|
|
@@ -64,8 +64,16 @@ def __post_init__(self): | |
| if not any([self.url, self.reference_id]): | ||
| raise TypeError | ||
|
|
||
| def normalized(self): | ||
| severities = sorted(self.severities) | ||
| return Reference( | ||
| reference_id=self.reference_id, | ||
| url=self.url, | ||
| severities=severities | ||
| ) | ||
|
|
||
| @dataclasses.dataclass | ||
|
|
||
| @dataclasses.dataclass(order=True) | ||
| class Advisory: | ||
| """ | ||
| This data class expresses the contract between data sources and the import runner. | ||
|
|
@@ -78,19 +86,27 @@ class Advisory: | |
| """ | ||
|
|
||
| summary: str | ||
| impacted_package_urls: Iterable[PackageURL] | ||
| vulnerability_id: Optional[str] = None | ||
| impacted_package_urls: Iterable[PackageURL] = dataclasses.field(default_factory=list) | ||
| resolved_package_urls: Iterable[PackageURL] = dataclasses.field(default_factory=list) | ||
| vuln_references: List[Reference] = dataclasses.field(default_factory=list) | ||
| vulnerability_id: Optional[str] = None | ||
|
|
||
| def __hash__(self): | ||
| s = "{}{}{}{}".format( | ||
| self.summary, | ||
| ''.join(sorted([str(p) for p in self.impacted_package_urls])), | ||
| ''.join(sorted([str(p) for p in self.resolved_package_urls])), | ||
| self.vulnerability_id, | ||
| def normalized(self): | ||
| impacted_package_urls = {package_url for package_url in self.impacted_package_urls} | ||
| resolved_package_urls = {package_url for package_url in self.resolved_package_urls} | ||
| vuln_references = sorted( | ||
| self.vuln_references, key=lambda reference: (reference.reference_id, reference.url) | ||
| ) | ||
| for index, _ in enumerate(self.vuln_references): | ||
| vuln_references[index] = (vuln_references[index].normalized()) | ||
|
|
||
| return Advisory( | ||
| summary=self.summary, | ||
| vulnerability_id=self.vulnerability_id, | ||
| impacted_package_urls=impacted_package_urls, | ||
| resolved_package_urls=resolved_package_urls, | ||
| vuln_references=vuln_references, | ||
| ) | ||
| return hash(s) | ||
|
|
||
|
|
||
| class InvalidConfigurationError(Exception): | ||
|
|
@@ -205,11 +221,15 @@ def batch_advisories(self, advisories: List[Advisory]) -> Set[Advisory]: | |
| """ | ||
| Yield batches of the passed in list of advisories. | ||
| """ | ||
| advisories = advisories[:] # copy the list as we are mutating it in the loop below | ||
|
|
||
| # TODO make this less cryptic and efficient | ||
|
|
||
| advisories = advisories[:] | ||
| # copy the list as we are mutating it in the loop below | ||
|
|
||
| while advisories: | ||
| b, advisories = advisories[: self.batch_size], advisories[self.batch_size:] | ||
| yield set(b) | ||
| yield b | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add TODO or ticket to revisit this cryptic code
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also do not use incorrect type hints as in |
||
|
|
||
|
|
||
| @dataclasses.dataclass | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -181,13 +181,14 @@ def _load_advisories( | |
| ) | ||
| ) | ||
|
|
||
| # TODO: Handle the CVE-????-????? case | ||
| advisories.append( | ||
| Advisory( | ||
| summary="", | ||
| 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 "", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add TODO or ticket to revisit this cryptic code |
||
| ) | ||
| ) | ||
|
|
||
|
|
||
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.
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