Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable reflowing of packages that don't pass the valid version range rule #5524

Merged
merged 1 commit into from
Feb 24, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/NuGetGallery.Core/Packaging/ManifestValidator.cs
Original file line number Diff line number Diff line change
@@ -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)
7 changes: 5 additions & 2 deletions src/NuGetGallery.Core/Packaging/PackageMetadata.cs
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Contributor

@shishirx34 shishirx34 Feb 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: may be rename this parameter to dependencyVersionStrictCheck to better reflect the change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to leave it as script so that future validations can be relaxed with this same parameter.

{
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()
9 changes: 6 additions & 3 deletions src/NuGetGallery/Controllers/PackagesController.cs
Original file line number Diff line number Diff line change
@@ -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
8 changes: 6 additions & 2 deletions src/NuGetGallery/Services/PackageService.cs
Original file line number Diff line number Diff line change
@@ -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);

4 changes: 3 additions & 1 deletion src/NuGetGallery/Services/ReflowPackageService.cs
Original file line number Diff line number Diff line change
@@ -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);
38 changes: 36 additions & 2 deletions tests/NuGetGallery.Core.Facts/Packaging/PackageMetadataFacts.cs
Original file line number Diff line number Diff line change
@@ -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()
Original file line number Diff line number Diff line change
@@ -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 };
Original file line number Diff line number Diff line change
@@ -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 };
92 changes: 83 additions & 9 deletions tests/NuGetGallery.Facts/Services/ReflowPackageServiceFacts.cs
Original file line number Diff line number Diff line change
@@ -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);