Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NuGetVersionRange allows logically incorrect ranges to be parsed #9145

Closed
nkolev92 opened this issue Feb 6, 2020 · 4 comments · Fixed by NuGet/NuGet.Client#3917
Closed
Assignees
Labels
Category:Quality Week Issues that should be considered for quality week Functionality:SDK The NuGet client packages published to nuget.org Priority:2 Issues for the current backlog. Type:Bug

Comments

@nkolev92
Copy link
Member

nkolev92 commented Feb 6, 2020

See: https://nugettoolsdev.azurewebsites.net/5.5.0-preview.2.6382/parse-version-range?versionRange=%5B10.0.0%2C+9.0.0%29

[10.0.0, 9.0.0)

This range is empty and there's no reasonable scenario where it's necessary.

This has always been the case, all the way to 2.5 https://nugettoolsdev.azurewebsites.net/2.5.0/parse-version-range?versionRange=%5B10.0.0%2C+9.0.0%29.

@nkolev92 nkolev92 added NuGet API Type:Bug Category:Quality Week Issues that should be considered for quality week Priority:2 Issues for the current backlog. Triage:NeedsTriageDiscussion and removed Priority:2 Issues for the current backlog. labels Feb 6, 2020
@nkolev92 nkolev92 added Functionality:SDK The NuGet client packages published to nuget.org and removed NuGet API labels Apr 24, 2020
@zivkan zivkan added Priority:2 Issues for the current backlog. Category:Customer Sprint labels Nov 17, 2020
@erdembayar erdembayar self-assigned this Feb 22, 2021
@erdembayar
Copy link
Contributor

@nkolev92
I see only 2 ways to handle this. Which one is more appropriate this case?

  1. Let exception through here.

https://github.com/NuGet/NuGet.Client/blob/35b6dcdb23642c502065458d91e644fb6fb17ecf/src/NuGet.Core/NuGet.Versioning/VersionRangeFactory.cs#L72-L74

  1. Return VersionRange.None.

@nkolev92
Copy link
Member Author

I would throw communicating that it's an invalid range.

@erdembayar
Copy link
Contributor

@nkolev92
How about (9.0.0, 9.0.0)? It's technically it's illogical range but also used as empty range in unit tests.
What to do in this case?

@nkolev92
Copy link
Member Author

nkolev92 commented Feb 23, 2021

I think it's fine to leave that as a valid range.
Let's not overdo it :)

Obviously not too many people were affected by this problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Quality Week Issues that should be considered for quality week Functionality:SDK The NuGet client packages published to nuget.org Priority:2 Issues for the current backlog. Type:Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants