From 4159f9de46d4c6755683d2440728f4ba4c927071 Mon Sep 17 00:00:00 2001 From: Sebastian Gomez Date: Mon, 18 Nov 2024 14:29:03 -0500 Subject: [PATCH] Handle semicolons in packageReferences (#10909) * Handle semicolons in packageReferences --- .../Discover/DiscoveryWorkerTests.cs | 100 +++++++ .../Run/RunWorkerTests.cs | 250 ++++++++++++++++++ .../UpdateWorkerTests.PackageReference.cs | 39 +++ .../Files/ProjectBuildFile.cs | 23 +- .../Updater/PackageReferenceUpdater.cs | 20 +- .../Utilities/MSBuildHelper.cs | 6 +- 6 files changed, 424 insertions(+), 14 deletions(-) diff --git a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Discover/DiscoveryWorkerTests.cs b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Discover/DiscoveryWorkerTests.cs index f91254b2db..401826e951 100644 --- a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Discover/DiscoveryWorkerTests.cs +++ b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Discover/DiscoveryWorkerTests.cs @@ -107,6 +107,106 @@ await TestDiscoveryAsync( ); } + [Fact] + public async Task TestDependenciesSeparatedBySemicolon() + { + await TestDiscoveryAsync( + packages: + [ + MockNuGetPackage.CreateSimplePackage("Some.Package", "9.0.1", "net8.0"), + MockNuGetPackage.CreateSimplePackage("Some.Package2", "9.0.1", "net8.0"), + ], + workspacePath: "src", + files: new[] + { + ("src/project.csproj", """ + + + net8.0 + 9.0.1 + + + + + + + """) + }, + expectedResult: new() + { + Path = "src", + Projects = [ + new() + { + FilePath = "project.csproj", + TargetFrameworks = ["net8.0"], + ReferencedProjectPaths = [], + ExpectedDependencyCount = 3, + Dependencies = [ + new("Microsoft.NET.Sdk", null, DependencyType.MSBuildSdk), + new("Some.Package", "9.0.1", DependencyType.PackageReference, TargetFrameworks: ["net8.0"], IsDirect: true), + new("Some.Package2", "9.0.1", DependencyType.PackageReference, TargetFrameworks: ["net8.0"], IsDirect: true), + ], + Properties = [ + new("SomePackageVersion", "9.0.1", "src/project.csproj"), + new("TargetFramework", "net8.0", "src/project.csproj"), + ] + } + ] + } + ); + } + + [Fact] + public async Task TestDependenciesSeparatedBySemicolonWithWhitespace() + { + await TestDiscoveryAsync( + packages: + [ + MockNuGetPackage.CreateSimplePackage("Some.Package", "9.0.1", "net8.0"), + MockNuGetPackage.CreateSimplePackage("Some.Package2", "9.0.1", "net8.0"), + ], + workspacePath: "src", + files: new[] + { + ("src/project.csproj", """ + + + net8.0 + 9.0.1 + + + + + + + """) + }, + expectedResult: new() + { + Path = "src", + Projects = [ + new() + { + FilePath = "project.csproj", + TargetFrameworks = ["net8.0"], + ReferencedProjectPaths = [], + ExpectedDependencyCount = 3, + Dependencies = [ + new("Microsoft.NET.Sdk", null, DependencyType.MSBuildSdk), + new("Some.Package", "9.0.1", DependencyType.PackageReference, TargetFrameworks: ["net8.0"], IsDirect: true), + new("Some.Package2", "9.0.1", DependencyType.PackageReference, TargetFrameworks: ["net8.0"], IsDirect: true), + ], + Properties = [ + new("SomePackageVersion", "9.0.1", "src/project.csproj"), + new("TargetFramework", "net8.0", "src/project.csproj"), + ] + } + ] + } + ); + } + [Fact] public async Task TestPackageConfig() { diff --git a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Run/RunWorkerTests.cs b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Run/RunWorkerTests.cs index 2e33f9e82e..83b221f64d 100644 --- a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Run/RunWorkerTests.cs +++ b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Run/RunWorkerTests.cs @@ -213,6 +213,256 @@ await File.WriteAllTextAsync(projectPath, """ ); } + [Fact] + public async Task UpdateHandlesSemicolonsInPackageReference() + { + var repoMetadata = XElement.Parse(""""""); + var repoMetadata2 = XElement.Parse(""""""); + await RunAsync( + packages: + [ + MockNuGetPackage.CreateSimplePackage("Some.Package", "1.0.0", "net8.0", additionalMetadata: [repoMetadata]), + MockNuGetPackage.CreateSimplePackage("Some.Package", "1.0.1", "net8.0", additionalMetadata: [repoMetadata]), + MockNuGetPackage.CreateSimplePackage("Some.Package2", "1.0.0", "net8.0", additionalMetadata: [repoMetadata2]), + MockNuGetPackage.CreateSimplePackage("Some.Package2", "1.0.1", "net8.0", additionalMetadata: [repoMetadata2]), + ], + job: new Job() + { + PackageManager = "nuget", + Source = new() + { + Provider = "github", + Repo = "test/repo", + Directory = "some-dir", + }, + AllowedUpdates = + [ + new() { UpdateType = "all" } + ] + }, + files: + [ + ("some-dir/project.csproj", """ + + + net8.0 + + + + + + """) + ], + discoveryWorker: new TestDiscoveryWorker(_input => + { + return Task.FromResult(new WorkspaceDiscoveryResult() + { + Path = "some-dir", + Projects = + [ + new() + { + FilePath = "project.csproj", + TargetFrameworks = ["net8.0"], + Dependencies = + [ + new("Some.Package", "1.0.0", DependencyType.PackageReference, TargetFrameworks: ["net8.0"]), + new("Some.Package2", "1.0.0", DependencyType.PackageReference, TargetFrameworks: ["net8.0"]), + ] + } + ] + }); + }), + analyzeWorker: new TestAnalyzeWorker(input => + { + return Task.FromResult(new AnalysisResult() + { + UpdatedVersion = "1.0.1", + CanUpdate = true, + UpdatedDependencies = + [ + new("Some.Package", "1.0.1", DependencyType.Unknown, TargetFrameworks: ["net8.0"], InfoUrl: "https://nuget.example.com/some-package"), + new("Some.Package2", "1.0.1", DependencyType.Unknown, TargetFrameworks: ["net8.0"], InfoUrl: "https://nuget.example.com/some-package2"), + ] + }); + }), + updaterWorker: new TestUpdaterWorker(async input => + { + Assert.Contains(input.Item3, ["Some.Package", "Some.Package2"]); + Assert.Equal("1.0.0", input.Item4); + Assert.Equal("1.0.1", input.Item5); + var projectPath = input.Item1 + input.Item2; + await File.WriteAllTextAsync(projectPath, """ + + + net8.0 + + + + + + """); + return new UpdateOperationResult(); + }), + expectedResult: new RunResult() + { + Base64DependencyFiles = + [ + new DependencyFile() + { + Directory = "/some-dir", + Name = "project.csproj", + Content = Convert.ToBase64String(Encoding.UTF8.GetBytes(""" + + + net8.0 + + + + + + """)) + } + ], + BaseCommitSha = "TEST-COMMIT-SHA", + }, + expectedApiMessages: + [ + new UpdatedDependencyList() + { + Dependencies = + [ + new ReportedDependency() + { + Name = "Some.Package", + Version = "1.0.0", + Requirements = + [ + new ReportedRequirement() + { + Requirement = "1.0.0", + File = "/some-dir/project.csproj", + Groups = ["dependencies"], + } + ] + }, + new ReportedDependency() + { + Name = "Some.Package2", + Version = "1.0.0", + Requirements = + [ + new ReportedRequirement() + { + Requirement = "1.0.0", + File = "/some-dir/project.csproj", + Groups = ["dependencies"], + } + ] + }, + ], + DependencyFiles = ["/some-dir/project.csproj"], + }, + new IncrementMetric() + { + Metric = "updater.started", + Tags = new() + { + ["operation"] = "group_update_all_versions" + } + }, + new CreatePullRequest() + { + Dependencies = + [ + new ReportedDependency() + { + Name = "Some.Package", + Version = "1.0.1", + Requirements = + [ + new ReportedRequirement() + { + Requirement = "1.0.1", + File = "/some-dir/project.csproj", + Groups = ["dependencies"], + Source = new() + { + SourceUrl = "https://nuget.example.com/some-package", + Type = "nuget_repo", + } + } + ], + PreviousVersion = "1.0.0", + PreviousRequirements = + [ + new ReportedRequirement() + { + Requirement = "1.0.0", + File = "/some-dir/project.csproj", + Groups = ["dependencies"], + } + ], + }, + new ReportedDependency() + { + Name = "Some.Package2", + Version = "1.0.1", + Requirements = + [ + new ReportedRequirement() + { + Requirement = "1.0.1", + File = "/some-dir/project.csproj", + Groups = ["dependencies"], + Source = new() + { + SourceUrl = "https://nuget.example.com/some-package2", + Type = "nuget_repo", + } + } + ], + PreviousVersion = "1.0.0", + PreviousRequirements = + [ + new ReportedRequirement() + { + Requirement = "1.0.0", + File = "/some-dir/project.csproj", + Groups = ["dependencies"], + } + ], + }, + ], + UpdatedDependencyFiles = + [ + new DependencyFile() + { + Name = "project.csproj", + Directory = "/some-dir", + Content = """ + + + net8.0 + + + + + + """, + } + + ], + BaseCommitSha = "TEST-COMMIT-SHA", + CommitMessage = "TODO: message", + PrTitle = "TODO: title", + PrBody = "TODO: body", + }, + new MarkAsProcessed("TEST-COMMIT-SHA") + ] + ); + } + [Fact] public async Task PrivateSourceAuthenticationFailureIsForwaredToApiHandler() { diff --git a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.PackageReference.cs b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.PackageReference.cs index 509f250b7e..d28af3270a 100644 --- a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.PackageReference.cs +++ b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core.Test/Update/UpdateWorkerTests.PackageReference.cs @@ -889,6 +889,45 @@ await TestUpdateForProject("Some.Package", "9.0.1", "13.0.1", ); } + [Fact] + public async Task UpdateVersionAttribute_InProjectFile_ForPackageReferenceUpdateWithSemicolon() + { + // update Some.Package from 9.0.1 to 13.0.1 + await TestUpdateForProject("Some.Package", "9.0.1", "13.0.1", + packages: + [ + MockNuGetPackage.CreateSimplePackage("Some.Package", "9.0.1", "net8.0"), + MockNuGetPackage.CreateSimplePackage("Some.Package2", "9.0.1", "net8.0"), + MockNuGetPackage.CreateSimplePackage("Some.Package", "13.0.1", "net8.0"), + MockNuGetPackage.CreateSimplePackage("Some.Package2", "13.0.1", "net8.0"), + ], + // initial + projectContents: """ + + + net8.0 + + + + + + + """, + // expected + expectedProjectContents: """ + + + net8.0 + + + + + + + """ + ); + } + [Fact] public async Task UpdateVersionAttribute_InDirectoryPackages_ForPackageVersion() { diff --git a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Files/ProjectBuildFile.cs b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Files/ProjectBuildFile.cs index 40db0af72d..b8da7e5787 100644 --- a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Files/ProjectBuildFile.cs +++ b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Files/ProjectBuildFile.cs @@ -43,7 +43,7 @@ public IEnumerable GetDependencies() { var sdkDependencies = GetSdkDependencies(); var packageDependencies = PackageItemNodes - .Select(GetPackageDependency) + .SelectMany(e => GetPackageDependencies(e) ?? Enumerable.Empty()) .OfType(); return sdkDependencies.Concat(packageDependencies); } @@ -89,8 +89,9 @@ private static Dependency GetMSBuildSdkDependency(string name, string? version = : new Dependency(name, version, DependencyType.MSBuildSdk); } - private static Dependency? GetPackageDependency(IXmlElementSyntax element) + private static IEnumerable? GetPackageDependencies(IXmlElementSyntax element) { + List dependencies = []; var isUpdate = false; var name = element.GetAttributeOrSubElementValue("Include", StringComparison.OrdinalIgnoreCase)?.Trim(); @@ -113,12 +114,18 @@ private static Dependency GetMSBuildSdkDependency(string name, string? version = isVersionOverride = version is not null; } - return new Dependency( - Name: name, - Version: version?.Length == 0 ? null : version, - Type: GetDependencyType(element.Name), - IsUpdate: isUpdate, - IsOverride: isVersionOverride); + dependencies.AddRange( + name.Split(';', StringSplitOptions.RemoveEmptyEntries) + .Select(dep => new Dependency( + Name: dep.Trim(), + Version: string.IsNullOrEmpty(version) ? null : version, + Type: GetDependencyType(element.Name), + IsUpdate: isUpdate, + IsOverride: isVersionOverride)) + ); + + + return dependencies; } private static DependencyType GetDependencyType(string name) diff --git a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/PackageReferenceUpdater.cs b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/PackageReferenceUpdater.cs index 0a60464ddd..161ae96cdc 100644 --- a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/PackageReferenceUpdater.cs +++ b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Updater/PackageReferenceUpdater.cs @@ -674,11 +674,21 @@ private static IEnumerable FindPackageNodes( ProjectBuildFile buildFile, string packageName) => buildFile.PackageItemNodes.Where(e => - string.Equals( - (e.GetAttributeOrSubElementValue("Include", StringComparison.OrdinalIgnoreCase) ?? e.GetAttributeOrSubElementValue("Update", StringComparison.OrdinalIgnoreCase))?.Trim(), - packageName, - StringComparison.OrdinalIgnoreCase) && - (e.GetAttributeOrSubElementValue("Version", StringComparison.OrdinalIgnoreCase) ?? e.GetAttributeOrSubElementValue("VersionOverride", StringComparison.OrdinalIgnoreCase)) is not null); + { + // Attempt to get "Include" or "Update" attribute values + var includeOrUpdateValue = e.GetAttributeOrSubElementValue("Include", StringComparison.OrdinalIgnoreCase) + ?? e.GetAttributeOrSubElementValue("Update", StringComparison.OrdinalIgnoreCase); + // Trim and split if there's a valid value + var packageNames = includeOrUpdateValue? + .Trim() + .Split(';', StringSplitOptions.RemoveEmptyEntries) + .Select(t => t.Trim()) + .Where(t => t.Equals(packageName.Trim(), StringComparison.OrdinalIgnoreCase)); + // Check if there's a matching package name and a non-null version attribute + return packageNames?.Any() == true && + (e.GetAttributeOrSubElementValue("Version", StringComparison.OrdinalIgnoreCase) + ?? e.GetAttributeOrSubElementValue("VersionOverride", StringComparison.OrdinalIgnoreCase)) is not null; + }); private static async Task AreDependenciesCoherentAsync(string repoRootPath, string projectPath, string dependencyName, ILogger logger, ImmutableArray buildFiles, string[] tfms) { diff --git a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/MSBuildHelper.cs b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/MSBuildHelper.cs index 744f1d36e7..e3af4ae9c4 100644 --- a/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/MSBuildHelper.cs +++ b/nuget/helpers/lib/NuGetUpdater/NuGetUpdater.Core/Utilities/MSBuildHelper.cs @@ -261,7 +261,11 @@ public static IEnumerable GetTopLevelPackageDependencyInfos(Immutabl ? evaluationResult.EvaluatedValue.TrimStart('[', '(').TrimEnd(']', ')') : evaluationResult.EvaluatedValue; - yield return new Dependency(name, packageVersion, dependencyType, EvaluationResult: evaluationResult, IsUpdate: isUpdate); + // If at this point we have a semicolon in the name then split it and yield multiple dependencies. + foreach (var splitName in name.Split(';', StringSplitOptions.RemoveEmptyEntries)) + { + yield return new Dependency(splitName.Trim(), packageVersion, dependencyType, EvaluationResult: evaluationResult, IsUpdate: isUpdate); + } } }