Expand view models for vulnerabilities in manage packages page #8362
Conversation
drewgillies
requested review from
agr,
cristinamanum,
dannyjdev,
joelverhagen,
loic-sharma,
lyndaidaii,
ryuyu,
shishirx34,
skofman1,
xavierdecoster and
zhhyu
as code owners
| { | ||
| return GetPackagesForOwners(new[] { user.Key }, includeUnlisted, includeVersions); | ||
| return GetPackagesForOwners(new[] { user.Key }, includeUnlisted, includeVersions, includeVulnerabilities: false); |
There was a problem hiding this comment.
How about that we also provide the flexibility to callers of this method?
public IEnumerable<Package> FindPackagesByOwner(User user,
bool includeUnlisted,
bool includeVersions = false,
bool includeVulnerabilities = false)
There was a problem hiding this comment.
It's unused, but a trivial change, so ...done.
| var listedPackages = packages | ||
| .Where(p => predicate(p)) | ||
| var filteredPackages = packages.Where(p => predicate(p)); | ||
| var latestPackages = PackageService.GetLatestVersions(filteredPackages); |
There was a problem hiding this comment.
Do we have any reasons to introduce the filter here and get latest packages, compared with the old approach?
There was a problem hiding this comment.
Yes, I need to cache all of the versions that clear the filter before filtering to latest versions only. It is this all-versions collection that I need to examine to determine if any version (not just the latest) has a vulnerability. Then we decorate the list item with a bang. We've not needed this distinction before now.
I split out latestPackages into a separate line for readability in the following statement, that it's the latest versions we're using to create our list.
There was a problem hiding this comment.
Thanks! That makes sense and is a workable solution.
drewgillies
force-pushed
the
dg-vulnerabilitymessaging
branch
3 times, most recently
from
91f55fa
to
6975afc
Compare
drewgillies
force-pushed
the
dg-add-vulnerabilities-to-viewmodels
branch
2 times, most recently
from
cd04946
to
83bb005
Compare
drewgillies
force-pushed
the
dg-vulnerabilitymessaging
branch
from
6975afc
to
65b1ace
Compare
drewgillies
force-pushed
the
dg-add-vulnerabilities-to-viewmodels
branch
from
83bb005
to
f3dd94f
Compare
drewgillies
force-pushed
the
dg-add-vulnerabilities-to-viewmodels
branch
from
f3dd94f
to
01b0055
Compare
drewgillies
force-pushed
the
dg-add-vulnerabilities-to-viewmodels
branch
from
01b0055
to
271f59a
Compare
| && package.VulnerablePackageRanges.FirstOrDefault(vpr => vpr.Vulnerability != null) != null) | ||
| { | ||
| first = package; | ||
| break; |
There was a problem hiding this comment.
We can simplify this part: here we can return true and at the end, we can return false. Then there is no need to define "first".
| IEnumerable<Package> FindPackagesByAnyMatchingOwner(User user, bool includeUnlisted, bool includeVersions = false); | ||
| IEnumerable<Package> FindPackagesByAnyMatchingOwner(User user, bool includeUnlisted, bool includeVersions = false, bool includeVulnerabilities = false); | ||
|
|
||
| IQueryable<Package> GetLatestVersions(IQueryable<Package> packages); |
There was a problem hiding this comment.
Maybe provide a summary for this new introduced method, because we have another similar method named "FilterLatestPackage".
The comment below is out of scope for this PR, so feel free to ignore it:
I am not sure whether it's a good practice to expose this method. For the implementation, it contains a similar logic as the private method named "FilterLatestPackageHelper" but not the exact same. So there may need a code refactoring. I am not very familiar with this part and haven't worked in this area before. Or if we don't want to expose this method, and keep the old implementation, in the "UsersController", we can call "PackageService.FindPackagesByAnyMatchingOwner" twice and get all packages maybe named "packagesWithAllVersions" and the packages with latest version named "packagesWithLatestVersion". The "GetPackages" private method is only used there, so we can easily pass these 2 variables there. However, it seems not efficient because we may need 2 DB calls.
Keeping the current implementation is correct and reasonable.
| { | ||
| Key = 5, | ||
| PackageRegistration = _registrationNotVulnerable, | ||
| Version = "0.1.1", | ||
| VulnerablePackageRanges = null | ||
| }; |
There was a problem hiding this comment.
Could we have some test coverage on other cases?
package.VulnerablePackageRanges.Any() && package.VulnerablePackageRanges.FirstOrDefault(vpr => vpr.Vulnerability != null) != null
|
Looks good to me. I suggest getting another approval before checking in this change. |
| @@ -521,7 +524,9 @@ public virtual ActionResult Packages() | |||
|
|
|||
| var wasAADLoginOrMultiFactorAuthenticated = User.WasMultiFactorAuthenticated() || User.WasAzureActiveDirectoryAccountUsedForSignin(); | |||
|
|
|||
| var packages = PackageService.FindPackagesByAnyMatchingOwner(currentUser, includeUnlisted: true); | |||
| var packages = PackageService.FindPackagesByAnyMatchingOwner( | |||
There was a problem hiding this comment.
FindPackagesByAnyMatchingOwner [](start = 42, length = 30)
This page is super slow, especially for orgs with a big amount of packages (like MSFT). I recommend you test the perf on DEV before merging. Loading version information may be super expensive.
|
Closing this PR for now (but keeping the branch) as this approach performs badly--in the order or a 3x or 4x degradation. Starting afresh and the new approach is tracked in the epic. |
Addresses view model portion of #8361 (I split the PR as best as is practical because it's large-ish). Full description of change to customer experience in PR (view changes) on this issue.