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.

[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

[glopesdev]

@zivkan Thanks for the feedback, no worries about any delay.

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.

This is a good point, and made me go back to double-check whether SearchQueryService/3.5.0 was actually requested as an endpoint. It turns out that it had been left out entirely from the list of versions in ServiceTypes. I have now fixed this in 1e4abdf.

However, I am a bit at a loss at how to implement the check you requested, since the way that search query service endpoints are accessed right now is not via the "resource" abstraction. Instead, the API requests a list of URI endpoints via the GetServiceEntryUris extension method:

NuGet.Client/src/NuGet.Core/NuGet.Protocol/Providers/PackageSearchResourceV3Provider.cs

Lines 25 to 26 in 949031c

	var endpoints = serviceIndex.GetServiceEntryUris(ServiceTypes.SearchQueryService);
	var httpSourceResource = await source.GetResourceAsync<HttpSourceResource>(token);

Because PackageSearchResourceV3 works directly on the list of searchEndpoints, there is no information about versions that can be accessed in the current version of the code.

My feeling is perhaps we should refactor the entire approach to use GetServiceEntries directly, which returns a list of ServiceIndexEntry objects. Inside this class there is a ClientVersion property which I hope corresponds to the version of the SearchQueryService (can you confirm this is true?).

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.

Given the above, I am going to interpret this as exposing a new boolean property in PackageSearchResourceV3, which will be true iff at least one of the endpoints is 3.5.0 compatible. We would then throw the exception only if none of the provided server endpoints supports 3.5.0.

Does this sound acceptable?

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.

Done, see cf99bb6.

[zivkan]

However, I am a bit at a loss at how to implement the check you requested, since the way that search query service endpoints are accessed right now is not via the "resource" abstraction. Instead, the API requests a list of URI endpoints via the GetServiceEntryUris extension method:

NuGet.Client/src/NuGet.Core/NuGet.Protocol/Providers/PackageSearchResourceV3Provider.cs

Lines 25 to 26 in 949031c

	var endpoints = serviceIndex.GetServiceEntryUris(ServiceTypes.SearchQueryService);
	var httpSourceResource = await source.GetResourceAsync<HttpSourceResource>(token);

Since PackageSearchResourceV3Provider gets the service entries, it can check the versions. Then PackageSearchResourceV3 can have a constructor parameter, or init property.

My feeling is perhaps we should refactor the entire approach to use GetServiceEntries directly, which returns a list of ServiceIndexEntry objects. Inside this class there is a ClientVersion property which I hope corresponds to the version of the SearchQueryService (can you confirm this is true?).

Something I don't like about the version strings we use is that it implies that newer client versions support all past features (making it "impossible" to deprecate features), and it implies that if a server wants to support a particular feature, it must also implement all past versions as well, even if they have features they don't want. This makes sense for some resources and features like package metadata (registration) where older versions needed the response to be uncompressed, and newer clients support gzip and deflate. I mean, that really should have been done via the Accept-Encoding header, but nuget.org's server architecture has a limitation that prevents that from being technically viable and therefore the people working on NuGet at the time decided to bake it into the service index resources instead.

Anyway, I'm getting way off topic. What I'm trying to get at, is that going forward we probably need to treat the service index entries as feature flags, not as a client version. bool supportsPackageTypeFilter = entries.Contains("3.5.0"), bool supportsSomeFutureFeature = entries.Contains("9.8.7"). Max resource version should not imply older features are supported.

Given the above, I am going to interpret this as exposing a new boolean property in PackageSearchResourceV3
...

Does this sound acceptable?

Yes, that's exactly what I was thinking.