From c73ce252c05739951297a545f0049de0c0cabe75 Mon Sep 17 00:00:00 2001
From: Joel Verhagen <jver@microsoft.com>
Date: Tue, 21 May 2024 11:39:02 -0700
Subject: [PATCH] Allow unrelated entity changes to be committed in
 PackageDeprecationService (#9951)

---
 .../Services/PackageDeprecationService.cs     |  27 +++--
 .../PackageDeprecationServiceFacts.cs         | 101 ++++++++++++++++++
 2 files changed, 120 insertions(+), 8 deletions(-)

diff --git a/src/NuGetGallery/Services/PackageDeprecationService.cs b/src/NuGetGallery/Services/PackageDeprecationService.cs
index 66f60f1280..969b192bc3 100644
--- a/src/NuGetGallery/Services/PackageDeprecationService.cs
+++ b/src/NuGetGallery/Services/PackageDeprecationService.cs
@@ -158,17 +158,28 @@ public async Task UpdateDeprecation(
                 {
                     await _entitiesContext.SaveChangesAsync();
 
-                    await _packageUpdateService.UpdatePackagesAsync(changedPackages);
+                    // Ideally, the number of changed packages should be zero if and only if the entity context has
+                    // no changes (therefore this line should not be reached). But it is possible that an entity can
+                    // be changed for other reasons, such as https://github.com/NuGet/NuGetGallery/issues/9950.
+                    // Therefore, allow the transaction to be committed but do not update the LastEdited property on
+                    // the package, to avoid unnecessary package edits flowing into V3.
+                    if (changedPackages.Count > 0)
+                    {
+                        await _packageUpdateService.UpdatePackagesAsync(changedPackages);
+                    }
 
                     transaction.Commit();
 
-                    _telemetryService.TrackPackageDeprecate(
-                        changedPackages,
-                        status,
-                        alternatePackageRegistration,
-                        alternatePackage,
-                        !string.IsNullOrWhiteSpace(customMessage),
-                        hasChanges: true);
+                    if (changedPackages.Count > 0)
+                    {
+                        _telemetryService.TrackPackageDeprecate(
+                            changedPackages,
+                            status,
+                            alternatePackageRegistration,
+                            alternatePackage,
+                            !string.IsNullOrWhiteSpace(customMessage),
+                            hasChanges: true);
+                    }
 
                     foreach (var package in changedPackages)
                     {
diff --git a/tests/NuGetGallery.Facts/Services/PackageDeprecationServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageDeprecationServiceFacts.cs
index 33ddf55590..914ce5bd75 100644
--- a/tests/NuGetGallery.Facts/Services/PackageDeprecationServiceFacts.cs
+++ b/tests/NuGetGallery.Facts/Services/PackageDeprecationServiceFacts.cs
@@ -520,6 +520,107 @@ await service.UpdateDeprecation(
                     auditingService.WroteNoRecords();
                 }
             }
+
+            [Fact]
+            public async Task SavesChangesWithExtraEntityChangesButNoDeprecationChanges()
+            {
+                // Arrange
+                var lastTimestamp = new DateTime(2019, 3, 4);
+
+                var id = "theId";
+                var registration = new PackageRegistration { Id = id };
+
+                var customMessage = "message";
+                var user = new User { Key = 1 };
+                var status = (PackageDeprecationStatus)99;
+
+                var alternatePackageRegistration = new PackageRegistration { Key = 1 };
+                var alternatePackage = new Package { Key = 1 };
+
+                var packageWithDeprecation = new Package
+                {
+                    PackageRegistration = registration,
+                    NormalizedVersion = "3.0.0",
+                    LastEdited = lastTimestamp,
+                    Deprecations = new List<PackageDeprecation>
+                    {
+                        new PackageDeprecation
+                        {
+                            DeprecatedByUserKey = user.Key,
+                            CustomMessage = customMessage,
+                            Status = status,
+                            AlternatePackageKey = alternatePackage.Key,
+                            AlternatePackage = alternatePackage,
+                            AlternatePackageRegistrationKey = alternatePackageRegistration.Key,
+                            AlternatePackageRegistration = alternatePackageRegistration,
+                        }
+                    }
+                };
+
+                var packages = new[]
+                {
+                    packageWithDeprecation,
+                };
+
+                var transactionMock = new Mock<IDbContextTransaction>();
+                transactionMock
+                    .Setup(x => x.Commit())
+                    .Verifiable();
+
+                var databaseMock = new Mock<IDatabase>();
+                databaseMock
+                    .Setup(x => x.BeginTransaction())
+                    .Returns(transactionMock.Object);
+
+                var context = GetFakeContext();
+                context.HasChanges = true;
+                context.SetupDatabase(databaseMock.Object);
+                context.Deprecations.AddRange(
+                    packages
+                        .Select(p => p.Deprecations.SingleOrDefault())
+                        .Where(d => d != null));
+
+                var packageUpdateService = GetMock<IPackageUpdateService>();
+                var auditingService = GetService<IAuditingService>();
+
+                var telemetryService = GetMock<ITelemetryService>();
+                telemetryService
+                    .Setup(x => x.TrackPackageDeprecate(packages, status, alternatePackageRegistration, alternatePackage, true, false))
+                    .Verifiable();
+
+                var service = Get<PackageDeprecationService>();
+
+                // Act
+                await service.UpdateDeprecation(
+                    packages,
+                    status,
+                    alternatePackageRegistration,
+                    alternatePackage,
+                    customMessage,
+                    user,
+                    ListedVerb.Unchanged,
+                    PackageUndeprecatedVia.Web);
+
+                // Assert
+                context.VerifyCommitChanges();
+                databaseMock.Verify();
+                transactionMock.Verify();
+                packageUpdateService.Verify();
+                telemetryService.Verify();
+
+                Assert.Equal(packages.Count(), context.Deprecations.Count());
+                foreach (var package in packages)
+                {
+                    var deprecation = package.Deprecations.Single();
+                    Assert.Contains(deprecation, context.Deprecations);
+                    Assert.Equal(status, deprecation.Status);
+                    Assert.Equal(alternatePackageRegistration, deprecation.AlternatePackageRegistration);
+                    Assert.Equal(alternatePackage, deprecation.AlternatePackage);
+                    Assert.Equal(customMessage, deprecation.CustomMessage);
+
+                    auditingService.WroteNoRecords();
+                }
+            }
         }
 
         public class TheGetDeprecationByPackageMethod : TestContainer