Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80c78df
Backend: Fix duplicate reference urls(#343)
savish28 Mar 29, 2021
78cbbf8
Backend: Handle none empty url case in vulnerabilityReference
savish28 Mar 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,5 @@ zipp==0.6.0
requests==2.23.0
toml==0.10.2
PyYAML==5.4
freezegun==1.1.0
freezegun==1.1.0
urlpy==0.5
33 changes: 33 additions & 0 deletions vulnerabilities/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import importlib
from datetime import datetime
from time import sleep
import urlpy

from django.db import models
from django.db import IntegrityError
Expand Down Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean any update would go through straight ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

super(VulnerabilityReference, self).save(*args, **kwargs)
else:
url_parsed = urlpy.parse(self.url)
self.url = str(url_parsed.canonical())
Comment on lines +118 to +119
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or self.url=""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

url_scheme = url_parsed.scheme
scheme_independent_url = self.url[len(url_scheme) :]
if url_scheme == "http":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block should be broken down into a separate function.

similar_instance = VulnerabilityReference.objects.filter(
vulnerability=self.vulnerability,
source=self.source,
reference_id=self.reference_id,
url="https" + scheme_independent_url,
).first()
if not similar_instance:
super(VulnerabilityReference, self).save(*args, **kwargs)
elif url_scheme == "https":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

similar_instance = VulnerabilityReference.objects.filter(
vulnerability=self.vulnerability,
source=self.source,
reference_id=self.reference_id,
url="http" + scheme_independent_url,
).first()
if similar_instance:
similar_instance.url = self.url
similar_instance.save()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else:
super(VulnerabilityReference, self).save(*args, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a return here

else:
super(VulnerabilityReference, self).save(*args, **kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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:-

  1. If no arguments are passed then all the entries are looked upon and are checked.
  2. 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

  1. If no arguments are passed then all the entries are looked upon and are checked.
  1. 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 )

Copy link
Contributor Author

@savish28 savish28 Mar 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


class Meta:
unique_together = ("vulnerability", "source", "reference_id", "url")

Expand Down