Ensure the namespaces filtering is respected in packages.config package installation/updates in PMUI #4140

erdembayar · 2021-07-07T19:40:12Z

Bug

Regression? Last working version: N/A

Description

This change ensuring that the namespaces are respected during package installation PMUI(solution and project). If there is same package Id exist in 2 repositories then it only request it from one it specified in namespace filter, previously we try to fetch from all possible repositories and install one from fastest which may not be what user really wanted.

PR Checklist

erdembayar · 2021-07-07T20:25:49Z

test/EndToEnd/tests/PackageNameSpaceTests.ps1

+        CreateCustomTestPackage "Contoso.MVC.ASP" "2.0.0" $opensourceRepo "Thisisfromopensourcerepo2.txt"
+
+        # Act
+        Install-Package "Contoso.MVC.ASP" -ProjectName $p.Name -version 1.0.0


Not sure if it's installing from Solution level PMUI or project level PMUI.
Please let me know if this new tests are not enough.

The install-package command that's used in End to end tests is the PMC one.

To add a PM UI specific test, you'd need to use apex tests instead which know how to call the PM UI instead.

NuGet.Client/test/NuGet.Tests.Apex/NuGet.Tests.Apex/NuGetEndToEndTests/NuGetUITestCase.cs

Line 49 in 1d75910

public void InstallPackageFromUI()

Good news is that your change seems to affect all of packages.config in general, so it shouldn't be too bad :)

nkolev92

Glad you made progress narrowing down a change that would likely work everywhere.

The trick with that is we'd need to ensure that every component actually does work the way we'd expect it to.

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs

src/NuGet.Core/NuGet.PackageManagement/Resolution/GatherContext.cs

src/NuGet.Core/NuGet.PackageManagement/Resolution/ResolverGather.cs

erdembayar · 2021-07-12T19:17:22Z

test/NuGet.Tests.Apex/NuGet.Tests.Apex/NuGetEndToEndTests/NuGetUITestCase.cs

+            CommonUtility.AssertPackageInPackagesConfig(VisualStudio, nuProject, "contoso.a", "1.0.0", XunitLogger);
+        }
+
+        [StaFact]


@nkolev92
Here I was not able to add InstallPackageFromUI_PackageNamespace_WithMultiFeed_Success() test due to here unable to change source option to All with current Apex contract. Any change requires not only change in Apex contract also additional change (other to Apex test) in NuGet PMUI code is kind of out of scope of this PR.
Previously I thought NuGet PMUI searches from all sources for installing new package but it seems it's not true, if individual source is selected then it searches only from that source. It doesn't consider Mypackages for example in this case.

In order to make multifeed to work I need to select All, but currently not available due to previous mentioned limitation.

. Any change requires not only change in Apex contract also additional change (other to Apex test) in NuGet PMUI code is kind of out of scope of this PR.

Not sure I agree here.

We should add a test for this scenario. Apex test contracts are owned by us, it's equivalent to making a change to a type like: https://github.com/NuGet/NuGet.Client/blob/dev/test/TestUtilities/Test.Utility/Commands/TestRestoreRequest.cs

You can argue that this can be added in a new PR, but it's something that I'd say we should consider as part of the 6.0 MVP.

It can be done but changes would be kind of ugly, I have to change private methods/fields to public.
It only benefits Apex test but nothing else, so it would be to hard to justify it.

@nkolev92
Can I create follow up issue for this Apex test case and do it separate PR?

It can be done but changes would be kind of ugly, I have to change private methods/fields to public.
It only benefits Apex test but nothing else, so it would be to hard to justify it.

Code in NuGet.Clients can be refactored as needed.
Making the code testable would be a good investment.

Do you expect this to be a very involved work?

Can I create follow up issue for this Apex test case and do it separate PR?

Yep, I'm comfortable with that. In the meantime, please ensure the scenarios work manually (I know you've done that :) )

Ok. I created follow up issue NuGet/Home#11026
Until this one is completed below 2 new unit tests technically cover it.

NuGet.Client/test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerTests.cs

Lines 7117 to 7121 in a8975bb

[Fact]

public async Task TestPacManPreview_InstallForPC_PackageNamespace_WithMultipleFeedsWithIdenticalPackages_RestoresCorrectPackage()

{

// This test same as having multiple source repositories and `All` option is selected in PMUI.

// Arrange

NuGet.Client/test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerTests.cs

Lines 7219 to 7222 in a8975bb

public async Task TestPacManPreview_InstallForPC_PackageNamespace_WithMultipleFeeds_SecondarySourcesNotConsidered()

{

// This test same as having multiple source repositories but only 1 sourcerepository is selected in PMUI.

// Arrange

nkolev92

This is awesome progress.

I'm pretty impressed with how quickly you've been able to add these.

Mostly some cleanup comments.

I think you have identified, the exact location to make the change in, so that's great.

Are you planning on adding tests for PMC & the ivs APIs in a future PR?
Do the changes in this PR make it respected in PMC as well (I'm guessing yes is the answer here).

It might be helpful to add some update tests as well.
Note that adding those tests might mean adding tests to NuGetPackageManager that you are not necessarily testing.

src/NuGet.Core/NuGet.Configuration/PackageNamespaces/PackageNamespacesConfiguration.cs

test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGet.PackageManagement.Test.csproj

test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerTests.cs

nkolev92 · 2021-07-13T01:31:09Z

test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerTests.cs

@@ -7034,6 +7035,230 @@ public async Task TestPacMan_PreviewInstallPackage_BuildIntegrated_NullVersion_T
            }
        }

+        [Fact]
+        public async Task TestPacManPreview_Install_PackageNamespace_Succeed()


Consider making this name more explicit.

For example, this is testing a single package install with no dependencies.
There seems to be a namespace conflict but not sure that's super relevant.

However, 0, 1 or more sources being selected is relevant.

Renamed all tests like TestPacManPreview_InstallForPC_PackageNamespace_WithSingleFeed_Succeeds, TestPacManPreview_InstallForPC_PackageNamespace_WithMultipleFeedsWithIdenticalPackages_RestoresCorrectPackage etc

test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerTests.cs

test/NuGet.Core.Tests/NuGet.PackageManagement.Test/ResolverGatherTests.cs

test/NuGet.Tests.Apex/NuGet.Tests.Apex/NuGetEndToEndTests/NuGetUITestCase.cs

…date

erdembayar · 2021-07-13T21:36:10Z

@nkolev92
Thank you for review, I addressed latest PR comments. Please review again.

Are you planning on adding tests for PMC & the ivs APIs in a future PR?
Do the changes in this PR make it respected in PMC as well (I'm guessing yes is the answer here).

I'll create separate PR for PMC and IVs Api. Yes, this change is respected in PMC as well so next PR would be mostly End2End and Apex tests. I don't want too long PR.

It might be helpful to add some update tests as well.
Note that adding those tests might mean adding tests to NuGetPackageManager that you are not necessarily testing.

Since my Apex tests are not complete NuGetPackageManager tests technically cover scenarios for while. I guess more tests don't hurt, NuGetPackageManager tests are not heavy as integration tests and Apex tests.
Currently I have 1 update test and I'll create more with NuGet/Home#11026

NuGet.Client/test/NuGet.Tests.Apex/NuGet.Tests.Apex/NuGetEndToEndTests/NuGetUITestCase.cs

Line 318 in a8975bb

    
           public async Task UpdatePackageFromUI_PackageNamespace_WithSingleFeed_Succeeds()

nkolev92

Looks great.

A few more tests and 🚢

src/NuGet.Core/NuGet.PackageManagement/Resolution/ResolverGather.cs

nkolev92 · 2021-07-13T23:54:36Z

test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerTests.cs

+                });
+
+                // Even though Contoso.A 1.0.0 exist in ExternalRepository it wouldn't be picked for install.
+                // PrivateRepository is passed in secondary sources but for packages.config project not considered.


There are scenarios where secondary sources are considered correct/

Can we add tests for that?

@nkolev92
I was not able to find other scenarios where secondary sources are considered. Please note in above test secondary sources is not incorrect, simply not searched(considered) from it.
Could you be able to point what are other possible scenarios where secondary sources are considered?
I already checked update/downgrade action but it's actually just uninstall(no source is considered) + install(above tested case) action.

My recollection is that primary sources are considered for direct dependencies, while both primary and secondary are considered for transitive dependencies.

Ok. I added 2 more tests.

NuGet.Client/test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerTests.cs

Lines 7324 to 7325 in cd77b92

[Fact]

public async Task TestPacManPreview_InstallForPC_PackageNamespace_WithMultipleFeeds_ForTransitiveDepency_SecondarySourcesConsidered()

NuGet.Client/test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerTests.cs

Lines 7451 to 7452 in cd77b92

[Fact]

public async Task TestPacManPreview_UpdateForPC_PackageNamespace_WithMultipleFeeds_SecondarySourcesNotConsidered()

erdembayar · 2021-07-14T19:05:31Z

Looks great.

A few more tests and 🚢

Thank you for your latest review. I added 2 more tests for 2ndary sources are considered for transitive dependencies install and 2ndary sources are not considered update scenario.
Please review again.

nkolev92

LGTM!

Please ensure that there's tests for all the scenarios we have discussed.
Hopefully the improved test names aid that :)

nkolev92 · 2021-07-14T22:29:06Z

test/NuGet.Core.Tests/NuGet.PackageManagement.Test/NuGetPackageManagerTests.cs

+                };
+                ExternalDirectA.AddFile("lib/net461/directA.dll");
+
+                await SimpleTestPackageUtility.CreateFolderFeedV3Async(


you are mkaing a similar call for both external direct and externalcontoso.

Ok. I didn't know we can create multiple nupkgs same time. I combined them into 1 call.

zivkan · 2021-07-14T22:48:41Z

src/NuGet.Core/NuGet.Configuration/Properties/AssemblyInfo.cs

@@ -11,4 +11,4 @@
 [assembly: InternalsVisibleTo("NuGet.VisualStudio.Implementation.Test, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]
 [assembly: InternalsVisibleTo("NuGet.Configuration.Test, PublicKey=0024000004800000940000000602000000240000525341310004000001000100b5fc90e7027f67871e773a8fde8938c81dd402ba65b9201d60593e96c492651e889cc13f1415ebb53fac1131ae0bd333c5ee6021672d9718ea31a8aebd0da0072f25d87dba6fc90ffd598ed4da35e44c398c454307e8e33b8426143daec9f596836f97c8f74750e5975c64e2189f45def46b2a2b1247adc3652bf5c308055da9")]
 [assembly: InternalsVisibleTo("DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7")]
-
+[assembly: InternalsVisibleTo("Test.Utility, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")]


Why is this needed? What internal API from NuGet.Configuration is Test.Utility using?

@zivkan
It's used here.

NuGet.Client/test/TestUtilities/Test.Utility/PackageNamespacesConfigurationUtility.cs

Line 35 in cd77b92

return new PackageNamespacesConfiguration(namespaces);

Here is actual api:

NuGet.Client/src/NuGet.Core/NuGet.Configuration/PackageNamespaces/PackageNamespacesConfiguration.cs

Lines 41 to 46 in 9a72664

internal PackageNamespacesConfiguration(Dictionary<string, IReadOnlyList<string>> namespaces)

{

Namespaces = namespaces ?? throw new ArgumentNullException(nameof(namespaces));

AreNamespacesEnabled = Namespaces.Keys.Count > 0;

SearchTree = new Lazy<SearchTree>(() => GetSearchTree());

}

Wouldn't customers using our NuGet.Configuration package directly benefit from being able to use this API to mock configurations in their own tests, or even have hard-coded namespace configuration in their production applications, without having to read a file off disk?

Why isn't that API public?

I think we should err on the side of underexposing than overexposing APIs tbh.

Given that modes are coming at a later point, it might be better to consider that later.

erdembayar added 4 commits July 6, 2021 20:02

Project PMUI install action honors Namespace filtering.

acecf42

Add missing Id handle.

f56f01a

Add End2End tests.

961d2a5

Add back tests.

3dbf677

erdembayar requested a review from a team as a code owner July 7, 2021 19:40

Revert unwanted change.

bbf9c16

erdembayar marked this pull request as draft July 7, 2021 19:42

erdembayar added 4 commits July 7, 2021 13:14

Consolidate logging.

b0fc09e

Revert unneeded change.

46aaa99

Revert bad revert.

2357802

Clean up

ed0814f

erdembayar force-pushed the dev-eryondon-NameSpacePackageConfig-InstallUpdate branch from 3d63d13 to ed0814f Compare July 7, 2021 20:23

erdembayar marked this pull request as ready for review July 7, 2021 20:24

erdembayar commented Jul 7, 2021

View reviewed changes

nkolev92 reviewed Jul 7, 2021

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs Show resolved Hide resolved

src/NuGet.Core/NuGet.PackageManagement/Resolution/GatherContext.cs Show resolved Hide resolved

src/NuGet.Core/NuGet.PackageManagement/Resolution/ResolverGather.cs Outdated Show resolved Hide resolved

erdembayar added 2 commits July 7, 2021 15:40

Add NuGetPackageManager tests.

55b9b12

Add ResolverGatherTests tests for Package namespaces.

055b747

erdembayar force-pushed the dev-eryondon-NameSpacePackageConfig-InstallUpdate branch from a05bfc3 to 055b747 Compare July 8, 2021 02:56

Add Apex tests.

6bbffe7

erdembayar force-pushed the dev-eryondon-NameSpacePackageConfig-InstallUpdate branch 7 times, most recently from 62f3c02 to a53f409 Compare July 12, 2021 19:05

Clean up

1a74e36

erdembayar force-pushed the dev-eryondon-NameSpacePackageConfig-InstallUpdate branch from a53f409 to 1a74e36 Compare July 12, 2021 19:07

erdembayar commented Jul 12, 2021

View reviewed changes

erdembayar requested a review from nkolev92 July 12, 2021 19:17

erdembayar requested a review from kartheekp-ms July 12, 2021 19:22

erdembayar mentioned this pull request Jul 12, 2021

Ensure the namespaces filtering is respected in packages.config package installation/updates in PMC #4150

Merged

8 tasks

nkolev92 reviewed Jul 13, 2021

View reviewed changes

erdembayar mentioned this pull request Jul 13, 2021

Create Apex tests with multi source repositories for PMUI Package namespace filtering NuGet/Home#11026

Closed

Address PR comments

aaeea61

erdembayar force-pushed the dev-eryondon-NameSpacePackageConfig-InstallUpdate branch from 03a71ed to aaeea61 Compare July 13, 2021 21:02

erdembayar added 2 commits July 13, 2021 16:03

Merge branch 'dev' into dev-eryondon-NameSpacePackageConfig-InstallUp…

a8975bb

…date

Cleanup

87c9d78

Clean up

fbc8f35

nkolev92 reviewed Jul 13, 2021

View reviewed changes

erdembayar added 2 commits July 13, 2021 17:51

Rename variable

d56c016

Add test for 2ndary sources are considered for transitive dependencies.

cd77b92

nkolev92 approved these changes Jul 14, 2021

View reviewed changes

zivkan reviewed Jul 14, 2021

View reviewed changes

Combine create nupkg command call.

df99b25

erdembayar merged commit df6f261 into dev Jul 15, 2021

erdembayar deleted the dev-eryondon-NameSpacePackageConfig-InstallUpdate branch July 15, 2021 23:34

erdembayar mentioned this pull request Jul 16, 2021

Ensure the namespaces filtering is respected in packages.config package installation/updates in IVs installer API #4154

Merged

8 tasks

This was referenced Dec 7, 2022

Warn when vulnerabilities are detected during packages.config restore NuGet/Home#12307

Closed

Warn when vulnerabilities are detected during packages.config package installation NuGet/Home#12308

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure the namespaces filtering is respected in packages.config package installation/updates in PMUI #4140

Ensure the namespaces filtering is respected in packages.config package installation/updates in PMUI #4140

erdembayar commented Jul 7, 2021 •

edited

Loading

erdembayar Jul 7, 2021

nkolev92 Jul 7, 2021

nkolev92 left a comment

erdembayar Jul 12, 2021

nkolev92 Jul 13, 2021

erdembayar Jul 13, 2021

erdembayar Jul 13, 2021

nkolev92 Jul 13, 2021

erdembayar Jul 13, 2021

nkolev92 left a comment

nkolev92 Jul 13, 2021

erdembayar Jul 13, 2021

erdembayar commented Jul 13, 2021 •

edited

Loading

nkolev92 left a comment

nkolev92 Jul 13, 2021

erdembayar Jul 14, 2021

nkolev92 Jul 14, 2021

erdembayar Jul 14, 2021

erdembayar commented Jul 14, 2021

nkolev92 left a comment

nkolev92 Jul 14, 2021

erdembayar Jul 14, 2021

zivkan Jul 14, 2021

erdembayar Jul 14, 2021

zivkan Jul 14, 2021

nkolev92 Jul 14, 2021

	[Fact]
	public async Task TestPacManPreview_InstallForPC_PackageNamespace_WithMultipleFeedsWithIdenticalPackages_RestoresCorrectPackage()
	{
	// This test same as having multiple source repositories and `All` option is selected in PMUI.
	// Arrange

	[Fact]
	public async Task TestPacManPreview_UpdateForPC_PackageNamespace_WithMultipleFeeds_SecondarySourcesNotConsidered()

	internal PackageNamespacesConfiguration(Dictionary<string, IReadOnlyList<string>> namespaces)
	{
	Namespaces = namespaces ?? throw new ArgumentNullException(nameof(namespaces));
	AreNamespacesEnabled = Namespaces.Keys.Count > 0;
	SearchTree = new Lazy<SearchTree>(() => GetSearchTree());
	}

Ensure the namespaces filtering is respected in packages.config package installation/updates in PMUI #4140

Ensure the namespaces filtering is respected in packages.config package installation/updates in PMUI #4140

Conversation

erdembayar commented Jul 7, 2021 • edited Loading

Bug

Description

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

erdembayar commented Jul 13, 2021 • edited Loading

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

erdembayar commented Jul 14, 2021

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

Choose a reason for hiding this comment

erdembayar commented Jul 7, 2021 •

edited

Loading

erdembayar commented Jul 13, 2021 •

edited

Loading