From a96b33fa9d997069315bb33b70d2fd3289142ed7 Mon Sep 17 00:00:00 2001 From: Joel Verhagen Date: Tue, 17 Dec 2019 13:53:55 -0800 Subject: [PATCH] Add package type filtering to the autocomplete endpoint (#717) Progress on https://github.com/NuGet/NuGetGallery/issues/7754 --- .../SearchService/IndexOperationBuilder.cs | 19 +++++--- .../Models/AutocompleteRequest.cs | 1 + .../SearchService/SearchParametersBuilder.cs | 2 +- .../Controllers/SearchController.cs | 2 + .../IndexOperationBuilderFacts.cs | 10 +++++ .../SearchParametersBuilderFacts.cs | 45 ++++++++++++++++--- .../Controllers/SearchControllerFacts.cs | 5 +++ 7 files changed, 70 insertions(+), 14 deletions(-) diff --git a/src/NuGet.Services.AzureSearch/SearchService/IndexOperationBuilder.cs b/src/NuGet.Services.AzureSearch/SearchService/IndexOperationBuilder.cs index 7232207ff..e46ab49bf 100644 --- a/src/NuGet.Services.AzureSearch/SearchService/IndexOperationBuilder.cs +++ b/src/NuGet.Services.AzureSearch/SearchService/IndexOperationBuilder.cs @@ -30,10 +30,7 @@ public IndexOperationBuilder( public IndexOperation V3Search(V3SearchRequest request) { - // Requests with bad parameters yield no results. For the package type case, by specification a package type - // valid characters are the same as a package ID. - if (request.Skip > MaximumSkip - || (request.PackageType != null && !PackageIdValidator.IsValidPackageId(request.PackageType))) + if (HasInvalidParameters(request, request.PackageType)) { return IndexOperation.Empty(); } @@ -54,7 +51,7 @@ public IndexOperation V3Search(V3SearchRequest request) public IndexOperation V2SearchWithSearchIndex(V2SearchRequest request) { - if (request.Skip > MaximumSkip) + if (HasInvalidParameters(request, packageType: null)) { return IndexOperation.Empty(); } @@ -74,7 +71,7 @@ public IndexOperation V2SearchWithSearchIndex(V2SearchRequest request) public IndexOperation V2SearchWithHijackIndex(V2SearchRequest request) { - if (request.Skip > MaximumSkip) + if (HasInvalidParameters(request, packageType: null)) { return IndexOperation.Empty(); } @@ -94,7 +91,7 @@ public IndexOperation V2SearchWithHijackIndex(V2SearchRequest request) public IndexOperation Autocomplete(AutocompleteRequest request) { - if (request.Skip > MaximumSkip) + if (HasInvalidParameters(request, request.PackageType)) { return IndexOperation.Empty(); } @@ -181,6 +178,14 @@ private bool TryGetSingleVersion( return false; } + private static bool HasInvalidParameters(SearchRequest request, string packageType) + { + // Requests with bad parameters yield no results. For the package type case, by specification a package type + // valid characters are the same as a package ID. + return request.Skip > MaximumSkip + || (packageType != null && !PackageIdValidator.IsValidPackageId(packageType)); + } + private static bool PagedToFirstItem(SearchRequest request) { return request.Skip <= 0 && request.Take >= 1; diff --git a/src/NuGet.Services.AzureSearch/SearchService/Models/AutocompleteRequest.cs b/src/NuGet.Services.AzureSearch/SearchService/Models/AutocompleteRequest.cs index 0be081278..99cc770ea 100644 --- a/src/NuGet.Services.AzureSearch/SearchService/Models/AutocompleteRequest.cs +++ b/src/NuGet.Services.AzureSearch/SearchService/Models/AutocompleteRequest.cs @@ -9,5 +9,6 @@ namespace NuGet.Services.AzureSearch.SearchService public class AutocompleteRequest : SearchRequest { public AutocompleteRequestType Type { get; set; } + public string PackageType { get; set; } } } diff --git a/src/NuGet.Services.AzureSearch/SearchService/SearchParametersBuilder.cs b/src/NuGet.Services.AzureSearch/SearchService/SearchParametersBuilder.cs index 99aef7478..eaa21dd30 100644 --- a/src/NuGet.Services.AzureSearch/SearchService/SearchParametersBuilder.cs +++ b/src/NuGet.Services.AzureSearch/SearchService/SearchParametersBuilder.cs @@ -94,7 +94,7 @@ public SearchParameters Autocomplete(AutocompleteRequest request, bool isDefault { var searchParameters = NewSearchParameters(); - ApplySearchIndexFilter(searchParameters, request, isDefaultSearch, packageType: null); + ApplySearchIndexFilter(searchParameters, request, isDefaultSearch, request.PackageType); switch (request.Type) { diff --git a/src/NuGet.Services.SearchService/Controllers/SearchController.cs b/src/NuGet.Services.SearchService/Controllers/SearchController.cs index 29d8f91f7..fb0e365a6 100644 --- a/src/NuGet.Services.SearchService/Controllers/SearchController.cs +++ b/src/NuGet.Services.SearchService/Controllers/SearchController.cs @@ -135,6 +135,7 @@ public async Task AutocompleteAsync( string semVerLevel = null, string q = null, string id = null, + string packageType = null, bool? debug = false) { await EnsureInitializedAsync(); @@ -152,6 +153,7 @@ public async Task AutocompleteAsync( IncludeSemVer2 = GetIncludeSemVer2(semVerLevel), Query = q ?? id, Type = type, + PackageType = packageType, ShowDebug = debug ?? false, }; diff --git a/tests/NuGet.Services.AzureSearch.Tests/SearchService/IndexOperationBuilderFacts.cs b/tests/NuGet.Services.AzureSearch.Tests/SearchService/IndexOperationBuilderFacts.cs index ebdd194cc..79b869666 100644 --- a/tests/NuGet.Services.AzureSearch.Tests/SearchService/IndexOperationBuilderFacts.cs +++ b/tests/NuGet.Services.AzureSearch.Tests/SearchService/IndexOperationBuilderFacts.cs @@ -27,6 +27,16 @@ public void BuildsSearchOperation() TextBuilder.Verify(x => x.Autocomplete(AutocompleteRequest), Times.Once); ParametersBuilder.Verify(x => x.Autocomplete(AutocompleteRequest, It.IsAny()), Times.Once); } + + [Fact] + public void ReturnsEmptyQueryForInvalidPackageType() + { + AutocompleteRequest.PackageType = "invalid package type"; + + var actual = Build(); + + Assert.Equal(IndexOperationType.Empty, actual.Type); + } } public class V3Search : SearchIndexFacts diff --git a/tests/NuGet.Services.AzureSearch.Tests/SearchService/SearchParametersBuilderFacts.cs b/tests/NuGet.Services.AzureSearch.Tests/SearchService/SearchParametersBuilderFacts.cs index 341502896..76d724723 100644 --- a/tests/NuGet.Services.AzureSearch.Tests/SearchService/SearchParametersBuilderFacts.cs +++ b/tests/NuGet.Services.AzureSearch.Tests/SearchService/SearchParametersBuilderFacts.cs @@ -239,12 +239,7 @@ public void Defaults() } [Theory] - [InlineData("Dependency")] - [InlineData("DotnetTool")] - [InlineData("Template")] - [InlineData("PackageType.With.Dots")] - [InlineData("PackageType-With-Hyphens")] - [InlineData("PackageType_With_Underscores")] + [MemberData(nameof(ValidPackageTypes))] public void PackageTypeFiltering(string packageType) { var request = new V3SearchRequest @@ -468,6 +463,33 @@ public void SearchFilters(bool includePrerelease, bool includeSemVer2, string fi Assert.Equal(filter, output.Filter); } + + [Theory] + [MemberData(nameof(ValidPackageTypes))] + public void PackageTypeFiltering(string packageType) + { + var request = new AutocompleteRequest + { + PackageType = packageType, + }; + + var output = _target.Autocomplete(request, It.IsAny()); + + Assert.Equal($"searchFilters eq 'Default' and filterablePackageTypes/any(p: p eq '{packageType.ToLowerInvariant()}')", output.Filter); + } + + [Fact] + public void InvalidPackageType() + { + var request = new AutocompleteRequest + { + PackageType = "something's-weird", + }; + + var output = _target.Autocomplete(request, It.IsAny()); + + Assert.Equal("searchFilters eq 'Default'", output.Filter); + } } public abstract class BaseFacts @@ -503,6 +525,17 @@ public abstract class BaseFacts .Cast() .Select(x => new object[] { x }); + + public static IEnumerable ValidPackageTypes => new[] + { + new object[] { "Dependency" }, + new object[] { "DotnetTool" }, + new object[] { "Template" }, + new object[] { "PackageType.With.Dots" }, + new object[] { "PackageType-With-Hyphens" }, + new object[] { "PackageType_With_Underscores" }, + }; + public BaseFacts() { _target = new SearchParametersBuilder(); diff --git a/tests/NuGet.Services.SearchService.Tests/Controllers/SearchControllerFacts.cs b/tests/NuGet.Services.SearchService.Tests/Controllers/SearchControllerFacts.cs index aec60b97d..2edc0acb1 100644 --- a/tests/NuGet.Services.SearchService.Tests/Controllers/SearchControllerFacts.cs +++ b/tests/NuGet.Services.SearchService.Tests/Controllers/SearchControllerFacts.cs @@ -395,6 +395,7 @@ public async Task HasDefaultParameters() Assert.False(lastRequest.IncludeSemVer2); Assert.Null(lastRequest.Query); Assert.False(lastRequest.ShowDebug); + Assert.Null(lastRequest.PackageType); Assert.Equal(AutocompleteRequestType.PackageIds, lastRequest.Type); } @@ -414,6 +415,7 @@ await _target.AutocompleteAsync( semVerLevel: null, q: null, id: null, + packageType: null, debug: null); _searchService.Verify(x => x.AutocompleteAsync(It.IsAny()), Times.Once); @@ -424,6 +426,7 @@ await _target.AutocompleteAsync( Assert.False(lastRequest.IncludeSemVer2); Assert.Null(lastRequest.Query); Assert.False(lastRequest.ShowDebug); + Assert.Null(lastRequest.PackageType); Assert.Equal(AutocompleteRequestType.PackageIds, lastRequest.Type); } @@ -442,6 +445,7 @@ await _target.AutocompleteAsync( prerelease: true, semVerLevel: "2.0.0", q: "windows azure storage", + packageType: "DotnetTool", debug: true); _searchService.Verify(x => x.AutocompleteAsync(It.IsAny()), Times.Once); @@ -452,6 +456,7 @@ await _target.AutocompleteAsync( Assert.True(lastRequest.IncludeSemVer2); Assert.Equal("windows azure storage", lastRequest.Query); Assert.True(lastRequest.ShowDebug); + Assert.Equal("DotnetTool", lastRequest.PackageType); Assert.Equal(AutocompleteRequestType.PackageIds, lastRequest.Type); }