Symbols perf improvement (#8446)

* Symbols perf improvement

* applied feedback

Author: skofman1

src/NuGetGallery/App_Data/Files/Content/flags.json

@@ -25,7 +25,8 @@
     "NuGetGallery.DisplayVulnerabilities": "Enabled",
     "NuGetGallery.ManagePackagesVulnerabilities": "Enabled",
     "NuGetGallery.DisplayFuGetLinks": "Enabled",
-    "NuGetGallery.PatternSetTfmHeuristics": "Enabled"
+    "NuGetGallery.PatternSetTfmHeuristics": "Enabled",
+    "NuGetGallery.EmbeddedIcons": "Enabled"
   },
   "Flights": {
     "NuGetGallery.TyposquattingFlight": {

src/NuGetGallery/Controllers/ApiController.cs

@@ -193,17 +193,42 @@ public ApiController(
         [ActionName("GetSymbolPackageApi")]
         public virtual async Task<ActionResult> GetSymbolPackage(string id, string version)
         {
-            return await GetPackageInternal(id, version, isSymbolPackage: true);
+            // some security paranoia about URL hacking somehow creating e.g. open redirects
+            // validate user input: explicit calls to the same validators used during Package Registrations
+            // Ideally shouldn't be necessary?
+            if (!PackageIdValidator.IsValidPackageId(id ?? string.Empty))
+            {
+                return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, "The format of the package id is invalid");
+            }
+
+            if (string.IsNullOrEmpty(version))
+            {
+                return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, "Version is required");
+            }
+
+            // Check if version semantically correct
+            if (!NuGetVersion.TryParse(version, out NuGetVersion dummy))
+            {
+                return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, "The package version is not a valid semantic version");
+            }
+
+            // Normalize the version
+            version = NuGetVersionFormatter.Normalize(version);
+
+            // There's no guarantee the symbols package exists in the returned path. We just provide the path it should be at.
+            return await SymbolPackageFileService.CreateDownloadSymbolPackageActionResultAsync(
+                    HttpContext.Request.Url,
+                    id, version);
         }
 
         [HttpGet]
         [ActionName("GetPackageApi")]
         public virtual async Task<ActionResult> GetPackage(string id, string version)
         {
-            return await GetPackageInternal(id, version, isSymbolPackage: false);
+            return await GetPackageInternal(id, version);
         }
 
-        protected internal async Task<ActionResult> GetPackageInternal(string id, string version, bool isSymbolPackage = false)
+        protected internal async Task<ActionResult> GetPackageInternal(string id, string version)
         {
             // some security paranoia about URL hacking somehow creating e.g. open redirects
             // validate user input: explicit calls to the same validators used during Package Registrations
@@ -219,24 +244,13 @@ protected internal async Task<ActionResult> GetPackageInternal(string id, string
                 if (!string.IsNullOrEmpty(version))
                 {
                     // if version is non-null, check if it's semantically correct and normalize it.
-                    NuGetVersion dummy;
-                    if (!NuGetVersion.TryParse(version, out dummy))
+                    if (!NuGetVersion.TryParse(version, out NuGetVersion dummy))
                     {
                         return new HttpStatusCodeWithBodyResult(HttpStatusCode.BadRequest, "The package version is not a valid semantic version");
                     }
 
                     // Normalize the version
                     version = NuGetVersionFormatter.Normalize(version);
-
-                    if (isSymbolPackage)
-                    {
-                        package = PackageService.FindPackageByIdAndVersionStrict(id, version);
-
-                        if (package == null)
-                        {
-                            return new HttpStatusCodeWithBodyResult(HttpStatusCode.NotFound, string.Format(CultureInfo.CurrentCulture, Strings.PackageWithIdAndVersionNotFound, id, version));
-                        }
-                    }
                 }
                 else
                 {
@@ -271,30 +285,14 @@ protected internal async Task<ActionResult> GetPackageInternal(string id, string
                 return new HttpStatusCodeWithBodyResult(HttpStatusCode.ServiceUnavailable, Strings.DatabaseUnavailable_TrySpecificVersion);
             }
 
-            if (isSymbolPackage)
+            if (ConfigurationService.Features.TrackPackageDownloadCountInLocalDatabase)
             {
-                var latestAvailableSymbolsPackage = package.LatestAvailableSymbolPackage();
-
-                if (latestAvailableSymbolsPackage == null)
-                {
-                    return new HttpStatusCodeWithBodyResult(HttpStatusCode.NotFound, string.Format(CultureInfo.CurrentCulture, Strings.SymbolsPackage_PackageNotAvailable, id, version));
-                }
-
-                return await SymbolPackageFileService.CreateDownloadSymbolPackageActionResultAsync(
-                    HttpContext.Request.Url,
-                    id, version);
+                await PackageService.IncrementDownloadCountAsync(id, version);
             }
-            else
-            {
-                if (ConfigurationService.Features.TrackPackageDownloadCountInLocalDatabase)
-                {
-                    await PackageService.IncrementDownloadCountAsync(id, version);
-                }
 
-                return await PackageFileService.CreateDownloadPackageActionResultAsync(
-                    HttpContext.Request.Url,
-                    id, version);
-            }
+            return await PackageFileService.CreateDownloadPackageActionResultAsync(
+                HttpContext.Request.Url,
+                id, version);
         }
 
         [HttpGet]

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

@@ -1968,134 +1968,124 @@ public async Task WillNotUnlistThePackageIfItIsLocked()
             }
         }
 
-        public class TheGetPackageAction
+        public class TheGetSymbolPackage
             : TestContainer
         {
-            [Theory]
-            [InlineData(true)]
-            [InlineData(false)]
-            public async Task GetPackageReturns400ForEvilPackageName(bool isSymbolPackage)
+            [Fact]
+            public async Task GetSymbolPackageReturns400ForEvilPackageName()
             {
                 var controller = new TestableApiController(GetConfigurationService());
-                var result = await controller.GetPackageInternal("../..", "1.0.0.0", isSymbolPackage);
+                var result = await controller.GetSymbolPackage("../..", "1.0.0.0");
                 var badRequestResult = (HttpStatusCodeWithBodyResult)result;
                 Assert.Equal(400, badRequestResult.StatusCode);
             }
 
+            [Fact]
+            public async Task GetSymbolPackageReturns400ForEvilPackageVersion()
+            {
+                var controller = new TestableApiController(GetConfigurationService());
+                var result2 = await controller.GetSymbolPackage("Foo", "10../..1.0");
+                var badRequestResult2 = (HttpStatusCodeWithBodyResult)result2;
+                Assert.Equal(400, badRequestResult2.StatusCode);
+            }
+
             [Theory]
-            [InlineData(true)]
-            [InlineData(false)]
-            public async Task GetPackageReturns400ForEvilPackageVersion(bool isSymbolPackage)
+            [InlineData("")]
+            [InlineData(null)]
+            public async Task GetSymbolPackageReturns400IfNoVersionProvided(string version)
             {
                 var controller = new TestableApiController(GetConfigurationService());
-                var result2 = await controller.GetPackageInternal("Foo", "10../..1.0", isSymbolPackage);
+                var result2 = await controller.GetSymbolPackage("Foo", version);
                 var badRequestResult2 = (HttpStatusCodeWithBodyResult)result2;
                 Assert.Equal(400, badRequestResult2.StatusCode);
             }
 
             [Fact]
-            public async Task GetPackageReturns404IfPackageIsNotFound()
+            public async Task GetSymbolsPackageReturnsPackage()
             {
                 // Arrange
                 const string packageId = "Baz";
-                const string packageVersion = "1.0.1";
-                var actionResult = new RedirectResult("http://foo");
+                const string version = "1.0.1+notnormalized";
+                const string normalizedVersion = "1.0.1";
+
+                var actionResult = new EmptyResult();
 
                 var controller = new TestableApiController(GetConfigurationService(), MockBehavior.Strict);
-                controller.MockPackageService
-                    .Setup(x => x.FindPackageByIdAndVersionStrict(packageId, packageVersion))
-                    .Returns((Package)null).Verifiable();
-                controller.MockPackageFileService.Setup(s => s.CreateDownloadPackageActionResultAsync(It.IsAny<Uri>(), packageId, packageVersion))
+
+                controller
+                    .MockSymbolPackageFileService
+                    .Setup(s => s.CreateDownloadSymbolPackageActionResultAsync(HttpRequestUrl, packageId, normalizedVersion))
                     .Returns(Task.FromResult<ActionResult>(actionResult))
                     .Verifiable();
 
+                var httpRequest = new Mock<HttpRequestBase>(MockBehavior.Strict);
+                httpRequest.SetupGet(r => r.Url).Returns(HttpRequestUrl);
+
+                var httpContext = new Mock<HttpContextBase>(MockBehavior.Strict);
+                httpContext.SetupGet(c => c.Request).Returns(httpRequest.Object);
+
+                var controllerContext = new ControllerContext(new RequestContext(httpContext.Object, new RouteData()), controller);
+                controller.ControllerContext = controllerContext;
+
                 // Act
-                var result = await controller.GetPackageInternal(packageId, packageVersion);
+                var result = await controller.GetSymbolPackage(packageId, version);
 
                 // Assert
-                Assert.IsType<RedirectResult>(result);
+                Assert.Same(actionResult, result);
+
+                controller.MockSymbolPackageFileService.Verify();
             }
+        }
 
+        public class TheGetPackageAction
+            : TestContainer
+        {
             [Fact]
-            public async Task GetPackageReturns404ForSymbolPackageIfPackageIsNotFound()
+            public async Task GetPackageReturns400ForEvilPackageName()
             {
-                // Arrange
-                const string packageId = "Baz";
-                const string packageVersion = "1.0.1";
-
-                var controller = new TestableApiController(GetConfigurationService(), MockBehavior.Strict);
-                controller.MockPackageService
-                    .Setup(x => x.FindPackageByIdAndVersionStrict(packageId, packageVersion))
-                    .Returns((Package)null).Verifiable();
-
-                // Act
-                var result = (HttpStatusCodeWithBodyResult)await controller.GetPackageInternal(packageId, packageVersion, isSymbolPackage: true);
+                var controller = new TestableApiController(GetConfigurationService());
+                var result = await controller.GetPackage("../..", "1.0.0.0");
+                var badRequestResult = (HttpStatusCodeWithBodyResult)result;
+                Assert.Equal(400, badRequestResult.StatusCode);
+            }
 
-                // Assert
-                Assert.Equal((int)HttpStatusCode.NotFound, result.StatusCode);
+            [Fact]
+            public async Task GetPackageReturns400ForEvilPackageVersion()
+            {
+                var controller = new TestableApiController(GetConfigurationService());
+                var result2 = await controller.GetPackage("Foo", "10../..1.0");
+                var badRequestResult2 = (HttpStatusCodeWithBodyResult)result2;
+                Assert.Equal(400, badRequestResult2.StatusCode);
             }
 
-            [Theory]
-            [InlineData(PackageStatus.Deleted)]
-            [InlineData(PackageStatus.FailedValidation)]
-            [InlineData(PackageStatus.Validating)]
-            public async Task GetPackageReturnsLastAvailableSymbolPackage(PackageStatus status)
+            [Fact]
+            public async Task GetPackageReturns404IfPackageIsNotFound()
             {
                 // Arrange
                 const string packageId = "Baz";
-                const string packageVersion = "1.0.1";
-                var package = new Package() { PackageRegistration = new PackageRegistration() { Id = packageId }, Version = packageVersion };
-                var latestSymbolPackage = new SymbolPackage()
-                {
-                    Key = 1,
-                    Package = package,
-                    StatusKey = status,
-                    Created = DateTime.Today.AddDays(-1)
-                };
-                var oldAvailableSymbolPackage = new SymbolPackage()
-                {
-                    Key = 2,
-                    Package = package,
-                    StatusKey = PackageStatus.Available,
-                    Created = DateTime.Today.AddDays(-2)
-                };
-                package.SymbolPackages.Add(oldAvailableSymbolPackage);
-                package.SymbolPackages.Add(latestSymbolPackage);
 
                 var controller = new TestableApiController(GetConfigurationService(), MockBehavior.Strict);
                 controller.MockPackageService
-                    .Setup(x => x.FindPackageByIdAndVersionStrict(packageId, packageVersion))
-                    .Returns(package).Verifiable();
-                controller.MockSymbolPackageFileService
-                    .Setup(x => x.CreateDownloadSymbolPackageActionResultAsync(It.IsAny<Uri>(), packageId, packageVersion))
-                    .Returns(Task.FromResult<ActionResult>(new HttpStatusCodeWithBodyResult(HttpStatusCode.OK, "Test package")))
-                    .Verifiable();
+                    .Setup(x => x.FindPackageByIdAndVersion(packageId, null, SemVerLevelKey.SemVer2, false))
+                    .Returns((Package)null).Verifiable();
 
                 // Act
-                var result = (HttpStatusCodeWithBodyResult)await controller.GetPackageInternal(packageId, packageVersion, isSymbolPackage: true);
+                var result = await controller.GetPackageInternal(packageId, null);
 
                 // Assert
-                Assert.Equal((int)HttpStatusCode.OK, result.StatusCode);
-                controller.MockSymbolPackageFileService.Verify();
+                controller.MockPackageService.Verify();
+
+                var notFoundResult = (HttpStatusCodeWithBodyResult)result;
+                Assert.Equal(404, notFoundResult.StatusCode);
             }
 
-            [Theory]
-            [InlineData(true)]
-            [InlineData(false)]
-            public async Task GetPackageReturnsPackageIfItExists(bool isSymbolPackage)
+            [Fact]
+            public async Task GetPackageReturnsPackageIfItExists()
             {
                 // Arrange
                 const string packageId = "Baz";
                 var package = new Package() { Version = "1.0.01", NormalizedVersion = "1.0.1" };
                 var actionResult = new EmptyResult();
-                var availableSymbolPackage = new SymbolPackage()
-                {
-                    Key = 2,
-                    Package = package,
-                    StatusKey = PackageStatus.Available,
-                    Created = DateTime.Today.AddDays(-1)
-                };
-                package.SymbolPackages.Add(availableSymbolPackage);
 
                 var controller = new TestableApiController(GetConfigurationService(), MockBehavior.Strict);
                 controller
@@ -2107,11 +2097,6 @@ public async Task GetPackageReturnsPackageIfItExists(bool isSymbolPackage)
                     .Setup(s => s.CreateDownloadPackageActionResultAsync(HttpRequestUrl, packageId, package.NormalizedVersion))
                     .Returns(Task.FromResult<ActionResult>(actionResult))
                     .Verifiable();
-                controller
-                    .MockSymbolPackageFileService
-                    .Setup(s => s.CreateDownloadSymbolPackageActionResultAsync(HttpRequestUrl, packageId, package.NormalizedVersion))
-                    .Returns(Task.FromResult<ActionResult>(actionResult))
-                    .Verifiable();
 
                 NameValueCollection headers = new NameValueCollection();
                 headers.Add("NuGet-Operation", "Install");
@@ -2128,18 +2113,11 @@ public async Task GetPackageReturnsPackageIfItExists(bool isSymbolPackage)
                 controller.ControllerContext = controllerContext;
 
                 // Act
-                var result = await controller.GetPackageInternal(packageId, "1.0.01", isSymbolPackage);
+                var result = await controller.GetPackage(packageId, "1.0.01");
 
                 // Assert
                 Assert.Same(actionResult, result);
-                if (isSymbolPackage)
-                {
-                    controller.MockSymbolPackageFileService.Verify();
-                }
-                else
-                {
-                    controller.MockPackageFileService.Verify();
-                }
+                controller.MockPackageFileService.Verify();
 
                 controller.MockPackageService.Verify();
                 controller.MockUserService.Verify();