Add strict version check for dependencies in NuspecReader #1262
Conversation
|
@shishirx34, |
| /// </summary> | ||
| public IEnumerable<PackageDependencyGroup> GetDependencyGroups(bool useStrictVersionCheck) | ||
| { | ||
| return GetAllDependencyGroups(useStrictVersionCheck); |
There was a problem hiding this comment.
Any reason the implementation is in a private method instead of this one? It seems like it would be simpler to add it here.
There was a problem hiding this comment.
This new private method is now being called from two places:
There was a problem hiding this comment.
If we want to keep the implementation within the public method, we should consider merging the two public methods. To avoid breaking change in the public API of this class, the bool useStrictVersionCheck argument should be made optional: bool useStrictVersionCheck = false. Is that preferable @emgarten?
There was a problem hiding this comment.
We can keep the exact same public API service area:
NuSpecReader.GetDependencyGroups()NuSpecReader.GetDependencyGroups(bool useStrictVersionCheck)
Number 1 will just call number two with useStrictVersionCheck = false right?
Optional argument is actually less preferable since this is actually a breaking change at runtime (but not at build time).
There was a problem hiding this comment.
Yep, agreed. Will change it accordingly.
There was a problem hiding this comment.
Should be fixed with 0a35c93
There was a problem hiding this comment.
Yes @joelverhagen's comment. It can just be two methods instead of 3. And on the client we avoid optional params for public things.
| @@ -129,6 +129,9 @@ | |||
| <data name="ErrorInvalidPackageVersion" xml:space="preserve"> | |||
| <value>Invalid package version for package id '{0}': '{1}'</value> | |||
| </data> | |||
| <data name="ErrorInvalidPackageVersionForDependency" xml:space="preserve"> | |||
| <value>Invalid package version for a dependency with id '{0}' in package '{1}': '{2}'</value> | |||
There was a problem hiding this comment.
please document what each placeholder corresponds to (<comment>)
There was a problem hiding this comment.
Addressed with 5113343#diff-f49b90659a73b9b227fd920ed6ba82ff
| yield return "1238234.198231.2924324.2343432+final"; | ||
| yield return "00.00.00.00-alpha"; | ||
| yield return "0.0-alpha.1"; | ||
| yield return "9.9.9-9"; |
There was a problem hiding this comment.
Some examples with [ and ) and floating would be good. This looks like a list of just versions (not version ranges).
Also, you don't need this many test cases. You can just cover the major different sorts of formats and leave the big test list to NuGet.Versioning unit tests.
| try | ||
| { | ||
| var dependencies = reader.GetDependencyGroups().ToList(); | ||
| Assert.Equal(1, dependencies.Count); |
There was a problem hiding this comment.
Also, invalid should map to (, ) right?
There was a problem hiding this comment.
If so, we should assert that
There was a problem hiding this comment.
It should be (, ) when it can't parse which represents all
There was a problem hiding this comment.
Addressed with 5113343#diff-b6f1877463e746b0dfc956a837f5ad5aR409
There was a problem hiding this comment.
Taking over this PR from @shishirx34 who's on vacation. May reopen a new one and rename branch to dev-* to benefit from CI private builds :)
Will address the test comments and PR feedback. Have one question in my comments for @emgarten.
| /// </summary> | ||
| public IEnumerable<PackageDependencyGroup> GetDependencyGroups(bool useStrictVersionCheck) | ||
| { | ||
| return GetAllDependencyGroups(useStrictVersionCheck); |
There was a problem hiding this comment.
This new private method is now being called from two places:
| /// </summary> | ||
| public IEnumerable<PackageDependencyGroup> GetDependencyGroups(bool useStrictVersionCheck) | ||
| { | ||
| return GetAllDependencyGroups(useStrictVersionCheck); |
There was a problem hiding this comment.
If we want to keep the implementation within the public method, we should consider merging the two public methods. To avoid breaking change in the public API of this class, the bool useStrictVersionCheck argument should be made optional: bool useStrictVersionCheck = false. Is that preferable @emgarten?
|
@emgarten @joelverhagen pushed a commit that should address review feedback (except for the open question), and also fixed tests. Pushed another branch |
once we iron out
This is part 1 of the work for fixing issue:
NuGet/NuGetGallery#3482 - Gallery accepts packages with illegal package dependency version ranges.
Gallery uses
NuGet.PackagingNuspec reader, which currently does not do any strict version check for invalid versions in dependencies, for processing packages. This PR adds that ability and throws an exception based of which the gallery will reject bad packages.I have refactored code a bit, for reusability. Added unit tests for this feature. All unit tests seem to be passing for me. Let me know if I am missing something here.
Feel free to add more reviewers to this PR.