Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding interface, implementation, and tests for vulnerability capabilities for a package domain model #6279
base: dev-feature-PackageDomainModel
Are you sure you want to change the base?
Adding interface, implementation, and tests for vulnerability capabilities for a package domain model #6279
Changes from 1 commit
0901c77
a4e0382
9e51505
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
so you're saying it's possible to have a list of vulnerabilities where all entries are null?
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.
It is, programmatically. It doesn't make much sense to have that, but there a few strategies I can think of:
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.Can you think of another way you'd suggest instead?
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.
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.
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.
@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.
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 file has
#nullable enable
, and the definition of theVulnerabilities
property doesn't allow nulls, so either the definition is wrong, or this not null check shouldn't be needed.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 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!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.
@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!
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 great for performance, not sure if we are currently iterating. More context: https://learn.microsoft.com/en-us/nuget/api/vulnerability-info#vulnerability-page.
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.
If this is not officially part of the contract, we shouldn't depend on it.
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.
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?
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.
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
.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.
Doesn't that talk about the way the version ranges are sorted and not the list of vulnerabilities?