Add support for package type queries in both local and remote sources

[glopesdev]

Bug

Fixes: NuGet/Home#8915

Description

nuget.org allows filtering by package type using the packageType query parameter since SearchQueryService/3.5.0. This parameter has not been officially surfaced by the client API, although relevant infrastructure has already been introduced in the SearchFilter class:

NuGet.Client/src/NuGet.Core/NuGet.Protocol/SearchFilter.cs

Line 60 in c8d14f3

public IEnumerable<string> PackageTypes { get; set; } = Enumerable.Empty<string>();

Upon inspection of relevant source code, it looks like the relevant bits had already been introduced in PackageSearchResourceV3 but using the old query parameter name packageTypeFilter. The fix is simply replacing the below query parameter with packageType:

NuGet.Client/src/NuGet.Core/NuGet.Protocol/Resources/PackageSearchResourceV3.cs

Lines 140 to 147 in c8d14f3

	if (filters.PackageTypes != null
	&& filters.PackageTypes.Any())
	{
	var types = string.Join("&",
	filters.PackageTypes.Select(
	s => "packageTypeFilter=" + s));
	queryString += "&" + types;
	}

For completeness, we also add package type filtering functionality to LocalPackageSearchResource by filtering over the package types available through package.Nuspec.GetPackageTypes().

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.

[zivkan]

@joelverhagen are multiple package types defined as &packageType=type1&packageType=type2 or &packageType=type1+type2?

Additionally, are the semantics that the package must contain all package types, or just one? This is important, to make sure that local folder feed sources behave in the same way.

the docs are not sufficiently specific about multiple values.

[joelverhagen]

Only a single package type filter is supported. The package type parameter is plumbed through in our search service as a single string:
https://github.com/NuGet/NuGetGallery/blob/dc7abf2bd145d91596b032c19e1c9abe8b276956/src/NuGet.Services.AzureSearch/SearchService/SearchParametersBuilder.cs#L169-L172

PRs on the docs are welcome if they are unclear!

[joelverhagen]

Regarding multiple package types on the package, the package type parameter matches the package if ANY of the package types match, per:

The packageType parameter is used to further filter the search results to only packages that have at least one package type matching the package type name.

Emphasis mine.

[glopesdev]

@joelverhagen @zivkan Thank you both for the feedback. In that case, it seems that the API surface of SearchFilter might be slightly inconsistent with current SearchQueryService/3.5.0 capabilities, since SearchFilter allows for multiple package types to be specified.

How do you prefer to address this? Shall PackageSearchResourceV3 and LocalPackageSearchResource throw if more than one package type is provided? Or shall we deprecate the entire PackageTypes property and introduce a new PackageType property with a single value?

Personally I would prefer not touching the current surface API, since this feature is very much needed and breaking the surface API might delay adoption and confuse existing API consumers (having two properties vary their names by a single letter is usually very much discouraged).

@joelverhagen Is there any expectation that multiple package type filters will ever be supported? That might inform whether the deprecation route might be preferred.

[joelverhagen]

There is no expectation that NuGet.org should support multiple package type filters. If this is needed, we would need to design, implement, and document a protocol enhancement. I am not aware of any other package source that implements multiple package type filters.

IMHO, PackageTypes should be deprecated and PackageType should be introduced, to match the current state of the protocol. But I do not work on the client side much so I may be missing something.

[joelverhagen]

I don't know of any plans to add multiple package type support. It's not even clear to what existing scenario could benefit from such a filter.

[zivkan]

Personally I would prefer not touching the current surface API

To be fair, this PR is breaking server APIs, even if it's not breaking the assembly's API. If there are any servers (that are not nuget.org) that use the current packageTypeFilter, this PR will break clients using those servers. There's evidence that (in 2018) neither nuget.org nor myget implemented packageTypeFilter, but there's no way to know if some other server uses it. On the other hand, the V2 protocol isn't documented, and packageTypeFilter was never part of the official V3 protocol.

I'm trying to rush reviewing a bunch of old PRs that I haven't reviewed in a long time, so I haven't yet formed an opinion about breaking APIs (about changing NuGet.Protocol to remove the plural PackageTypes and have a singular PackageType). There's a case to be made that NuGet.Client should implement the official protocol spec.

[zivkan]

As per the PR checklist, please add tests. There's already a test class named LocalpackageSearchResourceTests

[glopesdev]

@zivkan I would love to, but I am blocked setting up a working full environment due to package restore issues:

Failed to download package 'Microsoft.VisualStudio.Sdk.17.10.40171' from 'https://pkgs.dev.azure.com/azure-public/3ccf6661-f8ce-4e8a-bb2e-eff943ddd3c7/_packaging/36a629e1-6c5b-4bcd-aa2e-6018802d6b99@2f2420a2-f975-407f-ad27-d1e0f85b9b15/nuget/v3/flat2/microsoft.visualstudio.sdk/17.10.40171/microsoft.visualstudio.sdk.17.10.40171.nupkg'.
  An error occurred while sending the request.
    The underlying connection was closed: An unexpected error occurred on a send.
    Unable to read data from the transport connection: An existing connection was forcibly closed by the remote host.
    An existing connection was forcibly closed by the remote host

I can restore nuget packages from any other project successfully on my machine and have confirmed all TLS mitigation strategies are in place as per https://github.com/microsoft/azure-devops-tls12 and mentioned in NuGet/NuGetGallery#9121.

I will continue investigating, but if I am missing anything obvious, please advise.

[glopesdev]

This is extremely weird, but solved this problem by forcing IPv4 using https://community.fabric.microsoft.com/t5/Power-Query/The-underlying-connection-was-closed-An-unexpected-error/m-p/3442332#M112277

Working on tests now, but just leaving this here as it might indicate some kind of possible TLS issue with IPv6 on some machines / connections that was hard to find / diagnose.

[glopesdev]

@zivkan One last API design question which just occurred to me: what should the behavior of LocalPackageSearchResource be if the package type Dependency is specified and the local package does not specify package types, e.g. for older packages without the packageTypes metadata?

In the docs it is stated that "Packages not marked with a type, including all packages created with earlier versions of NuGet, default to the Dependency type."

This would suggest that we should accept matches for all packages without packageTypes metadata if the specified package type filter is Dependency.

Alternatively we could change the implementation of GetPackageTypes() to return Dependency instead of an empty list, if no packageTypes node is found in the metadata, but that might break some client code.

[zivkan]

sorry for the delay in replying. Too much work and not enough time.

This would suggest that we should accept matches for all packages without packageTypes metadata if the specified package type filter is Dependency.

This is my preference. I'd rather GetPackageTypes return what the package's nuspec actually reports, just in case there are scenarios where the difference between Dependency not "not specified" is important elsewhere.

[fforjan]

any update on this ?
@zivkan or @nkolev92 ?

[fforjan]

the reason is that I could not find a way to filter on a package type for a search, i'm not aware of any workaround

[zivkan]

Sorry again for the delay. The lead up to .NET 9 GA, and the days following, were very busy.

Anyway, if someone in the NuGet team was doing this, I would ask them for the following:

1. The protocol docs say that the server needs to publish the SearchQueryService/3.5.0 resource in the service index for package type filtering to be available, so I think PackageSearchResourveV3 should throw a NotSupportedException when a package type is supplied, and the server does not have the SearchQueryService/3.5.0 resource.
2. Clients need to know when a server supports package type filtering, so add a boolean property so that clients can make decisions and avoid unexpected exceptions.
3. As stated here, I think we should break the existing API because the server protocol doesn't support multiple package type filters. I don't see the point in maintaining an API which never worked, and therefore is very unlikely that anyone actually uses. I used to be much more conservative in not breaking public APIs, but NuGet.Client has accumulated too much tech debt, and public APIs are one contributing factor. Alternatively throw an exception when more than one package type is provided, but this doesn't make me feel good. I don't see multi-package type filtering being added in the future.

I don't consider myself "good" at open source. After all, look at how much time passes between my comments here. I also don't know if it's reasonable to expect the same quality of contribution from external people as team members who work in the code every day. What I am confident about is that when something doesn't work as expected, NuGet will get the support requests, not the original contributor, which is why I typically don't approve PRs from external contributions unless they're close to the quality that I'd expect of a team member. And I'm working on higher priority changes, so I don't have the time to take over this PR and implement the 3 things I listed above.

[glopesdev]

I think we should break the existing API because the server protocol doesn't support multiple package type filters.

@zivkan I was trying to do this and started getting the below analyzer error when declaring a single string property:

public string PackageType { get; set; }

RS0016 All public types and members should be declared in PublicAPI.txt. This draws attention to API changes in the code reviews and source control history, and helps prevent breaking changes.

Upon investigation there are actually three different PublicAPI.Shipped.txt files (net472, netcoreapp5.0, and netstandard2.0) where I found the currrent API declared with the syntax:

~NuGet.Protocol.Core.Types.SearchFilter.PackageTypes.get -> System.Collections.Generic.IEnumerable<string>
~NuGet.Protocol.Core.Types.SearchFilter.PackageTypes.set -> void

Trying to follow the pattern for other properties I changed it to:

~NuGet.Protocol.Core.Types.SearchFilter.PackageType.get -> string
~NuGet.Protocol.Core.Types.SearchFilter.PackageType.set -> void

As expected this now raises the same RS0016 error for the existing PackageTypes property, but still does not let me specify the new PackageType property.

Is there anything obvious I might be missing? Also, should I be adding the new API to the Unshipped.txt files instead? This made me wonder whether we want to mark the old property PackageTypes as obsolete instead of simply removing it. Is there a convention or guidelines somewhere on the recommended approach to introducing such breaking changes?

Apologies in advance for not being very clear on the use and syntax of the PublicAPI.txt files.

[zivkan]

I also don't know the syntax for the public API files. When using an IDE, the analyzer will have a codefix (available from the "lightbulb" button), which will automatically add a syntactically correct line into the Unshipped file.

Unfortunately the analyzer (or roslyn itself) only works on one target framework at a time, so you need to repeat codefix for each target framework, as we documented here: https://github.com/NuGet/NuGet.Client/blob/dev/docs/nuget-sdk.md#development