[Pack] Handle empty Version tag in nuspec error #6092

martinrrm · 2024-10-09T19:41:27Z

Bug

Context of the bug

When running nuget pack with a .nuspec file that contains an empty version and passing -Version on the command line, nuget will fail with the error:

An error occured while trying to parse the value '' of property 'version' in the manifest file.
Value cannot be null or an empty string.
Parameter name: value

However, you put in a bogus version identifier in the nuspec file, the same command will run happily, with the version number passed on the command line.

Description

After having conversations, right now the solution is to override the nuspec version with the CLI version that the customer specified.

~~This PR makes that when the Version tag is empty it creates an 0.0.0 version which will be used to pack the package.~~

Currently how it works is that we read the .nuspec and validate that Version is not null, this validation is used by the nuget spec and nuget pack commands. Removing an error is a breaking change, so I'm not sure if this is the best solution but for me the scenario for an empty Version tag is not common and setting the 0.0.0 version sounds less disruptive. Also, I tried to lag a warning but looks like the logger is not close to the validation close.

Since we run this validation when reading the file, I cannot check if we are using the -Version property easily, the other option for to fix this is to move that validation to another place where we can check it against pack args.

PR Checklist

Meaningful title, helpful description and a linked NuGet/Home issue
Added tests Waiting for team feedback before adding tests
Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

nkolev92

A few questions.

nkolev92 · 2024-10-09T20:12:02Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestReader.cs

@@ -86,7 +86,9 @@ private static void ReadMetadataValue(ManifestMetadata manifestMetadata, XElemen
                        manifestMetadata.Id = value;
                        break;
                    case "version":
-                        manifestMetadata.Version = NuGetVersion.Parse(value);
+                        NuGetVersion version;


How does pack behave when someone passes an actual empty or unparseable version on the commandline?

We do a NuGetVersion.TryParse and if that fails we throw:

Error NU5010: Version string specified for package reference ' ' is invalid.
Error NU5010: Version string specified for package reference 'sdf' is invalid.

Empty string:
Missing option value for: '-Version'

Can you link where that happens?

Here: https://github.com/NuGet/NuGet.Client/blob/dev/src/NuGet.Clients/NuGet.CommandLine/Commands/PackCommand.cs#L131-L137

nkolev92 · 2024-10-09T20:23:15Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestReader.cs

-                        manifestMetadata.Version = NuGetVersion.Parse(value);
+                        NuGetVersion version;
+                        NuGetVersion.TryParse(value, out version);
+                        manifestMetadata.Version = version ?? new NuGetVersion(0, 0, 0);


What if we just allowed it be null instead of a default?
Do we ever need to differentiate between that and a 0,0,0?

Would we detect that no version has been set somewhere different?

Another idea would be, what if we didn't set a version element in the nuspec.

This version is used to create the nupkg name, so in this case an empty version in the nuspec will create the package as package.0.0.0.nupkg so most likely there is a way to detect that no version has been set later in the code.

Another idea would be, what if we didn't set a version element in the nuspec.

I'm not sure what would this cause, for me it looks like version is very important

I'm not sure what would this cause, for me it looks like version is very important

I was thinking as more of a test case actually. What would happen if there was no version specified in the nuspec when we attempt to pack and just pass it on the CLI.

That feels like the closest analog to this test case.

Version is a mandatory element, so I'm just worried about the potential fall out of setting a default version.

How are token replacements done? Can we parse the CLI version and validate it before the we read the manifest?

I was thinking as more of a test case actually. What would happen if there was no version specified in the nuspec when we attempt to pack and just pass it on the CLI.

Currently, if you don't have the version tag it will fail saying that the file is incorrect, if it is empty, it will fail because we try to parse that empty string and fail if the parsing returns null.

Can we parse the CLI version and validate it before the we read the manifest?

We do validate the CLI version before reading the manifest, and after reading the manifest we check if we specified a CLI version and replace it with it.

So I think after this conversation I think the best solution is:

Continue validating the CLI version

Don't immediately break when there is an empty Version in the nuspec

When we would do the "replacement", if CLI version is invalid and there is no version in nuspec break. Otherwise continue as how it is right now. (if CLI version is valid it would override the .nuspec version)

I was thinking, what if we validate once?
Can we inject the CLI version into the nuspec before reading the manifest, or is that not possible?

My fear with not validating the manifest is that we're just introducing an unusual behavior that may lead to missing out a mandatory metadata.

Sorry for the late reply, I think I was able to address this concern, I'm validating the CLI version and updating the manifest version if it's a valid version.

donnie-msft

The code looks fine to me, but since I don't know whether this is better, so just commenting for now.

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestReader.cs

donnie-msft

Thanks for addressing my earlier feedback. The implementation looks good to me but some of the tests are unclear.

test/NuGet.Core.Tests/NuGet.Commands.Test/PackCommandRunnerTests.cs

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestReader.cs

test/NuGet.Core.Tests/NuGet.Commands.Test/PackCommandRunnerTests.cs

zivkan · 2024-10-23T02:45:33Z

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/Manifest.cs

@@ -143,6 +145,16 @@ public static Manifest ReadFrom(Stream stream, Func<string, string> propertyProv
            // Deserialize it
            var manifest = ManifestReader.ReadManifest(document);

+            // Update manifest metadata version if version was provided by the CLI command
+            if (propertyProvider is not null && propertyProvider.Target.GetType().Name.Equals("PackArgs"))


I think I'd rather see a public API change, rather than adding a condition based on a delegate's type name, or other uses of reflection.

Before I spend time looking into the code paths too deeply myself, what other approaches have you considered? I was wondering if having an additional Action<Manifest> delegate (or Func<Manifest, Manifest> delegate, so the method signature/public API wouldn't need to change if Manifest eventually becomes immutable) would allow this method to provide extensibility after deserialization and before validation. But looking just a little bit into the code, that would need a bunch of public APIs on PackageBuilder as well, to pass through to here. I still feel like it might be better than checking the delegate's type name though

Did you try not putting this condition, and always overriding the version when propertyProvider has a value for it? What negative side effects are there?

also, the PR description talks about setting the value to 0.0.0, which is no longer happening, so the PR description needs to be updated.

Did you try not putting this condition, and always overriding the version when propertyProvider has a value for it? What negative side effects are there?
Yes, that was my first implementation but realized that some tests were failing. After doing some debugging, I realized that this function is used in multiple scenarios, the one I'm trying to solve is when the customer is using a nuspec.

The tests that were failing is in the scenario when we are using pack and a .csproj and nuspec. That path calls Manifest.ReadFrom using another propertyProvider which represents the project file (.csproj) not the nuspec, so the information wasn't correct.

I looked at other approach's but given that this is very specific scenario I wanted to do minimal changes.

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestReader.cs

test/NuGet.Core.Tests/NuGet.Commands.Test/PackCommandRunnerTests.cs

martinrrm requested a review from a team as a code owner October 9, 2024 19:41

nkolev92 reviewed Oct 9, 2024

View reviewed changes

donnie-msft reviewed Oct 17, 2024

View reviewed changes

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestReader.cs Outdated Show resolved Hide resolved

martinrrm force-pushed the dev-martinrrm-empty-version-nuspec branch 3 times, most recently from 9c783f0 to 67ed60d Compare October 21, 2024 18:26

martinrrm requested review from nkolev92 and donnie-msft October 21, 2024 18:27

donnie-msft requested changes Oct 21, 2024

View reviewed changes

jeffkl requested a review from donnie-msft October 22, 2024 22:40

zivkan reviewed Oct 23, 2024

View reviewed changes

donnie-msft previously approved these changes Oct 24, 2024

View reviewed changes

src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/ManifestReader.cs Outdated Show resolved Hide resolved

test/NuGet.Core.Tests/NuGet.Commands.Test/PackCommandRunnerTests.cs Outdated Show resolved Hide resolved

martinrrm dismissed donnie-msft’s stale review via c2236c1 October 29, 2024 18:01

martinrrm requested review from zivkan and donnie-msft October 29, 2024 18:01

donnie-msft approved these changes Oct 29, 2024

View reviewed changes

nkolev92 approved these changes Nov 1, 2024

View reviewed changes

martinrrm added 6 commits November 4, 2024 11:22

[Pack] Handle empty Version tag error in nuspec

74435c6

Added tests and update version with CLI argument

bd87f7e

TryParse if

7cd10ab

Update manifest version only if we are using PackArgs

617550b

PR comments on tests

b25d136

pr comments

8a02c4a

martinrrm force-pushed the dev-martinrrm-empty-version-nuspec branch from c2236c1 to 8a02c4a Compare November 4, 2024 19:22

martinrrm merged commit 9d5bde3 into dev Nov 4, 2024
23 checks passed

martinrrm deleted the dev-martinrrm-empty-version-nuspec branch November 4, 2024 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Pack] Handle empty Version tag in nuspec error #6092

[Pack] Handle empty Version tag in nuspec error #6092

martinrrm commented Oct 9, 2024 •

edited

Loading

nkolev92 left a comment

nkolev92 Oct 9, 2024

martinrrm Oct 10, 2024

nkolev92 Oct 11, 2024

martinrrm Oct 14, 2024

nkolev92 Oct 9, 2024

martinrrm Oct 10, 2024 •

edited by nkolev92

Loading

nkolev92 Oct 11, 2024

martinrrm Oct 14, 2024

nkolev92 Oct 15, 2024 •

edited

Loading

martinrrm Oct 21, 2024

donnie-msft left a comment

donnie-msft left a comment

zivkan Oct 23, 2024

martinrrm Oct 23, 2024

martinrrm Oct 23, 2024

[Pack] Handle empty Version tag in nuspec error #6092

[Pack] Handle empty Version tag in nuspec error #6092

Conversation

martinrrm commented Oct 9, 2024 • edited Loading

Bug

Context of the bug

Description

PR Checklist

nkolev92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinrrm Oct 10, 2024 • edited by nkolev92 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkolev92 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinrrm commented Oct 9, 2024 •

edited

Loading

martinrrm Oct 10, 2024 •

edited by nkolev92

Loading

nkolev92 Oct 15, 2024 •

edited

Loading