Fix concurrency/consistency issues in Gallery API push (#6600)

Author: shishirx34

src/NuGetGallery.Core/NuGetGallery.Core.csproj

@@ -161,6 +161,7 @@
     <Compile Include="Services\CoreSymbolPackageService.cs" />
     <Compile Include="Services\CorePackageService.cs" />
     <Compile Include="Services\CryptographyService.cs" />
+    <Compile Include="Services\PackageAlreadyExistsException.cs" />
     <Compile Include="Services\FileAlreadyExistsException.cs" />
     <Compile Include="Services\FileUriPermissions.cs" />
     <Compile Include="Services\IAccessCondition.cs" />

src/NuGetGallery.Core/Services/PackageAlreadyExistsException.cs

@@ -0,0 +1,15 @@
+// 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;
+
+namespace NuGetGallery
+{
+    [Serializable]
+    public sealed class PackageAlreadyExistsException : Exception
+    {
+        public PackageAlreadyExistsException() { }
+        public PackageAlreadyExistsException(string message) : base(message) { }
+        public PackageAlreadyExistsException(string message, Exception inner) : base(message, inner) { }
+    }
+}

src/NuGetGallery/Controllers/ApiController.cs

@@ -25,8 +25,6 @@
 using NuGetGallery.Configuration;
 using NuGetGallery.Filters;
 using NuGetGallery.Infrastructure.Authentication;
-using NuGetGallery.Infrastructure.Mail;
-using NuGetGallery.Infrastructure.Mail.Messages;
 using NuGetGallery.Packaging;
 using NuGetGallery.Security;
 using PackageIdValidator = NuGetGallery.Packaging.PackageIdValidator;
@@ -731,6 +729,10 @@ await AuditingService.SaveAuditRecordAsync(
                     {
                         return BadRequestForExceptionMessage(ex);
                     }
+                    catch (PackageAlreadyExistsException ex)
+                    {
+                        return new HttpStatusCodeWithBodyResult(HttpStatusCode.Conflict, ex.Message);
+                    }
                     catch (EntityException ex)
                     {
                         return BadRequestForExceptionMessage(ex);

src/NuGetGallery/Services/PackageService.cs

@@ -479,8 +479,8 @@ private Package CreatePackageFromNuGetPackage(PackageRegistration packageRegistr
 
             if (package != null)
             {
-                throw new EntityException(
-                    "A package with identifier '{0}' and version '{1}' already exists.", packageRegistration.Id, package.Version);
+                throw new PackageAlreadyExistsException(
+                    string.Format(Strings.PackageExistsAndCannotBeModified, packageRegistration.Id, package.Version));
             }
 
             package = new Package();

src/NuGetGallery/Services/PackageUploadService.cs

@@ -3,6 +3,8 @@
 
 using System;
 using System.Collections.Generic;
+using System.Data.Entity.Infrastructure;
+using System.Data.SqlClient;
 using System.IO;
 using System.Linq;
 using System.Threading;
@@ -427,7 +429,7 @@ await _packageFileService.DeleteValidationPackageFileAsync(
                 // commit all changes to database as an atomic transaction
                 await _entitiesContext.SaveChangesAsync();
             }
-            catch
+            catch (Exception ex)
             {
                 // If saving to the DB fails for any reason we need to delete the package we just saved.
                 if (package.PackageStatusKey == PackageStatus.Validating)
@@ -443,10 +445,36 @@ await _packageFileService.DeletePackageFileAsync(
                         package.Version);
                 }
 
-                throw;
+                return ReturnConflictOrThrow(ex);
             }
 
             return PackageCommitResult.Success;
         }
+
+        private PackageCommitResult ReturnConflictOrThrow(Exception ex)
+        {
+            if (ex is DbUpdateConcurrencyException concurrencyEx)
+            {
+                return PackageCommitResult.Conflict;
+            }
+            else if (ex is DbUpdateException dbUpdateEx)
+            {
+                if (dbUpdateEx.InnerException?.InnerException != null)
+                {
+                    if (dbUpdateEx.InnerException.InnerException is SqlException sqlException)
+                    {
+                        switch (sqlException.Number)
+                        {
+                            case 547:   // Constraint check violation
+                            case 2601:  // Duplicated key row error
+                            case 2627:  // Unique constraint error
+                                return PackageCommitResult.Conflict;
+                        }
+                    }
+                }
+            }
+
+            throw ex;
+        }
     }
 }

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

@@ -843,6 +843,52 @@ public async Task WillReturnConflictIfCommittingPackageReturnsConflict()
                 controller.MockEntitiesContext.VerifyCommitted(Times.Never());
             }
 
+            [Fact]
+            public async Task WillReturnConflictIfGeneratePackageThrowsPackageAlreadyExistsException()
+            {
+                // Arrange
+                var packageId = "theId";
+                var nuGetPackage = TestPackage.CreateTestPackageStream(packageId, "1.0.42");
+
+                var currentUser = new User("currentUser") { Key = 1, EmailAddress = "[email protected]" };
+                var controller = new TestableApiController(GetConfigurationService());
+                controller.SetCurrentUser(currentUser);
+                controller.SetupPackageFromInputStream(nuGetPackage);
+
+                var owner = new User("owner") { Key = 2, EmailAddress = "[email protected]" };
+
+                Expression<Func<IApiScopeEvaluator, ApiScopeEvaluationResult>> evaluateApiScope =
+                    x => x.Evaluate(
+                        currentUser,
+                        It.IsAny<IEnumerable<Scope>>(),
+                        ActionsRequiringPermissions.UploadNewPackageId,
+                        It.Is<ActionOnNewPackageContext>((context) => context.PackageId == packageId),
+                        NuGetScopes.PackagePush);
+
+                controller.MockApiScopeEvaluator
+                    .Setup(evaluateApiScope)
+                    .Returns(new ApiScopeEvaluationResult(owner, PermissionsCheckResult.Allowed, scopesAreValid: true));
+                controller
+                    .MockPackageUploadService
+                    .Setup(x => x.GeneratePackageAsync(It.IsAny<string>(),
+                        It.IsAny<PackageArchiveReader>(),
+                        It.IsAny<PackageStreamMetadata>(),
+                        It.IsAny<User>(),
+                        It.IsAny<User>()))
+                    .Throws(new PackageAlreadyExistsException("Package exists"));
+
+                // Act
+                var result = await controller.CreatePackagePut();
+
+                // Assert
+                ResultAssert.IsStatusCode(result, HttpStatusCode.Conflict);
+                controller.MockPackageUploadService.Verify(x => x.GeneratePackageAsync(It.IsAny<string>(),
+                        It.IsAny<PackageArchiveReader>(),
+                        It.IsAny<PackageStreamMetadata>(),
+                        It.IsAny<User>(),
+                        It.IsAny<User>()), Times.Once);
+            }
+
             [Fact]
             public async Task WillReturnValidationWarnings()
             {

tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs

@@ -423,6 +423,26 @@ private async Task WillSaveTheCreatedPackageWhenThePackageRegistrationAlreadyExi
                 Assert.Same(packageRegistration.Packages.ElementAt(0), package);
             }
 
+            [Fact]
+            private async Task WillThrowWhenThePackageRegistrationAndVersionAlreadyExists()
+            {
+                var currentUser = new User();
+                var packageId = "theId";
+                var packageVersion = "1.0.32";
+                var nugetPackage = PackageServiceUtility.CreateNuGetPackage(packageId, packageVersion);
+                var packageRegistration = new PackageRegistration
+                {
+                    Id = packageId,
+                    Owners = new HashSet<User> { currentUser },
+                };
+                packageRegistration.Packages.Add(new Package() { Version = packageVersion });
+                var packageRegistrationRepository = new Mock<IEntityRepository<PackageRegistration>>();
+                var service = CreateService(packageRegistrationRepository: packageRegistrationRepository, setup:
+                        mockPackageService => { mockPackageService.Setup(x => x.FindPackageRegistrationById(It.IsAny<string>())).Returns(packageRegistration); });
+
+                await Assert.ThrowsAsync<PackageAlreadyExistsException>(async () => await service.CreatePackageAsync(nugetPackage.Object, new PackageStreamMetadata(), currentUser, currentUser, isVerified: false));
+            }
+
             [Fact]
             private async Task WillThrowIfTheNuGetPackageIdIsLongerThanMaxPackageIdLength()
             {

tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Data.Entity.Infrastructure;
 using System.IO;
 using System.Linq;
 using System.Threading;
@@ -1037,6 +1038,27 @@ public async Task DeletesPackageIfDatabaseCommitFailsWhenAvailable()
                 Assert.Same(_unexpectedException, exception);
             }
 
+            [Fact]
+            public async Task ReturnsConflictWhenDBCommitThrowsConcurrencyViolations()
+            {
+                _package.PackageStatusKey = PackageStatus.Available;
+                var ex = new DbUpdateConcurrencyException("whoops!");
+                _entitiesContext
+                    .Setup(x => x.SaveChangesAsync())
+                    .Throws(ex);
+
+                var result = await _target.CommitPackageAsync(_package, _packageFile);
+
+                _packageFileService.Verify(
+                    x => x.DeletePackageFileAsync(Id, Version),
+                    Times.Once);
+                _packageFileService.Verify(
+                    x => x.DeletePackageFileAsync(It.IsAny<string>(), It.IsAny<string>()),
+                    Times.Once);
+
+                Assert.Equal(PackageCommitResult.Conflict, result);
+            }
+
             [Fact]
             public async Task DeletesPackageIfDatabaseCommitFailsWhenValidating()
             {