Enable reflowing of packages that don't pass valid version range rule (…

…#5524)

Fix #5522

src/NuGetGallery.Core/Packaging/ManifestValidator.cs

@@ -22,7 +22,7 @@ public static IEnumerable<ValidationResult> Validate(Stream nuspecStream, out Nu
                 var rawMetadata = nuspecReader.GetMetadata();
                 if (rawMetadata != null && rawMetadata.Any())
                 {
-                    return ValidateCore(PackageMetadata.FromNuspecReader(nuspecReader));
+                    return ValidateCore(PackageMetadata.FromNuspecReader(nuspecReader, strict: true));
                 }
             }
             catch (Exception ex)

src/NuGetGallery.Core/Packaging/PackageMetadata.cs

@@ -147,15 +147,18 @@ private Uri GetValue(string key, Uri alternateValue)
         /// Gets package metadata from a the provided <see cref="NuspecReader"/> instance.
         /// </summary>
         /// <param name="nuspecReader">The <see cref="NuspecReader"/> instance used to read the <see cref="PackageMetadata"/></param>
+        /// <param name="strict">
+        /// Whether or not to be strict when reading the <see cref="NuspecReader"/>. This should be <code>true</code>
+        /// on initial ingestion but false when a package that has already been accepted is being processed.</param>
         /// <exception cref="PackagingException">
         /// We default to use a strict version-check on dependency groups. 
         /// When an invalid dependency version range is detected, a <see cref="PackagingException"/> will be thrown.
         /// </exception>
-        public static PackageMetadata FromNuspecReader(NuspecReader nuspecReader)
+        public static PackageMetadata FromNuspecReader(NuspecReader nuspecReader, bool strict)
         {
             return new PackageMetadata(
                 nuspecReader.GetMetadata().ToDictionary(kvp => kvp.Key, kvp => kvp.Value),
-                nuspecReader.GetDependencyGroups(useStrictVersionCheck: true),
+                nuspecReader.GetDependencyGroups(useStrictVersionCheck: strict),
                 nuspecReader.GetFrameworkReferenceGroups(),
                 nuspecReader.GetPackageTypes(),
                 nuspecReader.GetMinClientVersion()

src/NuGetGallery/Controllers/PackagesController.cs

@@ -169,7 +169,8 @@ public async virtual Task<ActionResult> UploadPackage()
                     try
                     {
                         packageMetadata = PackageMetadata.FromNuspecReader(
-                            package.GetNuspecReader());
+                            package.GetNuspecReader(),
+                            strict: true);
                     }
                     catch (Exception ex)
                     {
@@ -407,7 +408,8 @@ public virtual async Task<JsonResult> UploadPackage(HttpPostedFileBase uploadFil
                 try
                 {
                     packageMetadata = PackageMetadata.FromNuspecReader(
-                        package.GetNuspecReader());
+                        package.GetNuspecReader(),
+                        strict: true);
                 }
                 catch (Exception ex)
                 {
@@ -1545,7 +1547,8 @@ public virtual async Task<JsonResult> VerifyPackage(VerifyPackageRequest formDat
                 Debug.Assert(nugetPackage != null);
 
                 var packageMetadata = PackageMetadata.FromNuspecReader(
-                    nugetPackage.GetNuspecReader());
+                    nugetPackage.GetNuspecReader(),
+                    strict: true);
 
                 // Rule out problem scenario with multiple tabs - verification request (possibly with edits) was submitted by user
                 // viewing a different package to what was actually most recently uploaded

src/NuGetGallery/Services/PackageService.cs

@@ -45,7 +45,9 @@ public void EnsureValid(PackageArchiveReader packageArchiveReader)
         {
             try
             {
-                var packageMetadata = PackageMetadata.FromNuspecReader(packageArchiveReader.GetNuspecReader());
+                var packageMetadata = PackageMetadata.FromNuspecReader(
+                    packageArchiveReader.GetNuspecReader(),
+                    strict: true);
 
                 ValidateNuGetPackageMetadata(packageMetadata);
 
@@ -82,7 +84,9 @@ public async Task<Package> CreatePackageAsync(PackageArchiveReader nugetPackage,
 
             try
             {
-                packageMetadata = PackageMetadata.FromNuspecReader(nugetPackage.GetNuspecReader());
+                packageMetadata = PackageMetadata.FromNuspecReader(
+                    nugetPackage.GetNuspecReader(),
+                    strict: true);
 
                 ValidateNuGetPackageMetadata(packageMetadata);
 

src/NuGetGallery/Services/ReflowPackageService.cs

@@ -50,7 +50,9 @@ public async Task<Package> ReflowAsync(string id, string version)
                             Size = packageStream.Length,
                         };
 
-                        var packageMetadata = PackageMetadata.FromNuspecReader(packageArchive.GetNuspecReader());
+                        var packageMetadata = PackageMetadata.FromNuspecReader(
+                            packageArchive.GetNuspecReader(),
+                            strict: false);
 
                         // 3) Clear referenced objects that will be reflowed
                         ClearSupportedFrameworks(package);

tests/NuGetGallery.Core.Facts/Packaging/PackageMetadataFacts.cs

@@ -23,7 +23,9 @@ public static void CanReadBasicMetadataProperties()
             var nuspec = nupkg.GetNuspecReader();
 
             // Act
-            var packageMetadata = PackageMetadata.FromNuspecReader(nuspec);
+            var packageMetadata = PackageMetadata.FromNuspecReader(
+                nuspec,
+                strict: true);
 
             // Assert
             Assert.Equal("TestPackage", packageMetadata.Id);
@@ -47,7 +49,39 @@ public static void ThrowsPackagingExceptionWhenInvalidDepencencyVersionRangeDete
             var nuspec = nupkg.GetNuspecReader();
 
             // Act & Assert
-            Assert.Throws<PackagingException>(() => PackageMetadata.FromNuspecReader(nuspec));
+            Assert.Throws<PackagingException>(() => PackageMetadata.FromNuspecReader(
+                nuspec,
+                strict: true));
+        }
+
+        [Fact]
+        public static void DoesNotThrowInvalidDepencencyVersionRangeDetectedAndParsingIsNotStrict()
+        {
+            var packageStream = CreateTestPackageStreamWithInvalidDependencyVersion();
+            var nupkg = new PackageArchiveReader(packageStream, leaveStreamOpen: false);
+            var nuspec = nupkg.GetNuspecReader();
+
+            // Act
+            var packageMetadata = PackageMetadata.FromNuspecReader(
+                nuspec,
+                strict: false);
+
+            // Assert
+            Assert.Equal("TestPackage", packageMetadata.Id);
+            Assert.Equal(NuGetVersion.Parse("0.0.0.1"), packageMetadata.Version);
+            Assert.Equal("Package A", packageMetadata.Title);
+            Assert.Equal(2, packageMetadata.Authors.Count);
+            Assert.Equal("ownera, ownerb", packageMetadata.Owners);
+            Assert.False(packageMetadata.RequireLicenseAcceptance);
+            Assert.Equal("package A description.", packageMetadata.Description);
+            Assert.Equal("en-US", packageMetadata.Language);
+            Assert.Equal("http://www.nuget.org/", packageMetadata.ProjectUrl.ToString());
+            Assert.Equal("http://www.nuget.org/", packageMetadata.IconUrl.ToString());
+            Assert.Equal("http://www.nuget.org/", packageMetadata.LicenseUrl.ToString());
+            var dependencyGroup = Assert.Single(packageMetadata.GetDependencyGroups());
+            var dependency = Assert.Single(dependencyGroup.Packages);
+            Assert.Equal("SampleDependency", dependency.Id);
+            Assert.Equal(VersionRange.All, dependency.VersionRange);
         }
 
         private static Stream CreateTestPackageStream()

tests/NuGetGallery.Facts/Controllers/ApiControllerFacts.cs

@@ -96,7 +96,9 @@ public TestableApiController(
 
             MockPackageUploadService.Setup(x => x.GeneratePackageAsync(It.IsAny<string>(), It.IsAny<PackageArchiveReader>(), It.IsAny<PackageStreamMetadata>(), It.IsAny<User>(), It.IsAny<User>()))
                 .Returns((string id, PackageArchiveReader nugetPackage, PackageStreamMetadata packageStreamMetadata, User owner, User currentUser) => {
-                    var packageMetadata = PackageMetadata.FromNuspecReader(nugetPackage.GetNuspecReader());
+                    var packageMetadata = PackageMetadata.FromNuspecReader(
+                        nugetPackage.GetNuspecReader(),
+                        strict: true);
 
                     var package = new Package();
                     package.PackageRegistration = new PackageRegistration { Id = packageMetadata.Id, IsVerified = false };

tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs

@@ -36,7 +36,9 @@ private static PackageUploadService CreateService(
                 .CreatePackageAsync(It.IsAny<PackageArchiveReader>(), It.IsAny<PackageStreamMetadata>(), It.IsAny<User>(), It.IsAny<User>(), It.IsAny<bool>()))
                 .Returns((PackageArchiveReader packageArchiveReader, PackageStreamMetadata packageStreamMetadata, User owner, User currentUser, bool isVerified) =>
                 {
-                    var packageMetadata = PackageMetadata.FromNuspecReader(packageArchiveReader.GetNuspecReader());
+                    var packageMetadata = PackageMetadata.FromNuspecReader(
+                        packageArchiveReader.GetNuspecReader(),
+                        strict: true);
 
                     var newPackage = new Package();
                     newPackage.PackageRegistration = new PackageRegistration { Id = packageMetadata.Id, IsVerified = isVerified };

tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs

@@ -262,6 +262,46 @@ public async Task CallsUpdateIsLatestAsync()
                 // Assert
                 packageService.Verify(s => s.UpdateIsLatestAsync(package.PackageRegistration, false), Times.Once);
             }
+
+            [Fact]
+            public async Task AllowsInvalidPackageDependencyVersion()
+            {
+                // Arrange
+                var package = PackageServiceUtility.CreateTestPackage();
+
+                var packageService = SetupPackageService(package);
+                var entitiesContext = SetupEntitiesContext();
+                var packageFileService = SetupPackageFileService(
+                    package,
+                    CreateInvalidDependencyVersionTestPackageStream());
+
+                var service = CreateService(
+                    packageService: packageService,
+                    entitiesContext: entitiesContext,
+                    packageFileService: packageFileService);
+
+                // Act
+                var result = await service.ReflowAsync("test", "1.0.0");
+
+                // Assert
+                Assert.Equal("test", result.PackageRegistration.Id);
+                Assert.Equal("1.0.0", result.NormalizedVersion);
+
+                Assert.True(result.Dependencies.Any(d =>
+                    d.Id == "WebActivator"
+                    && d.VersionSpec == "(, )"
+                    && d.TargetFramework == "net40"));
+
+                Assert.True(result.Dependencies.Any(d =>
+                    d.Id == "PackageC"
+                    && d.VersionSpec == "[1.1.0, 2.0.1)"
+                    && d.TargetFramework == "net40"));
+
+                Assert.True(result.Dependencies.Any(d =>
+                    d.Id == "jQuery"
+                    && d.VersionSpec == "(, )"
+                    && d.TargetFramework == "net451"));
+            }
         }
 
         private static Mock<PackageService> SetupPackageService(Package package)
@@ -325,27 +365,50 @@ private static Mock<IEntitiesContext> SetupEntitiesContext()
             return entitiesContext;
         }
 
-        private static Mock<IPackageFileService> SetupPackageFileService(Package package)
+        private static Mock<IPackageFileService> SetupPackageFileService(Package package, Stream packageStream = null)
         {
             var packageFileService = new Mock<IPackageFileService>();
 
             packageFileService
                 .Setup(s => s.DownloadPackageFileAsync(package))
-                .Returns(Task.FromResult(CreateTestPackageStream()))
+                .Returns(Task.FromResult(packageStream ?? CreateTestPackageStream()))
                 .Verifiable();
 
             return packageFileService;
         }
 
+        private static Stream CreateInvalidDependencyVersionTestPackageStream()
+        {
+            return CreateTestPackageStream(@"<?xml version=""1.0""?>
+                    <package xmlns=""http://schemas.microsoft.com/packaging/2011/08/nuspec.xsd"">
+                      <metadata>
+                        <id>test</id>
+                        <version>1.0.0</version>
+                        <title>Test package</title>
+                        <authors>authora, authorb</authors>
+                        <owners>ownera</owners>
+                        <requireLicenseAcceptance>false</requireLicenseAcceptance>
+                        <description>package A description.</description>
+                        <language>en-US</language>
+                        <projectUrl>http://www.nuget.org/</projectUrl>
+                        <iconUrl>http://www.nuget.org/</iconUrl>
+                        <licenseUrl>http://www.nuget.org/</licenseUrl>
+                        <dependencies>
+                            <group targetFramework=""net40"">
+                              <dependency id=""WebActivator"" version="""" />
+                              <dependency id=""PackageC"" version=""[1.1.0, 2.0.1)"" />
+                            </group>
+                            <group targetFramework=""net451"">
+                              <dependency id=""jQuery"" version=""$version$""/>
+                            </group>
+                        </dependencies>
+                      </metadata>
+                    </package>");
+        }
+
         private static Stream CreateTestPackageStream()
         {
-            var packageStream = new MemoryStream();
-            using (var packageArchive = new ZipArchive(packageStream, ZipArchiveMode.Create, true))
-            {
-                var nuspecEntry = packageArchive.CreateEntry("TestPackage.nuspec", CompressionLevel.Fastest);
-                using (var streamWriter = new StreamWriter(nuspecEntry.Open()))
-                {
-                    streamWriter.WriteLine(@"<?xml version=""1.0""?>
+            return CreateTestPackageStream(@"<?xml version=""1.0""?>
                     <package xmlns=""http://schemas.microsoft.com/packaging/2011/08/nuspec.xsd"">
                       <metadata>
                         <id>test</id>
@@ -370,6 +433,17 @@ private static Stream CreateTestPackageStream()
                         </dependencies>
                       </metadata>
                     </package>");
+        }
+
+        private static Stream CreateTestPackageStream(string nuspec)
+        {
+            var packageStream = new MemoryStream();
+            using (var packageArchive = new ZipArchive(packageStream, ZipArchiveMode.Create, true))
+            {
+                var nuspecEntry = packageArchive.CreateEntry("TestPackage.nuspec", CompressionLevel.Fastest);
+                using (var streamWriter = new StreamWriter(nuspecEntry.Open()))
+                {
+                    streamWriter.WriteLine(nuspec);
                 }
 
                 packageArchive.CreateEntry("content\\HelloWorld.cs", CompressionLevel.Fastest);