-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add fixed packages in packages endpoint #784
Conversation
c8a6320
to
131a823
Compare
131a823
to
b2507c5
Compare
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.
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 <[email protected]>
Signed-off-by: Tushar Goel <[email protected]>
Signed-off-by: Tushar Goel <[email protected]>
And address review comments Signed-off-by: Tushar Goel <[email protected]>
Signed-off-by: Tushar Goel <[email protected]>
9fd88d0
to
ceafdb1
Compare
Signed-off-by: Tushar Goel <[email protected]>
ac8b130
to
1edfb93
Compare
Signed-off-by: Tushar Goel [email protected]