Add initial fixed-affected-matching work #1228

[johnmhoran]

Reference: #1228

[TG1999]

@johnmhoran , thanks +++, some nits for your consideration

[johnmhoran]

Hmmm, I see some failed checks. Looking into it....

[johnmhoran]

The two failed tests relate solely to the same set of API tests -- perhaps because I started my Package API updating/refactoring and was interrupted by competing tasks before I could finish and test.

=========================== short test summary info ============================
FAILED vulnerabilities/tests/test_api.py::APITestCasePackage::test_api_response
FAILED vulnerabilities/tests/test_api.py::APITestCasePackage::test_api_status
FAILED vulnerabilities/tests/test_api.py::APITestCasePackage::test_api_with_ignorning_qualifiers
FAILED vulnerabilities/tests/test_api.py::APITestCasePackage::test_api_with_namespace_filter
FAILED vulnerabilities/tests/test_api.py::APITestCasePackage::test_api_with_single_vulnerability_and_fixed_package
FAILED vulnerabilities/tests/test_api.py::APITestCasePackage::test_api_with_single_vulnerability_and_vulnerable_package
FAILED vulnerabilities/tests/test_api.py::APITestCaseVulnerability::test_api_with_single_vulnerability
FAILED vulnerabilities/tests/test_api.py::APITestCaseVulnerability::test_api_with_single_vulnerability_with_filters
===== 8 failed, 292 passed, 1 skipped, 1 deselected, 7 warnings in 43.11s ======
make: *** [Makefile:116: test] Error 1
Error: Process completed with exit code 2.

[johnmhoran]

@tdruez @TG1999 I've just finished my final changes -- including @tdruez 's suggestion to change my get_fixed_by_package_versions() Package method to a PackageQuerySet method. My last check with make test showed just 6 failing tests, the same 6 @TG1999 and I looked at earlier today, all unrelated to my work.

I expect both of you gentlemen might have additional comments/suggestions, and I look forward to them. Meanwhile, thank you for taking the time to work with me on polishing up this Package UI/API code. 👍

[johnmhoran]

@TG1999 I was just looking at your recent commits and see that in 0ec7d6c, you've added two property methods to the Package class in models.py.

    @property
    def fixing_vulnerabilities(self):
        """
        Return only packages fixing a vulnerability .
        """
        return self.vulnerabilities.all().filter(packagerelatedvulnerability__fix=True)

    @property
    def affecting_vulnerabilities(self):
        """
        Return only packages fixing a vulnerability .
        """
        return self.vulnerabilities.all().filter(packagerelatedvulnerability__fix=False)

I could be mistaken, but these look similar to the method @tdruez had suggested I convert to a queryset method because [c]ode that deals specifically with database queries, such as returning multiple objects from filtering, should exist as methods of the QuerySet/Manager class. Should these be queryset methods?

And looking at the current PackageQuerySet() class, I see that it already contains two methods that seem to accomplish the same task as the two new property methods:

    def affected(self):
        """
        Return only packages affected by a vulnerability.
        """
        return self.filter(packagerelatedvulnerability__fix=False)

    vulnerable = affected

    def fixing(self):
        """
        Return only packages fixing a vulnerability .
        """
        return self.filter(packagerelatedvulnerability__fix=True)

Are these two sets of methods actually performing similar but different tasks?

[johnmhoran]

When I test the methods a bit I see that they are performing different tasks.

The new affecting_vulnerabilities() property method returns a queryset of vulnerabilities that affect a specific package -- the prefix self.vulnerabilities.all() indicates that the method will return a VulnerabilityQuerySet -- while the existing affected() queryset method returns a PackageQuerySet of all packages with one or more vulnerabilities. fixing_vulnerabilities() and fixing() have a similar specific package vs. all packages focus.

[TG1999]

@johnmhoran yes, you are right I botched up the docstrings pushing the correct docstrings now

[TG1999]

@johnmhoran a minor nit for you, please ensure all other methods on models are tested as well.

[johnmhoran]

@TG1999 Do you really want me to make sure we have tests for all methods on all models? That sounds like a rather large task unrelated to the work involved in this issue and PR. Please clarify.

[TG1999]

@johnmhoran sorry for the confusion, I meant add tests for all the methods in models added in this PR.

[johnmhoran]

@TG1999

I see that the affecting_vulnerabilities method in your example above is in the VulnerabilityQuerySet class. Is that the same affecting_vulnerabilities method you created last Friday as a Package property method, and which I asked whether it should actually be a queryset method (ditto for fixing_vulnerabilities)?

[TG1999]

No this is the different one, the one that added were by me in this commit here 0ec7d6c

[tdruez]

This is a good case to use a list comprehension:

affected_vulnerabilities = [
    self.get_vulnerability(vuln) for vuln in parent_affected_vulnerabilities
]

Also, what does vuln stand for? I'm guessing vulnerability and in that case, get_vulnerability(vulnerability) is not great.

[johnmhoran]

Thanks for this @tdruez . Re renaming, when you see terms or functions with names you don't like, if possible please suggest an alternative name that's acceptable so we don't repeat the process of me choosing a different name that you also don't like. ;-)

[johnmhoran]

@tdruez I can't think of a better naming convention so leaving as is. It's clearly labelled and I don't see a problem. Specific renaming suggestions welcome from anyone and everyone.

[tdruez]

Missing unit test

[johnmhoran]

Test added yesterday.

[tdruez]

Missing unit test

[johnmhoran]

Test added yesterday.

[tdruez]

The all() is not needed since your apply a filter.

[johnmhoran]

Deleted .all().

[tdruez]

The all() is not needed since your apply a filter.

[johnmhoran]

Deleted .all().

[johnmhoran]

@TG1999 @tdruez All comments addressed, all tests pass, all GH checks pass. Are we ready to merge?

[tdruez]

@johnmhoran Yes, ready to merge.

[johnmhoran]

Thank you @tdruez . 👍

[TG1999]

LGTM!