Skip to content
Merged
Show file tree
Hide file tree
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
Original file line number Diff line number Diff line change
Expand Up @@ -1553,7 +1553,7 @@ private void ExecuteInstallPackageCommand(object sender, ExecutedRoutedEventArgs
private void PackageList_UpdateButtonClicked(PackageItemListViewModel[] selectedPackages)
{
var packagesToUpdate = selectedPackages
.Select(package => new PackageIdentity(package.Id, package.LatestVersion))
.Select(package => new PackageIdentity(package.Id, package.Version))
.ToList();

UpdatePackage(packagesToUpdate);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,11 @@ private async Task<IEnumerable<NuGetProjectAction>> PreviewUpdatePackagesAsync(
IEnumerable<SourceRepository> secondarySources,
CancellationToken token)
{
if (packageIdentities == null)
{
throw new ArgumentNullException(nameof(packageIdentities));
}

if (nuGetProjects == null)
{
throw new ArgumentNullException(nameof(nuGetProjects));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6975,6 +6975,73 @@ await nuGetPackageManager.PreviewBuildIntegratedProjectsActionsAsync(
}
}

/// <summary>
/// Repro for a bug caused by a NullReferenceException being thrown due to a null <see cref="PackageIdentity.Version"/>
Copy link
Member

Choose a reason for hiding this comment

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

What kind of value does this test provide us beyond being great for this review?

It's an input error, not necessarily a contract we're going to try to maintain.

Copy link
Contributor Author

@donnie-msft donnie-msft Jan 26, 2021

Choose a reason for hiding this comment

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

The input error is "by design". So you either handle the input error (I did not change this),
or you have a test proving it does what you found it does. It also helps someone see this code depends on non-null versions

Then, you have a test that can be easily flipped if/when we decide on a different design. Most of the test work is done, and the PR will be more clear.

Copy link
Member

Choose a reason for hiding this comment

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

Why did you decide against adding an input check?
Will the call always fail if the input PackageIdentity has a null version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I'm trying to fix a bug specifically. Not redesign the inners of Package Manager.
Not always, nope. Has to be BuildIntegrated project, with at least 1 update available.

Copy link
Member

Choose a reason for hiding this comment

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

Because I'm trying to fix a bug specifically. Not redesign the inners of Package Manager.

I think this can be considered in scope.
Changing the NuGetPackageManager class to ensure we don't run into unexpected exceptions is in scope.
Especially because there wouldn't be any side-effects.

Copy link
Contributor Author

@donnie-msft donnie-msft Jan 26, 2021

Choose a reason for hiding this comment

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

Especially because there wouldn't be any side-effects.

I don't have proof of that. And it's a 16.9 blocking bug, so not the time to start paying our debts. I think as part of 16.10 refactoring, this could be evaluated.

Copy link
Member

Choose a reason for hiding this comment

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

Synced up offline.

Call it good for 16.9, consider additional quality improvements in NuGetPackageManager for 16.10.

/// (https://github.com/NuGet/Home/issues/9882).
/// </summary>
/// <returns></returns>
[Fact]
public async Task TestPacMan_PreviewInstallPackage_BuildIntegrated_NullVersion_Throws()
{
// Arrange

// Set up Package Source
var packages = new List<SourcePackageDependencyInfo>
{
new SourcePackageDependencyInfo("a", new NuGetVersion(1, 0, 0), new PackageDependency[] { }, true, null),
new SourcePackageDependencyInfo("a", new NuGetVersion(2, 0, 0), new PackageDependency[] { }, true, null),
new SourcePackageDependencyInfo("a", new NuGetVersion(3, 0, 0), new PackageDependency[] { }, true, null),
};

SourceRepositoryProvider sourceRepositoryProvider = CreateSource(packages);

// Set up NuGetProject
var fwk45 = NuGetFramework.Parse("net45");

var installedPackages = new List<NuGet.Packaging.PackageReference>
{
new PackageReference(new PackageIdentity("a", new NuGetVersion(1, 0, 0)), fwk45, true),
};

var packageIdentity = _packageWithDependents[0];

// Create Package Manager
using (var solutionManager = new TestSolutionManager())
{
var nuGetPackageManager = new NuGetPackageManager(
sourceRepositoryProvider,
NullSettings.Instance,
solutionManager,
new TestDeleteOnRestartManager());

var buildIntegratedProjectA = new Mock<BuildIntegratedNuGetProject>();
buildIntegratedProjectA.Setup(p => p.GetInstalledPackagesAsync(CancellationToken.None))
.Returns(() => Task.FromResult(installedPackages.AsEnumerable()));

var projectList = new List<NuGetProject> { buildIntegratedProjectA.Object };
solutionManager.NuGetProjects = projectList;

// Main Act
var targets = new List<PackageIdentity>
{
new PackageIdentity("a", null)
};

// Assert
var ex = await Assert.ThrowsAsync<NullReferenceException>(async () =>
{
IEnumerable<NuGetProjectAction> result = await nuGetPackageManager.PreviewUpdatePackagesAsync(
targets,
projectList,
new ResolutionContext(),
new TestNuGetProjectContext(),
sourceRepositoryProvider.GetRepositories(),
sourceRepositoryProvider.GetRepositories(),
CancellationToken.None);
});
}
}

private void VerifyPreviewActionsTelemetryEvents_PackagesConfig(IEnumerable<string> actual)
{
Assert.True(actual.Contains(TelemetryConstants.GatherDependencyStepName));
Expand Down