JSON API #8 by kartiksibal · Pull Request #26 · aboutcode-org/vulnerablecode

kartiksibal · 2017-08-14T22:27:42Z

Signed-off-by: Kartik sibal kartiksibal@gmail.com

* updated views * updated urls.py Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

…o json_api

tdruez · 2017-08-17T08:24:12Z

+class VulnerabilitySerializer(serializers.ModelSerializer):
+    class Meta:
+        model = Vulnerability
+        exclude = ('id', 'cvss')


Avoid using exclude, use fields instead, see https://docs.djangoproject.com/en/1.11/topics/forms/modelforms/#selecting-the-fields-to-use

It is strongly recommended that you explicitly set all fields that should be edited in the form using the fields attribute. Failure to do so can easily lead to security problems when a form unexpectedly allows a user to set certain fields, especially when new fields are added to a model.

tdruez · 2017-08-17T08:24:25Z

+class VulnerabilityReferenceSerializer(serializers.ModelSerializer):
+    class Meta:
+        model = VulnerabilityReference
+        exclude = ('id', 'source', 'url', 'vulnerability')


Similar comment, use fields.

* use natural impactpackage link * add impact package link in data_dump Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne

Looking good! Only a few comments to address and of course some tests

pombredanne · 2017-08-21T11:55:32Z


 from vulncode_app.models import Vulnerability
 from vulncode_app.models import VulnerabilityReference
+from vulncode_app.models import ImpactedPackage


Imports should be sorted

pombredanne · 2017-08-21T11:56:20Z

+
+from rest_framework import serializers
+
+from vulncode_app.models import Vulnerability


Import should be sorted

pombredanne · 2017-08-21T11:57:03Z

 from django.conf.urls import url
-
-from . import views
+from rest_framework.urlpatterns import format_suffix_patterns


Empty line needed after this

pombredanne · 2017-08-21T11:57:47Z

 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'),
+    url(r'^cve-search/(?P<name>[a-z]+)/(?P<version>[0-9]+)',


Why do you call this cve-search?

@pombredanne because it is getting its's data from CVE-SEARCH API, that we worked out in the beginning.

tdruez · 2017-08-21T14:27:17Z

+
+        pk = Package.objects.filter(name=package_name)
+
+        for i, v in enumerate(pk):


This is the wrong approach and should be handled by the proper serializers.

@kartiksibal rather than for i, v in enumerate(pk): just iterate on the packages:

packages = Package.objects.filter(name=package_name) for package in packages: ....

* use dict instead of sets in data output Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne · 2017-08-22T12:02:01Z

+
+        pk = Package.objects.filter(name=package_name)
+
+        for i, v in enumerate(pk):


you are still enumerating pk. Instead loop on packages:
for package in Package.objects.filter(name=package_name): ....

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne · 2017-08-23T06:59:08Z

    """
    vulnerability = models.ForeignKey('Vulnerability')
-    package = models.ForeignKey('Package')
+    package_fk = models.ForeignKey('Package')


You should not rename this. This is a package object, not only a package_fk..

Also since you updated the models, you need to run makemigrations to generate the new migrations alright.

@pombredanne It was necessary to rename this since the reverse query name for Package.impacted_package clashes with ImpactedPackage.package

What do you mean by clash? can you be more specific? at the db level? in code? where? provide an example with some stack/error trace.
In any case, package_fk is not a good name.
If a name change is really need to avoid some collision, then this would be the ManyToMany field name in Package that would need to be changed instead.

Or maybe what you are really returning in you API as a primary model is not a Package... but an ImpactedPackage instead which would be actually more natural, what do you think?

Error Trace:

And I'll take another look into it, before lodging a ticket.

You also likely need a unique together on these two fields to avoid dupe entries

pombredanne · 2017-08-23T07:04:55Z

+class VulnerabilityReferenceSerializer(serializers.ModelSerializer):
+    class Meta:
+        model = VulnerabilityReference
+        fields = ('reference_id',)


You want to add the other reference fields too there.

pombredanne · 2017-08-23T07:16:55Z

+
+
+class PackageSerializer(serializers.ModelSerializer):
+    vulnerability = ImpactedPackageSerializer(source='impactedpackage_set', many=True)


This should be vulnerabilities, plural IMHO

* sort imports Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne · 2017-08-24T07:23:37Z


-from vulncode_app.models import Vulnerability
-from vulncode_app.models import VulnerabilityReference
+from vulncode_app.models import ImpactedPackage


Sorts import

pombredanne

I have added some comments.
Please check all import sort order...
Also the name collision that you experience is because the lack of plural.

pombredanne · 2017-08-24T07:24:23Z

        )
+        impacted_package = ImpactedPackage.objects.create(
+            vulnerability=vulnerability,
+            package_fk=package


Do not call this package_fk in the model, but package.

pombredanne · 2017-08-24T07:25:12Z

+            vulnerability=vulnerability,
+            package_fk=package
+        )
+        package.impacted_package.add(


Why do you add this? You created it alright in line 46?

pombredanne · 2017-08-24T07:25:42Z

        )
+        ImpactedPackage.objects.create(
+            vulnerability=vulnerability,
+            package_fk=package


Same as above: use package, not package_fk as an attribute name in the model

pombredanne · 2017-08-24T07:26:28Z

@@ -50,13 +50,16 @@ class VulnerabilityReference(models.Model):
    class Meta:
        unique_together = ('vulnerability', 'source', 'reference_id')


Would not the URL also be part of the unique together? just a question, not sure. We can quite likely have multiple URLs for a source that refer to the same reference_id.

I think more than one URL can point at a particular reference_id

ok, then you need to update the unique together then?

And you may want to check other models that may need updated unique together too

pombredanne · 2017-08-24T07:27:27Z

    """
    vulnerability = models.ForeignKey('Vulnerability')
-    package = models.ForeignKey('Package')
+    package_fk = models.ForeignKey('Package')


What do you mean by clash? can you be more specific? at the db level? in code? where? provide an example with some stack/error trace.
In any case, package_fk is not a good name.
If a name change is really need to avoid some collision, then this would be the ManyToMany field name in Package that would need to be changed instead.

Or maybe what you are really returning in you API as a primary model is not a Package... but an ImpactedPackage instead which would be actually more natural, what do you think?

pombredanne · 2017-08-24T07:36:53Z

+from rest_framework.response import Response
+from rest_framework import status

+from vulncode_app.serializers import PackageSerializer


Sort your imports

pombredanne · 2017-08-24T07:37:15Z

+    def get(self, request, package_name):
+        pk = Package.objects.filter(name=package_name)
+        response = PackageSerializer(pk, many=True).data
+


Empty line not needed here

pombredanne · 2017-08-24T07:37:32Z

+        response = PackageSerializer(pk, many=True).data
+
+        return Response(response)
+


Only one line between class methods

Not a class method.

pombredanne · 2017-08-24T07:38:33Z

+from vulncode_app.models import Package
+from vulncode_app.models import PackageReference
+from vulncode_app.models import VulnerabilityReference
+from vulncode_app.models import Vulnerability


Sort your imports, this comes before VulnerabilityReference

pombredanne · 2017-08-24T07:42:27Z

+    impacted_package = models.ManyToManyField('ImpactedPackage')
    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='Package version', blank=True)


in the line below

def __str__(self): return self.name

we likely need more than just name ... , but this is minor

pombredanne · 2017-08-24T07:55:54Z

+
+from django.test import TestCase
+from rest_framework.response import Response
+from collections import OrderedDict


e.g. this is stdlib and should not be in this block

pombredanne · 2017-08-24T08:02:13Z

Also you likely need to run makemigrations and commit the migrations for the model updates

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

* remove M2M on package * rename package_fk to package Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne · 2017-08-24T08:45:50Z

@@ -50,13 +50,16 @@ class VulnerabilityReference(models.Model):
    class Meta:
        unique_together = ('vulnerability', 'source', 'reference_id')


ok, then you need to update the unique together then?

pombredanne · 2017-08-24T08:46:33Z

@@ -50,13 +50,16 @@ class VulnerabilityReference(models.Model):
    class Meta:
        unique_together = ('vulnerability', 'source', 'reference_id')


And you may want to check other models that may need updated unique together too

* test blank response Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

* add url in vulnerability reference unique together Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne

So I guess you removed the manytomany. Can you elaborate on this?

Also Would not the root of your seralizers be the impacted package in earnest? (that's not a big deal as the serialization would end up the same)

Finally I think you are missing the all important Resolved packages in your output

If you do not mind, to not force push future commits... it will be easier for review!
Thanks

pombredanne · 2017-08-25T11:03:17Z

-        Package.objects.create(
+        package = Package.objects.create(
            name=data.get('package_name', ''),
            version=data.get('fixed_version', ''),


Based on our chat, does it ever make sense to create package without name and version or both empty values?
Also what about the platform field? Same for Ubuntu

pombredanne · 2017-08-25T11:05:58Z

+class PackageReferenceSerializer(serializers.ModelSerializer):
+    class Meta:
+        model = PackageReference
+        fields = ('name', 'version', 'repository', 'platform', )


Can you keep the field order from the model?

pombredanne · 2017-08-25T11:06:38Z

+class VulnerabilityReferenceSerializer(serializers.ModelSerializer):
+    class Meta:
+        model = VulnerabilityReference
+        fields = ('reference_id', 'source', 'url')


Here and elsewhere, please keep the model fields order

pombredanne · 2017-08-25T11:09:16Z

+#  Visit https://github.com/nexB/vulnerablecode/ for support and download.
+
+import json
+


no empty lines between a block of imports from the same kind (here stdlib)

pombredanne · 2017-08-25T11:15:10Z

+                                 'summary',
+                                 'Multiple stack-based buffer overflows in mimetex.cgi in mimeTeX'
+                               ),
+                               ('reference',


Should not this be plural "references"?

atm, every reference contains a single reference id. So, I think not.

My point is that there are multiple references, hence the list should be plural

Signed-off-by: Thomas Druez <tdruez@nexb.com>

tdruez · 2017-09-07T21:38:05Z

@kartiksibal The API code is in much better shape now, thanks to using proper Serializers ;)

I've made some cleanup in 8158a84

Some refinements are still needed from you before the merge:

Add some documentation on how to use the API in the README.md. You can create a new section at the end of the file.
All the test_* files should be located in a tests directory
data is a terrible name for the API URL, please use a proper name. Since this API endpoint returns Packages, I would suggest /api/packages/

Avoid using data for naming things in general.

@pombredanne could you review the shape of the returned package data structure?

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

…o json_api

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne

Thanks! I added a few comments on the overall structure of the returned data

pombredanne · 2017-09-12T22:49:25Z

+        debian_dump(extract_data)
+        response = self.client.get('/vulncode_app/api/mimetex', format='json')
+
+        expected = [{


I would expect the returned payload to have some "header" data (e.g. some tool version, what was the query made, number of results returned, ... And to have the list of packages returned under the packages: element

Create as #30

pombredanne · 2017-09-12T22:50:07Z

+        response = self.client.get('/vulncode_app/api/mimetex', format='json')
+
+        expected = [{
+            "name": "mimetex",


May be we are missing other Package fields at the same level as this such as the "platform" ?

pombredanne · 2017-09-12T22:50:34Z

+            "vulnerabilities": [{
+                "vulnerability": {
+                    "summary": "Multiple stack-based buffer overflows in mimetex.cgi in mimeTeX",
+                    "reference": [{


Should this be references (plural) and not reference, since this is also a list of mappings?

pombredanne · 2017-09-12T22:51:05Z

+            "name": "mimetex",
+            "version": "1.50-1.1",
+            "vulnerabilities": [{
+                "vulnerability": {


IMHO we do not need vulnerabilities/vulnerability
Only vulnerabilities as a list of mappings, each being a vulnerability since we only list these there. No need for an extra level of nesting and a "vulnerability" nested mapping?

- Following Django best-practices - Simplify the layout - Use proper naming convention Signed-off-by: Thomas Druez <tdruez@nexb.com>

Signed-off-by: Thomas Druez <tdruez@nexb.com>

tdruez · 2017-09-21T14:44:51Z

+    url(r'^cve-search/(?P<name>[a-z]+)',
+        views.package,
+        name='package'),
+    url(r'^api/(?P<package_name>[a-z]+)',


This pattern [a-z]+ is not good enough to catch real package names, for example: ruby1.9.1 and gcc-4.6. Use (?P<package_name>.+) instead.

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Set the new 2m field on the PackageSerializer Signed-off-by: Thomas Druez <tdruez@nexb.com>

Create junction from Scripts to bin

kartiksibal added 4 commits August 15, 2017 03:56

Add serializers based on models #8

bfa5bc7

* updated views * updated urls.py Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Add DRF to requirements.txt

89f2bb1

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Remove un-used imports #8

c75ae67

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Merge branch 'json_api' of https://github.com/nexB/vulnerablecode int…

4d1c6c1

…o json_api

tdruez reviewed Aug 17, 2017

View reviewed changes

kartiksibal added 2 commits August 21, 2017 02:32

Change data output format #8

585327a

* use natural impactpackage link * add impact package link in data_dump Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Change API data format to as discussed #8

37d16b2

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne requested changes Aug 21, 2017

View reviewed changes

tdruez reviewed Aug 21, 2017

View reviewed changes

Add test cases and change data output format #8

96d6bfd

* use dict instead of sets in data output Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne reviewed Aug 22, 2017

View reviewed changes

kartiksibal added 3 commits August 23, 2017 07:29

Updated serialization logic

0705c24

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Add many to many field on Package #8

f08d614

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Remove un-used imports #8

692d72d

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne requested changes Aug 23, 2017

View reviewed changes

pombredanne reviewed Aug 23, 2017

View reviewed changes

Add test cases #8

fcadb17

* sort imports Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne reviewed Aug 24, 2017

View reviewed changes

pombredanne requested changes Aug 24, 2017

View reviewed changes

pombredanne reviewed Aug 24, 2017

View reviewed changes

kartiksibal added 3 commits August 24, 2017 13:39

New migrations #8

d41215d

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Update models #8

9015a68

* remove M2M on package * rename package_fk to package Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Minor formatting changes #8

bdcb807

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne requested changes Aug 24, 2017

View reviewed changes

kartiksibal added 2 commits August 25, 2017 15:34

Add test cases #8

42c7be2

* test blank response Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Update unique together #8

d1821c4

* add url in vulnerability reference unique together Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

pombredanne requested changes Aug 25, 2017

View reviewed changes

pombredanne reviewed Aug 25, 2017

View reviewed changes

Add missing migration file along some code cleanup #8

8158a84

Signed-off-by: Thomas Druez <tdruez@nexb.com>

kartiksibal added 7 commits September 12, 2017 10:02

Move tests to vulncode_app/tests #8

0317f8a

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Re-order serializers according to models #8

d533162

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

change API url to api/ from data/ #8

5469f99

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Update travis according to new dir structure

0bf6354

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Update tests according to changed api url #8

9b277d6

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Merge branch 'json_api' of https://github.com/nexB/vulnerablecode int…

5d5f089

…o json_api

Update readme for API instructions #8

16ea31a

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

kartiksibal mentioned this pull request Sep 12, 2017

Evolve data dump to store or not to store data without a package name. #29

Closed

pombredanne requested changes Sep 12, 2017

View reviewed changes

tdruez added 3 commits September 20, 2017 19:51

Refactor the codebase structure

86c359a

- Following Django best-practices - Simplify the layout - Use proper naming convention Signed-off-by: Thomas Druez <tdruez@nexb.com>

Fix failing tests following codebase structure changes

b016877

Signed-off-by: Thomas Druez <tdruez@nexb.com>

Add back Gitter webhooks removed by mistake

23fc544

Signed-off-by: Thomas Druez <tdruez@nexb.com>

tdruez mentioned this pull request Sep 21, 2017

Refactor the current API view into a proper DRF view #30

Closed

tdruez reviewed Sep 21, 2017

View reviewed changes

kartiksibal and others added 4 commits September 22, 2017 22:54

Update URL regex #8

a860bef

Signed-off-by: Kartik sibal <kartiksibal@gmail.com>

Included platform in package serializer #8

c42a775

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Update tests according to changes #8

2d6d5eb

Signed-off-by: kartik sibal <kartiksibal@gmail.com>

Add a vulnerabilities m2m field on the Package model

2c5f78d

Set the new 2m field on the PackageSerializer Signed-off-by: Thomas Druez <tdruez@nexb.com>

tdruez merged commit 7048c43 into develop Sep 22, 2017

tdruez deleted the json_api branch September 22, 2017 20:31

pombredanne added a commit that referenced this pull request Apr 2, 2025

Merge pull request #26 from nexB/windows-junction

b735a3f

Create junction from Scripts to bin


		from rest_framework import serializers

		from vulncode_app.models import Vulnerability


		pk = Package.objects.filter(name=package_name)

		for i, v in enumerate(pk):



		class PackageSerializer(serializers.ModelSerializer):
		vulnerability = ImpactedPackageSerializer(source='impactedpackage_set', many=True)

		@@ -50,13 +50,16 @@ class VulnerabilityReference(models.Model):
		class Meta:
		unique_together = ('vulnerability', 'source', 'reference_id')

		response = PackageSerializer(pk, many=True).data

		return Response(response)

		# Visit https://github.com/nexB/vulnerablecode/ for support and download.

		import json

Uh oh!

Uh oh!

Conversation

kartiksibal commented Aug 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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

kartiksibal Aug 24, 2017 •

edited

Loading