Add feature flag for new TFM heuristics (#8448)

drewgillies · web-flow · commit 21348d69e346 · 2021-03-11T12:56:25.000+10:00
Add feature flag for new TFM heuristics
diff --git a/src/NuGetGallery.Services/Configuration/FeatureFlagService.cs b/src/NuGetGallery.Services/Configuration/FeatureFlagService.cs
@@ -40,6 +40,7 @@ public class FeatureFlagService : IFeatureFlagService
         private const string ShowEnable2FADialog = GalleryPrefix + "ShowEnable2FADialog";
         private const string Get2FADismissFeedback = GalleryPrefix + "Get2FADismissFeedback";
         private const string PackageRenamesFeatureName = GalleryPrefix + "PackageRenames";
+        private const string PatternSetTfmHeuristicsFeatureName = GalleryPrefix + "PatternSetTfmHeuristics";
         private const string EmbeddedReadmeFlightName = GalleryPrefix + "EmbeddedReadmes";
         private const string LicenseMdRenderingFlightName = GalleryPrefix + "LicenseMdRendering";
         private const string MarkdigMdRenderingFlightName = GalleryPrefix + "MarkdigMdRendering";
@@ -220,6 +221,11 @@ public bool IsPackageRenamesEnabled(User user)
             return _client.IsEnabled(PackageRenamesFeatureName, user, defaultValue: false);
         }
 
+        public bool ArePatternSetTfmHeuristicsEnabled()
+        {
+            return _client.IsEnabled(PatternSetTfmHeuristicsFeatureName, defaultValue: false);
+        }
+
         public bool AreEmbeddedReadmesEnabled(User user)
         {
             return _client.IsEnabled(EmbeddedReadmeFlightName, user, defaultValue: false);
diff --git a/src/NuGetGallery.Services/Configuration/IFeatureFlagService.cs b/src/NuGetGallery.Services/Configuration/IFeatureFlagService.cs
@@ -158,6 +158,11 @@ public interface IFeatureFlagService
         /// </summary>
         bool IsPackageRenamesEnabled(User user);
 
+        /// <summary>
+        /// Whether we're using pattern set based TFM determination on ingested packages
+        /// </summary>
+        bool ArePatternSetTfmHeuristicsEnabled();
+
         /// <summary>
         /// Whether the user is able to publish the package with an embedded readme file.
         /// </summary>
diff --git a/src/NuGetGallery.Services/PackageManagement/IPackageService.cs b/src/NuGetGallery.Services/PackageManagement/IPackageService.cs
@@ -114,6 +114,8 @@ Package FilterLatestPackageBySuffix(IReadOnlyCollection<Package> packages,
 
         Package EnrichPackageFromNuGetPackage(Package package, PackageArchiveReader packageArchive, PackageMetadata packageMetadata, PackageStreamMetadata packageStreamMetadata, User user);
 
+        IEnumerable<NuGetFramework> GetSupportedFrameworks(PackageArchiveReader package);
+
         IEnumerable<NuGetFramework> GetSupportedFrameworks(NuspecReader nuspecReader, IList<string> packageFiles);
 
         Task PublishPackageAsync(string id, string version, bool commitChanges = true);
diff --git a/src/NuGetGallery.Services/PackageManagement/PackageService.cs b/src/NuGetGallery.Services/PackageManagement/PackageService.cs
@@ -32,6 +32,7 @@ public class PackageService : CorePackageService, IPackageService
         private readonly ISecurityPolicyService _securityPolicyService;
         private readonly IEntitiesContext _entitiesContext;
         private readonly IContentObjectService _contentObjectService;
+        private readonly IFeatureFlagService _featureFlagService;
         private const int packagesDisplayed = 5;
 
         public PackageService(
@@ -42,14 +43,16 @@ public PackageService(
             ITelemetryService telemetryService,
             ISecurityPolicyService securityPolicyService,
             IEntitiesContext entitiesContext,
-            IContentObjectService contentObjectService)
+            IContentObjectService contentObjectService,
+            IFeatureFlagService featureFlagService)
             : base(packageRepository, packageRegistrationRepository, certificateRepository)
         {
             _auditingService = auditingService ?? throw new ArgumentNullException(nameof(auditingService));
             _telemetryService = telemetryService ?? throw new ArgumentNullException(nameof(telemetryService));
             _securityPolicyService = securityPolicyService ?? throw new ArgumentNullException(nameof(securityPolicyService));
             _entitiesContext = entitiesContext ?? throw new ArgumentNullException(nameof(entitiesContext));
             _contentObjectService = contentObjectService ?? throw new ArgumentNullException(nameof(contentObjectService));
+            _featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService));
         }
 
         /// <summary>
@@ -702,8 +705,15 @@ public virtual Package EnrichPackageFromNuGetPackage(
             return package;
         }
 
-        public virtual IEnumerable<NuGetFramework> GetSupportedFrameworks(PackageArchiveReader package) =>
-            GetSupportedFrameworks(package.NuspecReader, package.GetFiles().ToList());
+        public virtual IEnumerable<NuGetFramework> GetSupportedFrameworks(PackageArchiveReader package)
+        {
+            if (_featureFlagService.ArePatternSetTfmHeuristicsEnabled())
+            {
+                return GetSupportedFrameworks(package.NuspecReader, package.GetFiles().ToList());
+            }
+
+            return package.GetSupportedFrameworks();
+        }
 
         /// <summary>
         /// This method combines the logic used in restore operations to make a determination about the TFM supported by the package.
diff --git a/src/NuGetGallery/App_Data/Files/Content/flags.json b/src/NuGetGallery/App_Data/Files/Content/flags.json
@@ -23,7 +23,8 @@
     "NuGetGallery.ODataV2SearchNonHijacked": "Enabled",
     "NuGetGallery.ODataV2SearchCountNonHijacked": "Enabled",
     "NuGetGallery.DisplayVulnerabilities": "Enabled",
-    "NuGetGallery.DisplayFuGetLinks": "Enabled"
+    "NuGetGallery.DisplayFuGetLinks": "Enabled",
+    "NuGetGallery.PatternSetTfmHeuristics": "Enabled"
   },
   "Flights": {
     "NuGetGallery.TyposquattingFlight": {
diff --git a/src/VerifyMicrosoftPackage/Application.cs b/src/VerifyMicrosoftPackage/Application.cs
@@ -266,6 +266,7 @@ private static PackageService GetPackageService()
             var securityPolicyService = new FakeSecurityPolicyService();
             var contextFake = new FakeEntitiesContext();
             var contentObjectService = new FakeContentObjectService();
+            var featureFlagService = new FakeFeatureFlagService();
 
             var packageService = new PackageService(
                 packageRegistrationRepository,
@@ -275,7 +276,8 @@ private static PackageService GetPackageService()
                 telemetryService,
                 securityPolicyService,
                 contextFake,
-                contentObjectService);
+                contentObjectService,
+                featureFlagService);
 
             return packageService;
         }
diff --git a/src/VerifyMicrosoftPackage/Fakes/FakeFeatureFlagService.cs b/src/VerifyMicrosoftPackage/Fakes/FakeFeatureFlagService.cs
@@ -0,0 +1,108 @@
+﻿// 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;
+using System.Collections.Generic;
+using System.Net.Http;
+using System.Security.Principal;
+using NuGet.Services.Entities;
+using NuGet.Versioning;
+using NuGetGallery;
+
+namespace NuGet.VerifyMicrosoftPackage.Fakes
+{
+    public class FakeFeatureFlagService : IFeatureFlagService
+    {
+        public bool IsAlternateStatisticsSourceEnabled() => throw new NotImplementedException();
+
+        public bool IsAsyncAccountDeleteEnabled() => throw new NotImplementedException();
+
+        public bool IsSelfServiceAccountDeleteEnabled() => throw new NotImplementedException();
+
+        public bool IsTyposquattingEnabled() => throw new NotImplementedException();
+
+        public bool IsTyposquattingEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsPackagesAtomFeedEnabled() => throw new NotImplementedException();
+
+        public bool IsManageDeprecationEnabled(User user, PackageRegistration registration) => throw new NotImplementedException();
+
+        public bool IsManageDeprecationEnabled(User user, IEnumerable<Package> allVersions) => throw new NotImplementedException();
+
+        public bool IsManageDeprecationApiEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsDisplayVulnerabilitiesEnabled() => throw new NotImplementedException();
+
+        public bool IsDisplayFuGetLinksEnabled() => throw new NotImplementedException();
+
+        public bool AreEmbeddedIconsEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsForceFlatContainerIconsEnabled() => throw new NotImplementedException();
+
+        public bool IsSearchSideBySideEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsGitHubUsageEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsAdvancedSearchEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsPackageDependentsEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsODataDatabaseReadOnlyEnabled() => throw new NotImplementedException();
+
+        public bool IsABTestingEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsPreviewHijackEnabled() => throw new NotImplementedException();
+
+        public bool IsGravatarProxyEnabled() => throw new NotImplementedException();
+
+        public bool ProxyGravatarEnSubdomain() => throw new NotImplementedException();
+
+        public bool AreDynamicODataCacheDurationsEnabled() => throw new NotImplementedException();
+
+        public bool IsShowEnable2FADialogEnabled() => throw new NotImplementedException();
+
+        public bool IsGet2FADismissFeedbackEnabled() => throw new NotImplementedException();
+
+        public bool IsPackageRenamesEnabled(User user) => throw new NotImplementedException();
+
+        public bool ArePatternSetTfmHeuristicsEnabled() => true;
+
+        public bool AreEmbeddedReadmesEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsODataV1GetAllEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV1GetAllCountEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV1GetSpecificNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV1FindPackagesByIdNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV1FindPackagesByIdCountNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV1SearchNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV1SearchCountNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV2GetAllNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV2GetAllCountNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV2GetSpecificNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV2FindPackagesByIdNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV2FindPackagesByIdCountNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsODataV2SearchNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsLicenseMdRenderingEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsODataV2SearchCountNonHijackedEnabled() => throw new NotImplementedException();
+
+        public bool IsMarkdigMdRenderingEnabled() => throw new NotImplementedException();
+
+        public bool IsDeletePackageApiEnabled(User user) => throw new NotImplementedException();
+
+        public bool IsImageAllowlistEnabled() => throw new NotImplementedException();
+    }
+}
diff --git a/src/VerifyMicrosoftPackage/VerifyMicrosoftPackage.csproj b/src/VerifyMicrosoftPackage/VerifyMicrosoftPackage.csproj
@@ -44,6 +44,7 @@
     <Compile Include="Fakes\FakeEntitiesContext.cs" />
     <Compile Include="Fakes\FakeEntityRepository.cs" />
     <Compile Include="Fakes\FakeSecurityPolicyService.cs" />
+    <Compile Include="Fakes\FakeFeatureFlagService.cs" />
     <Compile Include="Fakes\FakeTelemetryService.cs" />
     <Compile Include="Program.cs" />
     <Compile Include="Properties\AssemblyInfo.cs" />
diff --git a/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj b/tests/NuGetGallery.Facts/NuGetGallery.Facts.csproj
@@ -119,6 +119,7 @@
     <Compile Include="Services\StatusServiceFacts.cs" />
     <Compile Include="Telemetry\CustomerResourceIdEnricherFacts.cs" />
     <Compile Include="TestData\TestDataResourceUtility.cs" />
+    <Compile Include="TestUtils\MockPackageArchiveReader.cs" />
     <Compile Include="UsernameValidationRegex.cs" />
     <Compile Include="Extensions\NumberExtensionsFacts.cs" />
     <Compile Include="Extensions\RouteExtensionsFacts.cs" />
diff --git a/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/PackageServiceFacts.cs
@@ -33,7 +33,8 @@ private static IPackageService CreateService(
             Mock<ISecurityPolicyService> securityPolicyService = null,
             Action<Mock<PackageService>> setup = null,
             Mock<IEntitiesContext> context = null,
-            Mock<IContentObjectService> contentObjectService = null)
+            Mock<IContentObjectService> contentObjectService = null,
+            Mock<IFeatureFlagService> featureFlagService = null)
         {
             packageRegistrationRepository = packageRegistrationRepository ?? new Mock<IEntityRepository<PackageRegistration>>();
             packageRepository = packageRepository ?? new Mock<IEntityRepository<Package>>();
@@ -49,6 +50,12 @@ private static IPackageService CreateService(
                 contentObjectService.Setup(x => x.QueryHintConfiguration).Returns(Mock.Of<IQueryHintConfiguration>());
             }
 
+            if (featureFlagService == null)
+            {
+                featureFlagService = new Mock<IFeatureFlagService>();
+                featureFlagService.Setup(x => x.ArePatternSetTfmHeuristicsEnabled()).Returns(true);
+            }
+
             var packageService = new Mock<PackageService>(
                 packageRegistrationRepository.Object,
                 packageRepository.Object,
@@ -57,7 +64,8 @@ private static IPackageService CreateService(
                 telemetryService.Object,
                 securityPolicyService.Object,
                 context.Object,
-                contentObjectService.Object);
+                contentObjectService.Object,
+                featureFlagService.Object);
 
             packageService.CallBase = true;
 
@@ -970,6 +978,41 @@ private async Task WillNotSaveAnySupportedFrameworksWhenThereIsAnAnyTargetFramew
                 Assert.Empty(package.SupportedFrameworks);
             }
 
+            [Theory]
+            [InlineData(false, new[] { "net40", "net5.0", "netcore21" })]
+            [InlineData(true, new[] { "net5.0", "netcore21" })]
+            private void UsesTfmHeuristicsBasedOnFeatureFlag(bool useNewTfmHeuristics, IEnumerable<string> expectedSupportedTfms)
+            {
+                // arrange
+                // - create a package that responds differently to each set of heuristics
+                var nuspec =
+                    $@"<?xml version=""1.0""?>
+                        <package xmlns = ""http://schemas.microsoft.com/packaging/2011/08/nuspec.xsd"">
+                            <metadata>
+                                <id>Foo</id>
+                                <frameworkAssemblies>
+                                    <frameworkAssembly assemblyName=""System"" targetFramework="".NETFramework4.0"" />
+                                </frameworkAssemblies>
+                            </metadata>
+                        </package>";
+                var nuspecReader = new NuspecReader(XDocument.Parse(nuspec));
+                var files = new List<string> { "lib/netcore2.1/_._", "lib/net5.0/_._" };
+                var package = new MockPackageArchiveReader(nuspecReader, files);
+
+                // - create feature flag services and package services for both scenarios
+                var featureFlagService = new Mock<IFeatureFlagService>();
+                featureFlagService.Setup(x => x.ArePatternSetTfmHeuristicsEnabled()).Returns(useNewTfmHeuristics);
+
+                // act
+                var supportedFrameworks = CreateService(featureFlagService: featureFlagService).GetSupportedFrameworks(package)
+                    .Select(f => f.GetShortFolderName())
+                    .OrderBy(f => f)
+                    .ToList();
+
+                // assert
+                Assert.Equal<string>(expectedSupportedTfms, supportedFrameworks);
+            }
+
             [Theory]
             [MemberData(nameof(TargetFrameworkCases))]
             private void DeterminesCorrectSupportedFrameworksFromFileList(bool isTools, List<string> files, List<string> expectedSupportedFrameworks)
@@ -991,9 +1034,7 @@ private void DeterminesCorrectSupportedFrameworksFromFileList(bool isTools, List
                                 <id>Foo</id>
                             </metadata>
                         </package>";
-                var xml = XDocument.Parse(nuspec);
-
-                var nuspecReader = new NuspecReader(xml);
+                var nuspecReader = new NuspecReader(XDocument.Parse(nuspec));
 
                 // act
                 var supportedFrameworks = CreateService().GetSupportedFrameworks(nuspecReader, files)
diff --git a/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs b/tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs
@@ -345,6 +345,9 @@ private static Mock<PackageService> SetupPackageService(Package package)
             var securityPolicyService = new Mock<ISecurityPolicyService>();
             var entitiesContext = new Mock<IEntitiesContext>();
             var contentObjectService = new Mock<IContentObjectService>();
+            var featureFlagService = new Mock<IFeatureFlagService>();
+            featureFlagService.Setup(x => x.ArePatternSetTfmHeuristicsEnabled()).Returns(true);
+
 
             var packageService = new Mock<PackageService>(
                 packageRegistrationRepository.Object,
@@ -354,7 +357,8 @@ private static Mock<PackageService> SetupPackageService(Package package)
                 telemetryService.Object,
                 securityPolicyService.Object,
                 entitiesContext.Object,
-                contentObjectService.Object);
+                contentObjectService.Object,
+                featureFlagService.Object);
 
             packageService.CallBase = true;
 
diff --git a/tests/NuGetGallery.Facts/TestUtils/MockPackageArchiveReader.cs b/tests/NuGetGallery.Facts/TestUtils/MockPackageArchiveReader.cs
@@ -0,0 +1,26 @@
+// 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.Collections.Generic;
+using System.IO;
+using System.IO.Compression;
+using NuGet.Packaging;
+
+namespace NuGetGallery.TestUtils
+{
+    class MockPackageArchiveReader : PackageArchiveReader
+    {
+        private NuspecReader _nuspecReader;
+        private IEnumerable<string> _files;
+
+        public MockPackageArchiveReader(NuspecReader nuspecReader, IEnumerable<string> files)
+            : base(new ZipArchive(new MemoryStream(), ZipArchiveMode.Create, true))
+        {
+            _nuspecReader = nuspecReader;
+            _files = files;
+        }
+
+        public override NuspecReader NuspecReader => _nuspecReader;
+        public override IEnumerable<string> GetFiles() => _files;
+    }
+}
diff --git a/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs b/tests/NuGetGallery.Facts/TestUtils/TestServiceUtility.cs
@@ -197,6 +197,8 @@ private Mock<PackageService> SetupPackageService()
             var securityPolicyService = new Mock<ISecurityPolicyService>();
             var entitiesContext = new Mock<IEntitiesContext>();
             var contentObjectService = new Mock<IContentObjectService>();
+            var featureFlagService = new Mock<IFeatureFlagService>();
+            featureFlagService.Setup(x => x.ArePatternSetTfmHeuristicsEnabled()).Returns(true);
 
             var packageService = new Mock<PackageService>(
                  packageRegistrationRepository.Object,
@@ -206,7 +208,8 @@ private Mock<PackageService> SetupPackageService()
                  telemetryService.Object,
                  securityPolicyService.Object,
                  entitiesContext.Object,
-                 contentObjectService.Object);
+                 contentObjectService.Object,
+                 featureFlagService.Object);
 
             packageService.CallBase = true;
 
