Fix version handling in dotnet add file.cs package (#50924)

Author: jjonescz

src/Cli/dotnet/Commands/Package/Add/PackageAddCommand.cs

@@ -163,10 +163,12 @@ private int ExecuteForFileBasedApp(string path)
             }
         }
 
-        bool hasVersion = _packageId.HasVersion;
+        string? specifiedVersion = _packageId.HasVersion
+            ? _packageId.VersionRange?.OriginalString ?? string.Empty
+            : _parseResult.GetValue(PackageAddCommandParser.VersionOption);
         bool prerelease = _parseResult.GetValue(PackageAddCommandParser.PrereleaseOption);
 
-        if (hasVersion && prerelease)
+        if (specifiedVersion != null && prerelease)
         {
             throw new GracefulException(CliCommandStrings.PrereleaseAndVersionAreNotSupportedAtTheSameTime);
         }
@@ -193,11 +195,7 @@ private int ExecuteForFileBasedApp(string path)
 
         // Set initial version to Directory.Packages.props and/or C# file
         // (we always need to add the package reference to the C# file but when CPM is enabled, it's added without a version).
-        string version = hasVersion
-            ? _packageId.VersionRange?.OriginalString ?? string.Empty
-            : (prerelease
-                ? "*-*"
-                : "*");
+        string version = specifiedVersion ?? (prerelease ? "*-*" : "*");
         bool skipUpdate = false;
         var central = SetCentralVersion(version);
         var local = SetLocalVersion(central != null ? null : version);
@@ -214,7 +212,7 @@ private int ExecuteForFileBasedApp(string path)
             }
 
             // If no version was specified by the user, save the actually restored version.
-            if (!hasVersion && !skipUpdate)
+            if (specifiedVersion == null && !skipUpdate)
             {
                 var projectAssetsFile = projectInstance.GetProperty("ProjectAssetsFile")?.EvaluatedValue;
                 if (!File.Exists(projectAssetsFile))
@@ -306,7 +304,7 @@ void Update(string value)
 
                     // If user didn't specify a version and a version is already specified in Directory.Packages.props,
                     // don't update the Directory.Packages.props (that's how the project-based equivalent behaves as well).
-                    if (!hasVersion)
+                    if (specifiedVersion == null)
                     {
                         skipUpdate = true;
                         return (Revert: NoOp, Update: Unreachable, Save: Revert);

test/dotnet.Tests/CommandTests/Package/Add/GivenDotnetPackageAdd.cs

@@ -290,16 +290,45 @@ public void VersionRange(bool asArgument)
             cmd.StdErr.Should().BeEmpty();
         }
 
-        [Fact]
-        public void FileBasedApp()
+        private string[]? GetFileBasedAppArgs(bool legacyForm, bool? versionOption, bool fileOption, bool noRestore, string packageName = "Humanizer")
         {
+            if (!legacyForm && !fileOption)
+            {
+                Log.WriteLine("Skipping invalid combination of parameters");
+                return null;
+            }
+
+            (string, string) commandArgs = legacyForm
+                ? ("add", "package")
+                : ("package", "add");
+
+            return [
+                commandArgs.Item1,
+                .. (ReadOnlySpan<string>)(fileOption ? [] : ["Program.cs"]),
+                commandArgs.Item2,
+                .. (ReadOnlySpan<string>)(versionOption switch
+                {
+                    true => [packageName, "--version", "2.14.1"],
+                    false => [$"{packageName}@2.14.1"],
+                    null => [packageName],
+                }),
+                .. (ReadOnlySpan<string>)(fileOption ? ["--file", "Program.cs"] : []),
+                .. (ReadOnlySpan<string>)(noRestore ? ["--no-restore"] : []),
+            ];
+        }
+
+        [Theory, CombinatorialData]
+        public void FileBasedApp(bool legacyForm, bool versionOption, bool fileOption, bool noRestore)
+        {
+            if (GetFileBasedAppArgs(legacyForm, versionOption, fileOption, noRestore) is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
             var file = Path.Join(testInstance.Path, "Program.cs");
             File.WriteAllText(file, """
                 Console.WriteLine();
                 """);
 
-            new DotnetCommand(Log, "package", "add", "[email protected]", "--file", "Program.cs")
+            new DotnetCommand(Log, args)
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Pass();
@@ -311,20 +340,21 @@ public void FileBasedApp()
                 """);
         }
 
-        [Theory]
-        [InlineData("Humanizer")]
-        [InlineData("humanizer")]
+        [Theory, CombinatorialData]
         public void FileBasedApp_ReplaceExisting(
-            string sourceFilePackageId)
+            [CombinatorialValues("Humanizer", "humanizer")] string sourceFilePackageId,
+            bool legacyForm, bool versionOption, bool fileOption, bool noRestore)
         {
+            if (GetFileBasedAppArgs(legacyForm, versionOption, fileOption, noRestore) is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
             var file = Path.Join(testInstance.Path, "Program.cs");
             File.WriteAllText(file, $"""
                 #:package {sourceFilePackageId}@2.9.9
                 Console.WriteLine();
                 """);
 
-            new DotnetCommand(Log, "package", "add", "[email protected]", "--file", "Program.cs")
+            new DotnetCommand(Log, args)
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Pass();
@@ -400,16 +430,18 @@ public void FileBasedApp_NoVersion_Prerelease(string[] inputVersions, string? _,
                     """);
         }
 
-        [Fact]
-        public void FileBasedApp_NoVersionAndNoRestore()
+        [Theory, CombinatorialData]
+        public void FileBasedApp_NoVersionAndNoRestore(bool legacyForm, bool fileOption)
         {
+            if (GetFileBasedAppArgs(legacyForm, versionOption: null, fileOption, noRestore: true) is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
             var file = Path.Join(testInstance.Path, "Program.cs");
             File.WriteAllText(file, """
                 Console.WriteLine();
                 """);
 
-            new DotnetCommand(Log, "package", "add", "Humanizer", "--file", "Program.cs", "--no-restore")
+            new DotnetCommand(Log, args)
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Pass();
@@ -421,17 +453,19 @@ public void FileBasedApp_NoVersionAndNoRestore()
                 """);
         }
 
-        [Fact]
-        public void FileBasedApp_VersionAndPrerelease()
+        [Theory, CombinatorialData]
+        public void FileBasedApp_VersionAndPrerelease(bool legacyForm, bool versionOption, bool fileOption, bool noRestore)
         {
+            if (GetFileBasedAppArgs(legacyForm, versionOption, fileOption, noRestore) is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
             var file = Path.Join(testInstance.Path, "Program.cs");
             var source = """
                 Console.WriteLine();
                 """;
             File.WriteAllText(file, source);
 
-            new DotnetCommand(Log, "package", "add", "[email protected]", "--file", "Program.cs", "--prerelease")
+            new DotnetCommand(Log, [.. args, "--prerelease"])
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Fail()
@@ -440,34 +474,38 @@ public void FileBasedApp_VersionAndPrerelease()
             File.ReadAllText(file).Should().Be(source);
         }
 
-        [Fact]
-        public void FileBasedApp_InvalidPackage()
+        [Theory, CombinatorialData]
+        public void FileBasedApp_InvalidPackage(bool legacyForm, bool fileOption)
         {
+            if (GetFileBasedAppArgs(legacyForm, versionOption: null, fileOption, noRestore: false, packageName: "Microsoft.ThisPackageDoesNotExist") is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
             var file = Path.Join(testInstance.Path, "Program.cs");
             var source = """
                 Console.WriteLine();
                 """;
             File.WriteAllText(file, source);
 
-            new DotnetCommand(Log, "package", "add", "Microsoft.ThisPackageDoesNotExist", "--file", "Program.cs")
+            new DotnetCommand(Log, args)
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Fail();
 
             File.ReadAllText(file).Should().Be(source);
         }
 
-        [Fact]
-        public void FileBasedApp_InvalidPackage_NoRestore()
+        [Theory, CombinatorialData]
+        public void FileBasedApp_InvalidPackage_NoRestore(bool legacyForm, bool fileOption)
         {
+            if (GetFileBasedAppArgs(legacyForm, versionOption: null, fileOption, noRestore: true, packageName: "Microsoft.ThisPackageDoesNotExist") is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
             var file = Path.Join(testInstance.Path, "Program.cs");
             File.WriteAllText(file, """
                 Console.WriteLine();
                 """);
 
-            new DotnetCommand(Log, "package", "add", "Microsoft.ThisPackageDoesNotExist", "--file", "Program.cs", "--no-restore")
+            new DotnetCommand(Log, args)
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Pass();
@@ -479,9 +517,11 @@ public void FileBasedApp_InvalidPackage_NoRestore()
                 """);
         }
 
-        [Fact]
-        public void FileBasedApp_CentralPackageManagement()
+        [Theory, CombinatorialData]
+        public void FileBasedApp_CentralPackageManagement(bool legacyForm, bool versionOption, bool fileOption, bool noRestore)
         {
+            if (GetFileBasedAppArgs(legacyForm, versionOption, fileOption, noRestore) is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
             var file = Path.Join(testInstance.Path, "Program.cs");
             var source = """
@@ -498,7 +538,7 @@ public void FileBasedApp_CentralPackageManagement()
                 </Project>
                 """);
 
-            new DotnetCommand(Log, "package", "add", "[email protected]", "--file", "Program.cs")
+            new DotnetCommand(Log, args)
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Pass();
@@ -522,8 +562,10 @@ public void FileBasedApp_CentralPackageManagement()
         }
 
         [Theory, CombinatorialData]
-        public void FileBasedApp_CentralPackageManagement_ReplaceExisting(bool wasInFile)
+        public void FileBasedApp_CentralPackageManagement_ReplaceExisting(bool wasInFile, bool legacyForm, bool versionOption, bool fileOption, bool noRestore)
         {
+            if (GetFileBasedAppArgs(legacyForm, versionOption, fileOption, noRestore) is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
             var file = Path.Join(testInstance.Path, "Program.cs");
             var source = """
@@ -553,7 +595,7 @@ public void FileBasedApp_CentralPackageManagement_ReplaceExisting(bool wasInFile
                 </Project>
                 """);
 
-            new DotnetCommand(Log, "package", "add", "[email protected]", "--file", "Program.cs")
+            new DotnetCommand(Log, args)
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Pass();
@@ -576,9 +618,11 @@ public void FileBasedApp_CentralPackageManagement_ReplaceExisting(bool wasInFile
                 """);
         }
 
-        [Fact]
-        public void FileBasedApp_CentralPackageManagement_NoVersionSpecified()
+        [Theory, CombinatorialData]
+        public void FileBasedApp_CentralPackageManagement_NoVersionSpecified(bool legacyForm, bool fileOption)
         {
+            if (GetFileBasedAppArgs(legacyForm, versionOption: null, fileOption, noRestore: false, packageName: "A") is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
 
             string[] versions = ["0.0.5", "0.9.0", "1.0.0-preview.3"];
@@ -602,7 +646,7 @@ public void FileBasedApp_CentralPackageManagement_NoVersionSpecified()
                 </Project>
                 """);
 
-            new DotnetCommand(Log, "package", "add", "A", "--file", "Program.cs")
+            new DotnetCommand(Log, args)
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Pass();
@@ -624,9 +668,11 @@ public void FileBasedApp_CentralPackageManagement_NoVersionSpecified()
                 """);
         }
 
-        [Fact]
-        public void FileBasedApp_CentralPackageManagement_NoVersionSpecified_KeepExisting()
+        [Theory, CombinatorialData]
+        public void FileBasedApp_CentralPackageManagement_NoVersionSpecified_KeepExisting(bool legacyForm, bool fileOption, bool noRestore)
         {
+            if (GetFileBasedAppArgs(legacyForm, versionOption: null, fileOption, noRestore) is not { } args) return;
+
             var testInstance = _testAssetsManager.CreateTestDirectory();
             var file = Path.Join(testInstance.Path, "Program.cs");
             var source = """
@@ -648,7 +694,7 @@ public void FileBasedApp_CentralPackageManagement_NoVersionSpecified_KeepExistin
                 """;
             File.WriteAllText(directoryPackagesProps, directoryPackagesPropsSource);
 
-            new DotnetCommand(Log, "package", "add", "Humanizer", "--file", "Program.cs")
+            new DotnetCommand(Log, args)
                 .WithWorkingDirectory(testInstance.Path)
                 .Execute()
                 .Should().Pass();