From f975b50ffa16a5e64e0358737b90a9fb5624b8d4 Mon Sep 17 00:00:00 2001 From: CrispyDrone Date: Sun, 30 Aug 2020 16:46:09 +0200 Subject: [PATCH 1/2] test: Showcase incorrect update behavior Added test for `PackageUpdatesLookup` that demonstrates that updates are incorrectly applied across different projects. `AllowedChange` needs to be applied to each version independently, not across all projects as we currently cannot guarantee that these different projects are loaded in the same process at runtime. --- .../NuGetApi/PackageUpdatesLookupTests.cs | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 NuKeeper.Inspection.Tests/NuGetApi/PackageUpdatesLookupTests.cs diff --git a/NuKeeper.Inspection.Tests/NuGetApi/PackageUpdatesLookupTests.cs b/NuKeeper.Inspection.Tests/NuGetApi/PackageUpdatesLookupTests.cs new file mode 100644 index 000000000..685557dfb --- /dev/null +++ b/NuKeeper.Inspection.Tests/NuGetApi/PackageUpdatesLookupTests.cs @@ -0,0 +1,129 @@ +using NSubstitute; +using NUnit.Framework; +using NuGet.Configuration; +using NuGet.Packaging.Core; +using NuGet.Versioning; +using NuKeeper.Abstractions.Configuration; +using NuKeeper.Abstractions.NuGet; +using NuKeeper.Abstractions.NuGetApi; +using NuKeeper.Abstractions.RepositoryInspection; +using NuKeeper.Inspection.NuGetApi; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; + +namespace NuKeeper.Inspection.Tests.NuGetApi +{ + [TestFixture] + public class PackageUpdatesLookupTests + { + IApiPackageLookup _apiPackageLookup; + + [SetUp] + public void Initialize() + { + _apiPackageLookup = Substitute.For(); + + _apiPackageLookup + .FindVersionUpdate( + Arg.Any(), + Arg.Any(), + VersionChange.Minor, + Arg.Any() + ) + .Returns(ci => + GetPackageLookupResult( + (PackageIdentity)ci[0], + (VersionChange)ci[2] + ) + ); + } + + [Test] + public async Task FindUpdatesForPackages_OnePackageDifferentVersionsInDifferentProjects_RespectsAllowedChangeForEachVersionIndependently() + { + var packagesInProjects = new List + { + MakePackageInProject("foo.bar", "6.0.1", "root", "myprojectA"), + MakePackageInProject("foo.bar", "10.2.1", "root", "myprojectB") + }; + var sut = MakePackageUpdatesLookup(); + + var result = await sut.FindUpdatesForPackages( + packagesInProjects, + new NuGetSources("https://api.nuget.com/"), + VersionChange.Minor, + UsePrerelease.Never + ); + + var versionUpdates = result.Select(p => p.SelectedVersion.ToNormalizedString()); +#pragma warning disable CA1307 // Specify StringComparison + Assert.That(versionUpdates, Has.Some.Matches(version => version.StartsWith("6."))); + Assert.That(versionUpdates, Has.Some.Matches(version => version.StartsWith("10."))); +#pragma warning restore CA1307 // Specify StringComparison + } + + private static PackageLookupResult GetPackageLookupResult( + PackageIdentity package, + VersionChange versionChange + ) + { + var packageName = package.Id; + var major = package.Version.Major; + var minor = package.Version.Minor; + var patch = package.Version.Patch; + + return new PackageLookupResult( + versionChange, + MakePackageSearchMetadata(packageName, $"{major + 1}.0.0"), + MakePackageSearchMetadata(packageName, $"{major}.{minor+1}.0"), + MakePackageSearchMetadata(packageName, $"{major}.{minor}.{patch+1}") + ); + } + + private static PackageSearchMetadata MakePackageSearchMetadata( + string packageName, + string version, + string source = "https://api.nuget.com/" + ) + { + return new PackageSearchMetadata( + new PackageIdentity( + packageName, + new NuGetVersion(version) + ), + new PackageSource(source), + null, + null + ); + } + + private static PackageInProject MakePackageInProject( + string packageName, + string version, + string rootDir, + string relativeDir + ) + { + return new PackageInProject( + packageName, + version, + new PackagePath( + rootDir, + relativeDir, + PackageReferenceType.PackagesConfig + ) + ); + } + + private PackageUpdatesLookup MakePackageUpdatesLookup() + { + return new PackageUpdatesLookup( + new BulkPackageLookup( + _apiPackageLookup, + Substitute.For() + ) + ); + } + } +} From 8aaa8a7cd9a0ec744bd1a9f830300e9f16223b53 Mon Sep 17 00:00:00 2001 From: CrispyDrone Date: Sun, 30 Aug 2020 17:46:01 +0200 Subject: [PATCH 2/2] fix: Ensure AllowedChange works cross-project In case a package is installed in multiple projects, allowed change will only consider the highest installed version instead of looking at each version independently. This would result in an update being applied that is outside of the allowed change range for projects for which the version of another project with the highest version is considered outside of their allowed change range. --- .../NuGetApi/BulkPackageLookupTests.cs | 14 +++--- .../NuGetApi/BulkPackageLookup.cs | 44 ++++++++++++------- .../NuGetApi/IBulkPackageLookup.cs | 2 +- .../NuGetApi/PackageUpdatesLookup.cs | 2 +- .../NuGet/Api/BulkPackageLookupTests.cs | 15 ++++--- 5 files changed, 47 insertions(+), 30 deletions(-) diff --git a/NuKeeper.Inspection.Tests/NuGetApi/BulkPackageLookupTests.cs b/NuKeeper.Inspection.Tests/NuGetApi/BulkPackageLookupTests.cs index 10b25e829..acc32d3c0 100644 --- a/NuKeeper.Inspection.Tests/NuGetApi/BulkPackageLookupTests.cs +++ b/NuKeeper.Inspection.Tests/NuGetApi/BulkPackageLookupTests.cs @@ -59,7 +59,7 @@ public async Task CanLookupOnePackage() Assert.That(results, Is.Not.Null); Assert.That(results, Is.Not.Empty); Assert.That(results.Count, Is.EqualTo(1)); - Assert.That(results.ContainsKey("foo"), Is.True); + Assert.That(results.ElementAt(0).Key.Id, Is.EqualTo("foo")); } [Test] @@ -106,10 +106,11 @@ public async Task CanLookupTwoPackages() VersionChange.Major, UsePrerelease.FromPrerelease); + var packages = results.Select(kvp => kvp.Key); Assert.That(results.Count, Is.EqualTo(2)); - Assert.That(results.ContainsKey("foo"), Is.True); - Assert.That(results.ContainsKey("bar"), Is.True); - Assert.That(results.ContainsKey("fish"), Is.False); + Assert.That(packages, Has.Some.Matches(pi => pi.Id == "foo")); + Assert.That(packages, Has.Some.Matches(pi => pi.Id == "bar")); + Assert.That(packages, Has.None.Matches(pi => pi.Id == "fish")); } [Test] @@ -163,9 +164,10 @@ await apiLookup.Received(1).FindVersionUpdate(Arg.Is( pi => pi.Id == "foo" && pi.Version == new NuGetVersion(1, 3, 4)), Arg.Any(), Arg.Any(), Arg.Any()); + var packages = results.Select(kvp => kvp.Key); Assert.That(results.Count, Is.EqualTo(1)); - Assert.That(results.ContainsKey("foo"), Is.True); - Assert.That(results.ContainsKey("bar"), Is.False); + Assert.That(packages, Has.Some.Matches(pi => pi.Id == "foo")); + Assert.That(packages, Has.None.Matches(pi => pi.Id == "bar")); } private static void ApiHasNewVersionForPackage(IApiPackageLookup lookup, string packageName) diff --git a/NuKeeper.Inspection/NuGetApi/BulkPackageLookup.cs b/NuKeeper.Inspection/NuGetApi/BulkPackageLookup.cs index d7dfa166d..8f1f0a4a2 100644 --- a/NuKeeper.Inspection/NuGetApi/BulkPackageLookup.cs +++ b/NuKeeper.Inspection/NuGetApi/BulkPackageLookup.cs @@ -22,42 +22,56 @@ public BulkPackageLookup( _lookupReporter = lookupReporter; } - public async Task> FindVersionUpdates( + public async Task> FindVersionUpdates( IEnumerable packages, NuGetSources sources, VersionChange allowedChange, - UsePrerelease usePrerelease) + UsePrerelease usePrerelease + ) { - var latestOfEach = packages - .GroupBy(pi => pi.Id.ToUpperInvariant()) - .Select(HighestVersion); - - var lookupTasks = latestOfEach - .Select(id => _packageLookup.FindVersionUpdate(id, sources, allowedChange, usePrerelease)) + var lookupTasks = packages + .Distinct() + .GroupBy(pi => (pi.Id, MaxVersion: GetMaxVersion(pi, allowedChange))) + .Select(HighestVersion) + .Select(id => new { Package = id, Update = _packageLookup.FindVersionUpdate(id, sources, allowedChange, usePrerelease) }) .ToList(); - await Task.WhenAll(lookupTasks); + await Task.WhenAll(lookupTasks.Select(l => l.Update)); - var result = new Dictionary(StringComparer.OrdinalIgnoreCase); + var result = new Dictionary(); foreach (var lookupTask in lookupTasks) { - var serverVersions = lookupTask.Result; - ProcessLookupResult(serverVersions, result); + ProcessLookupResult(lookupTask.Package, lookupTask.Update.Result, result); } return result; } - private void ProcessLookupResult(PackageLookupResult packageLookup, IDictionary result) + private static string GetMaxVersion(PackageIdentity pi, VersionChange allowedChange) + { + return allowedChange switch + { + VersionChange.Major => "X.X.X", + VersionChange.Minor => $"{pi.Version.Major}.X.X", + VersionChange.Patch => $"{pi.Version.Major}.{pi.Version.Minor}.X", + VersionChange.None => $"{pi.Version.Major}.{pi.Version.Minor}.{pi.Version.Patch}", + _ => throw new ArgumentOutOfRangeException(nameof(allowedChange)), + }; + } + + private void ProcessLookupResult( + PackageIdentity package, + PackageLookupResult packageLookup, + IDictionary result + ) { var selectedVersion = packageLookup.Selected(); if (selectedVersion?.Identity?.Version != null) { _lookupReporter.Report(packageLookup); - var packageId = selectedVersion.Identity.Id; - result.Add(packageId, packageLookup); + result.Add(package, packageLookup); } } diff --git a/NuKeeper.Inspection/NuGetApi/IBulkPackageLookup.cs b/NuKeeper.Inspection/NuGetApi/IBulkPackageLookup.cs index df8e9f577..fe44f3806 100644 --- a/NuKeeper.Inspection/NuGetApi/IBulkPackageLookup.cs +++ b/NuKeeper.Inspection/NuGetApi/IBulkPackageLookup.cs @@ -9,7 +9,7 @@ namespace NuKeeper.Inspection.NuGetApi { public interface IBulkPackageLookup { - Task> FindVersionUpdates( + Task> FindVersionUpdates( IEnumerable packages, NuGetSources sources, VersionChange allowedChange, diff --git a/NuKeeper.Inspection/NuGetApi/PackageUpdatesLookup.cs b/NuKeeper.Inspection/NuGetApi/PackageUpdatesLookup.cs index 9f0aae060..409d02cac 100644 --- a/NuKeeper.Inspection/NuGetApi/PackageUpdatesLookup.cs +++ b/NuKeeper.Inspection/NuGetApi/PackageUpdatesLookup.cs @@ -38,7 +38,7 @@ public async Task> FindUpdatesForPackages( var matchVersion = latestPackage.Selected().Identity.Version; var updatesForThisPackage = packages - .Where(p => p.Id.Equals(packageId,StringComparison.InvariantCultureIgnoreCase) && p.Version < matchVersion) + .Where(p => p.Identity.Equals(packageId) && p.Version < matchVersion) .ToList(); if (updatesForThisPackage.Count > 0) diff --git a/NuKeeper.Integration.Tests/NuGet/Api/BulkPackageLookupTests.cs b/NuKeeper.Integration.Tests/NuGet/Api/BulkPackageLookupTests.cs index 71b7182d7..60313b968 100644 --- a/NuKeeper.Integration.Tests/NuGet/Api/BulkPackageLookupTests.cs +++ b/NuKeeper.Integration.Tests/NuGet/Api/BulkPackageLookupTests.cs @@ -25,9 +25,10 @@ public async Task CanFindUpdateForOneWellKnownPackage() packages, NuGetSources.GlobalFeed, VersionChange.Major, UsePrerelease.FromPrerelease); + var updatedPackages = results.Select(p => p.Key); Assert.That(results, Is.Not.Null); Assert.That(results.Count, Is.EqualTo(1)); - Assert.That(results, Does.ContainKey("Moq")); + Assert.That(updatedPackages, Has.Some.Matches(p => p.Id == "Moq")); } [Test] @@ -45,10 +46,11 @@ public async Task CanFindUpdateForTwoWellKnownPackages() packages, NuGetSources.GlobalFeed, VersionChange.Major, UsePrerelease.FromPrerelease); + var updatedPackages = results.Select(p => p.Key); Assert.That(results, Is.Not.Null); Assert.That(results.Count, Is.EqualTo(2)); - Assert.That(results, Does.ContainKey("Moq")); - Assert.That(results, Does.ContainKey("Newtonsoft.Json")); + Assert.That(updatedPackages, Has.Some.Matches(p => p.Id == "Moq")); + Assert.That(updatedPackages, Has.Some.Matches(p => p.Id == "Newtonsoft.Json")); } [Test] @@ -68,8 +70,6 @@ public async Task FindsSingleUpdateForPackageDifferingOnlyByCase() Assert.That(results, Is.Not.Null); Assert.That(results.Count, Is.EqualTo(1)); - Assert.That(results.ContainsKey("NUnit"), "results.ContainsKey('NUnit')"); - Assert.That(results.ContainsKey("nunit"), "results.ContainsKey('nunit')"); } [Test] @@ -120,10 +120,11 @@ public async Task ValidPackagesWorkDespiteInvalidPackages() packages, NuGetSources.GlobalFeed, VersionChange.Major, UsePrerelease.FromPrerelease); + var updatedPackages = results.Select(p => p.Key); Assert.That(results, Is.Not.Null); Assert.That(results.Count, Is.EqualTo(2)); - Assert.That(results, Does.ContainKey("Moq")); - Assert.That(results, Does.ContainKey("Newtonsoft.Json")); + Assert.That(updatedPackages, Has.Some.Matches(p => p.Id == "Moq")); + Assert.That(updatedPackages, Has.Some.Matches(p => p.Id == "Newtonsoft.Json")); } private BulkPackageLookup BuildBulkPackageLookup()