-
Notifications
You must be signed in to change notification settings - Fork 188
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.
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
Redesign the rest API #247
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API code looks good.
See my comments for models and urls.
vulnerabilities/models.py
Outdated
|
||
@property | ||
def vulnerable_to(self): | ||
qs = PackageRelatedVulnerability.objects.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Many2Many QuerySet syntax instead.
vulnerabilities/models.py
Outdated
|
||
@property | ||
def vulnerable_to(self): | ||
qs = PackageRelatedVulnerability.objects.filter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, use Many2Many syntax.
), | ||
null=True | ||
null=True, | ||
) | ||
|
||
def set_package_url(self, package_url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we overriding the default method from PackageURLMixin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one uses JSONField the oldset_package_url
converts qualifiers into normalized string. More on this package-url/packageurl-python#35 (comment)
vulnerablecode/urls.py
Outdated
path('admin/', admin.site.urls), | ||
re_path(r'^api/', include(api_router.urls)) | ||
path("admin/", admin.site.urls), | ||
re_path(r"^api/", include(api_router.urls)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the need for re_path
instead of path
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't touched that, that's black doing it's thing. Having said that we can use path
their alright.
vulnerablecode/urls.py
Outdated
|
||
|
||
api_router = DefaultRouter() | ||
api_router.register(r'packages', PackageViewSet) | ||
api_router = DefaultRouter(trailing_slash=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning behind overriding the default trailing_slash convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to discuss this with you guys.
For the ListView
using trailing_slash the endpoints look odd. The endpoints with queries look like :
/api/vulnerabilities/?vulnerability_id=foo
After the override you get the :
/api/vulnerabilities?vulnerability_id=foo
Not sure this matter tho, so I might be missing something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the benefit of removing the trailing slash.
From https://www.django-rest-framework.org/api-guide/routers/
Trailing slashes are conventional in Django, but are not used by default in some other frameworks such as Rails.
I think we should follow Django's conventions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
The REST API now has a detail views for package and vulnerability. The search API allows packages and vulnerabilities to be searched for using various parameters. The design now uses hyperlinks and provides easy navigation. Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
vulnerabilities/models.py
Outdated
return self.packagerelatedvulnerability_set.filter(is_vulnerable=True) | ||
return self.package_set.filter( | ||
packagerelatedvulnerability__is_vulnerable=True, | ||
packagerelatedvulnerability__vulnerability_id=self.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this lookup is necessary since the related manager is starting on self
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Although I don't think those lookups are necessary:
packagerelatedvulnerability__vulnerability_id=self.id,
and
packagerelatedvulnerability__package_id=self.id,
Please confirm and update accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
TODOs
Add tests specific to checking new API additions2. Deal with inputting packageurls as query param.