Adding interface, implementation, and tests for vulnerability capabilities for a package domain model

[jebriede]

Bug

Fixes:

Description

Encapsulates the vulnerability list and logic. Provides an interface for use in a package model type as well as the implementation and unit tests.

PR Checklist

• Meaningful title, helpful description and a linked NuGet/Home issue
• Added tests
• Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

[jgonz120]

so you're saying it's possible to have a list of vulnerabilities where all entries are null?

[jebriede]

It is, programmatically. It doesn't make much sense to have that, but there a few strategies I can think of:

1. Prevent the construction of a VulnerableCapability if the collection of vulnerabilities contains any nulls. This would require iterating over the collection upon construction to validate if any of the entries are null. With big lists this could take a performance hit for constructing many Packages with IVulnerable and I would not favor this.
2. Allow the collection to contain nulls because it's just a list of objects, but handle them gracefully so we essentially treat them as if they did not exist. This is what I've done in this PR and made that behavior explicit by adding a test around it and to ensure that it is handled gracefully.

Can you think of another way you'd suggest instead?

[jgonz120]

Yea, I think iterating through them to validate there aren't any nulls seems excessive. I guess I didn't realize we had been accounting for this in the old code too.

[jebriede]

@jgonz120 We aren't accounting for it explicitly in the ViewModels today which presents a potential flaw. The goal here is to make this code more robust than what we had before and put it under test. By handling this edge case gracefully, we avoid any NRE's in case a null makes its way into the list at any point.

[zivkan]

This file has #nullable enable, and the definition of the Vulnerabilities property doesn't allow nulls, so either the definition is wrong, or this not null check shouldn't be needed.

[jebriede]

This file has #nullable enable, and the definition of the Vulnerabilities property doesn't allow nulls, so either the definition is wrong, or this not null check shouldn't be needed.

I see what you mean. Both the tests and the VulnerabilityCapability classes themselves have #nullable enable yet the Test allows a null value to be used. As such, it proves that with nullable enabled on the caller, we can still end up with nulls making their way into the list. While a test can show that it's possible, we should have code that handles this edge case and a test that proves it's being handled. If I wrote the test incorrectly though, please let me know how and I'll fix it!

[jebriede]

@zivkan I removed the null checks and the test. This bug is open against Roslyn dotnet/roslyn#72142 which was erroneously allowing the list to have a null in only one case that I could tell. I'm deciding not to code around this bug for an unlikely edge case. Let me know what you think!

[martinrrm]

This is great for performance, not sure if we are currently iterating. More context: https://learn.microsoft.com/en-us/nuget/api/vulnerability-info#vulnerability-page.

[nkolev92]

If this is not officially part of the contract, we shouldn't depend on it.

[jebriede]

Correct, there's no way to guarantee the list will be ordered from any and all vulnerability sources, both current and future. This comment points out that the list is ordered. It's ordered in this class (see the setter) explicitly to avoid making assumptions about the data source. Does that help? Do you have any suggested edits?

[martinrrm]

It does, right? The list of known vulnerabilities for a package should be sorted in descending order of the max version of the version range.

[nkolev92]

Doesn't that talk about the way the version ranges are sorted and not the list of vulnerabilities?