Utilize vulnerabilities caching service for in manage packages page #8620
Conversation
drewgillies
force-pushed
the
dg-managepackagespage-usesvulncache
branch
2 times, most recently
from
fb57d56
to
bab06bd
Compare
drewgillies
force-pushed
the
dg-managepackagespage-usesvulncache
branch
from
bab06bd
to
37865bb
Compare
|
How is perf looking? |
@skofman1 I'd like to see it in INT before evaluating, but this change introduces no new queries, simply a dictionary lookup. Perf on the caching itself (a different change for Package Details) is looking fine in INT, with no discernable regression in the INT Package Details page as it has many queries already and is not under high load. |
|
@skofman1 I've done some preliminary perf investigations (comparing enabling/disabling the feature) in DEV and:
This is different from the previous approach (direct SQL queries) where a consistent perf regression of ~1000ms applied to manage packages packages regardless of package list size. |
| [Fact] | ||
| public void GetsVulnerableStatusOfPackage() | ||
| { | ||
| // Arrange | ||
| Setup(); | ||
| var entitiesContext = new Mock<IEntitiesContext>(); |
There was a problem hiding this comment.
For discussions: I am not very sure whether we need to set up and mock so many dependencies here. I can see that "PackageVulnerabilitiesService" only has one dependency: "IPackageVulnerabilitiesCacheService". Will it work if we just mock the method "_packageVulnerabilitiesCacheService.GetVulnerabilitiesById(id)"? This will help us keep the unit test efficient and clean with the minimal scope.
There was a problem hiding this comment.
Good call, Zhaohua--that simplifies the tests. I've included this change now.
drewgillies
force-pushed
the
dg-managepackagespage-usesvulncache
branch
from
83cfe00
to
0d080c6
Compare
| { | ||
| Key = 0, | ||
| PackageRegistration = registrationVulnerable, | ||
| Version = "1.0.0", | ||
| VulnerablePackageRanges = new List<VulnerablePackageVersionRange> {versionRangeModerate} | ||
| VulnerablePackageRanges = new List<VulnerablePackageVersionRange> { versionRangeModerate } | ||
| }; |
There was a problem hiding this comment.
It will be better if we have a test to cover the following case:
The package registration Id matches, but the package key can't be found in the dictionary returned by "GetVulnerabilitiesById".
|
Nice work! 👍 |
Addresses #8361, with a far more performant approach (no direct database queries) than #8435 which we've left behind a disabled feature flag for this reason. This change utilizes the cache set up here: #8580.