From c0a6f85583de25c5913ed37f9a6b5784ffa49119 Mon Sep 17 00:00:00 2001 From: Drew Gillies Date: Thu, 13 May 2021 13:58:07 +1000 Subject: [PATCH 1/5] Add caching to package details page vulnerabilities query --- ...PackageVulnerabilitiesManagementService.cs | 8 +++ ...PackageVulnerabilitiesManagementService.cs | 3 + .../App_Start/DefaultDependenciesModule.cs | 5 ++ src/NuGetGallery/NuGetGallery.csproj | 2 + .../IPackageVulnerabilitiesCacheService.cs | 21 ++++++ .../PackageVulnerabilitiesCacheService.cs | 65 +++++++++++++++++++ .../Services/PackageVulnerabilitiesService.cs | 39 ++++------- .../Verify/PackageVulnerabilitiesVerifier.cs | 2 + 8 files changed, 119 insertions(+), 26 deletions(-) create mode 100644 src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs create mode 100644 src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs diff --git a/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs index e114bc37af..bcd9705899 100644 --- a/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs +++ b/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Threading.Tasks; +using System.Linq; using NuGet.Services.Entities; namespace NuGetGallery @@ -22,5 +23,12 @@ public interface IPackageVulnerabilitiesManagementService /// /// Whether or not the vulnerability was withdrawn. Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, bool withdrawn); + + /// + /// Get a package's collection of vulnerable ranges. + /// + /// The package's Id + /// The package's vulnerable ranges, connecting it to instances + IQueryable GetVulnerableRangesById(string packageId); } } \ No newline at end of file diff --git a/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs index f16dcf0d42..214393e6b2 100644 --- a/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs +++ b/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs @@ -73,6 +73,9 @@ public async Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, b } } + public IQueryable GetVulnerableRangesById(string packageId) => + _entitiesContext.VulnerableRanges.Where(x => x.PackageId == packageId); + /// /// Updates the database with . /// diff --git a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs index af69ee6d18..96950c4ca3 100644 --- a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs +++ b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs @@ -460,6 +460,11 @@ protected override void Load(ContainerBuilder builder) .As() .InstancePerLifetimeScope(); + builder.RegisterType() + .AsSelf() + .As() + .SingleInstance(); + services.AddHttpClient(); services.AddScoped(); diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index 6c07fd3f05..b4232f1076 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -314,8 +314,10 @@ + + diff --git a/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs new file mode 100644 index 0000000000..8dc22c6184 --- /dev/null +++ b/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs @@ -0,0 +1,21 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using NuGet.Services.Entities; + +namespace NuGetGallery +{ + /// + /// This interface is used to implement a basic caching for vulnerabilities querying. + /// /// + public interface IPackageVulnerabilitiesCacheService + { + /// + /// This function is used to get the packages by id dictionary from the cache + /// + IReadOnlyDictionary> GetVulnerabilitiesById(string id, + IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService); + } +} diff --git a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs new file mode 100644 index 0000000000..2a6f80a630 --- /dev/null +++ b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs @@ -0,0 +1,65 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Data.Entity; +using System.Linq; +using NuGet.Services.Entities; + +namespace NuGetGallery +{ + public class PackageVulnerabilitiesCacheService : IPackageVulnerabilitiesCacheService + { + private const int CachingLimitMinutes = 30; + private readonly object Locker = new object(); + private readonly IDictionary> vulnerabilitiesById)> vulnerabilitiesByIdCache + = new Dictionary>)>(); + + public IReadOnlyDictionary> GetVulnerabilitiesById(string id, + IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService) + { + if (string.IsNullOrEmpty(id)) + { + throw new ArgumentException("Must have a value.", nameof(id)); + } + if (packageVulnerabilitiesManagementService == null) + { + throw new ArgumentNullException(nameof(packageVulnerabilitiesManagementService)); + } + + if (ShouldCachedValueBeUpdated(id)) + { + lock (Locker) + { + if (ShouldCachedValueBeUpdated(id)) + { + var packageKeyAndVulnerability = packageVulnerabilitiesManagementService + .GetVulnerableRangesById(id) + .Include(x => x.Vulnerability) + .Where(x => x.PackageId == id) + .SelectMany(x => x.Packages.Select(p => new {PackageKey = p.Key, x.Vulnerability})) + .GroupBy(pv => pv.PackageKey, pv => pv.Vulnerability) + .ToDictionary(pv => pv.Key, + pv => pv.ToList().AsReadOnly() as IReadOnlyList); + + var result = !packageKeyAndVulnerability.Any() + ? null + : new ReadOnlyDictionary>( + packageKeyAndVulnerability); + + vulnerabilitiesByIdCache[id] = (cachedAt: DateTime.Now, vulnerabilitiesById: result); + } + } + } + + return vulnerabilitiesByIdCache[id].vulnerabilitiesById; + } + + private bool ShouldCachedValueBeUpdated(string id) => !vulnerabilitiesByIdCache.ContainsKey(id) || + vulnerabilitiesByIdCache[id].cachedAt + .AddMinutes(CachingLimitMinutes) < DateTime.Now; + } +} \ No newline at end of file diff --git a/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs b/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs index d8b335c019..42bc0f36df 100644 --- a/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs +++ b/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs @@ -12,36 +12,23 @@ namespace NuGetGallery { public class PackageVulnerabilitiesService : IPackageVulnerabilitiesService { - private readonly IEntitiesContext _entitiesContext; + private readonly IPackageVulnerabilitiesManagementService _packageVulnerabilitiesManagementService; + private readonly IPackageVulnerabilitiesCacheService _packageVulnerabilitiesCacheService; - public PackageVulnerabilitiesService(IEntitiesContext entitiesContext) + public PackageVulnerabilitiesService( + IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService, + IPackageVulnerabilitiesCacheService packageVulnerabilitiesCacheService) { - _entitiesContext = entitiesContext ?? throw new ArgumentNullException(nameof(entitiesContext)); + _packageVulnerabilitiesManagementService = packageVulnerabilitiesManagementService ?? + throw new ArgumentNullException( + nameof(packageVulnerabilitiesManagementService)); + _packageVulnerabilitiesCacheService = packageVulnerabilitiesCacheService ?? + throw new ArgumentNullException( + nameof(packageVulnerabilitiesCacheService)); } - public IReadOnlyDictionary> GetVulnerabilitiesById(string id) - { - var result = new Dictionary>(); - var packagesMatchingId = _entitiesContext.Packages - .Where(p => p.PackageRegistration != null && p.PackageRegistration.Id == id) - .Include($"{nameof(Package.VulnerablePackageRanges)}.{nameof(VulnerablePackageVersionRange.Vulnerability)}"); - foreach (var package in packagesMatchingId) - { - if (package.VulnerablePackageRanges == null) - { - continue; - } - - if (package.VulnerablePackageRanges.Any()) - { - result.Add(package.Key, - package.VulnerablePackageRanges.Select(vr => vr.Vulnerability).ToList()); - } - } - - return !result.Any() ? null : - result.ToDictionary(kv => kv.Key, kv => kv.Value as IReadOnlyList); - } + public IReadOnlyDictionary> GetVulnerabilitiesById(string id) => + _packageVulnerabilitiesCacheService.GetVulnerabilitiesById(id, _packageVulnerabilitiesManagementService); public bool IsPackageVulnerable(Package package) { diff --git a/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs b/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs index cc0775ddc5..f94766b3ce 100644 --- a/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs +++ b/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs @@ -71,6 +71,8 @@ public Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, bool wi return Task.CompletedTask; } + public IQueryable GetVulnerableRangesById(string packageId) => throw new NotImplementedException(); + private void VerifyVulnerabilityInDatabase(PackageVulnerability vulnerability, bool withdrawn) { Console.WriteLine($"[Database] Verifying vulnerability {vulnerability.GitHubDatabaseKey}."); From 2e4e8733aed19cc899adf210dd4c142afd08ea99 Mon Sep 17 00:00:00 2001 From: Drew Gillies Date: Mon, 17 May 2021 11:31:46 +1000 Subject: [PATCH 2/5] Addressed feedback - loading cache on startup --- ...PackageVulnerabilitiesManagementService.cs | 7 + ...PackageVulnerabilitiesManagementService.cs | 3 + .../App_Start/DefaultDependenciesModule.cs | 2 +- .../IPackageVulnerabilitiesCacheService.cs | 3 +- .../PackageVulnerabilitiesCacheService.cs | 56 ++++-- .../Services/PackageVulnerabilitiesService.cs | 10 +- .../NuGetGallery.Facts.csproj | 1 + ...PackageVulnerabilitiesCacheServiceFacts.cs | 162 ++++++++++++++++++ 8 files changed, 214 insertions(+), 30 deletions(-) create mode 100644 tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs diff --git a/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs index bcd9705899..20d89fcb79 100644 --- a/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs +++ b/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs @@ -30,5 +30,12 @@ public interface IPackageVulnerabilitiesManagementService /// The package's Id /// The package's vulnerable ranges, connecting it to instances IQueryable GetVulnerableRangesById(string packageId); + + /// + /// Get the full set of vulnerable package entities + /// + /// Vulnerable package version ranges + IQueryable GetAllVulnerableRanges(); + } } \ No newline at end of file diff --git a/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs index 214393e6b2..f5f73edd70 100644 --- a/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs +++ b/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs @@ -76,6 +76,9 @@ public async Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, b public IQueryable GetVulnerableRangesById(string packageId) => _entitiesContext.VulnerableRanges.Where(x => x.PackageId == packageId); + public IQueryable GetAllVulnerableRanges() => + _entitiesContext.Set(); + /// /// Updates the database with . /// diff --git a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs index 96950c4ca3..dcac2a5189 100644 --- a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs +++ b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs @@ -460,7 +460,7 @@ protected override void Load(ContainerBuilder builder) .As() .InstancePerLifetimeScope(); - builder.RegisterType() + builder.Register(c => new PackageVulnerabilitiesCacheService(c.Resolve())) .AsSelf() .As() .SingleInstance(); diff --git a/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs index 8dc22c6184..88a8dea3a9 100644 --- a/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs +++ b/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs @@ -15,7 +15,6 @@ public interface IPackageVulnerabilitiesCacheService /// /// This function is used to get the packages by id dictionary from the cache /// - IReadOnlyDictionary> GetVulnerabilitiesById(string id, - IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService); + IReadOnlyDictionary> GetVulnerabilitiesById(string id); } } diff --git a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs index 2a6f80a630..c7d1079b63 100644 --- a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs +++ b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs @@ -12,23 +12,25 @@ namespace NuGetGallery { public class PackageVulnerabilitiesCacheService : IPackageVulnerabilitiesCacheService { - private const int CachingLimitMinutes = 30; + private const int CachingLimitMinutes = 1440; // We could make this 1 day instead (same value) but this is easier for spot testing the cache private readonly object Locker = new object(); - private readonly IDictionary> vulnerabilitiesById)> vulnerabilitiesByIdCache - = new Dictionary>)>(); + private IDictionary> vulnerabilitiesById)> vulnerabilitiesByIdCache + = new Dictionary>)>(); - public IReadOnlyDictionary> GetVulnerabilitiesById(string id, - IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService) + private readonly IPackageVulnerabilitiesManagementService _packageVulnerabilitiesManagementService; + public PackageVulnerabilitiesCacheService(IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService) + { + _packageVulnerabilitiesManagementService = packageVulnerabilitiesManagementService; + Initialize(); + } + + public IReadOnlyDictionary> GetVulnerabilitiesById(string id) { if (string.IsNullOrEmpty(id)) { throw new ArgumentException("Must have a value.", nameof(id)); } - if (packageVulnerabilitiesManagementService == null) - { - throw new ArgumentNullException(nameof(packageVulnerabilitiesManagementService)); - } if (ShouldCachedValueBeUpdated(id)) { @@ -36,26 +38,42 @@ public IReadOnlyDictionary> GetVulnerab { if (ShouldCachedValueBeUpdated(id)) { - var packageKeyAndVulnerability = packageVulnerabilitiesManagementService + var packageKeyAndVulnerability = _packageVulnerabilitiesManagementService .GetVulnerableRangesById(id) .Include(x => x.Vulnerability) - .Where(x => x.PackageId == id) .SelectMany(x => x.Packages.Select(p => new {PackageKey = p.Key, x.Vulnerability})) .GroupBy(pv => pv.PackageKey, pv => pv.Vulnerability) .ToDictionary(pv => pv.Key, pv => pv.ToList().AsReadOnly() as IReadOnlyList); - var result = !packageKeyAndVulnerability.Any() - ? null - : new ReadOnlyDictionary>( - packageKeyAndVulnerability); - - vulnerabilitiesByIdCache[id] = (cachedAt: DateTime.Now, vulnerabilitiesById: result); + vulnerabilitiesByIdCache[id] = (cachedAt: DateTime.Now, vulnerabilitiesById: packageKeyAndVulnerability); } } } - return vulnerabilitiesByIdCache[id].vulnerabilitiesById; + return vulnerabilitiesByIdCache[id].vulnerabilitiesById.Any() + ? new ReadOnlyDictionary>(vulnerabilitiesByIdCache[id].vulnerabilitiesById) + : null; + } + + private void Initialize() + { + // We need to build a dictionary of dictionaries. Breaking it down: + // - this give us a list of all vulnerable package version ranges + vulnerabilitiesByIdCache = _packageVulnerabilitiesManagementService.GetAllVulnerableRanges() + .Include(x => x.Vulnerability) + // - from these we want a list in this format: (, (, )) + // which will allow us to look up the dictionary by id, and return a dictionary of version -> vulnerability + .SelectMany(x => x.Packages.Select(p => new + {PackageId = x.PackageId ?? string.Empty, KeyVulnerability = new {PackageKey = p.Key, x.Vulnerability}})) + .GroupBy(ikv => ikv.PackageId, ikv => ikv.KeyVulnerability) + // - build the outer dictionary, keyed by - each inner dictionary is paired with a time of creation (for cache invalidation) + .ToDictionary(ikv => ikv.Key, + ikv => (cachedAt: DateTime.Now, + vulnerabilitiesById: ikv.GroupBy(kv => kv.PackageKey, kv => kv.Vulnerability) + // - build the inner dictionaries, all under the same , each keyed by + .ToDictionary(kv => kv.Key, + kv => kv.ToList().AsReadOnly() as IReadOnlyList))); } private bool ShouldCachedValueBeUpdated(string id) => !vulnerabilitiesByIdCache.ContainsKey(id) || diff --git a/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs b/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs index 42bc0f36df..e0d7759e8c 100644 --- a/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs +++ b/src/NuGetGallery/Services/PackageVulnerabilitiesService.cs @@ -12,23 +12,17 @@ namespace NuGetGallery { public class PackageVulnerabilitiesService : IPackageVulnerabilitiesService { - private readonly IPackageVulnerabilitiesManagementService _packageVulnerabilitiesManagementService; private readonly IPackageVulnerabilitiesCacheService _packageVulnerabilitiesCacheService; - public PackageVulnerabilitiesService( - IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService, - IPackageVulnerabilitiesCacheService packageVulnerabilitiesCacheService) + public PackageVulnerabilitiesService(IPackageVulnerabilitiesCacheService packageVulnerabilitiesCacheService) { - _packageVulnerabilitiesManagementService = packageVulnerabilitiesManagementService ?? - throw new ArgumentNullException( - nameof(packageVulnerabilitiesManagementService)); _packageVulnerabilitiesCacheService = packageVulnerabilitiesCacheService ?? throw new ArgumentNullException( nameof(packageVulnerabilitiesCacheService)); } public IReadOnlyDictionary> GetVulnerabilitiesById(string id) => - _packageVulnerabilitiesCacheService.GetVulnerabilitiesById(id, _packageVulnerabilitiesManagementService); + _packageVulnerabilitiesCacheService.GetVulnerabilitiesById(id); public bool IsPackageVulnerable(Package package) { diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj index 4c87efaa41..5a13c85526 100644 --- a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj +++ b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj @@ -103,6 +103,7 @@ + diff --git a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs new file mode 100644 index 0000000000..308c978c71 --- /dev/null +++ b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs @@ -0,0 +1,162 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; +using System.Linq; +using Moq; +using NuGet.Services.Entities; +using NuGetGallery.Framework; +using Xunit; + +namespace NuGetGallery.Services +{ + public class PackageVulnerabilitiesCacheServiceFacts : TestContainer + { + [Fact] + public void InitializesVulnerabilitiesCache() + { + // Arrange + var vulnerableVersionRanges = GetVersionRanges(); + var pvmService = new Mock(); + pvmService.Setup(stub => stub.GetAllVulnerableRanges()).Returns(vulnerableVersionRanges); + pvmService.Setup(stub => stub.GetVulnerableRangesById(It.IsAny())).Verifiable(); + var cacheService = new PackageVulnerabilitiesCacheService(pvmService.Object); + + // Act + var vulnerabilitiesFoo = cacheService.GetVulnerabilitiesById("Foo"); + var vulnerabilitiesBar = cacheService.GetVulnerabilitiesById("Bar"); + + // Assert + // This method should never be called (it's only called when cache can't provide, and these values are loaded into the cache on initialize) + pvmService.Verify(s => s.GetVulnerableRangesById(It.IsAny()), Times.Never); + // Test cache contents + Assert.Equal(4, vulnerabilitiesFoo.Count); + Assert.Equal(1, vulnerabilitiesFoo[0].Count); + Assert.Equal(1, vulnerabilitiesFoo[1].Count); + Assert.Equal(2, vulnerabilitiesFoo[2].Count); + Assert.Equal(1234, vulnerabilitiesFoo[2][0].GitHubDatabaseKey); + Assert.Equal(5678, vulnerabilitiesFoo[2][1].GitHubDatabaseKey); + Assert.Equal(1, vulnerabilitiesFoo[3].Count); + Assert.Equal(2, vulnerabilitiesBar.Count); + Assert.Equal(1, vulnerabilitiesBar[5].Count); + Assert.Equal(9012, vulnerabilitiesBar[5][0].GitHubDatabaseKey); + Assert.Equal(1, vulnerabilitiesBar[6].Count); + } + + private IQueryable GetVersionRanges() + { + var registrationFoo = new PackageRegistration { Id = "Foo" }; + var registrationBar = new PackageRegistration { Id = "Bar" }; + + var vulnerabilityCriticalFoo = new PackageVulnerability + { + AdvisoryUrl = "http://theurl/1234", + GitHubDatabaseKey = 1234, + Severity = PackageVulnerabilitySeverity.Critical + }; + var vulnerabilityModerateFoo = new PackageVulnerability + { + AdvisoryUrl = "http://theurl/5678", + GitHubDatabaseKey = 5678, + Severity = PackageVulnerabilitySeverity.Moderate + }; + var vulnerabilityCriticalBar = new PackageVulnerability + { + AdvisoryUrl = "http://theurl/9012", + GitHubDatabaseKey = 9012, + Severity = PackageVulnerabilitySeverity.Critical + }; + + var versionRangeCriticalFoo = new VulnerablePackageVersionRange + { + Vulnerability = vulnerabilityCriticalFoo, + PackageId = "Foo", + PackageVersionRange = "1.1.1", + FirstPatchedPackageVersion = "1.1.2" + }; + var versionRangeModerateFoo = new VulnerablePackageVersionRange + { + Vulnerability = vulnerabilityModerateFoo, + PackageId = "Foo", + PackageVersionRange = "<=1.1.2", + FirstPatchedPackageVersion = "1.1.3" + }; + var versionRangeCriticalBar = new VulnerablePackageVersionRange + { + Vulnerability = vulnerabilityCriticalBar, + PackageId = "Bar", + PackageVersionRange = "<=1.1.0", + FirstPatchedPackageVersion = "1.1.1" + }; + + var packageFoo100 = new Package + { + Key = 0, + PackageRegistration = registrationFoo, + Version = "1.0.0", + VulnerablePackageRanges = new List + { + versionRangeModerateFoo + } + }; + var packageFoo110 = new Package + { + Key = 1, + PackageRegistration = registrationFoo, + Version = "1.1.0", + VulnerablePackageRanges = new List + { + versionRangeModerateFoo + } + }; + var packageFoo111 = new Package + { + Key = 2, + PackageRegistration = registrationFoo, + Version = "1.1.1", + VulnerablePackageRanges = new List + { + versionRangeModerateFoo, + versionRangeCriticalFoo + } + }; + var packageFoo112 = new Package + { + Key = 3, + PackageRegistration = registrationFoo, + Version = "1.1.2", + VulnerablePackageRanges = new List + { + versionRangeModerateFoo + } + }; + var packageBar100 = new Package + { + Key = 5, + Version = "1.0.0", + PackageRegistration = registrationBar, + VulnerablePackageRanges = new List + { + versionRangeCriticalBar + } + }; + var packageBar110 = new Package + { + Key = 6, + PackageRegistration = registrationBar, + Version = "1.1.0", + VulnerablePackageRanges = new List + { + versionRangeCriticalBar + } + }; + + versionRangeCriticalFoo.Packages = new List {packageFoo111}; + versionRangeModerateFoo.Packages = new List {packageFoo100, packageFoo110, packageFoo111, packageFoo112}; + versionRangeCriticalBar.Packages = new List {packageBar100, packageBar110}; + + return new List + {versionRangeCriticalFoo, versionRangeModerateFoo, versionRangeCriticalBar}.AsQueryable(); + } + } +} From a4032d16a9f033eaffcc7b547f29cdf6da8d1413 Mon Sep 17 00:00:00 2001 From: Drew Gillies Date: Tue, 18 May 2021 11:15:58 +1000 Subject: [PATCH 3/5] Addressed feedback --- .../Gallery/ThrowingTelemetryService.cs | 5 + ...PackageVulnerabilitiesManagementService.cs | 8 -- ...PackageVulnerabilitiesManagementService.cs | 3 - .../Telemetry/ITelemetryService.cs | 6 + .../Telemetry/TelemetryService.cs | 6 + src/NuGetGallery/App_Start/AppActivator.cs | 20 ++- .../App_Start/DefaultDependenciesModule.cs | 4 +- .../PackageVulnerabilitiesCacheRefreshJob.cs | 25 ++++ src/NuGetGallery/NuGetGallery.csproj | 1 + .../PackageVulnerabilitiesCacheService.cs | 105 ++++++++------ .../Verify/PackageVulnerabilitiesVerifier.cs | 2 +- .../Fakes/FakeTelemetryService.cs | 5 + ...PackageVulnerabilitiesCacheServiceFacts.cs | 12 +- .../PackageVulnerabilitiesServiceFacts.cs | 135 +++--------------- .../Services/TelemetryServiceFacts.cs | 4 + 15 files changed, 157 insertions(+), 184 deletions(-) create mode 100644 src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs diff --git a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs index 49fe225d07..cee59f9c4e 100644 --- a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs +++ b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs @@ -370,5 +370,10 @@ public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, { throw new NotImplementedException(); } + + public void TrackVulnerabilitiesCacheRefreshDuration(long duration) + { + throw new NotImplementedException(); + } } } \ No newline at end of file diff --git a/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs index 20d89fcb79..35958a64a8 100644 --- a/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs +++ b/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs @@ -24,18 +24,10 @@ public interface IPackageVulnerabilitiesManagementService /// Whether or not the vulnerability was withdrawn. Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, bool withdrawn); - /// - /// Get a package's collection of vulnerable ranges. - /// - /// The package's Id - /// The package's vulnerable ranges, connecting it to instances - IQueryable GetVulnerableRangesById(string packageId); - /// /// Get the full set of vulnerable package entities /// /// Vulnerable package version ranges IQueryable GetAllVulnerableRanges(); - } } \ No newline at end of file diff --git a/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs index f5f73edd70..ecfad5abd6 100644 --- a/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs +++ b/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs @@ -73,9 +73,6 @@ public async Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, b } } - public IQueryable GetVulnerableRangesById(string packageId) => - _entitiesContext.VulnerableRanges.Where(x => x.PackageId == packageId); - public IQueryable GetAllVulnerableRanges() => _entitiesContext.Set(); diff --git a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs index 54c852733d..0305f35802 100644 --- a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs @@ -404,5 +404,11 @@ void TrackABTestEvaluated( bool isAuthenticated, int testBucket, int testPercentage); + + /// + /// Track how long it takes to populate the vulnerabilities cache + /// + /// Refresh duration for vulnerabilities cache + void TrackVulnerabilitiesCacheRefreshDuration(long milliseconds); } } \ No newline at end of file diff --git a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs index 7d49f8b335..b46ae8670d 100644 --- a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs @@ -91,6 +91,7 @@ public class Events public const string ABTestEvaluated = "ABTestEvaluated"; public const string PackagePushDisconnect = "PackagePushDisconnect"; public const string SymbolPackagePushDisconnect = "SymbolPackagePushDisconnect"; + public const string VulnerabilitiesCacheRefreshDuration = "VulnerabilitiesCacheRefreshDuration"; } private readonly IDiagnosticsSource _diagnosticsSource; @@ -1103,6 +1104,11 @@ public void TrackSymbolPackagePushDisconnectEvent() TrackMetric(Events.SymbolPackagePushDisconnect, 1, p => { }); } + public void TrackVulnerabilitiesCacheRefreshDuration(long milliseconds) + { + TrackMetric(Events.VulnerabilitiesCacheRefreshDuration, milliseconds, properties => { }); + } + /// /// We use instead of /// diff --git a/src/NuGetGallery/App_Start/AppActivator.cs b/src/NuGetGallery/App_Start/AppActivator.cs index 1a437e7fe0..2d7d80760c 100644 --- a/src/NuGetGallery/App_Start/AppActivator.cs +++ b/src/NuGetGallery/App_Start/AppActivator.cs @@ -265,12 +265,28 @@ private static void BackgroundJobsPostStart(IAppConfiguration configuration) if (configuration.StorageType == StorageType.AzureStorage) { - var cloudDownloadCountService = DependencyResolver.Current.GetService() as CloudDownloadCountService; + var cloudDownloadCountService = + DependencyResolver.Current.GetService() as CloudDownloadCountService; if (cloudDownloadCountService != null) { // Perform initial refresh + schedule new refreshes every 15 minutes HostingEnvironment.QueueBackgroundWorkItem(_ => cloudDownloadCountService.RefreshAsync()); - jobs.Add(new CloudDownloadCountServiceRefreshJob(TimeSpan.FromMinutes(15), cloudDownloadCountService)); + jobs.Add(new CloudDownloadCountServiceRefreshJob(TimeSpan.FromMinutes(15), + cloudDownloadCountService)); + } + } + + var packageVulnerabilitiesCacheService = + DependencyResolver.Current.GetService() as + PackageVulnerabilitiesCacheService; + if (packageVulnerabilitiesCacheService != null) + { + // Perform initial refresh + schedule new refreshes every 30 minutes + HostingEnvironment.QueueBackgroundWorkItem(_ => packageVulnerabilitiesCacheService.RefreshCache()); + if (configuration.StorageType == StorageType.AzureStorage) + { + jobs.Add(new PackageVulnerabilitiesCacheRefreshJob(TimeSpan.FromMinutes(30), + packageVulnerabilitiesCacheService)); } } diff --git a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs index dcac2a5189..931185d68c 100644 --- a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs +++ b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs @@ -460,7 +460,9 @@ protected override void Load(ContainerBuilder builder) .As() .InstancePerLifetimeScope(); - builder.Register(c => new PackageVulnerabilitiesCacheService(c.Resolve())) + builder.Register(c => + new PackageVulnerabilitiesCacheService(c.Resolve(), + c.Resolve())) .AsSelf() .As() .SingleInstance(); diff --git a/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs b/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs new file mode 100644 index 0000000000..cbe3eded5c --- /dev/null +++ b/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs @@ -0,0 +1,25 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Threading.Tasks; +using WebBackgrounder; + +namespace NuGetGallery +{ + public class PackageVulnerabilitiesCacheRefreshJob : Job + { + private readonly PackageVulnerabilitiesCacheService _packageVulnerabilitiesCacheService; + + public PackageVulnerabilitiesCacheRefreshJob(TimeSpan interval, PackageVulnerabilitiesCacheService packageVulnerabilitiesCacheService) + : base("", interval) + { + _packageVulnerabilitiesCacheService = packageVulnerabilitiesCacheService; + } + + public override Task Execute() + { + return new Task(() => _packageVulnerabilitiesCacheService.RefreshCache()); + } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index b4232f1076..e574f390f5 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -227,6 +227,7 @@ + diff --git a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs index c7d1079b63..d7b8f49d5a 100644 --- a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs +++ b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs @@ -2,9 +2,10 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Collections.Concurrent; using System.Collections.Generic; -using System.Collections.ObjectModel; using System.Data.Entity; +using System.Diagnostics; using System.Linq; using NuGet.Services.Entities; @@ -12,17 +13,21 @@ namespace NuGetGallery { public class PackageVulnerabilitiesCacheService : IPackageVulnerabilitiesCacheService { - private const int CachingLimitMinutes = 1440; // We could make this 1 day instead (same value) but this is easier for spot testing the cache - private readonly object Locker = new object(); private IDictionary> vulnerabilitiesById)> vulnerabilitiesByIdCache - = new Dictionary>)>(); + Dictionary>> _vulnerabilitiesByIdCache + = new ConcurrentDictionary>>(); + private readonly object _refreshLock = new object(); + private bool _isRefreshing; private readonly IPackageVulnerabilitiesManagementService _packageVulnerabilitiesManagementService; - public PackageVulnerabilitiesCacheService(IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService) + private readonly ITelemetryService _telemetryService; + + public PackageVulnerabilitiesCacheService( + IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService, + ITelemetryService telemetryService) { _packageVulnerabilitiesManagementService = packageVulnerabilitiesManagementService; - Initialize(); + _telemetryService = telemetryService; } public IReadOnlyDictionary> GetVulnerabilitiesById(string id) @@ -32,52 +37,58 @@ public IReadOnlyDictionary> GetVulnerab throw new ArgumentException("Must have a value.", nameof(id)); } - if (ShouldCachedValueBeUpdated(id)) + if (_vulnerabilitiesByIdCache.TryGetValue(id, out var result)) { - lock (Locker) - { - if (ShouldCachedValueBeUpdated(id)) - { - var packageKeyAndVulnerability = _packageVulnerabilitiesManagementService - .GetVulnerableRangesById(id) - .Include(x => x.Vulnerability) - .SelectMany(x => x.Packages.Select(p => new {PackageKey = p.Key, x.Vulnerability})) - .GroupBy(pv => pv.PackageKey, pv => pv.Vulnerability) - .ToDictionary(pv => pv.Key, - pv => pv.ToList().AsReadOnly() as IReadOnlyList); - - vulnerabilitiesByIdCache[id] = (cachedAt: DateTime.Now, vulnerabilitiesById: packageKeyAndVulnerability); - } - } + return result; } - return vulnerabilitiesByIdCache[id].vulnerabilitiesById.Any() - ? new ReadOnlyDictionary>(vulnerabilitiesByIdCache[id].vulnerabilitiesById) - : null; + return null; } - private void Initialize() + public void RefreshCache() { - // We need to build a dictionary of dictionaries. Breaking it down: - // - this give us a list of all vulnerable package version ranges - vulnerabilitiesByIdCache = _packageVulnerabilitiesManagementService.GetAllVulnerableRanges() - .Include(x => x.Vulnerability) - // - from these we want a list in this format: (, (, )) - // which will allow us to look up the dictionary by id, and return a dictionary of version -> vulnerability - .SelectMany(x => x.Packages.Select(p => new - {PackageId = x.PackageId ?? string.Empty, KeyVulnerability = new {PackageKey = p.Key, x.Vulnerability}})) - .GroupBy(ikv => ikv.PackageId, ikv => ikv.KeyVulnerability) - // - build the outer dictionary, keyed by - each inner dictionary is paired with a time of creation (for cache invalidation) - .ToDictionary(ikv => ikv.Key, - ikv => (cachedAt: DateTime.Now, - vulnerabilitiesById: ikv.GroupBy(kv => kv.PackageKey, kv => kv.Vulnerability) - // - build the inner dictionaries, all under the same , each keyed by - .ToDictionary(kv => kv.Key, - kv => kv.ToList().AsReadOnly() as IReadOnlyList))); - } + bool shouldRefresh = false; + lock (_refreshLock) + { + if (!_isRefreshing) + { + _isRefreshing = true; + shouldRefresh = true; + } + } - private bool ShouldCachedValueBeUpdated(string id) => !vulnerabilitiesByIdCache.ContainsKey(id) || - vulnerabilitiesByIdCache[id].cachedAt - .AddMinutes(CachingLimitMinutes) < DateTime.Now; + if (shouldRefresh) + { + try + { + var stopwatch = Stopwatch.StartNew(); + + // We need to build a dictionary of dictionaries. Breaking it down: + // - this give us a list of all vulnerable package version ranges + _vulnerabilitiesByIdCache = _packageVulnerabilitiesManagementService.GetAllVulnerableRanges() + .Include(x => x.Vulnerability) + // - from these we want a list in this format: (, (, )) + // which will allow us to look up the dictionary by id, and return a dictionary of version -> vulnerability + .SelectMany(x => x.Packages.Select(p => new + { PackageId = x.PackageId ?? string.Empty, KeyVulnerability = new { PackageKey = p.Key, x.Vulnerability } })) + .GroupBy(ikv => ikv.PackageId, ikv => ikv.KeyVulnerability) + // - build the outer dictionary, keyed by - each inner dictionary is paired with a time of creation (for cache invalidation) + .ToDictionary(ikv => ikv.Key, + ikv => + ikv.GroupBy(kv => kv.PackageKey, kv => kv.Vulnerability) + // - build the inner dictionaries, all under the same , each keyed by + .ToDictionary(kv => kv.Key, + kv => kv.ToList().AsReadOnly() as IReadOnlyList)); + + stopwatch.Stop(); + + _telemetryService.TrackVulnerabilitiesCacheRefreshDuration(stopwatch.ElapsedMilliseconds); + } + finally + { + _isRefreshing = false; + } + } + } } } \ No newline at end of file diff --git a/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs b/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs index f94766b3ce..4771260f89 100644 --- a/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs +++ b/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs @@ -71,7 +71,7 @@ public Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, bool wi return Task.CompletedTask; } - public IQueryable GetVulnerableRangesById(string packageId) => throw new NotImplementedException(); + public IQueryable GetAllVulnerableRanges() => throw new NotImplementedException(); private void VerifyVulnerabilityInDatabase(PackageVulnerability vulnerability, bool withdrawn) { diff --git a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs index 47f3a21d39..5cdf19f846 100644 --- a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs +++ b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs @@ -372,5 +372,10 @@ public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, { throw new NotImplementedException(); } + + public void TrackVulnerabilitiesCacheRefreshDuration(long milliseconds) + { + throw new NotImplementedException(); + } } } diff --git a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs index 308c978c71..2fa508e656 100644 --- a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs @@ -13,23 +13,23 @@ namespace NuGetGallery.Services public class PackageVulnerabilitiesCacheServiceFacts : TestContainer { [Fact] - public void InitializesVulnerabilitiesCache() + public void RefreshesVulnerabilitiesCache() { // Arrange var vulnerableVersionRanges = GetVersionRanges(); var pvmService = new Mock(); pvmService.Setup(stub => stub.GetAllVulnerableRanges()).Returns(vulnerableVersionRanges); - pvmService.Setup(stub => stub.GetVulnerableRangesById(It.IsAny())).Verifiable(); - var cacheService = new PackageVulnerabilitiesCacheService(pvmService.Object); + var telemetryService = new Mock(); + telemetryService.Setup(stub => stub.TrackVulnerabilitiesCacheRefreshDuration(It.IsAny())).Verifiable(); + var cacheService = new PackageVulnerabilitiesCacheService(pvmService.Object, telemetryService.Object); + cacheService.RefreshCache(); // Act var vulnerabilitiesFoo = cacheService.GetVulnerabilitiesById("Foo"); var vulnerabilitiesBar = cacheService.GetVulnerabilitiesById("Bar"); // Assert - // This method should never be called (it's only called when cache can't provide, and these values are loaded into the cache on initialize) - pvmService.Verify(s => s.GetVulnerableRangesById(It.IsAny()), Times.Never); - // Test cache contents + telemetryService.Verify(stub => stub.TrackVulnerabilitiesCacheRefreshDuration(It.IsAny()), Times.Once); Assert.Equal(4, vulnerabilitiesFoo.Count); Assert.Equal(1, vulnerabilitiesFoo[0].Count); Assert.Equal(1, vulnerabilitiesFoo[1].Count); diff --git a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesServiceFacts.cs index 77121a2ab9..4324a2a5b3 100644 --- a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesServiceFacts.cs @@ -11,146 +11,49 @@ namespace NuGetGallery.Services { public class PackageVulnerabilitiesServiceFacts : TestContainer { - private PackageRegistration _registrationVulnerable; - - private PackageVulnerability _vulnerabilityCritical; - private PackageVulnerability _vulnerabilityModerate; - - private VulnerablePackageVersionRange _versionRangeCritical; - private VulnerablePackageVersionRange _versionRangeModerate; - - private Package _packageVulnerable100; - private Package _packageVulnerable110; - private Package _packageVulnerable111; - private Package _packageVulnerable112; - - private Package _packageNotVulnerable; - - [Fact] - public void GetsVulnerabilitiesOfPackage() - { - // Arrange - SetUp(); - var packages = new[] - { - _packageVulnerable100, - _packageVulnerable110, - _packageVulnerable111, - _packageVulnerable112, - _packageNotVulnerable - }; - var context = GetFakeContext(); - context.Packages.AddRange(packages); - var target = Get(); - - // Act - var vulnerableResult = target.GetVulnerabilitiesById("Vulnerable"); - var notVulnerableResult = target.GetVulnerabilitiesById("NotVulnerable"); - - // Assert - Assert.Equal(3, vulnerableResult.Count); - var vulnerabilitiesFor100 = vulnerableResult[_packageVulnerable100.Key]; - var vulnerabilitiesFor110 = vulnerableResult[_packageVulnerable110.Key]; - var vulnerabilitiesFor111 = vulnerableResult[_packageVulnerable111.Key]; - Assert.Equal(_vulnerabilityModerate, vulnerabilitiesFor100[0]); - Assert.Equal(_vulnerabilityModerate, vulnerabilitiesFor110[0]); - Assert.Equal(_vulnerabilityModerate, vulnerabilitiesFor111[0]); - Assert.Equal(_vulnerabilityCritical, vulnerabilitiesFor111[1]); - - Assert.Null(notVulnerableResult); - } - [Fact] public void GetsVulnerableStatusOfPackage() { // Arrange - SetUp(); - var context = GetFakeContext(); - var target = Get(); - - // Act - var shouldBeVulnerable = target.IsPackageVulnerable(_packageVulnerable100); - var shouldNotBeVulnerable = target.IsPackageVulnerable(_packageNotVulnerable); - - // Assert - Assert.True(shouldBeVulnerable); - Assert.False(shouldNotBeVulnerable); - } - - private void SetUp() - { - _registrationVulnerable = new PackageRegistration { Id = "Vulnerable" }; + var registrationVulnerable = new PackageRegistration { Id = "Vulnerable" }; - _vulnerabilityCritical = new PackageVulnerability - { - AdvisoryUrl = "http://theurl/1234", - GitHubDatabaseKey = 1234, - Severity = PackageVulnerabilitySeverity.Critical - }; - _vulnerabilityModerate = new PackageVulnerability + var vulnerabilityModerate = new PackageVulnerability { AdvisoryUrl = "http://theurl/5678", GitHubDatabaseKey = 5678, Severity = PackageVulnerabilitySeverity.Moderate }; - _versionRangeCritical = new VulnerablePackageVersionRange + var versionRangeModerate = new VulnerablePackageVersionRange { - Vulnerability = _vulnerabilityCritical, - PackageVersionRange = "1.1.1", - FirstPatchedPackageVersion = "1.1.2" - }; - _versionRangeModerate = new VulnerablePackageVersionRange - { - Vulnerability = _vulnerabilityModerate, + Vulnerability = vulnerabilityModerate, PackageVersionRange = "<=1.1.1", FirstPatchedPackageVersion = "1.1.2" }; - _packageVulnerable100 = new Package + var packageVulnerable = new Package { Key = 0, - PackageRegistration = _registrationVulnerable, + PackageRegistration = registrationVulnerable, Version = "1.0.0", - VulnerablePackageRanges = new List - { - _versionRangeModerate - } - }; - _packageVulnerable110 = new Package - { - Key = 1, - PackageRegistration = _registrationVulnerable, - Version = "1.1.0", - VulnerablePackageRanges = new List - { - _versionRangeModerate - } - }; - _packageVulnerable111 = new Package - { - Key = 3, // simulate a different order in db - create a non-contiguous range of rows, even if the range is contiguous - PackageRegistration = _registrationVulnerable, - Version = "1.1.1", - VulnerablePackageRanges = new List - { - _versionRangeModerate, - _versionRangeCritical - } - }; - _packageVulnerable112 = new Package - { - Key = 2, // simulate a different order in db - create a non-contiguous range of rows, even if the range is contiguous - PackageRegistration = _registrationVulnerable, - Version = "1.1.2", - VulnerablePackageRanges = new List() + VulnerablePackageRanges = new List {versionRangeModerate} }; - _packageNotVulnerable = new Package + var packageNotVulnerable = new Package { Key = 4, PackageRegistration = new PackageRegistration { Id = "NotVulnerable" }, VulnerablePackageRanges = new List() - }; + }; + + var target = Get(); + + // Act + var shouldBeVulnerable = target.IsPackageVulnerable(packageVulnerable); + var shouldNotBeVulnerable = target.IsPackageVulnerable(packageNotVulnerable); + + // Assert + Assert.True(shouldBeVulnerable); + Assert.False(shouldNotBeVulnerable); } } } diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs index 7cc3362273..ae089fcc1b 100644 --- a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs @@ -343,6 +343,10 @@ public static IEnumerable TrackMetricNames_Data yield return new object[] { "SymbolPackagePushDisconnect", (TrackAction)(s => s.TrackSymbolPackagePushDisconnectEvent()) }; + + yield return new object[] { "VulnerabilitiesCacheRefreshDuration", + (TrackAction)(s => s.TrackVulnerabilitiesCacheRefreshDuration(0)) + }; } } From 7064b800c2f0638cf7a627734fc3d20f7ad2bad1 Mon Sep 17 00:00:00 2001 From: Drew Gillies Date: Wed, 19 May 2021 16:20:02 +1000 Subject: [PATCH 4/5] Addressed feedback --- .../Gallery/ThrowingTelemetryService.cs | 2 +- ...PackageVulnerabilitiesManagementService.cs | 6 -- ...PackageVulnerabilitiesManagementService.cs | 3 - .../Telemetry/ITelemetryService.cs | 2 +- .../Telemetry/TelemetryService.cs | 6 +- src/NuGetGallery/App_Start/AppActivator.cs | 7 ++- .../App_Start/DefaultDependenciesModule.cs | 4 +- .../PackageVulnerabilitiesCacheRefreshJob.cs | 9 ++- .../PackageVulnerabilitiesCacheService.cs | 61 +++++++++++-------- .../Verify/PackageVulnerabilitiesVerifier.cs | 2 - .../Fakes/FakeTelemetryService.cs | 2 +- ...PackageVulnerabilitiesCacheServiceFacts.cs | 48 +++++++++++---- .../Services/TelemetryServiceFacts.cs | 4 +- 13 files changed, 92 insertions(+), 64 deletions(-) diff --git a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs index cee59f9c4e..5f1638a083 100644 --- a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs +++ b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs @@ -371,7 +371,7 @@ public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, throw new NotImplementedException(); } - public void TrackVulnerabilitiesCacheRefreshDuration(long duration) + public void TrackVulnerabilitiesCacheRefreshDurationMs(long duration) { throw new NotImplementedException(); } diff --git a/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs index 35958a64a8..73770dbe00 100644 --- a/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs +++ b/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs @@ -23,11 +23,5 @@ public interface IPackageVulnerabilitiesManagementService /// /// Whether or not the vulnerability was withdrawn. Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, bool withdrawn); - - /// - /// Get the full set of vulnerable package entities - /// - /// Vulnerable package version ranges - IQueryable GetAllVulnerableRanges(); } } \ No newline at end of file diff --git a/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs index ecfad5abd6..f16dcf0d42 100644 --- a/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs +++ b/src/NuGetGallery.Services/PackageManagement/PackageVulnerabilitiesManagementService.cs @@ -73,9 +73,6 @@ public async Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, b } } - public IQueryable GetAllVulnerableRanges() => - _entitiesContext.Set(); - /// /// Updates the database with . /// diff --git a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs index 0305f35802..11bef83bf4 100644 --- a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs @@ -409,6 +409,6 @@ void TrackABTestEvaluated( /// Track how long it takes to populate the vulnerabilities cache /// /// Refresh duration for vulnerabilities cache - void TrackVulnerabilitiesCacheRefreshDuration(long milliseconds); + void TrackVulnerabilitiesCacheRefreshDurationMs(long milliseconds); } } \ No newline at end of file diff --git a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs index b46ae8670d..e4796e50a0 100644 --- a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs @@ -91,7 +91,7 @@ public class Events public const string ABTestEvaluated = "ABTestEvaluated"; public const string PackagePushDisconnect = "PackagePushDisconnect"; public const string SymbolPackagePushDisconnect = "SymbolPackagePushDisconnect"; - public const string VulnerabilitiesCacheRefreshDuration = "VulnerabilitiesCacheRefreshDuration"; + public const string VulnerabilitiesCacheRefreshDurationMs = "VulnerabilitiesCacheRefreshDurationMs"; } private readonly IDiagnosticsSource _diagnosticsSource; @@ -1104,9 +1104,9 @@ public void TrackSymbolPackagePushDisconnectEvent() TrackMetric(Events.SymbolPackagePushDisconnect, 1, p => { }); } - public void TrackVulnerabilitiesCacheRefreshDuration(long milliseconds) + public void TrackVulnerabilitiesCacheRefreshDurationMs(long milliseconds) { - TrackMetric(Events.VulnerabilitiesCacheRefreshDuration, milliseconds, properties => { }); + TrackMetric(Events.VulnerabilitiesCacheRefreshDurationMs, milliseconds, properties => { }); } /// diff --git a/src/NuGetGallery/App_Start/AppActivator.cs b/src/NuGetGallery/App_Start/AppActivator.cs index 2d7d80760c..3a796c97c0 100644 --- a/src/NuGetGallery/App_Start/AppActivator.cs +++ b/src/NuGetGallery/App_Start/AppActivator.cs @@ -15,7 +15,7 @@ using System.Web.Routing; using System.Web.UI; using Elmah; -using Microsoft.WindowsAzure.ServiceRuntime; +using Microsoft.Extensions.DependencyInjection; using NuGetGallery; using NuGetGallery.Configuration; using NuGetGallery.Diagnostics; @@ -282,11 +282,12 @@ private static void BackgroundJobsPostStart(IAppConfiguration configuration) if (packageVulnerabilitiesCacheService != null) { // Perform initial refresh + schedule new refreshes every 30 minutes - HostingEnvironment.QueueBackgroundWorkItem(_ => packageVulnerabilitiesCacheService.RefreshCache()); + var serviceScopeFactory = DependencyResolver.Current.GetService(); + HostingEnvironment.QueueBackgroundWorkItem(_ => packageVulnerabilitiesCacheService.RefreshCache(serviceScopeFactory)); if (configuration.StorageType == StorageType.AzureStorage) { jobs.Add(new PackageVulnerabilitiesCacheRefreshJob(TimeSpan.FromMinutes(30), - packageVulnerabilitiesCacheService)); + packageVulnerabilitiesCacheService, serviceScopeFactory)); } } diff --git a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs index 931185d68c..b2331f198b 100644 --- a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs +++ b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs @@ -460,9 +460,7 @@ protected override void Load(ContainerBuilder builder) .As() .InstancePerLifetimeScope(); - builder.Register(c => - new PackageVulnerabilitiesCacheService(c.Resolve(), - c.Resolve())) + builder.Register(c => new PackageVulnerabilitiesCacheService(c.Resolve())) .AsSelf() .As() .SingleInstance(); diff --git a/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs b/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs index cbe3eded5c..f68c9ef762 100644 --- a/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs +++ b/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs @@ -3,6 +3,7 @@ using System; using System.Threading.Tasks; +using Microsoft.Extensions.DependencyInjection; using WebBackgrounder; namespace NuGetGallery @@ -10,16 +11,20 @@ namespace NuGetGallery public class PackageVulnerabilitiesCacheRefreshJob : Job { private readonly PackageVulnerabilitiesCacheService _packageVulnerabilitiesCacheService; + private IServiceScopeFactory _serviceScopeFactory; - public PackageVulnerabilitiesCacheRefreshJob(TimeSpan interval, PackageVulnerabilitiesCacheService packageVulnerabilitiesCacheService) + public PackageVulnerabilitiesCacheRefreshJob(TimeSpan interval, + PackageVulnerabilitiesCacheService packageVulnerabilitiesCacheService, + IServiceScopeFactory serviceScopeFactory) : base("", interval) { _packageVulnerabilitiesCacheService = packageVulnerabilitiesCacheService; + _serviceScopeFactory = serviceScopeFactory; } public override Task Execute() { - return new Task(() => _packageVulnerabilitiesCacheService.RefreshCache()); + return new Task(() => _packageVulnerabilitiesCacheService.RefreshCache(_serviceScopeFactory)); } } } \ No newline at end of file diff --git a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs index d7b8f49d5a..66248fe1f8 100644 --- a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs +++ b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs @@ -7,6 +7,7 @@ using System.Data.Entity; using System.Diagnostics; using System.Linq; +using Microsoft.Extensions.DependencyInjection; using NuGet.Services.Entities; namespace NuGetGallery @@ -15,18 +16,14 @@ public class PackageVulnerabilitiesCacheService : IPackageVulnerabilitiesCacheSe { private IDictionary>> _vulnerabilitiesByIdCache - = new ConcurrentDictionary>>(); + = new Dictionary>>(); private readonly object _refreshLock = new object(); private bool _isRefreshing; - private readonly IPackageVulnerabilitiesManagementService _packageVulnerabilitiesManagementService; private readonly ITelemetryService _telemetryService; - public PackageVulnerabilitiesCacheService( - IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService, - ITelemetryService telemetryService) + public PackageVulnerabilitiesCacheService(ITelemetryService telemetryService) { - _packageVulnerabilitiesManagementService = packageVulnerabilitiesManagementService; _telemetryService = telemetryService; } @@ -45,8 +42,13 @@ public IReadOnlyDictionary> GetVulnerab return null; } - public void RefreshCache() + public void RefreshCache(IServiceScopeFactory serviceScopeFactory) { + if (serviceScopeFactory == null) + { + throw new ArgumentNullException(nameof(serviceScopeFactory)); + } + bool shouldRefresh = false; lock (_refreshLock) { @@ -63,26 +65,37 @@ public void RefreshCache() { var stopwatch = Stopwatch.StartNew(); - // We need to build a dictionary of dictionaries. Breaking it down: - // - this give us a list of all vulnerable package version ranges - _vulnerabilitiesByIdCache = _packageVulnerabilitiesManagementService.GetAllVulnerableRanges() - .Include(x => x.Vulnerability) - // - from these we want a list in this format: (, (, )) - // which will allow us to look up the dictionary by id, and return a dictionary of version -> vulnerability - .SelectMany(x => x.Packages.Select(p => new + // Create a unique service scope for each refresh to ensure a fresh entities context + using (var serviceScope = serviceScopeFactory.CreateScope()) + { + var serviceProvider = serviceScope.ServiceProvider; + var entitiesContext = serviceProvider.GetService(); + + // We need to build a dictionary of dictionaries. Breaking it down: + // - this give us a list of all vulnerable package version ranges + _vulnerabilitiesByIdCache = entitiesContext.Set() + .Include(x => x.Vulnerability) + // - from these we want a list in this format: (, (, )) + // which will allow us to look up the dictionary by id, and return a dictionary of version -> vulnerability + .SelectMany(x => x.Packages.Select(p => new { PackageId = x.PackageId ?? string.Empty, KeyVulnerability = new { PackageKey = p.Key, x.Vulnerability } })) - .GroupBy(ikv => ikv.PackageId, ikv => ikv.KeyVulnerability) - // - build the outer dictionary, keyed by - each inner dictionary is paired with a time of creation (for cache invalidation) - .ToDictionary(ikv => ikv.Key, - ikv => - ikv.GroupBy(kv => kv.PackageKey, kv => kv.Vulnerability) - // - build the inner dictionaries, all under the same , each keyed by - .ToDictionary(kv => kv.Key, - kv => kv.ToList().AsReadOnly() as IReadOnlyList)); - + .GroupBy(ikv => ikv.PackageId, ikv => ikv.KeyVulnerability) + // - build the outer dictionary, keyed by - each inner dictionary is paired with a time of creation (for cache invalidation) + .ToDictionary(ikv => ikv.Key, + ikv => + ikv.GroupBy(kv => kv.PackageKey, kv => kv.Vulnerability) + // - build the inner dictionaries, all under the same , each keyed by + .ToDictionary(kv => kv.Key, + kv => kv.ToList().AsReadOnly() as IReadOnlyList)); + } + stopwatch.Stop(); - _telemetryService.TrackVulnerabilitiesCacheRefreshDuration(stopwatch.ElapsedMilliseconds); + _telemetryService.TrackVulnerabilitiesCacheRefreshDurationMs(stopwatch.ElapsedMilliseconds); + } + catch (Exception ex) + { + _telemetryService.TraceException(ex); } finally { diff --git a/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs b/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs index 4771260f89..cc0775ddc5 100644 --- a/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs +++ b/src/VerifyGitHubVulnerabilities/Verify/PackageVulnerabilitiesVerifier.cs @@ -71,8 +71,6 @@ public Task UpdateVulnerabilityAsync(PackageVulnerability vulnerability, bool wi return Task.CompletedTask; } - public IQueryable GetAllVulnerableRanges() => throw new NotImplementedException(); - private void VerifyVulnerabilityInDatabase(PackageVulnerability vulnerability, bool withdrawn) { Console.WriteLine($"[Database] Verifying vulnerability {vulnerability.GitHubDatabaseKey}."); diff --git a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs index 5cdf19f846..ce12561c5b 100644 --- a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs +++ b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs @@ -373,7 +373,7 @@ public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, throw new NotImplementedException(); } - public void TrackVulnerabilitiesCacheRefreshDuration(long milliseconds) + public void TrackVulnerabilitiesCacheRefreshDurationMs(long milliseconds) { throw new NotImplementedException(); } diff --git a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs index 2fa508e656..8c510ba0c7 100644 --- a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs @@ -1,8 +1,11 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; +using System.Data.Entity; using System.Linq; +using Microsoft.Extensions.DependencyInjection; using Moq; using NuGet.Services.Entities; using NuGetGallery.Framework; @@ -16,20 +19,30 @@ public class PackageVulnerabilitiesCacheServiceFacts : TestContainer public void RefreshesVulnerabilitiesCache() { // Arrange - var vulnerableVersionRanges = GetVersionRanges(); - var pvmService = new Mock(); - pvmService.Setup(stub => stub.GetAllVulnerableRanges()).Returns(vulnerableVersionRanges); + var entitiesContext = new Mock(); + entitiesContext.Setup(x => x.Set()).Returns(GetVulnerableRanges()); + var serviceProvider = new Mock(); + serviceProvider.Setup(x => x.GetService(typeof(IEntitiesContext))).Returns(entitiesContext.Object); + var serviceScope = new Mock(); + serviceScope.Setup(x => x.ServiceProvider).Returns(serviceProvider.Object); + serviceScope.Setup(x => x.Dispose()).Verifiable(); + var serviceScopeFactory = new Mock(); + serviceScopeFactory.Setup(x => x.CreateScope()).Returns(serviceScope.Object); var telemetryService = new Mock(); - telemetryService.Setup(stub => stub.TrackVulnerabilitiesCacheRefreshDuration(It.IsAny())).Verifiable(); - var cacheService = new PackageVulnerabilitiesCacheService(pvmService.Object, telemetryService.Object); - cacheService.RefreshCache(); + telemetryService.Setup(x => x.TrackVulnerabilitiesCacheRefreshDurationMs(It.IsAny())).Verifiable(); + var cacheService = new PackageVulnerabilitiesCacheService(telemetryService.Object); + cacheService.RefreshCache(serviceScopeFactory.Object); // Act var vulnerabilitiesFoo = cacheService.GetVulnerabilitiesById("Foo"); var vulnerabilitiesBar = cacheService.GetVulnerabilitiesById("Bar"); // Assert - telemetryService.Verify(stub => stub.TrackVulnerabilitiesCacheRefreshDuration(It.IsAny()), Times.Once); + // - ensure telemetry is sent + telemetryService.Verify(x => x.TrackVulnerabilitiesCacheRefreshDurationMs(It.IsAny()), Times.Once); + // - ensure scope is disposed + serviceScope.Verify(x => x.Dispose(), Times.AtLeastOnce); + // - ensure contants of cache are correct Assert.Equal(4, vulnerabilitiesFoo.Count); Assert.Equal(1, vulnerabilitiesFoo[0].Count); Assert.Equal(1, vulnerabilitiesFoo[1].Count); @@ -43,7 +56,7 @@ public void RefreshesVulnerabilitiesCache() Assert.Equal(1, vulnerabilitiesBar[6].Count); } - private IQueryable GetVersionRanges() + DbSet GetVulnerableRanges() { var registrationFoo = new PackageRegistration { Id = "Foo" }; var registrationBar = new PackageRegistration { Id = "Bar" }; @@ -151,12 +164,21 @@ private IQueryable GetVersionRanges() } }; - versionRangeCriticalFoo.Packages = new List {packageFoo111}; - versionRangeModerateFoo.Packages = new List {packageFoo100, packageFoo110, packageFoo111, packageFoo112}; - versionRangeCriticalBar.Packages = new List {packageBar100, packageBar110}; + versionRangeCriticalFoo.Packages = new List { packageFoo111 }; + versionRangeModerateFoo.Packages = new List { packageFoo100, packageFoo110, packageFoo111, packageFoo112 }; + versionRangeCriticalBar.Packages = new List { packageBar100, packageBar110 }; - return new List - {versionRangeCriticalFoo, versionRangeModerateFoo, versionRangeCriticalBar}.AsQueryable(); + var vulnerableRangeList = new List { versionRangeCriticalFoo, versionRangeModerateFoo, versionRangeCriticalBar }.AsQueryable(); + var vulnerableRangeDbSet = new Mock>(); + + // boilerplate mock DbSet redirects: + vulnerableRangeDbSet.As().Setup(x => x.Provider).Returns(vulnerableRangeList.Provider); + vulnerableRangeDbSet.As().Setup(x => x.Expression).Returns(vulnerableRangeList.Expression); + vulnerableRangeDbSet.As().Setup(x => x.ElementType).Returns(vulnerableRangeList.ElementType); + vulnerableRangeDbSet.As().Setup(x => x.GetEnumerator()).Returns(vulnerableRangeList.GetEnumerator()); + vulnerableRangeDbSet.Setup(x => x.Include(It.IsAny())).Returns(vulnerableRangeDbSet.Object); // bypass includes (which break the test) + + return vulnerableRangeDbSet.Object; } } } diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs index ae089fcc1b..96f11e938a 100644 --- a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs @@ -344,8 +344,8 @@ public static IEnumerable TrackMetricNames_Data (TrackAction)(s => s.TrackSymbolPackagePushDisconnectEvent()) }; - yield return new object[] { "VulnerabilitiesCacheRefreshDuration", - (TrackAction)(s => s.TrackVulnerabilitiesCacheRefreshDuration(0)) + yield return new object[] { "VulnerabilitiesCacheRefreshDurationMs", + (TrackAction)(s => s.TrackVulnerabilitiesCacheRefreshDurationMs(0)) }; } } From 5a92c9fd906fd0a03135b661c8a8d4610d5503ec Mon Sep 17 00:00:00 2001 From: Drew Gillies Date: Thu, 20 May 2021 09:00:35 +1000 Subject: [PATCH 5/5] Addressed feedback --- .../Gallery/ThrowingTelemetryService.cs | 4 ++-- .../Telemetry/ITelemetryService.cs | 6 ++--- .../Telemetry/TelemetryService.cs | 8 +++---- src/NuGetGallery/App_Start/AppActivator.cs | 22 +++++-------------- .../App_Start/DefaultDependenciesModule.cs | 2 +- .../PackageVulnerabilitiesCacheRefreshJob.cs | 4 ++-- .../Services/CloudDownloadCountService.cs | 2 +- .../IPackageVulnerabilitiesCacheService.cs | 7 ++++++ .../PackageVulnerabilitiesCacheService.cs | 4 ++-- .../Fakes/FakeTelemetryService.cs | 4 ++-- ...PackageVulnerabilitiesCacheServiceFacts.cs | 4 ++-- .../Services/TelemetryServiceFacts.cs | 4 ++-- 12 files changed, 34 insertions(+), 37 deletions(-) diff --git a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs index 5f1638a083..6eb5a27ecc 100644 --- a/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs +++ b/src/GitHubVulnerabilities2Db/Gallery/ThrowingTelemetryService.cs @@ -66,7 +66,7 @@ public void TrackDownloadCountDecreasedDuringRefresh(string packageId, string pa throw new NotImplementedException(); } - public void TrackDownloadJsonRefreshDuration(long milliseconds) + public void TrackDownloadJsonRefreshDuration(TimeSpan duration) { throw new NotImplementedException(); } @@ -371,7 +371,7 @@ public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, throw new NotImplementedException(); } - public void TrackVulnerabilitiesCacheRefreshDurationMs(long duration) + public void TrackVulnerabilitiesCacheRefreshDuration(TimeSpan duration) { throw new NotImplementedException(); } diff --git a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs index 11bef83bf4..14b9695318 100644 --- a/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/ITelemetryService.cs @@ -16,7 +16,7 @@ public interface ITelemetryService void TrackGetPackageRegistrationDownloadCountFailed(string packageId); - void TrackDownloadJsonRefreshDuration(long milliseconds); + void TrackDownloadJsonRefreshDuration(TimeSpan duration); void TrackDownloadCountDecreasedDuringRefresh(string packageId, string packageVersion, long oldCount, long newCount); @@ -408,7 +408,7 @@ void TrackABTestEvaluated( /// /// Track how long it takes to populate the vulnerabilities cache /// - /// Refresh duration for vulnerabilities cache - void TrackVulnerabilitiesCacheRefreshDurationMs(long milliseconds); + /// Refresh duration for vulnerabilities cache + void TrackVulnerabilitiesCacheRefreshDuration(TimeSpan duration); } } \ No newline at end of file diff --git a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs index e4796e50a0..c31c7c29b4 100644 --- a/src/NuGetGallery.Services/Telemetry/TelemetryService.cs +++ b/src/NuGetGallery.Services/Telemetry/TelemetryService.cs @@ -261,9 +261,9 @@ public void TrackGetPackageRegistrationDownloadCountFailed(string packageId) }); } - public void TrackDownloadJsonRefreshDuration(long milliseconds) + public void TrackDownloadJsonRefreshDuration(TimeSpan duration) { - TrackMetric(Events.DownloadJsonRefreshDuration, milliseconds, properties => { }); + TrackMetric(Events.DownloadJsonRefreshDuration, duration.TotalMilliseconds, properties => { }); } public void TrackDownloadCountDecreasedDuringRefresh(string packageId, string packageVersion, long oldCount, long newCount) @@ -1104,9 +1104,9 @@ public void TrackSymbolPackagePushDisconnectEvent() TrackMetric(Events.SymbolPackagePushDisconnect, 1, p => { }); } - public void TrackVulnerabilitiesCacheRefreshDurationMs(long milliseconds) + public void TrackVulnerabilitiesCacheRefreshDuration(TimeSpan duration) { - TrackMetric(Events.VulnerabilitiesCacheRefreshDurationMs, milliseconds, properties => { }); + TrackMetric(Events.VulnerabilitiesCacheRefreshDurationMs, duration.TotalMilliseconds, properties => { }); } /// diff --git a/src/NuGetGallery/App_Start/AppActivator.cs b/src/NuGetGallery/App_Start/AppActivator.cs index 3a796c97c0..20de1a9c2a 100644 --- a/src/NuGetGallery/App_Start/AppActivator.cs +++ b/src/NuGetGallery/App_Start/AppActivator.cs @@ -265,8 +265,7 @@ private static void BackgroundJobsPostStart(IAppConfiguration configuration) if (configuration.StorageType == StorageType.AzureStorage) { - var cloudDownloadCountService = - DependencyResolver.Current.GetService() as CloudDownloadCountService; + var cloudDownloadCountService = DependencyResolver.Current.GetService() as CloudDownloadCountService; if (cloudDownloadCountService != null) { // Perform initial refresh + schedule new refreshes every 15 minutes @@ -276,20 +275,11 @@ private static void BackgroundJobsPostStart(IAppConfiguration configuration) } } - var packageVulnerabilitiesCacheService = - DependencyResolver.Current.GetService() as - PackageVulnerabilitiesCacheService; - if (packageVulnerabilitiesCacheService != null) - { - // Perform initial refresh + schedule new refreshes every 30 minutes - var serviceScopeFactory = DependencyResolver.Current.GetService(); - HostingEnvironment.QueueBackgroundWorkItem(_ => packageVulnerabilitiesCacheService.RefreshCache(serviceScopeFactory)); - if (configuration.StorageType == StorageType.AzureStorage) - { - jobs.Add(new PackageVulnerabilitiesCacheRefreshJob(TimeSpan.FromMinutes(30), - packageVulnerabilitiesCacheService, serviceScopeFactory)); - } - } + // Perform initial refresh for vulnerabilities cache + schedule new refreshes every 30 minutes + var packageVulnerabilitiesCacheService = DependencyResolver.Current.GetService(); + var serviceScopeFactory = DependencyResolver.Current.GetService(); + HostingEnvironment.QueueBackgroundWorkItem(_ => packageVulnerabilitiesCacheService.RefreshCache(serviceScopeFactory)); + jobs.Add(new PackageVulnerabilitiesCacheRefreshJob(TimeSpan.FromMinutes(30), packageVulnerabilitiesCacheService, serviceScopeFactory)); if (jobs.AnySafe()) { diff --git a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs index b2331f198b..96950c4ca3 100644 --- a/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs +++ b/src/NuGetGallery/App_Start/DefaultDependenciesModule.cs @@ -460,7 +460,7 @@ protected override void Load(ContainerBuilder builder) .As() .InstancePerLifetimeScope(); - builder.Register(c => new PackageVulnerabilitiesCacheService(c.Resolve())) + builder.RegisterType() .AsSelf() .As() .SingleInstance(); diff --git a/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs b/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs index f68c9ef762..3c866437a8 100644 --- a/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs +++ b/src/NuGetGallery/Infrastructure/Jobs/PackageVulnerabilitiesCacheRefreshJob.cs @@ -10,11 +10,11 @@ namespace NuGetGallery { public class PackageVulnerabilitiesCacheRefreshJob : Job { - private readonly PackageVulnerabilitiesCacheService _packageVulnerabilitiesCacheService; + private readonly IPackageVulnerabilitiesCacheService _packageVulnerabilitiesCacheService; private IServiceScopeFactory _serviceScopeFactory; public PackageVulnerabilitiesCacheRefreshJob(TimeSpan interval, - PackageVulnerabilitiesCacheService packageVulnerabilitiesCacheService, + IPackageVulnerabilitiesCacheService packageVulnerabilitiesCacheService, IServiceScopeFactory serviceScopeFactory) : base("", interval) { diff --git a/src/NuGetGallery/Services/CloudDownloadCountService.cs b/src/NuGetGallery/Services/CloudDownloadCountService.cs index 4380558d19..12306a2b7c 100644 --- a/src/NuGetGallery/Services/CloudDownloadCountService.cs +++ b/src/NuGetGallery/Services/CloudDownloadCountService.cs @@ -106,7 +106,7 @@ public async Task RefreshAsync() var stopwatch = Stopwatch.StartNew(); await RefreshCoreAsync(); stopwatch.Stop(); - _telemetryService.TrackDownloadJsonRefreshDuration(stopwatch.ElapsedMilliseconds); + _telemetryService.TrackDownloadJsonRefreshDuration(TimeSpan.FromMilliseconds(stopwatch.ElapsedMilliseconds)); } catch (WebException ex) diff --git a/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs index 88a8dea3a9..8f10fe113d 100644 --- a/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs +++ b/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using Microsoft.Extensions.DependencyInjection; using NuGet.Services.Entities; namespace NuGetGallery @@ -16,5 +17,11 @@ public interface IPackageVulnerabilitiesCacheService /// This function is used to get the packages by id dictionary from the cache /// IReadOnlyDictionary> GetVulnerabilitiesById(string id); + + /// + /// This function will refresh the cache from the database, to be called at regular intervals + /// + /// The factory which will provide a new service scope for each refresh + void RefreshCache(IServiceScopeFactory serviceScopeFactory); } } diff --git a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs index 66248fe1f8..b305a65542 100644 --- a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs +++ b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs @@ -24,7 +24,7 @@ Dictionary>> _vulnerabilitiesByIdCache public PackageVulnerabilitiesCacheService(ITelemetryService telemetryService) { - _telemetryService = telemetryService; + _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService)); } public IReadOnlyDictionary> GetVulnerabilitiesById(string id) @@ -91,7 +91,7 @@ public void RefreshCache(IServiceScopeFactory serviceScopeFactory) stopwatch.Stop(); - _telemetryService.TrackVulnerabilitiesCacheRefreshDurationMs(stopwatch.ElapsedMilliseconds); + _telemetryService.TrackVulnerabilitiesCacheRefreshDuration(TimeSpan.FromMilliseconds(stopwatch.ElapsedMilliseconds)); } catch (Exception ex) { diff --git a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs index ce12561c5b..3451277f76 100644 --- a/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs +++ b/src/VerifyMicrosoftPackage/Fakes/FakeTelemetryService.cs @@ -63,7 +63,7 @@ public void TrackDownloadCountDecreasedDuringRefresh(string packageId, string pa throw new NotImplementedException(); } - public void TrackDownloadJsonRefreshDuration(long milliseconds) + public void TrackDownloadJsonRefreshDuration(TimeSpan duration) { throw new NotImplementedException(); } @@ -373,7 +373,7 @@ public void TrackVerifyPackageKeyEvent(string packageId, string packageVersion, throw new NotImplementedException(); } - public void TrackVulnerabilitiesCacheRefreshDurationMs(long milliseconds) + public void TrackVulnerabilitiesCacheRefreshDuration(TimeSpan duration) { throw new NotImplementedException(); } diff --git a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs index 8c510ba0c7..04d9dc043b 100644 --- a/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/PackageVulnerabilitiesCacheServiceFacts.cs @@ -29,7 +29,7 @@ public void RefreshesVulnerabilitiesCache() var serviceScopeFactory = new Mock(); serviceScopeFactory.Setup(x => x.CreateScope()).Returns(serviceScope.Object); var telemetryService = new Mock(); - telemetryService.Setup(x => x.TrackVulnerabilitiesCacheRefreshDurationMs(It.IsAny())).Verifiable(); + telemetryService.Setup(x => x.TrackVulnerabilitiesCacheRefreshDuration(It.IsAny())).Verifiable(); var cacheService = new PackageVulnerabilitiesCacheService(telemetryService.Object); cacheService.RefreshCache(serviceScopeFactory.Object); @@ -39,7 +39,7 @@ public void RefreshesVulnerabilitiesCache() // Assert // - ensure telemetry is sent - telemetryService.Verify(x => x.TrackVulnerabilitiesCacheRefreshDurationMs(It.IsAny()), Times.Once); + telemetryService.Verify(x => x.TrackVulnerabilitiesCacheRefreshDuration(It.IsAny()), Times.Once); // - ensure scope is disposed serviceScope.Verify(x => x.Dispose(), Times.AtLeastOnce); // - ensure contants of cache are correct diff --git a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs index 96f11e938a..2fc8ea2e84 100644 --- a/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs +++ b/tests/NuGetGallery.Facts/Services/TelemetryServiceFacts.cs @@ -61,7 +61,7 @@ public static IEnumerable TrackMetricNames_Data }; yield return new object[] { "DownloadJsonRefreshDuration", - (TrackAction)(s => s.TrackDownloadJsonRefreshDuration(0)) + (TrackAction)(s => s.TrackDownloadJsonRefreshDuration(TimeSpan.FromMilliseconds(0))) }; yield return new object[] { "DownloadCountDecreasedDuringRefresh", @@ -345,7 +345,7 @@ public static IEnumerable TrackMetricNames_Data }; yield return new object[] { "VulnerabilitiesCacheRefreshDurationMs", - (TrackAction)(s => s.TrackVulnerabilitiesCacheRefreshDurationMs(0)) + (TrackAction)(s => s.TrackVulnerabilitiesCacheRefreshDuration(TimeSpan.FromMilliseconds(0))) }; } }