Redesign the rest API - #247
Conversation
tdruez
left a comment
tdruez
left a comment
There was a problem hiding this comment.
The API code looks good.
See my comments for models and urls.
|
|
||
| @property | ||
| def vulnerable_to(self): | ||
| qs = PackageRelatedVulnerability.objects.filter( |
There was a problem hiding this comment.
Use Many2Many QuerySet syntax instead.
|
|
||
| @property | ||
| def vulnerable_to(self): | ||
| qs = PackageRelatedVulnerability.objects.filter( |
There was a problem hiding this comment.
Same as above, use Many2Many syntax.
| null=True, | ||
| ) | ||
|
|
||
| def set_package_url(self, package_url): |
There was a problem hiding this comment.
Why are we overriding the default method from PackageURLMixin?
There was a problem hiding this comment.
This one uses JSONField the oldset_package_url converts qualifiers into normalized string. More on this package-url/packageurl-python#35 (comment)
| 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.
What is the need for re_path instead of path here?
There was a problem hiding this comment.
I haven't touched that, that's black doing it's thing. Having said that we can use path their alright.
|
|
||
| api_router = DefaultRouter() | ||
| api_router.register(r'packages', PackageViewSet) | ||
| api_router = DefaultRouter(trailing_slash=False) |
There was a problem hiding this comment.
What's the reasoning behind overriding the default trailing_slash convention?
There was a problem hiding this comment.
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.
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.
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>
| 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.
I don't think this lookup is necessary since the related manager is starting on self.
tdruez
left a comment
tdruez
left a comment
There was a problem hiding this comment.
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.
Signed-off-by: Shivam Sandbhor <shivam.sandbhor@gmail.com>
3 participants
TODOs
Add tests specific to checking new API additions2. Deal with inputting packageurls as query param.