-
Notifications
You must be signed in to change notification settings - Fork 230
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
expose severities inside the affected by packages #1799
base: main
Are you sure you want to change the base?
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.
Thanks. Here is some feedback!
@@ -216,7 +216,7 @@ def test_list_packages(self): | |||
Should return a list of packages with their details and associated vulnerabilities. | |||
""" | |||
url = reverse("package-v2-list") | |||
with self.assertNumQueries(32): | |||
with self.assertNumQueries(33): |
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.
Is there a way to avoid the extra query?
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.
yes , i had tested select_related(), but it didn't worked .
I can investigate further to see if there is a for it . Let me know if you have any suggestions on alternative approach , which means alot to me . thank you
vulnerabilities/tests/test_api_v2.py
Outdated
@@ -395,6 +396,7 @@ def test_get_affected_by_vulnerabilities(self): | |||
"VCID-1234": { | |||
"code_fixes": [], | |||
"vulnerability_id": "VCID-1234", | |||
"severities": [], |
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.
Can you add a new test that effectively return severities?
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.
Got it, fine ! i will add a test case where a vulnerability has severities and ensure that the API correctly returns them in the response. I'll update the PR once it's ready.
@@ -260,7 +265,7 @@ class PackageV2ViewSet(viewsets.ReadOnlyModelViewSet): | |||
queryset = Package.objects.all().prefetch_related( | |||
Prefetch( | |||
"affected_by_vulnerabilities", | |||
queryset=Vulnerability.objects.prefetch_related("fixed_by_packages"), | |||
queryset=Vulnerability.objects.prefetch_related("fixed_by_packages","severities"), |
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.
Is this really the way to send both to the same attribute below?
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 verified that severities and fixed_by_packages are sent separately in the API response. Also, I tested removing severities from prefetch_related(), and the response remained the same, confirming that explicit prefetching was unnecessary. So in further changes i will remove severities .
Let me know if you have any further suggestions.
Thank you for spending your time on reviewing the changes i made and giving feedback..!!
Signed-off-by: Arshad <[email protected]>
fix: #1669

Added severities field in affected_by_vulnerabilities, and also updated test files. because of adding severities in apiv2 , test cases are failing due to Query count mismatch and Assertion errors in api response
I got no test fails after changing the testing files in my local machine ."If @keshav-space and @pombredanne suggest any alternative approaches to solving this issue, I am happy to consider and implement their perspectives."
Here is the result:
let me know is the output is in correct order.
Thank You