Addressed feedback (WIP)

NuGet · May 17, 2021 · 842628b · 842628b
1 parent c0a6f85
commit 842628b
Show file tree

Hide file tree

Showing 6 changed files with 45 additions and 30 deletions.
diff --git a/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs b/src/NuGetGallery.Services/PackageManagement/IPackageVulnerabilitiesManagementService.cs
@@ -30,5 +30,12 @@ public interface IPackageVulnerabilitiesManagementService
         /// <param name="packageId">The package's Id</param>
         /// <returns>The package's vulnerable ranges, connecting it to <see cref="PackageVulnerability" /> instances</returns>
         IQueryable<VulnerablePackageVersionRange> GetVulnerableRangesById(string packageId);
+
+        /// <summary>
+        /// Get the full set of vulnerable package entities
+        /// </summary>
+        /// <returns>Vulnerable package version ranges</returns>
+        IQueryable<VulnerablePackageVersionRange> GetAllVulnerableRanges();
+
     }
 }
diff --git 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<VulnerablePackageVersionRange> GetVulnerableRangesById(string packageId) =>
             _entitiesContext.VulnerableRanges.Where(x => x.PackageId == packageId);
 
+        public IQueryable<VulnerablePackageVersionRange> GetAllVulnerableRanges() =>
+            _entitiesContext.Set<VulnerablePackageVersionRange>();
+
         /// <summary>
         /// Updates the database with <paramref name="vulnerability"/>.
         /// </summary>

diff --git 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<IPackageVulnerabilitiesManagementService>()
                 .InstancePerLifetimeScope();
 
-            builder.RegisterType<PackageVulnerabilitiesCacheService>()
+            builder.Register(c => new PackageVulnerabilitiesCacheService(c.Resolve<IPackageVulnerabilitiesManagementService>()))
                 .AsSelf()
                 .As<IPackageVulnerabilitiesCacheService>()
                 .SingleInstance();

diff --git a/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/IPackageVulnerabilitiesCacheService.cs
@@ -15,7 +15,6 @@ public interface IPackageVulnerabilitiesCacheService
         /// <summary>
         /// This function is used to get the packages by id dictionary from the cache
         /// </summary>
-        IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> GetVulnerabilitiesById(string id,
-            IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService);
+        IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> GetVulnerabilitiesById(string id);
     }
 }
diff --git a/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs b/src/NuGetGallery/Services/PackageVulnerabilitiesCacheService.cs
@@ -12,50 +12,62 @@ 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<string,
-            (DateTime cachedAt, IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> vulnerabilitiesById)> vulnerabilitiesByIdCache
-            = new Dictionary<string, (DateTime, IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>>)>();
+        private IDictionary<string,
+            (DateTime cachedAt, Dictionary<int, IReadOnlyList<PackageVulnerability>> vulnerabilitiesById)> vulnerabilitiesByIdCache
+            = new Dictionary<string, (DateTime, Dictionary<int, IReadOnlyList<PackageVulnerability>>)>();
 
-        public IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> GetVulnerabilitiesById(string id,
-            IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService)
+        private readonly IPackageVulnerabilitiesManagementService _packageVulnerabilitiesManagementService;
+        public PackageVulnerabilitiesCacheService(IPackageVulnerabilitiesManagementService packageVulnerabilitiesManagementService)
+        {
+            _packageVulnerabilitiesManagementService = packageVulnerabilitiesManagementService;
+            Initialize();
+        }
+
+        public IReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>> 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))
             {
                 lock (Locker)
                 {
                     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<PackageVulnerability>);
 
-                        var result = !packageKeyAndVulnerability.Any()
-                            ? null
-                            : new ReadOnlyDictionary<int, IReadOnlyList<PackageVulnerability>>(
-                                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<int, IReadOnlyList<PackageVulnerability>>(vulnerabilitiesByIdCache[id].vulnerabilitiesById)
+                : null;
+        }
+
+        private void Initialize()
+        {
+            vulnerabilitiesByIdCache = _packageVulnerabilitiesManagementService.GetAllVulnerableRanges()
+                .Include(x => x.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)
+                .ToDictionary(ikv => ikv.Key,
+                    ikv => (cachedAt: DateTime.Now, 
+                            vulnerabilitiesById: ikv.GroupBy(kv => kv.PackageKey, kv => kv.Vulnerability)
+                        .ToDictionary(kv => kv.Key,
+                            kv => kv.ToList().AsReadOnly() as IReadOnlyList<PackageVulnerability>)));
         }
 
         private bool ShouldCachedValueBeUpdated(string id) => !vulnerabilitiesByIdCache.ContainsKey(id) ||

diff --git 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<int, IReadOnlyList<PackageVulnerability>> GetVulnerabilitiesById(string id) =>
-            _packageVulnerabilitiesCacheService.GetVulnerabilitiesById(id, _packageVulnerabilitiesManagementService);
+            _packageVulnerabilitiesCacheService.GetVulnerabilitiesById(id);
 
         public bool IsPackageVulnerable(Package package)
         {