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.

Sign up for GitHub

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

Fix #1260 by updating the Id casing on each upload. #1373

Closed
wants to merge 1 commit into from
Closed

Fix #1260 by updating the Id casing on each upload. #1373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
45 changes: 41 additions & 4 deletions Facts/Controllers/ApiControllerFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ public async Task CreatePackageWillSavePackageFileToFileStorage()
nuGetPackage.Setup(x => x.Metadata.Version).Returns(new SemanticVersion("1.0.42"));
controller.SetupPackageFromInputStream(nuGetPackage);

controller
.MockPackageService
.Setup(p => p.CreatePackage(nuGetPackage.Object, fakeUser, false))
.Returns(new Package() { PackageRegistration = new PackageRegistration() { Id = "theId" } });

// Act
await controller.CreatePackagePut(guid);

Expand Down Expand Up @@ -192,7 +197,33 @@ public void WillCreateAPackageFromTheNuGetPackage()

controller.CreatePackagePut(Guid.NewGuid().ToString());

controller.MockPackageService.Verify(x => x.CreatePackage(nuGetPackage.Object, It.IsAny<User>(), true));
controller.MockPackageService.Verify(x => x.CreatePackage(nuGetPackage.Object, It.IsAny<User>(), false));
}

[Fact]
public void WillUpdateThePackageRegistrationIdIfTheCasingIsDifferent()
{
var nuGetPackage = new Mock<INupkg>();
nuGetPackage.Setup(x => x.Metadata.Id).Returns("theId");
nuGetPackage.Setup(x => x.Metadata.Version).Returns(new SemanticVersion("1.0.42"));

var controller = new TestableApiController();
controller.MockUserService.Setup(x => x.FindByApiKey(It.IsAny<Guid>())).Returns(new User());
controller.SetupPackageFromInputStream(nuGetPackage);

var package = new Package() { PackageRegistration = new PackageRegistration() { Id = "TheID" } };
controller
.MockPackageService
.Setup(x => x.CreatePackage(nuGetPackage.Object, It.IsAny<User>(), false))
.Returns(package)
.Verifiable();


controller.CreatePackagePut(Guid.NewGuid().ToString());

Assert.Equal("theId", package.PackageRegistration.Id);
controller.MockEntitiesContext.Verify(e => e.SaveChanges());
controller.MockPackageService.VerifyAll();
}

[Fact]
Expand All @@ -208,7 +239,7 @@ public void WillCreateAPackageWithTheUserMatchingTheApiKey()

controller.CreatePackagePut(Guid.NewGuid().ToString());

controller.MockPackageService.Verify(x => x.CreatePackage(It.IsAny<INupkg>(), matchingUser, true));
controller.MockPackageService.Verify(x => x.CreatePackage(It.IsAny<INupkg>(), matchingUser, false));
}

[Fact]
Expand All @@ -220,12 +251,18 @@ public void CreatePackageRefreshesNuGetExeIfCommandLinePackageIsUploaded()
nuGetPackage.Setup(x => x.Metadata.Version).Returns(new SemanticVersion("1.0.42"));
var matchingUser = new User();
var controller = new TestableApiController();
controller.MockPackageService.Setup(p => p.CreatePackage(nuGetPackage.Object, It.IsAny<User>(), true))
controller.MockPackageService.Setup(p => p.CreatePackage(nuGetPackage.Object, It.IsAny<User>(), false))
.Returns(new Package { IsLatestStable = true });
controller.MockNuGetExeDownloaderService.Setup(s => s.UpdateExecutableAsync(nuGetPackage.Object)).Verifiable();
controller.MockUserService.Setup(x => x.FindByApiKey(It.IsAny<Guid>())).Returns(matchingUser);
controller.SetupPackageFromInputStream(nuGetPackage);

controller
.MockPackageService
.Setup(p => p.CreatePackage(nuGetPackage.Object, matchingUser, false))
.Returns(new Package() { IsLatestStable = true, PackageRegistration = new PackageRegistration() { Id = "NuGet.CommandLine" } });


// Act
controller.CreatePackagePut(Guid.NewGuid().ToString());

Expand All @@ -242,7 +279,7 @@ public void CreatePackageDoesNotRefreshNuGetExeIfItIsNotLatestStable()
nuGetPackage.Setup(x => x.Metadata.Version).Returns(new SemanticVersion("2.0.0-alpha"));
var matchingUser = new User();
var controller = new TestableApiController();
controller.MockPackageService.Setup(p => p.CreatePackage(nuGetPackage.Object, It.IsAny<User>(), true))
controller.MockPackageService.Setup(p => p.CreatePackage(nuGetPackage.Object, It.IsAny<User>(), false))
.Returns(new Package { IsLatest = true, IsLatestStable = false });
controller.MockNuGetExeDownloaderService.Setup(s => s.UpdateExecutableAsync(nuGetPackage.Object)).Verifiable();
controller.MockUserService.Setup(x => x.FindByApiKey(It.IsAny<Guid>())).Returns(matchingUser);
Expand Down
51 changes: 49 additions & 2 deletions Facts/Controllers/PackagesControllerFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,6 +1077,7 @@ public async Task WillCreateThePackage()
fakePackageService.Setup(x => x.CreatePackage(It.IsAny<INupkg>(), It.IsAny<User>(), It.IsAny<bool>())).Returns(
new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" });
var fakeNuGetPackage = new Mock<INupkg>();
fakeNuGetPackage.Setup(p => p.Metadata).Returns(new Mock<IPackageMetadata>().Object);

var controller = CreateController(
packageService: fakePackageService,
Expand Down Expand Up @@ -1108,6 +1109,7 @@ public async Task WillSavePackageToFileStorage()
var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" };
fakePackageService.Setup(x => x.CreatePackage(It.IsAny<INupkg>(), It.IsAny<User>(), It.IsAny<bool>())).Returns(fakePackage);
var fakeNuGetPackage = new Mock<INupkg>();
fakeNuGetPackage.Setup(p => p.Metadata).Returns(new Mock<IPackageMetadata>().Object);
var fakePackageFileService = new Mock<IPackageFileService>();
fakePackageFileService.Setup(x => x.SavePackageFileAsync(fakePackage, It.IsAny<Stream>())).Returns(Task.FromResult(0)).Verifiable();

Expand Down Expand Up @@ -1145,6 +1147,7 @@ public async Task WillUpdateIndexingService()
var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" };
fakePackageService.Setup(x => x.CreatePackage(It.IsAny<INupkg>(), It.IsAny<User>(), It.IsAny<bool>())).Returns(fakePackage);
var fakeNuGetPackage = new Mock<INupkg>();
fakeNuGetPackage.Setup(p => p.Metadata).Returns(new Mock<IPackageMetadata>().Object);
var fakePackageFileService = new Mock<IPackageFileService>();
fakePackageFileService.Setup(x => x.SavePackageFileAsync(fakePackage, It.IsAny<Stream>())).Returns(Task.FromResult(0)).Verifiable();

Expand Down Expand Up @@ -1185,6 +1188,7 @@ public async Task WillSaveChangesToEntitiesContext()
var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" };
fakePackageService.Setup(x => x.CreatePackage(It.IsAny<INupkg>(), It.IsAny<User>(), It.IsAny<bool>())).Returns(fakePackage);
var fakeNuGetPackage = new Mock<INupkg>();
fakeNuGetPackage.Setup(p => p.Metadata).Returns(new Mock<IPackageMetadata>().Object);

var entitiesContext = new Mock<IEntitiesContext>();
entitiesContext.Setup(e => e.SaveChanges()).Verifiable();
Expand Down Expand Up @@ -1224,6 +1228,7 @@ public async Task WillNotCommitChangesToPackageService()
fakePackageService.Setup(x => x.PublishPackage(fakePackage, false));
fakePackageService.Setup(x => x.MarkPackageUnlisted(fakePackage, false));
var fakeNuGetPackage = new Mock<INupkg>();
fakeNuGetPackage.Setup(p => p.Metadata).Returns(new Mock<IPackageMetadata>().Object);

var controller = CreateController(
packageService: fakePackageService,
Expand Down Expand Up @@ -1257,6 +1262,7 @@ public async Task WillPublishThePackage()
var fakePackageService = new Mock<IPackageService>();
fakePackageService.Setup(x => x.CreatePackage(It.IsAny<INupkg>(), It.IsAny<User>(), It.IsAny<bool>())).Returns(fakePackage);
var fakeNuGetPackage = new Mock<INupkg>();
fakeNuGetPackage.Setup(p => p.Metadata).Returns(new Mock<IPackageMetadata>().Object);
var controller = CreateController(
packageService: fakePackageService,
uploadFileService: fakeUploadFileService,
Expand All @@ -1270,6 +1276,40 @@ public async Task WillPublishThePackage()
fakeFileStream.Dispose();
}

[Fact]
public async Task WillUpdateTheIdOfTheRegistrationIfTheCasingChanges()
{
var fakeCurrentUser = new User { Key = 42 };
var fakeUserService = new Mock<IUserService>();
fakeUserService.Setup(x => x.FindByUsername(It.IsAny<string>())).Returns(fakeCurrentUser);
var fakeIdentity = new Mock<IIdentity>();
fakeIdentity.Setup(x => x.Name).Returns("theUsername");
var fakeUploadFileService = new Mock<IUploadFileService>();
var fakeFileStream = new MemoryStream();
fakeUploadFileService.Setup(x => x.GetUploadFileAsync(42)).Returns(Task.FromResult<Stream>(fakeFileStream));
fakeUploadFileService.Setup(x => x.DeleteUploadFileAsync(42)).Returns(Task.FromResult(0));
var fakePackage = new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" };
var fakePackageService = new Mock<IPackageService>();
fakePackageService.Setup(x => x.CreatePackage(It.IsAny<INupkg>(), It.IsAny<User>(), It.IsAny<bool>())).Returns(fakePackage);
var fakeNuGetPackage = new Mock<INupkg>();
fakeNuGetPackage.Setup(p => p.Metadata.Id).Returns("TheID");
var fakeEntitiesContext = new Mock<IEntitiesContext>();
var controller = CreateController(
packageService: fakePackageService,
uploadFileService: fakeUploadFileService,
userService: fakeUserService,
fakeIdentity: fakeIdentity,
fakeNuGetPackage: fakeNuGetPackage,
entitiesContext: fakeEntitiesContext);

await controller.VerifyPackage(null);

fakePackageService.Verify(x => x.PublishPackage(fakePackage, false), Times.Once());
Assert.Equal("TheID", fakePackage.PackageRegistration.Id);
fakeEntitiesContext.Verify(e => e.SaveChanges());
fakeFileStream.Dispose();
}

[Fact]
public async Task WillMarkThePackageUnlistedWhenListedArgumentIsFalse()
{
Expand Down Expand Up @@ -1318,6 +1358,7 @@ public async Task WillNotMarkThePackageUnlistedWhenListedArgumentIsNullorTrue(bo
fakePackageService.Setup(x => x.CreatePackage(It.IsAny<INupkg>(), It.IsAny<User>(), It.IsAny<bool>())).Returns(
new Package { PackageRegistration = new PackageRegistration { Id = "theId" }, Version = "theVersion" });
var fakeNuGetPackage = new Mock<INupkg>();
fakeNuGetPackage.Setup(p => p.Metadata).Returns(new Mock<IPackageMetadata>().Object);
var controller = CreateController(
packageService: fakePackageService,
uploadFileService: fakeUploadFileService,
Expand Down Expand Up @@ -1472,12 +1513,15 @@ public async Task WillExtractNuGetExe()
fakePackageService.Setup(x => x.CreatePackage(It.IsAny<INupkg>(), It.IsAny<User>(), It.IsAny<bool>())).Returns(commandLinePackage);
var nugetExeDownloader = new Mock<INuGetExeDownloaderService>(MockBehavior.Strict);
nugetExeDownloader.Setup(d => d.UpdateExecutableAsync(It.IsAny<INupkg>())).Returns(Task.FromResult(0)).Verifiable();
var fakeNupkg = new Mock<INupkg>();
fakeNupkg.Setup(p => p.Metadata.Id).Returns(commandLinePackage.PackageRegistration.Id);
var controller = CreateController(
packageService: fakePackageService,
uploadFileService: fakeUploadFileService,
fakeIdentity: fakeIdentity,
userService: fakeUserService,
downloaderService: nugetExeDownloader);
downloaderService: nugetExeDownloader,
fakeNuGetPackage: fakeNupkg);

// Act
await controller.VerifyPackage(false);
Expand Down Expand Up @@ -1507,12 +1551,15 @@ public async Task WillNotExtractNuGetExeIfIsNotLatestStable()
};
fakePackageService.Setup(x => x.CreatePackage(It.IsAny<INupkg>(), It.IsAny<User>(), It.IsAny<bool>())).Returns(commandLinePackage);
var nugetExeDownloader = new Mock<INuGetExeDownloaderService>(MockBehavior.Strict);
var fakeNupkg = new Mock<INupkg>();
fakeNupkg.Setup(p => p.Metadata.Id).Returns(commandLinePackage.PackageRegistration.Id);
var controller = CreateController(
packageService: fakePackageService,
uploadFileService: fakeUploadFileService,
fakeIdentity: fakeIdentity,
userService: fakeUserService,
downloaderService: nugetExeDownloader);
downloaderService: nugetExeDownloader,
fakeNuGetPackage: fakeNupkg);

// Act
await controller.VerifyPackage(false);
Expand Down
13 changes: 12 additions & 1 deletion Website/Controllers/ApiController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ private async Task<ActionResult> CreatePackageInternal(string apiKey)
{
// Ensure that the user can push packages for this partialId.
var packageRegistration = PackageService.FindPackageRegistrationById(packageToPush.Metadata.Id);

if (packageRegistration != null)
{
if (!packageRegistration.IsOwner(user))
Expand All @@ -244,12 +245,22 @@ private async Task<ActionResult> CreatePackageInternal(string apiKey)
}
}

var package = PackageService.CreatePackage(packageToPush, user, commitChanges: true);
var package = PackageService.CreatePackage(packageToPush, user, commitChanges: false);

// Update the package registration's ID if it doesn't already match.
if (packageToPush != null && packageToPush.Metadata != null) // A little extra defensiveness makes the tests simpler :)
{
package.PackageRegistration.Id = packageToPush.Metadata.Id;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Q: "But Andrew, what if we need the history of the Id for previous packages?" A: "It's in the Nupkgs :)"

}

using (Stream uploadStream = packageToPush.GetStream())
{
await PackageFileService.SavePackageFileAsync(package, uploadStream);
}

EntitiesContext.SaveChanges();
IndexingService.UpdateIndex();

if (
packageToPush.Metadata.Id.Equals(Constants.NuGetCommandLinePackageId,
StringComparison.OrdinalIgnoreCase) && package.IsLatestStable)
Expand Down
9 changes: 8 additions & 1 deletion Website/Controllers/PackagesController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,12 @@ public virtual async Task<ActionResult> VerifyPackage(bool? listed)
package = _packageService.CreatePackage(nugetPackage, currentUser, commitChanges: false);
Debug.Assert(package.PackageRegistration != null);

// Update the package registration's ID if it doesn't already match.
if (nugetPackage.Metadata != null) // A little extra defensiveness makes the tests simpler :)
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't there a throwing else block, would that also make the tests more complicated? It seems wrong to me to accept package uploads without a package ID in the package (although it's likely we would fail before we got to this point, if that is the case maybe we don't need an if statement?).

Copy link
Member

Choose a reason for hiding this comment

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

github makes this code look like it's in GetPackageOwnerActionFormResult which doesn't make sense to me as an abstraction. Is this a github bug, or a real oddity in the code?

{
package.PackageRegistration.Id = nugetPackage.Metadata.Id;
}

_packageService.PublishPackage(package, commitChanges: false);

if (listed == false)
Expand All @@ -642,7 +648,8 @@ public virtual async Task<ActionResult> VerifyPackage(bool? listed)
_indexingService.UpdateIndex();

// If we're pushing a new stable version of NuGet.CommandLine, update the extracted executable.
if (package.PackageRegistration.Id.Equals(Constants.NuGetCommandLinePackageId, StringComparison.OrdinalIgnoreCase) &&
if (!String.IsNullOrEmpty(package.PackageRegistration.Id) &&
Copy link
Member

Choose a reason for hiding this comment

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

Again accepting null or empty package ids feels kinda wrong.

package.PackageRegistration.Id.Equals(Constants.NuGetCommandLinePackageId, StringComparison.OrdinalIgnoreCase) &&
package.IsLatestStable)
{
await _nugetExeDownloaderService.UpdateExecutableAsync(nugetPackage);
Expand Down