Conversation
kartiksibal
commented
Jul 27, 2017
- Also adds description in Debian's scraper logic.
Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
|
|
||
| vulnerability.save() | ||
| vulnerability_reference.save() | ||
| package.save() |
There was a problem hiding this comment.
the save() need to be in the loop...
Otherwise you are only saving the latest item. Add some tests that show that
|
|
||
|
|
||
| def ubuntu_dump(): | ||
|
|
|
|
||
|
|
||
| def debian_dump(): | ||
|
|
| json_data = debian.json_data() | ||
| extracted_data = debian.extract_data(json_data) | ||
|
|
||
| for i, v in enumerate(extract_data): |
There was a problem hiding this comment.
Why do you enumerate here at all? this code makes no sense at all.
There was a problem hiding this comment.
You still need to process the comment I made above ;)
* moved .save() in the loop * remove enumerate Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
* uses debian' test data to scrape * dumps data in the test DB and verifies it Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
* Changed test cases accordingly Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
pombredanne
left a comment
There was a problem hiding this comment.
You need to start by testing things with more assertions and ensure that all the records you expect are there. You will then see that your code is not inserting all the records
|
|
||
| def debian_dump(extract_data): | ||
| """ | ||
| Dump data scraped from Debian' security tracker. |
There was a problem hiding this comment.
Save might be a better name than dump ;)
| vulnerability_reference.save() | ||
| package.save() | ||
|
|
||
| return vulnerability, vulnerability_reference, package |
There was a problem hiding this comment.
Why do you need to return anything? Things are saved in the DB.
Also if you return here.... this will return at the first iteration. skipping all the other records.
| extract_data = debian.extract_data(test_data) | ||
| data_dump = debian_dump(extract_data) | ||
|
|
||
| self.assertEqual(data_dump[0].summary, extract_data[0].get('description')) |
There was a problem hiding this comment.
Your tests should instead do a query in the DB. In particular you should start by asserting that the number of records you think you inserted based on the contents of tests/test_data/debian.json is correct.
Then test the values of all records
| summary_create = Vulnerability.objects.create(summary="Affected package xyz") | ||
| summary_get = Vulnerability.objects.get(pk=summary_create.pk) | ||
|
|
||
| self.assertEqual(str(summary_create), "Affected package xyz") |
There was a problem hiding this comment.
Do not convert to a string. Instead test the attributes values here and in your other tests
There was a problem hiding this comment.
@pombredanne Do you think we should be keeping test_models. Won't, test_data_dump suffice?
* put expected value before test value * retrieve attr values instead of converting to strings * remove return statemetn in Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
| """ | ||
| Dump data scraped from Ubuntu's security tracker. | ||
| """ | ||
| for data in extract_data: |
There was a problem hiding this comment.
You did not run the tests ;) where could extract_data come from?
There was a problem hiding this comment.
@pombredanne It is being passed as a parameter in the method?
https://github.com/nexB/vulnerablecode/blob/data_dump/app/vulncode_app/data_dump.py#L44
There was a problem hiding this comment.
I cannot see a parameter in https://github.com/nexB/vulnerablecode/pull/25/files#diff-102b650ccd1cc2cb42f5359f5101e8e1R66
def ubuntu_dump(): .... there is no args: hence I can only conclude that the code has never be run or tested ;)
| extract_data = debian.extract_data(test_data) | ||
| data_dump = debian_dump(extract_data) | ||
|
|
||
| for i in range(3): |
There was a problem hiding this comment.
But why doing three iterations?
There was a problem hiding this comment.
@pombredanne I was iterating over extract_data and incrementing pk. To match the expected data with the data we are getting. And since we have three entries, in the test data. Hence, 3
There was a problem hiding this comment.
This is not a robust approach for testing. Instead use a query, this is an iterable that you can then handle. Never expect that the pk have a specific values.
| self.assertEqual(extract_data[i].get('vulnerability_id'), | ||
| VulnerabilityReference.objects.get(pk=i+1).reference_id) | ||
| self.assertEqual(extract_data[i].get('package_name'), | ||
| Package.objects.get(pk=i+1).name) |
There was a problem hiding this comment.
You should do a query, not hope that the PK will be incremented linearly
| data_dump = debian_dump(extract_data) | ||
|
|
||
| for i in range(3): | ||
| self.assertEqual(3, len(Vulnerability.objects.all())) |
There was a problem hiding this comment.
Why is the query in the range loop?
There was a problem hiding this comment.
Also you should not assert on all() but instead with a filter that select what you expect there.
There was a problem hiding this comment.
@pombredanne The query shouldn't be in the loop.
I'll update that. I'll factor in the second point.
There was a problem hiding this comment.
@pombredanne Since, we are testing the number of entries. Isn't .all() a requirement here?
There was a problem hiding this comment.
My main point was not to make that test three times in a range loop.
As far all is concerned, that is fine to count, but then use count() not all()
There was a problem hiding this comment.
Gotcha 👍
I have addressed your first point.
Well there is no loop anymore. So, the root is gone.
Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
tdruez
left a comment
There was a problem hiding this comment.
It seems that you did not run the code nor the tests.
You need to document how to run those new tests in the README and make sure those are part of the continuous integration test suite (Travis CI)
| from scraper import debian, ubuntu | ||
|
|
||
|
|
||
| def debian_data(): |
There was a problem hiding this comment.
This should be in the scraper/debian.py module, for consistency with https://github.com/nexB/vulnerablecode/blob/develop/scraper/ubuntu.py#L50
| """ | ||
| Save data scraped from Debian' security tracker. | ||
| """ | ||
| for data in extract_data: |
There was a problem hiding this comment.
You could use ModelClass.objects.create() instead of save() for simplicity.
There was a problem hiding this comment.
Also, I doubt the following objects creation works, the required fields are not properly set.
There was a problem hiding this comment.
@tdruez Could you elaborate this? Are you talking in context to models.py?
There was a problem hiding this comment.
For example: Vulnerability.objects.create(summary="Affected package xyz")
-> django.db.utils.IntegrityError: NOT NULL constraint failed: vulncode_app_vulnerability.cvss
Since the cvss is not a nullable field, and since you do not set a value for it, your save() cannot work
There was a problem hiding this comment.
@tdruez Yes, I get your point. I'll add the updated models along the next commit.
| package.save() | ||
|
|
||
|
|
||
| def ubuntu_data(): |
There was a problem hiding this comment.
This is un-needed, just call ubuntu.scrape_cves() in the ubuntu_dump() function.
| class TestVulnerabilityReference(TestCase): | ||
| def test_vulnerability_reference(self): | ||
| ref_id_create = VulnerabilityReference.objects.create(reference_id="CVE-2017-8564") | ||
| ref_id_get = VulnerabilityReference.objects.get(pk=ref_id_create.pk) |
| class TestPackage(TestCase): | ||
| def test_package(self): | ||
| package_name_create = Package.objects.create(name="Firefox") | ||
| package_name_get = Package.objects.get(pk=package_name_create.pk) |
| class TestPackageReference(TestCase): | ||
| def test_package_reference(self): | ||
| platform_create = PackageReference.objects.create(platform="Maven") | ||
| platform_get = PackageReference.objects.get(pk=platform_create.pk) |
| class TestVulnerability(TestCase): | ||
| def test_vulnerability(self): | ||
| summary_create = Vulnerability.objects.create(summary="Affected package xyz") | ||
| summary_get = Vulnerability.objects.get(pk=summary_create.pk) |
There was a problem hiding this comment.
@tdruez test_models.py was just a trial method to test models, initially. I haven't updated it. I was under the impression that test_data_dump will suffice.
Will update this now. 👍
* removed loop, added individual asserts * removed str() * added test cases for ubuntu * updated code formatting and test cases in test_models.py Signed-off-by: kartik sibal <kartiksibal@gmail.com>
* Moved tests and scraper code in app/ Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
* added null=true in fields which can be nullable Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
* used Class.objects.create() instead of .save() * remove un-necessary ubuntu_data method Signed-off-by: Kartik Sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
There was a problem hiding this comment.
Do not use null=True on CharField but blank=True instead, see https://docs.djangoproject.com/en/1.11/ref/models/fields/#null
| class Vulnerability(models.Model): | ||
| summary = models.TextField(max_length=50, help_text="Summary of the vulnerability") | ||
| cvss = models.FloatField(max_length=50, help_text="CVSS Score") | ||
| summary = models.TextField(max_length=50, help_text="Summary of the vulnerability", null=True) |
There was a problem hiding this comment.
What's the use case for creating Vulnerability without a summary?
| source = models.CharField(max_length=50, help_text="Source's name eg:NVD") | ||
| reference_id = models.CharField(max_length=50, help_text="Reference ID, eg:CVE-ID") | ||
| url = models.URLField(max_length=1024, help_text="URL of Vulnerability data") | ||
| vulnerability = models.ForeignKey('Vulnerability', null=True) |
There was a problem hiding this comment.
What's the use case for creating VulnerabilityReference without a Vulnerability reference?
| name = models.CharField(max_length=50, help_text="Package name") | ||
| version = models.CharField(max_length=50, help_text="Pacakge version") | ||
| platform = models.CharField(max_length=50, help_text="Package platform eg:maven", null=True) | ||
| name = models.CharField(max_length=50, help_text="Package name", null=True) |
There was a problem hiding this comment.
What's the use case for creating Package without a name?
|
|
||
| class PackageReference(models.Model): | ||
| package = models.ForeignKey('Package') | ||
| package = models.ForeignKey('Package', null=True) |
There was a problem hiding this comment.
What's the use case for creating PackageReference without a Package reference
* changed test cases accordingly * added blank=true * cleaned test_models.py Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
* an empty string will be returned instead of None Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
* added `cd app/` in before_script Signed-off-by: kartik sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
| 'django.contrib.sessions', | ||
| 'django.contrib.messages', | ||
| 'django.contrib.staticfiles', | ||
|
|
| import json | ||
| from urllib.request import urlopen | ||
|
|
||
| import pprint |
| # VulnerableCode is a free software code scanning tool from nexB Inc. and others. | ||
| # Visit https://github.com/nexB/vulnerablecode/ for support and download. | ||
|
|
||
| from vulncode_app.models import Vulnerability |
| """ | ||
| for data in extract_data: | ||
| vulnerability = Vulnerability.objects.create(summary=data.get('description', '')) | ||
| vulnerability_ref = VulnerabilityReference.objects.create( |
There was a problem hiding this comment.
No need to create a variable if you do not need to use the object.
| url = models.URLField(max_length=1024, help_text="URL of Vulnerability data") | ||
| source = models.CharField(max_length=50, help_text="Source's name eg:NVD", blank=True) | ||
| reference_id = models.CharField(max_length=50, help_text="Reference ID, eg:CVE-ID", blank=True) | ||
| url = models.URLField(max_length=1024, help_text="URL of Vulnerability data", null=True) |
There was a problem hiding this comment.
URLField is a subclass of CharField, use blank instead.
| version = models.CharField(max_length=50, help_text="Pacakge version") | ||
| platform = models.CharField(max_length=50, help_text="Package platform eg:maven", blank=True) | ||
| name = models.CharField(max_length=50, help_text="Package name", blank=True) | ||
| version = models.CharField(max_length=50, help_text="Pacakge version", blank=True) |
| class Vulnerability(models.Model): | ||
| summary = models.TextField(max_length=50, help_text="Summary of the vulnerability") | ||
| cvss = models.FloatField(max_length=50, help_text="CVSS Score") | ||
| summary = models.TextField(max_length=50, help_text="Summary of the vulnerability", blank=True) |
There was a problem hiding this comment.
Why using a TextField and not a CharField here?
|
|
||
| from django.test import TestCase | ||
|
|
||
| from vulncode_app.models import Vulnerability |
There was a problem hiding this comment.
Again, un-used import and un-used variable in that file.
* removed un-wanted creation of vars * added blank=true in urlfield * removed un-used imports Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
* added doc-strings * added unique together Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
- Add missing initial migration file - Code style and naming consistency - Refine the README content - Remove un-used module and section of code Signed-off-by: Thomas Druez <tdruez@nexb.com>
Signed-off-by: Thomas Druez <tdruez@nexb.com>
pombredanne
left a comment
There was a problem hiding this comment.
I made some comments, most are minor and easy to address. And then we can merge this
| before_script: | ||
| - pycodestyle --exclude=migrations,settings.py,lib,tests --max-line-length=100 . | ||
| - cd app/ | ||
| - python3 manage.py makemigrations |
There was a problem hiding this comment.
Remove this. Migrations should be committed
|
|
||
| script: | ||
| - python3.6 -m pytest -v tests/ | ||
| - python3.6 manage.py test |
There was a problem hiding this comment.
We should use one or the other for running tests.... do not run tests twice
| @@ -28,18 +29,28 @@ pycodestyle --exclude=migrations,settings.py,lib,tests --max-line-length=100 . | |||
| cd app/ | |||
| python3.6 -m pytest -v tests/ | |||
There was a problem hiding this comment.
Here too, we should only use one way to run tests. Not two
| summary = models.TextField(max_length=1024, | ||
| help_text="Summary of the vulnerability") | ||
| cvss = models.FloatField(help_text="CVSS Score") | ||
| summary = models.CharField(max_length=50, help_text="Summary of the vulnerability", blank=True) |
There was a problem hiding this comment.
Please use single quotes for all strings (unless this is a docstring or it contains a single quote itself)
This applies here and below
|
|
||
| self.assertTrue(VulnerabilityReference.objects.get(reference_id="CVE-2009-2458")) | ||
|
|
||
| self.assertTrue(VulnerabilityReference.objects.get(reference_id="CVE-2009-2459")) |
There was a problem hiding this comment.
Single quotes for strings throughout.
|
|
||
| urlpatterns = [ | ||
| url(r'(?P<name>[a-z]+)/(?P<version>[0-9]+)', views.package_version, name="package_version"), | ||
| url(r'^(?P<name>[a-z]+)', views.package, name="package"), |
Signed-off-by: Kartik sibal <kartiksibal@gmail.com>
Signed-off-by: Kartik sibal <kartiksibal@gmail.com>