Factorize PackageDownloadRunner.TryGetRepositoriesForPackage method and add test

[Nigusu-Allehu]

Bug

Fixes: https://github.com/NuGet/NuGet.Client/pull/6885/files#r2487713673
Fixes: #6885 (comment)

Description

This PR Factorizes the PackageDownloadRunner.TryGetRepositoriesForPackage method, to improve readability. In addition, it adds one more test scenario for the package download command: " no --source, mapping -> A&B, package in both A and B. Latest version from A installed". In addition, it also addresses a bug where http source validation was not being done for package sources when source mapping is enabled and the package is not mapped to any of the sources.

PR Checklist

• Meaningful title, helpful description and a linked NuGet/Home issue
• Added tests
• Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

[aortiz-msft]

@Nigusu-Allehu - How does this implementation differ now from the Restore code path? Any chance of refactoring into a common function?

cc @nkolev92

[Nigusu-Allehu]

@Nigusu-Allehu - How does this implementation differ now from the Restore code path? Any chance of refactoring into a common function?

cc @nkolev92

Oh yes, a refactoring would definitely be nice. One question I had while looking at this: since both restore's remote walker and this command already share the core mapping logic through PackageSourceMapping.GetConfiguredPackageSources(...), do you think there’s much common code left to factor out?

After that call, each path just converts source names into its own representation (IRemoteDependencyProvider in the remote walker vs SourceRepository here)

Curious what others think. happy to adjust if you see a clean way to unify those parts.

[nkolev92]

After that call, each path just converts source names into its own representation (IRemoteDependencyProvider in the remote walker vs SourceRepository here)

Yeah, unfortunately any refactoring would probably add complexity, since it'd probably need to use Func or something to choose the comparer and that just adds allocations.

[aortiz-msft]

Any tests from the restore implementation that we can copy over or viceversa?

---

In reply to: 3554996830

[aortiz-msft]

🕐

[Nigusu-Allehu]

Any tests from the restore implementation that we can copy over or viceversa?

In reply to: 3554996830

Missing in PackageDownloadRunnerTests vs FilterDependencyProvidersForLibraryTests:

• Case-insensitive matching of source names (Added)

Missing in FilterDependencyProvidersForLibraryTests vs PackageDownloadRunnerTests :

• Package pattern mapped to multiple sources all applicable providers returned.

• Mapping includes a source name that doesn’t exist in the provider list

sorry, bad click, I meant to comment.

[aortiz-msft]

Missing in FilterDependencyProvidersForLibraryTests vs PackageDownloadRunnerTests :

• Package pattern mapped to multiple sources all applicable providers returned.
• Mapping includes a source name that doesn’t exist in the provider list

I'm not seeing tests added to FilterDependencyProvidersForLibraryTests. Could you please add the appropriate tests there as well as part of this PR?

[aortiz-msft]

See comments

[Nigusu-Allehu]

Missing in FilterDependencyProvidersForLibraryTests vs PackageDownloadRunnerTests :

• Package pattern mapped to multiple sources all applicable providers returned.
• Mapping includes a source name that doesn’t exist in the provider list

I'm not seeing tests added to FilterDependencyProvidersForLibraryTests. Could you please add the appropriate tests there as well as part of this PR?

As discussed, offline, added here instead of a separate follow up pr.

[aortiz-msft]

Approved for now, but refactoring to be done in a separate PR.