Backend: Fix duplicate reference urls(#343) by savish28 · Pull Request #408 · aboutcode-org/vulnerablecode

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 <savishbedi1@gmail.com>

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

        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

+            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

+            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

+                ).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

+                ).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

+                    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

+                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 <savishbedi1@gmail.com>

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 <savishbedi1@gmail.com>

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 <savishbedi1@gmail.com>

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

savish28 closed this Jul 15, 2021

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

Uh oh!

Uh oh!

Conversation

savish28 commented Mar 29, 2021

Uh oh!

savish28 commented Mar 29, 2021

Uh oh!

sbs2001 commented Mar 31, 2021

Uh oh!

sbs2001 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!

savish28 Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbs2001 commented Mar 31, 2021

Uh oh!

sbs2001 commented Apr 22, 2021

Uh oh!

pombredanne commented Jul 15, 2021

Uh oh!

pombredanne commented Jul 15, 2021

Uh oh!

savish28 commented Jul 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

savish28 Mar 31, 2021 •

edited

Loading