Backend: Fix duplicate reference urls(#343) #408

savish28 · 2021-03-29T08:37:34Z

Fix: #343.

The following changes are been proposed in this PR:

Saving URLs in canonical form ('http://foo.com/?b=2&a=1&d=3' -> 'http://foo.com/?a=1&b=2&d=3'
If provided url is http but we already have the url with https we ignore the http url.
If provided url is https but we already have the url with http we replace the http url with https one

Signed-off-by: savish <[email protected]>

savish28 · 2021-03-29T08:51:18Z

@sbs2001 ping. Could you please review the PR.

sbs2001 · 2021-03-31T03:09:15Z

@savish28 I'm not really sure how much of impact this is going to have on the db side things, since we are doing more queries per save. The imports are already slow enough.

So here's what I suggest. Keep the url normalization part in the save method. The scheme thing, should be moved into a separate job/command, https://docs.djangoproject.com/en/3.1/howto/custom-management-commands/ .

sbs2001

@savish28 as mentioned in the comment, keep the url normalization in the save method.

Move scheme inference thing t management command

sbs2001 · 2021-03-31T03:10:25Z

vulnerabilities/models.py

@@ -110,6 +111,38 @@ class VulnerabilityReference(models.Model):
    def scores(self):
        return VulnerabilitySeverity.objects.filter(reference=self.id)

+    def save(self, *args, **kwargs):
+        if self.id:


Does this mean any update would go through straight ?

@sbs2001 Yes. Because whenever we add a new object we might need to update a previously saved object and if we don't do this it might go into an infinite loop.

sbs2001 · 2021-03-31T03:11:51Z

vulnerabilities/models.py

+            url_parsed = urlpy.parse(self.url)
+            self.url = str(url_parsed.canonical())


Does this handle the case where self.url=None ?

Or self.url=""

Updated the code. It returned "/" for the same but keeping "" for "" would make more sense so updated the changes.

sbs2001 · 2021-03-31T03:14:13Z

vulnerabilities/models.py

+            self.url = str(url_parsed.canonical())
+            url_scheme = url_parsed.scheme
+            scheme_independent_url = self.url[len(url_scheme) :]
+            if url_scheme == "http":


This block should be broken down into a separate function.

sbs2001 · 2021-03-31T03:14:22Z

vulnerabilities/models.py

+                ).first()
+                if not similar_instance:
+                    super(VulnerabilityReference, self).save(*args, **kwargs)
+            elif url_scheme == "https":


Same as above, This block should be broken down into a separate function.

sbs2001 · 2021-03-31T03:17:10Z

vulnerabilities/models.py

+                ).first()
+                if similar_instance:
+                    similar_instance.url = self.url
+                    similar_instance.save()


return similar_instance.save() would be better here for readability

sbs2001 · 2021-03-31T03:17:44Z

vulnerabilities/models.py

+                    similar_instance.url = self.url
+                    similar_instance.save()
+                else:
+                    super(VulnerabilityReference, self).save(*args, **kwargs)


Use a return here

sbs2001 · 2021-03-31T03:18:50Z

vulnerabilities/models.py

+                else:
+                    super(VulnerabilityReference, self).save(*args, **kwargs)
+            else:
+                super(VulnerabilityReference, self).save(*args, **kwargs)


Same as above, for the computers it doesn't make difference, but when reading I don't have to verify whether the return value is correct.

@savish28 I'm not really sure how much of impact this is going to have on the db side things, since we are doing more queries per save. The imports are already slow enough.

So here's what I suggest. Keep the url normalization part in the save method. The scheme thing, should be moved into a separate job/command, https://docs.djangoproject.com/en/3.1/howto/custom-management-commands/ .

@sbs2001 So, a nice idea, Does this mean that the command/job python manage.py job_name have to be called explicitly by the admin to run the following job and it would go through all the entries and it would update the db. I think it can be extended in two ways:-

If no arguments are passed then all the entries are looked upon and are checked.

If arguments like id/Vulnerability/source etc are passed and only filtered objects are processed.

I think that in this case, each run would cost O(N) queries and would not be automated.

But in the save method update, it would cost 1 extra query per update and would be automated. So I think query wise the override save method is more time-efficient and more automated.

What would be your suggestion in the case and if we are going with the job idea, does the proposed method would work?

@savish28

called explicitly by the admin

We could get clever by running the job after each import, so this is a non-issue, I guess.

If no arguments are passed then all the entries are looked upon and are checked.

If arguments like id/Vulnerability/source etc are passed and only filtered objects are processed.

Btw I want to remove the source thing when I get time :) it's not populated anywhere and is a bad way to keep track of source(s) . With that out of the way, what would be the use case of (2) ? In almost all cases (1) would be used.

each run would cost O(N) queries

IMHO that can be done 1 + (no of duplicates) . One query to fetch all references (just the id and url columns) . Iterate over all records from server side, check for dups in the already fetched records. If a dup is found follow the procedure along the lines of what you're doing here, so that's gonna cost 1 DELETE query per dup.

Dups are rare anyways so it won't be O(N) .

it would cost 1 extra query per update

IMHO that'd be 1 extra query per insert/ (save method invocation in most cases )

Ohh okay, I get the idea. We are importing all the data and then doing this job. This would result in more efficiency rather than doing operation at each save.

So I guess these are the deliverables now,

Create Job for removing duplicates.

Add functionality to run the job after each import.
Does this seem right? @sbs2001

Signed-off-by: savish <[email protected]>

sbs2001 · 2021-03-31T09:45:28Z

@savish28 I just realized there's a third way to do this which would be even better.

Let's move your logic in a wrapper method like _get_or_create_keep_secure (this a bad name, need to discuss this more). We don't use save method explicitly anyways. https://github.com/nexB/vulnerablecode/blob/dfb1ab5558c597240573988cb981b498f98a964c/vulnerabilities/import_runner.py#L127 that's where refs get created. A get_or_create results in 2 queries, we can replace that with _get_or_create_keep_secure .Your logic is essentially create_keep_secure part, need to implement the get part .

So as far as queries are concerned _get_or_create_keep_secure would atmost require 2 queries which is same as that of get_or_create.

sbs2001 · 2021-04-22T11:11:46Z

@savish28 ping :)

pombredanne · 2021-07-15T13:51:44Z

@savish28 gentle ping.

pombredanne · 2021-07-15T14:07:44Z

@savish28 do you think you want to complete and bring this to a mergeable state?
Otherwise we may close for now and you can reopen later.

savish28 · 2021-07-15T14:38:49Z

@savish28 do you think you want to complete and bring this to a mergeable state?
Otherwise we may close for now and you can reopen later.

@pombredanne Sure, closing this PR for now.

Backend: Fix duplicate reference urls(aboutcode-org#343)

80c78df

Signed-off-by: savish <[email protected]>

savish28 force-pushed the Issue#343 branch from fda6303 to 80c78df Compare March 29, 2021 08:38

sbs2001 requested changes Mar 31, 2021

View reviewed changes

Backend: Handle none empty url case in vulnerabilityReference

78cbbf8

Signed-off-by: savish <[email protected]>

savish28 force-pushed the Issue#343 branch from 9e8fb3c to 78cbbf8 Compare March 31, 2021 07:42

savish28 closed this Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend: Fix duplicate reference urls(#343) #408

Backend: Fix duplicate reference urls(#343) #408

savish28 commented Mar 29, 2021

savish28 commented Mar 29, 2021

sbs2001 commented Mar 31, 2021

sbs2001 left a comment

sbs2001 Mar 31, 2021

savish28 Mar 31, 2021

sbs2001 Mar 31, 2021

sbs2001 Mar 31, 2021

savish28 Mar 31, 2021

sbs2001 Mar 31, 2021

sbs2001 Mar 31, 2021

sbs2001 Mar 31, 2021

sbs2001 Mar 31, 2021

sbs2001 Mar 31, 2021

savish28 Mar 31, 2021

sbs2001 Mar 31, 2021

savish28 Mar 31, 2021 •

edited

Loading

sbs2001 commented Mar 31, 2021

sbs2001 commented Apr 22, 2021

pombredanne commented Jul 15, 2021

pombredanne commented Jul 15, 2021

savish28 commented Jul 15, 2021

		url_parsed = urlpy.parse(self.url)
		self.url = str(url_parsed.canonical())

Backend: Fix duplicate reference urls(#343) #408

Backend: Fix duplicate reference urls(#343) #408

Conversation

savish28 commented Mar 29, 2021

savish28 commented Mar 29, 2021

sbs2001 commented Mar 31, 2021

sbs2001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savish28 Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

sbs2001 commented Mar 31, 2021

sbs2001 commented Apr 22, 2021

pombredanne commented Jul 15, 2021

pombredanne commented Jul 15, 2021

savish28 commented Jul 15, 2021

savish28 Mar 31, 2021 •

edited

Loading