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

Do not install unlisted package in NuGet.exe #3795

Merged
merged 3 commits into from Dec 14, 2020
Merged

Conversation

heng-liu
Copy link
Contributor

@heng-liu heng-liu commented Dec 11, 2020

Bug

Fixes: NuGet/Home#7466
Regression: No

  • Last working version:
  • How are we preventing it in future:

Fix

Details:

  1. Change includeUnlisted from true to false in InstallPackageAsync.
    2. Change FileSystemBackedV3MockServer.cs to handle path.StartsWith("/packages/").
  2. Change the package content format from {server.Uri}packages/{id}.{version}.nupkg to {server.Uri}flat/{id}/{version}/{id}.{version}.nupkg
  3. Change SimpleTestSettingsContext.cs to add a method removing a package source.
  4. Add 2 tests.

Testing/Validation

Tests Added: Yes
Reason for not adding tests:
Validation:

@heng-liu heng-liu requested a review from a team as a code owner December 11, 2020 08:04
@@ -128,6 +128,30 @@ private Action<HttpListenerResponse> ServerHandlerV3(HttpListenerRequest request
});
}
}
else if (path.StartsWith("/packages/"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this /packages path where newly created packages go by default or why it's so specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following are what I learnt, I hope it would help.
FileSystemBackedV3MockServer is a mock Server used for testing. When we create a mock server and add the service index Uri into our testing NuGet.Config file, then running NuGet.exe install, the mock server will respond like a real V3 http package source.
However, the ServerHandlerV3 method in FileSystemBackedV3MockServer is not complete to run my newly added tests.
When NuGet.exe tries the registration URL first, it will BuildRegistrationIndexResponse, and CreatePackageRegistrationBlob, and it will set packageContent blob as "packageContent", $"{server.Uri}packages/{id}.{version}.nupkg"

During NuGet.exe install, package needs to be downloaded, the DownloadResourceV3.cs will finally get the packageContent Uri , which is $"{server.Uri}packages/{id}.{version}.nupkg. So that why I need to add the logic to handle path.StartsWith("/packages/"). Only in this way, the test of install could be completed, or else, the download package could not be done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this server should mimic what a proper v3 server would do.
Can we change

item.Add(new JProperty("packageContent", $"{server.Uri}packages/{id}.{version}.nupkg"));
instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular look what https://api.nuget.org/v3/registration5-semver1/newtonsoft.json/index.json packageContent points to.

Copy link
Contributor Author

@heng-liu heng-liu Dec 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Nikolche!
I see
"packageContent":"https://api.nuget.org/v3-flatcontainer/newtonsoft.json/3.5.8/newtonsoft.json.3.5.8.nupkg"
So I need to change the line to
item.Add(new JProperty("packageContent", $"{server.Uri}v3-flatcontainer/{id}/{version}/{id}.{version}.nupkg")); , right?
Then there will be following things need to be changed as well:

  1. We still need to add logic to handle path.StartsWith("/v3-flatcontainer/") in FileSystemBackedV3MockServer
  2. 100+ lines of packageContent with "/packages/" in JsonData.cs need to be changed to the new format, right?
  3. NetworkCallCountTest.cs also handles the path.StartsWith("/packages/"). We need to change that as well.
  4. Fixed other tests.

May I know if my understanding is correct? Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flat is handled in https://github.com/NuGet/NuGet.Client/blob/dev/test/NuGet.Clients.Tests/NuGet.CommandLine.Test/FileSystemBackedV3MockServer.cs#L58.

You can chang that to v3-flatcontainer if you want, and then change the index.json that it advertises.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Only changed the package content format in util works.
No other change is needed. So my understanding of changing 4 parts is incorrect.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done 👍

Great test names.

@heng-liu heng-liu merged commit e9722d6 into dev Dec 14, 2020
@heng-liu heng-liu deleted the dev-hengliu-CM-unlisted branch December 14, 2020 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants