Added additional check for version while resolving through packages lock file #2583
Conversation
jainaashish
requested review from
dtivel,
nkolev92,
dominoFire,
PatoBeltran and
zivkan
| @@ -31,8 +31,7 @@ public class TestSolutionManager : ISolutionManager, IDisposable | |||
|
|
|||
| public string SolutionDirectory { get; } | |||
|
|
|||
|
|
|||
| private TestDirectory _testDirectory; | |||
| public TestDirectory TestDirectory; | |||
There was a problem hiding this comment.
public TestDirectory TestDirectory { get; private set; }
| @@ -211,17 +211,21 @@ public static GraphItem<RemoteResolveResult> CreateUnresolvedMatch(LibraryRange | |||
| // check for the exact library through local repositories | |||
| var localMatch = await FindLibraryByVersionAsync(library, framework, localProviders, cacheContext, logger, cancellationToken); | |||
|
|
|||
| if (localMatch != null) | |||
| if (localMatch != null | |||
| && localMatch.Library.Version.Equals(library.Version)) | |||
There was a problem hiding this comment.
Here and here RemoteMatch.Library and LibraryIdentity.Version are both nullable. They're public properties with a public getter and setter. This code assumes that both properties --- Library and Version --- are not null. While this assumption may be currently valid by convention, I think this change should be more defensive when dereferencing these properties.
There was a problem hiding this comment.
I understand the concern, but both instances localMatch as well as remoteMatch are constructed with libraryIdentity with version in it if they are constructed. We can't have a match without the library in it. And there are already bunch of checks down below (like this)
I don't mind being more defensive here but feels it will not make any difference.
There was a problem hiding this comment.
discussed offline with Damon, and he's good with the existing code.
jainaashish
force-pushed
the
dev-asja-fix7667
branch
from
709eb97 to
721981b
Compare
Bug
Fixes: NuGet/Home#7667
Regression: No
If Regression then when did it last work:
If Regression then how are we preventing it in future:
Fix
Details: While resolving a version from packages lock file, this PR adds an additional check to make sure that it has resolved the exact same version otherwise fails restore.
Testing/Validation
Tests Added: Yes
Reason for not adding tests:
Validation done: