Properly no-op deprecations and allow unlisting at the same time in API (#9922)

* Allow a package to be unlisted at the same time as deprecated via API
* Add proper package-level deprecation no-oping

This will improve V3 performance for cases when a deprecation has not changed on a version
Plumb through proper audit reason for deprecation via API

* Only update deprecated-by if the deprecation changed
* Use ListedVerb instead of nullable bool

Fix #9921

Author: joelverhagen

src/GitHubVulnerabilities2Db/Gallery/GalleryDbVulnerabilityWriter.cs

@@ -25,10 +25,10 @@ public GalleryDbVulnerabilityWriter(
             _logger = logger ?? throw new ArgumentNullException(nameof(logger));
         }
 
-        public async Task FlushAsync(string outputFileName = null)
+        public Task FlushAsync(string outputFileName = null)
         {
             // This method is unused for this implementation.
-            return;
+            return Task.CompletedTask;
         }
 
         public async Task<int> WriteVulnerabilitiesAsync(IEnumerable<Tuple<PackageVulnerability,bool>> vulnerabilities)

src/GitHubVulnerabilities2v3/Extensions/BlobStorageVulnerabilityWriter.cs

@@ -122,7 +122,7 @@ public async Task<int> WriteVulnerabilitiesAsync(IEnumerable<Tuple<PackageVulner
             return vulnerabilities.Count();
         }
 
-        public async Task WriteVulnerabilityAsync(PackageVulnerability packageVulnerability, bool wasWithdrawn)
+        public Task WriteVulnerabilityAsync(PackageVulnerability packageVulnerability, bool wasWithdrawn)
         {
             _firstVulnWrittenTimestamp = _firstVulnWrittenTimestamp == DateTime.MinValue ? DateTimeOffset.UtcNow : _firstVulnWrittenTimestamp;
             try
@@ -177,6 +177,8 @@ public async Task WriteVulnerabilityAsync(PackageVulnerability packageVulnerabil
                 _logger.LogError(e, "WriteVulnerability Failed for Advisory Url {AdvisoryUrl}", packageVulnerability.AdvisoryUrl);
                 throw;
             }
+
+            return Task.CompletedTask;
         }
 
         public async Task<RunMode> DetermineRunMode(ReadWriteCursor<DateTimeOffset> cursor)

src/NuGetGallery.Core/Auditing/PackageAuditReasons.cs

@@ -31,6 +31,11 @@ public static class PackageDeletedVia
 
     public static class PackageDeprecatedVia
     {
+        /// <summary>
+        /// Package has been deprecated via NuGet API
+        /// </summary>
+        public const string Api = "Deprecated via API.";
+
         /// <summary>
         /// Package has been deprecated via NuGet web interface (browser)
         /// </summary>
@@ -39,6 +44,11 @@ public static class PackageDeprecatedVia
 
     public static class PackageUndeprecatedVia
     {
+        /// <summary>
+        /// Package has been undeprecated via NuGet API
+        /// </summary>
+        public const string Api = "Undeprecated via API.";
+
         /// <summary>
         /// Package has been undeprecated via NuGet web interface (browser)
         /// </summary>

src/NuGetGallery.Core/Frameworks/PackageFrameworkCompatibilityFactory.cs

@@ -19,7 +19,7 @@ public class PackageFrameworkCompatibilityFactory : IPackageFrameworkCompatibili
             FrameworkProductNames.NetStandard,
             FrameworkProductNames.NetFramework
         };
-        private readonly NuGetFrameworkSorter Sorter = new NuGetFrameworkSorter();
+        private readonly NuGetFrameworkSorter Sorter = NuGetFrameworkSorter.Instance;
         private readonly int NetStartingMajorVersion = 5;
 
         public PackageFrameworkCompatibility Create(ICollection<PackageFramework> packageFrameworks, string packageId, string packageVersion, bool includeComputedBadges = false)

src/NuGetGallery/Controllers/ApiController.cs

@@ -28,6 +28,7 @@
 using NuGetGallery.Infrastructure.Authentication;
 using NuGetGallery.Infrastructure.Mail.Messages;
 using NuGetGallery.Packaging;
+using NuGetGallery.RequestModels;
 using NuGetGallery.Security;
 using PackageIdValidator = NuGetGallery.Packaging.PackageIdValidator;
 
@@ -1010,7 +1011,8 @@ public virtual async Task<ActionResult> DeprecatePackage(
             bool isOther = false, 
             string alternatePackageId = null, 
             string alternatePackageVersion = null, 
-            string message = null)
+            string message = null,
+            ListedVerb listedVerb = ListedVerb.Unchanged)
         {
             var registration = PackageService.FindPackageRegistrationById(id);
             if (registration == null)
@@ -1035,12 +1037,14 @@ public virtual async Task<ActionResult> DeprecatePackage(
                 apiScopeEvaluationResult.Owner,
                 id,
                 versions.ToList(),
+                isLegacy || hasCriticalBugs || isOther ? PackageDeprecatedVia.Api : PackageUndeprecatedVia.Api,
                 isLegacy,
                 hasCriticalBugs,
                 isOther,
                 alternatePackageId,
                 alternatePackageVersion,
-                message);
+                message,
+                listedVerb);
 
             if (error != null)
             {

src/NuGetGallery/Controllers/ManageDeprecationJsonApiController.cs

@@ -6,6 +6,7 @@
 using System.Net;
 using System.Threading.Tasks;
 using System.Web.Mvc;
+using NuGetGallery.Auditing;
 using NuGetGallery.Filters;
 using NuGetGallery.RequestModels;
 
@@ -37,10 +38,13 @@ public virtual JsonResult GetAlternatePackageVersions(string id)
         public virtual async Task<JsonResult> Deprecate(
             DeprecatePackageRequest request)
         {
+            var isDeprecated = request.IsLegacy || request.HasCriticalBugs || request.IsOther;
+
             var error = await _deprecationManagementService.UpdateDeprecation(
                 GetCurrentUser(),
                 request.Id,
                 request.Versions.ToList(),
+                isDeprecated ? PackageDeprecatedVia.Web : PackageUndeprecatedVia.Web,
                 request.IsLegacy,
                 request.HasCriticalBugs,
                 request.IsOther,
@@ -54,8 +58,7 @@ public virtual async Task<JsonResult> Deprecate(
             }
 
             var packagePluralString = request.Versions.Count() > 1 ? "packages have" : "package has";
-            var deprecatedString = request.IsLegacy || request.HasCriticalBugs || request.IsOther
-                ? "deprecated" : "undeprecated";
+            var deprecatedString = isDeprecated ? "deprecated" : "undeprecated";
             TempData["Message"] = 
                 $"Your {packagePluralString} been {deprecatedString}. " +
                 $"It may take several hours for this change to propagate through our system.";

src/NuGetGallery/NuGetGallery.csproj

@@ -350,6 +350,7 @@
     <Compile Include="Modules\CookieComplianceHttpModule.cs" />
     <Compile Include="RequestModels\DeletePackagesApiRequest.cs" />
     <Compile Include="RequestModels\UpdateListedRequest.cs" />
+    <Compile Include="Services\ListedVerb.cs" />
     <Compile Include="Services\MissingLicenseValidationMessageV2.cs" />
     <Compile Include="Services\NullTyposquattingService.cs" />
     <Compile Include="Services\UploadPackageMissingReadme.cs" />

src/NuGetGallery/Services/IPackageDeprecationManagementService.cs

@@ -22,12 +22,14 @@ IReadOnlyCollection<string> GetPossibleAlternatePackageVersions(
         /// <param name="currentUser">The user that is performing the update.</param>
         /// <param name="id">The ID of the package to update.</param>
         /// <param name="versions">The versions of the package to update.</param>
+        /// <param name="auditReason">A reason description used for auditing purposes.</param>
         /// <param name="isLegacy">Whether or not the packages are legacy.</param>
         /// <param name="hasCriticalBugs">Whether or not the packages have critical bugs.</param>
         /// <param name="isOther">Whether or not the packages have an unlisted reason for being deprecated.</param>
         /// <param name="alternatePackageId">An alternate package ID to use instead.</param>
         /// <param name="alternatePackageVersion">A version of <paramref name="alternatePackageId"/> to use instead.</param>
         /// <param name="message">A custom message to add to the deprecation.</param>
+        /// <param name="listedVerb">The action to perform on the listed status of each package version.</param>
         /// <returns>
         /// <c>null</c> if there were no issues updating the deprecation.
         /// Otherwise, a <see cref="UpdateDeprecationError"/> that describes the problem that was encountered.
@@ -36,11 +38,13 @@ Task<UpdateDeprecationError> UpdateDeprecation(
             User currentUser,
             string id,
             IReadOnlyCollection<string> versions,
+            string auditReason,
             bool isLegacy = false,
             bool hasCriticalBugs = false,
             bool isOther = false,
             string alternatePackageId = null,
             string alternatePackageVersion = null,
-            string message = null);
+            string message = null,
+            ListedVerb listedVerb = ListedVerb.Unchanged);
     }
 }

src/NuGetGallery/Services/IPackageDeprecationService.cs

@@ -22,7 +22,9 @@ Task UpdateDeprecation(
             PackageRegistration alternatePackageRegistration,
             Package alternatePackage,
             string customMessage,
-            User user);
+            User user,
+            ListedVerb listedVerb,
+            string auditReason);
 
         IReadOnlyList<PackageDeprecation> GetDeprecationsById(string id);
     }

src/NuGetGallery/Services/ListedVerb.cs

@@ -0,0 +1,23 @@
+// 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.
+
+namespace NuGetGallery
+{
+    public enum ListedVerb
+    {
+        /// <summary>
+        /// Leave the current listed status as is (either unlisted or listed).
+        /// </summary>
+        Unchanged,
+
+        /// <summary>
+        /// Change the listed status to unlisted, allowing for no-op if the package is already unlisted.
+        /// </summary>
+        Unlist,
+
+        /// <summary>
+        /// Change the listed status to listed, allowing for no-op if the package is already listed.
+        /// </summary>
+        Relist,
+    }
+}