Address Julie's feedback

- Move all request building logic to KnnSearchRequestBuilder - Move a check for allowing filtered aliases to IndicesService
elastic · Oct 25, 2021 · bd484b1 · bd484b1
1 parent d522f12
commit bd484b1
Show file tree

Hide file tree

Showing 9 changed files with 40 additions and 37 deletions.
diff --git a/...ava/org/elasticsearch/action/admin/cluster/shards/TransportClusterSearchShardsAction.java b/...ava/org/elasticsearch/action/admin/cluster/shards/TransportClusterSearchShardsAction.java
@@ -61,8 +61,9 @@ protected void masterOperation(Task task, final ClusterSearchShardsRequest reque
         Map<String, Set<String>> routingMap = indexNameExpressionResolver.resolveSearchRouting(state, request.routing(), request.indices());
         Map<String, AliasFilter> indicesAndFilters = new HashMap<>();
         Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, request.indices());
+        boolean forbidFilteredAliases = request.indicesOptions().forbidFilteredAliases();
         for (String index : concreteIndices) {
-            final AliasFilter aliasFilter = indicesService.buildAliasFilter(clusterState, index, indicesAndAliases);
+            final AliasFilter aliasFilter = indicesService.buildAliasFilter(clusterState, index, indicesAndAliases, forbidFilteredAliases);
             final String[] aliases = indexNameExpressionResolver.indexAliases(clusterState, index,
                 aliasMetadata -> true, dataStreamAlias -> true, true, indicesAndAliases);
             indicesAndFilters.put(index, new AliasFilter(aliasFilter.getQueryBuilder(), aliases));

diff --git a/...a/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/...a/org/elasticsearch/action/admin/indices/validate/query/TransportValidateQueryAction.java
@@ -109,7 +109,8 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener
     protected ShardValidateQueryRequest newShardRequest(int numShards, ShardRouting shard, ValidateQueryRequest request) {
         final ClusterState clusterState = clusterService.state();
         final Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, request.indices());
-        final AliasFilter aliasFilter = searchService.buildAliasFilter(clusterState, shard.getIndexName(), indicesAndAliases);
+        final AliasFilter aliasFilter = searchService.buildAliasFilter(clusterState, shard.getIndexName(), indicesAndAliases,
+            request.indicesOptions().forbidFilteredAliases());
         return new ShardValidateQueryRequest(shard.shardId(), aliasFilter, request);
     }
 

diff --git a/server/src/main/java/org/elasticsearch/action/explain/TransportExplainAction.java b/server/src/main/java/org/elasticsearch/action/explain/TransportExplainAction.java
@@ -70,7 +70,8 @@ protected boolean resolveIndex(ExplainRequest request) {
     @Override
     protected void resolveRequest(ClusterState state, InternalRequest request) {
         final Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(state, request.request().index());
-        final AliasFilter aliasFilter = searchService.buildAliasFilter(state, request.concreteIndex(), indicesAndAliases);
+        final AliasFilter aliasFilter = searchService.buildAliasFilter(state, request.concreteIndex(), indicesAndAliases,
+            request.request().indicesOptions().forbidFilteredAliases());
         request.request().filteringAlias(aliasFilter);
         // Fail fast on the node that received the request.
         if (request.request().routing() == null && state.getMetadata().routingRequired(request.concreteIndex())) {

diff --git a/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java b/server/src/main/java/org/elasticsearch/action/search/TransportSearchAction.java
@@ -183,26 +183,20 @@ private Map<String, OriginalIndices> buildPerIndexOriginalIndices(ClusterState c
     private Map<String, AliasFilter> buildPerIndexAliasFilter(ClusterState clusterState,
                                                               Set<String> indicesAndAliases,
                                                               Index[] concreteIndices,
-                                                              Map<String, AliasFilter> remoteAliasMap) {
+                                                              Map<String, AliasFilter> remoteAliasMap,
+                                                              boolean forbidFilteredAliases) {
         final Map<String, AliasFilter> aliasFilterMap = new HashMap<>();
         for (Index index : concreteIndices) {
             clusterState.blocks().indexBlockedRaiseException(ClusterBlockLevel.READ, index.getName());
-            AliasFilter aliasFilter = searchService.buildAliasFilter(clusterState, index.getName(), indicesAndAliases);
+            AliasFilter aliasFilter = searchService.buildAliasFilter(
+                clusterState, index.getName(), indicesAndAliases, forbidFilteredAliases);
             assert aliasFilter != null;
             aliasFilterMap.put(index.getUUID(), aliasFilter);
         }
         aliasFilterMap.putAll(remoteAliasMap);
         return aliasFilterMap;
     }
 
-    private static void checkForFilteredAliases(Map<String, AliasFilter> aliasFilterMap) {
-        for (AliasFilter aliasFilter : aliasFilterMap.values()) {
-            if (aliasFilter.getQueryBuilder() != null) {
-                throw new IllegalArgumentException("Filtered aliases are not supported, use general aliases or concrete indices instead.");
-            }
-        }
-    }
-
     private Map<String, Float> resolveIndexBoosts(SearchRequest searchRequest, ClusterState clusterState) {
         if (searchRequest.source() == null) {
             return Collections.emptyMap();
@@ -706,10 +700,8 @@ private void executeSearch(SearchTask task, SearchTimeProvider timeProvider, Sea
                 concreteLocalIndices, routingMap, searchRequest.preference(),
                 searchService.getResponseCollectorService(), nodeSearchCounts);
             final Set<String> indicesAndAliases = indexNameExpressionResolver.resolveExpressions(clusterState, searchRequest.indices());
-            aliasFilter = buildPerIndexAliasFilter(clusterState, indicesAndAliases, indices, remoteAliasMap);
-            if (searchRequest.indicesOptions().forbidFilteredAliases()) {
-                checkForFilteredAliases(aliasFilter);
-            }
+            aliasFilter = buildPerIndexAliasFilter(clusterState, indicesAndAliases, indices,
+                remoteAliasMap, searchRequest.indicesOptions().forbidFilteredAliases());
             final Map<String, OriginalIndices> finalIndicesMap =
                 buildPerIndexOriginalIndices(clusterState, indicesAndAliases, indices, searchRequest.indicesOptions());
             localShardIterators = StreamSupport.stream(localShardRoutings.spliterator(), false)

diff --git a/server/src/main/java/org/elasticsearch/indices/IndicesService.java b/server/src/main/java/org/elasticsearch/indices/IndicesService.java
@@ -1520,7 +1520,15 @@ interface IndexDeletionAllowedPredicate {
         (Index index, IndexSettings indexSettings) -> canDeleteIndexContents(index);
     private final IndexDeletionAllowedPredicate ALWAYS_TRUE = (Index index, IndexSettings indexSettings) -> true;
 
-    public AliasFilter buildAliasFilter(ClusterState state, String index, Set<String> resolvedExpressions) {
+    public AliasFilter buildAliasFilter(ClusterState state, String index, Set<String> resolvedExpressions, boolean forbidFilteredAliases) {
+       AliasFilter aliasFilter = buildAliasFilterInteral(state, index, resolvedExpressions);
+       if (forbidFilteredAliases && aliasFilter.getQueryBuilder() != null) {
+           throw new IllegalArgumentException("Filtered aliases are not supported, use general aliases or concrete indices instead.");
+       }
+       return aliasFilter;
+    }
+
+    private AliasFilter buildAliasFilterInteral(ClusterState state, String index, Set<String> resolvedExpressions) {
         /* Being static, parseAliasFilter doesn't have access to whatever guts it needs to parse a query. Instead of passing in a bunch
          * of dependencies we pass in a function that can perform the parsing. */
         CheckedFunction<BytesReference, QueryBuilder, IOException> filterParser = bytes -> {

diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java
@@ -1310,8 +1310,8 @@ public void run() {
         }
     }
 
-    public AliasFilter buildAliasFilter(ClusterState state, String index, Set<String> resolvedExpressions) {
-        return indicesService.buildAliasFilter(state, index, resolvedExpressions);
+    public AliasFilter buildAliasFilter(ClusterState state, String index, Set<String> resolvedExpressions, boolean forbidFilteredAliases) {
+        return indicesService.buildAliasFilter(state, index, resolvedExpressions, forbidFilteredAliases);
     }
 
     public void canMatch(ShardSearchRequest request, ActionListener<CanMatchShardResponse> listener) {

diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesServiceTests.java
@@ -581,17 +581,17 @@ public void testBuildAliasFilter() {
             );
         ClusterState state = ClusterState.builder(new ClusterName("_name")).metadata(mdBuilder).build();
         {
-            AliasFilter result = indicesService.buildAliasFilter(state, "test-0", Set.of("test-alias-0"));
+            AliasFilter result = indicesService.buildAliasFilter(state, "test-0", Set.of("test-alias-0"), false);
             assertThat(result.getAliases(), arrayContainingInAnyOrder("test-alias-0"));
             assertThat(result.getQueryBuilder(), equalTo(QueryBuilders.termQuery("foo", "bar")));
         }
         {
-            AliasFilter result = indicesService.buildAliasFilter(state, "test-0", Set.of("test-alias-1"));
+            AliasFilter result = indicesService.buildAliasFilter(state, "test-0", Set.of("test-alias-1"), false);
             assertThat(result.getAliases(), arrayContainingInAnyOrder("test-alias-1"));
             assertThat(result.getQueryBuilder(), equalTo(QueryBuilders.termQuery("foo", "baz")));
         }
         {
-            AliasFilter result = indicesService.buildAliasFilter(state, "test-0", Set.of("test-alias-0", "test-alias-1"));
+            AliasFilter result = indicesService.buildAliasFilter(state, "test-0", Set.of("test-alias-0", "test-alias-1"), false);
             assertThat(result.getAliases(), arrayContainingInAnyOrder("test-alias-0", "test-alias-1"));
             BoolQueryBuilder filter = (BoolQueryBuilder) result.getQueryBuilder();
             assertThat(filter.filter(), empty());
@@ -601,7 +601,7 @@ public void testBuildAliasFilter() {
         }
         {
             AliasFilter result =
-                indicesService.buildAliasFilter(state, "test-0", Set.of("test-alias-0", "test-alias-1", "test-alias-non-filtering"));
+                indicesService.buildAliasFilter(state, "test-0", Set.of("test-alias-0", "test-alias-1", "test-alias-non-filtering"), false);
             assertThat(result.getAliases(), emptyArray());
             assertThat(result.getQueryBuilder(), nullValue());
         }
@@ -621,13 +621,13 @@ public void testBuildAliasFilterDataStreamAliases() {
         ClusterState state = ClusterState.builder(new ClusterName("_name")).metadata(mdBuilder).build();
         {
             String index = backingIndex1.getIndex().getName();
-            AliasFilter result = indicesService.buildAliasFilter(state, index, Set.of("logs_foo"));
+            AliasFilter result = indicesService.buildAliasFilter(state, index, Set.of("logs_foo"), false);
             assertThat(result.getAliases(), arrayContainingInAnyOrder("logs_foo"));
             assertThat(result.getQueryBuilder(), equalTo(QueryBuilders.termQuery("foo", "bar")));
         }
         {
             String index = backingIndex1.getIndex().getName();
-            AliasFilter result = indicesService.buildAliasFilter(state, index, Set.of("logs_foo", "logs"));
+            AliasFilter result = indicesService.buildAliasFilter(state, index, Set.of("logs_foo", "logs"), false);
             assertThat(result.getAliases(), arrayContainingInAnyOrder("logs_foo", "logs"));
             BoolQueryBuilder filter = (BoolQueryBuilder) result.getQueryBuilder();
             assertThat(filter.filter(), empty());
@@ -637,7 +637,7 @@ public void testBuildAliasFilterDataStreamAliases() {
         }
         {
             String index = backingIndex1.getIndex().getName();
-            AliasFilter result = indicesService.buildAliasFilter(state, index, Set.of("logs_foo", "logs", "logs_bar"));
+            AliasFilter result = indicesService.buildAliasFilter(state, index, Set.of("logs_foo", "logs", "logs_bar"), false);
             assertThat(result.getAliases(), arrayContainingInAnyOrder("logs_foo", "logs"));
             BoolQueryBuilder filter = (BoolQueryBuilder) result.getQueryBuilder();
             assertThat(filter.filter(), empty());

diff --git a/...vectors/src/main/java/org/elasticsearch/xpack/vectors/action/KnnSearchRequestBuilder.java b/...vectors/src/main/java/org/elasticsearch/xpack/vectors/action/KnnSearchRequestBuilder.java
@@ -8,6 +8,7 @@
 package org.elasticsearch.xpack.vectors.action;
 
 import org.elasticsearch.action.search.SearchRequestBuilder;
+import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.search.builder.SearchSourceBuilder;
@@ -23,6 +24,7 @@
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.EnumSet;
 import java.util.List;
 import java.util.Objects;
 
@@ -133,6 +135,13 @@ public void build(SearchRequestBuilder builder) {
         builder.setIndices(indices);
         builder.setRouting(routing);
 
+        // Forbid filtered aliases in _knn_search request
+        IndicesOptions indicesOptions = builder.request().indicesOptions();
+        EnumSet<IndicesOptions.Option> options = indicesOptions.getOptions();
+        options.add(IndicesOptions.Option.FORBID_FILTERED_ALIASES);
+        IndicesOptions newIndicesOptions = new IndicesOptions(options, indicesOptions.getExpandWildcards());
+        builder.setIndicesOptions(newIndicesOptions);
+
         SearchSourceBuilder sourceBuilder = new SearchSourceBuilder();
         sourceBuilder.trackTotalHitsUpTo(SearchContext.TRACK_TOTAL_HITS_ACCURATE);
 

diff --git a/...gin/vectors/src/main/java/org/elasticsearch/xpack/vectors/action/RestKnnSearchAction.java b/...gin/vectors/src/main/java/org/elasticsearch/xpack/vectors/action/RestKnnSearchAction.java
@@ -8,15 +8,13 @@
 
 
 import org.elasticsearch.action.search.SearchRequestBuilder;
-import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.rest.BaseRestHandler;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.action.RestCancellableNodeClient;
 import org.elasticsearch.rest.action.RestStatusToXContentListener;
 
 import java.io.IOException;
-import java.util.EnumSet;
 import java.util.List;
 
 import static org.elasticsearch.rest.RestRequest.Method.GET;
@@ -47,15 +45,8 @@ protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient
         // This will allow to cancel the search request if the http channel is closed
         RestCancellableNodeClient cancellableNodeClient = new RestCancellableNodeClient(client, restRequest.getHttpChannel());
         KnnSearchRequestBuilder request = KnnSearchRequestBuilder.parseRestRequest(restRequest);
-        SearchRequestBuilder searchRequestBuilder = cancellableNodeClient.prepareSearch();
-
-        // Forbid filtered aliases in _knn_search request
-        IndicesOptions indicesOptions = searchRequestBuilder.request().indicesOptions();
-        EnumSet<IndicesOptions.Option> options = indicesOptions.getOptions();
-        options.add(IndicesOptions.Option.FORBID_FILTERED_ALIASES);
-        IndicesOptions newIndicesOptions = new IndicesOptions(options, indicesOptions.getExpandWildcards());
-        searchRequestBuilder.setIndicesOptions(newIndicesOptions);
 
+        SearchRequestBuilder searchRequestBuilder = cancellableNodeClient.prepareSearch();
         request.build(searchRequestBuilder);
 
         return channel -> searchRequestBuilder.execute(new RestStatusToXContentListener<>(channel));