Refactor ImpactedPackage and ResolvedPackage by sbs2001 · Pull Request #219 · aboutcode-org/vulnerablecode

sbs2001 · 2020-06-28T08:16:48Z

Changes are made in import_runner.py to refactor the models.
Other major changes in import_runner.py is to change the _process_updated_advisories
method to use less db queries and increase performance by several times.

Having a single table for these, fixes the following:

Issue 1 :

You can do this, which doesn't make any sense.

In [1]: from vulnerabilities import models                                                                                   

In [2]: v1 = models.Vulnerability.objects.create(cve_id="CVE-foo")                                                           

In [3]: p1 = models.Package.objects.create(name="cream",type="ice",version='mango') 
   
In [4]: vp1 = models.ImpactedPackage.objects.create(vulnerability=v1, package=p1)                                            

In [5]: vp2 = models.ResolvedPackage.objects.create(vulnerability=v1, package=p1)

This is pure garbage, nothing can be interpreted from these entries.

With a single table + flag, we can use a unique_together=('vulnerability','package')

Issue 2 :

Updating the vulnerability status of a package is not possible due to cascade deletes.

Check https://github.com/nexB/vulnerablecode/blob/58d0376e7319d06387662cb393f3c39d9893088d/vulnerabilities/import_runner.py#L121 .

And also check for more details https://github.com/nexB/vulnerablecode/blob/58d0376e7319d06387662cb393f3c39d9893088d/vulnerabilities/tests/test_import_runner.py#L201

Signed-off-by: Shivam Sandbhor shivam.sandbhor@gmail.com

sbs2001 · 2020-06-30T11:31:42Z

@haikoschol I have pushed a commit to stop using ignore_conflicts=True and properties and magic methods. It's still faster than the current implementation, but not as fast as the one with properties.

I think I can acheive the same results as the one with properties, if I could remove duplicates of model instances(these dont yet have pk ) from a list. Do you have any ideas to do that ?

haikoschol · 2020-06-30T13:36:29Z

I think I can acheive the same results as the one with properties, if I could remove duplicates of model instances(these dont yet have pk ) from a list. Do you have any ideas to do that ?

Have you tried the naive approach of just iterating the list and comparing all fields?

sbs2001 · 2020-07-01T08:12:14Z

@haikoschol

Have you tried the naive approach of just iterating the list and comparing all fields?

That is a valid approach, I instead went with creating a dataclass and put it into a set and then instantiate model instances. I've tested it for alpine, debian, rust, openssl importers, all works fine.

Could you suggest an OK name for the dataclass ?

…ackage_Relation Changes are made in import_runner.py to refactore the models. Other major changes in import_runner.py is to change the _process_updated_advisories method to use less db queries and increase performance by several times. Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>

Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>

pombredanne

Thanks!
I commented mostly here, but I think I would still prefer concrete types (e.g. a real ImpactedPackage and ResolvedPackage class) that a generic type and an attribute.

pombredanne · 2020-07-13T10:48:29Z

+
+
+@dataclasses.dataclass(frozen=True)
+class VulnerabilityReference_inserter:


Please use TitleCase for classes, not a mix of Title_and_snake case

pombredanne · 2020-07-13T10:49:11Z

+
+
+@dataclasses.dataclass(frozen=True)
+class PackageRelatedVulnerability_inserter:


Same as above and elsewhere: stick to TitleCase for classes and snake_case elsewhere

pombredanne · 2020-07-13T10:49:57Z


+def _process_updated_advisories(data_source: DataSource) -> None:
+    """
+    TODO: Break this method into smaller functions


Does this comment still applies?

pombredanne · 2020-07-13T10:50:36Z

+    bulk_create_vuln_pkg_refs = set()
+    for batch in data_source.updated_advisories():
+        for advisory in batch:
+


Why this blank line?

pombredanne · 2020-07-13T10:52:26Z

+                vuln, vuln_created, advisory.resolved_package_urls, is_vulnerable=False)
+            bulk_create_vuln_pkg_refs.update(inew_refs.union(rnew_refs))
+
+    models.VulnerabilityReference.objects.bulk_create(


What benefit do you get from calling bulk_create vs creating as you go above?

We had this discussion in the chat. I think this is justified

pombredanne · 2020-07-13T11:08:54Z

-    packages = models.Package.objects.bulk_create(packages)
+        impacted: List[PackageURL],
+        resolved: List[PackageURL]
+) -> Tuple[Dict[PackageURL, int], Dict[PackageURL, int]]:


Have you enabled type checking at least in the CI?
Otherwise, type hints are not always helpful and if the types are not checked somehow, they can be misleading and i most case they are not conducive to improve readability... So let's not get carried away with type hints for the sake of them. Instead enter a ticket so we can discuss and plan for their possible usage

Agreed. In the CI it would be something along the lines of running a simple Mypy check right ? That probably won't be much of pain.

IMHO using typing help in most cases to atleast know what's going in and out, way better than comments, but that's just my opinion. Looking at this again I feel rather ashamed of this case though, it really seems I've used typing in a rather cryptic way.

Reverting this method, to the old one which was readable.

the ticket #226 :)

pombredanne · 2020-07-13T11:11:51Z

-            impacted_packages[purl] = pkg
-        elif purl in resolved:
-            resolved_packages[purl] = pkg
+    impacted_packages = dict(


I understand the zip here, but is there a simpler way to get there?
I am always nervous when we start using "parallel lists" to later zip them.

Reverted to previous approach, which was more readable.

pombredanne · 2020-07-13T11:13:01Z

    vulnerabilities: Set[models.Vulnerability],
-    impacted_packages: Dict[PackageURL, models.Package],
-    resolved_packages: Dict[PackageURL, models.Package],
+    impacted_packages: Dict[PackageURL, int],


What is this int about?

Overenginnering/pre-mature optimization . The idea was to use Dict[PackageURL, int] where the int is pk of Package , to save some memory because these dictionaries would grow big very fast. Carrying the Objects is way more expensive than pks so I had used it .

Reverting this.

pombredanne · 2020-07-13T11:13:55Z

+            ip = models.PackageRelatedVulnerability(
                vulnerability=vuln,
-                package=p,
+                package_id=p,


is p a package or a package_id?

Reverting this. p is a package.

pombredanne · 2020-07-13T11:14:31Z

                p = _package_url_to_package(resolved_purl)
                p.save()
-                resolved_packages[resolved_purl] = p
+                resolved_packages[resolved_purl] = p.id


What are the benefits of using ids vs objects there?

As mentioned above this is :

Overenginnering/pre-mature optimization . The idea was to use Dict[PackageURL, int] where the int is pk of Package , to save some memory because these dictionaries would grow big very fast. Carrying the Objects is way more expensive than pks so I had used it .

Reverting this.

pombredanne · 2020-07-13T14:25:20Z

@sbs2001 after our discussion I am not fine with the merging of ImpactedPackage and ResolvedPackage in a single model. :)
You call for a simpler DB-level referential integrity makes sense!

Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>

pombredanne

I added some more comments

pombredanne · 2020-07-22T11:11:31Z

-    for batch in data_source.updated_advisories():
-        for advisory in batch:
-            vuln, _ = _get_or_create_vulnerability(advisory)
+def _create_vulnerability_and_references(advisory: Advisory):


Just asking :)

pombredanne · 2020-07-22T11:14:07Z

At this stage since this is blocking further updates, I am merging it with the caveats noted otherwise... 👍

sbs2001 requested a review from haikoschol June 28, 2020 08:20

haikoschol reviewed Jun 29, 2020

View reviewed changes

Comment thread vulnerabilities/import_runner.py Outdated

Comment thread vulnerabilities/import_runner.py Outdated

sbs2001 added 3 commits July 1, 2020 13:53

Remove magic methods and properties in ImportRunner

bfda9af

Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>

Bulk insert at an importer level in ImporterRunner

5fcb5ea

Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>

sbs2001 force-pushed the remove-impacted_package branch from 71eacc8 to 5fcb5ea Compare July 1, 2020 08:23

sbs2001 requested a review from haikoschol July 3, 2020 11:11

sbs2001 mentioned this pull request Jul 4, 2020

[EXPERIMENT] New models #216

Closed

Refactor codebase to use PackageRelatedVulnerability

35280ff

Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>

sbs2001 force-pushed the remove-impacted_package branch from de105b8 to 35280ff Compare July 9, 2020 16:49

sbs2001 changed the title ~~[WIP] Refactor ImpactedPackage and ResolvedPackage~~ Refactor ImpactedPackage and ResolvedPackage Jul 9, 2020

sbs2001 requested a review from pombredanne July 9, 2020 17:01

pombredanne requested changes Jul 13, 2020

View reviewed changes

Revert _process_added_advisories, make style changes

189582d

Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>

sbs2001 force-pushed the remove-impacted_package branch from 60b63f8 to 189582d Compare July 14, 2020 11:05

This was referenced Jul 16, 2020

Stick ref ids with urls #227

Merged

Add UI for VulnerableCode #230

Merged

pombredanne reviewed Jul 22, 2020

View reviewed changes

pombredanne merged commit 72abb0d into aboutcode-org:develop Jul 22, 2020

sbs2001 mentioned this pull request Jul 22, 2020

Refactor import runner(no change to ImpactedPackage, ResolvedPackage) #225

Closed

sbs2001 mentioned this pull request Aug 24, 2020

Investigate the bottlenecks in the codebase, causing slow imports #210

Closed



		@dataclasses.dataclass(frozen=True)
		class VulnerabilityReference_inserter:



		@dataclasses.dataclass(frozen=True)
		class PackageRelatedVulnerability_inserter:

Uh oh!

Uh oh!

Conversation

sbs2001 commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbs2001 commented Jun 30, 2020

Uh oh!

haikoschol commented Jun 30, 2020

Uh oh!

sbs2001 commented Jul 1, 2020

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pombredanne commented Jul 13, 2020

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pombredanne commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbs2001 commented Jun 28, 2020 •

edited

Loading