-
-
Notifications
You must be signed in to change notification settings - Fork 264
Add fixed packages in packages endpoint #784
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
Conversation
c8a6320 to
131a823
Compare
131a823 to
b2507c5
Compare
tdruez
left a comment
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.
@TG1999 The new code looks fine.
You should add a simple unit test for the fixed_packages property.
Model properties should be easy to test and you should always add a unit test when you add new properties.
tdruez
left a comment
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.
@TG1999 Your code is not tested. How can we know it works?
vulnerabilities/api.py
Outdated
| params = self.context["request"].query_params | ||
| name = params.get("name") | ||
| if name: | ||
| data = data.filter(name=name) | ||
| namespace = params.get("namespace") | ||
| if namespace: | ||
| data = data.filter(namespace=namespace) | ||
| type = params.get("type") | ||
| if type: | ||
| data = data.filter(type=type) |
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 not using proper filters instead of this duplicated logic?
vulnerabilities/api.py
Outdated
| fields = ["url", "vulnerability_id"] | ||
|
|
||
|
|
||
| class MinimalPackageSerializerWithFixedVulnerabilities(serializers.HyperlinkedModelSerializer): |
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 class name is too long thus not readable.
1a70d47 to
944cb23
Compare
vulnerabilities/api.py
Outdated
| def get_queryset(self): | ||
| params = self.request.query_params | ||
| query = Q() | ||
| name = params.get("name") | ||
| if name: | ||
| query &= Q(name=name) | ||
| namespace = params.get("namespace") | ||
| if namespace: | ||
| query &= Q(namespace=namespace) | ||
| type = params.get("type") | ||
| if type: | ||
| query &= Q(type=type) | ||
| queryset = Vulnerability.objects.prefetch_related( | ||
| Prefetch( | ||
| "packages", | ||
| queryset=Package.objects.filter(query, packagerelatedvulnerability__fix=True), | ||
| to_attr="filtered_fixed_packages", | ||
| ) | ||
| ) | ||
| return queryset |
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 is still duplicated code and much harder to read and understand than the previous code.
This is not what I meant by "proper filters" but I was rather referring to https://www.django-rest-framework.org/api-guide/filtering/#djangofilterbackend.
Any reason to not use a FilterSet 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.
The filters applied by me are for Package model and the viewset is on Vulnerabilities model
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 understand, but relational fields are not available on FilterSet?
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 can do something like this https://github.com/philipn/django-rest-framework-filters#filtering-across-relationships, but the query will then look like api?package__name=name not api?name=name
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.
api?package__name=name
Is it a problem?
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.
@TG1999 you already defined a PackageFilterSet, you should use it, for example:
class VulnerabilityViewSet(viewsets.ReadOnlyModelViewSet):
def get_fixed_packages_qs(self):
package_filter_data = {"packagerelatedvulnerability__fix": True}
query_params = self.request.query_params
for field_name in ["name", "namespace", "type"]:
value = query_params.get(field_name)
if value:
package_filter_data[field_name] = value
return PackageFilterSet(package_filter_data).qs
def get_queryset(self):
queryset = Vulnerability.objects.prefetch_related(
Prefetch(
"packages",
queryset=self.get_fixed_packages_qs(),
to_attr="filtered_fixed_packages",
)
)
return queryset
Also, make sure to add packagerelatedvulnerability__fix to the PackageFilterSet.fields.
Lastly, you should always document such methods using the docstring, it is not obvious to the reader what is happening here and why we are prefetching filtered package data.
13aa93f to
9fd88d0
Compare
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
And address review comments Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
9fd88d0 to
ceafdb1
Compare
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
ac8b130 to
1edfb93
Compare
Signed-off-by: Tushar Goel tushar.goel.dav@gmail.com