From af537cc4a377613ba7c018eea8b3bcd27a6c1fa9 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Fri, 4 Nov 2022 18:11:12 +0200 Subject: [PATCH] Fix index expression options for requests with a single expression (#91231) This PR affects requests that contain a single index name or a single pattern (wildcard/datemath). It aims to systematize the handling of the `allow_no_indices` and `ignore_unavailable`indices options: * the allow_no_indices option is to be concerned with wildcards that expand to nothing (or the entire request expands to nothing) * the ignore_unavailable option is to be concerned with explicit names only (not wildcards) In addition, the behavior of the above options will now be independent of the number of expressions in a request. --- docs/changelog/91231.yaml | 5 ++ .../index/rankeval/RankEvalRequestIT.java | 2 +- .../50_wildcard_expansion.yml | 14 +++++ .../validate/SimpleValidateQueryIT.java | 2 +- .../metadata/IndexNameExpressionResolver.java | 56 +++++++++---------- .../IndexNameExpressionResolverTests.java | 25 +++++---- 6 files changed, 61 insertions(+), 43 deletions(-) create mode 100644 docs/changelog/91231.yaml diff --git a/docs/changelog/91231.yaml b/docs/changelog/91231.yaml new file mode 100644 index 0000000000000..1ca88ff84d8dc --- /dev/null +++ b/docs/changelog/91231.yaml @@ -0,0 +1,5 @@ +pr: 91231 +summary: Fix index expression options for requests with a single name or pattern +area: Infra/Core +type: bug +issues: [] diff --git a/modules/rank-eval/src/internalClusterTest/java/org/elasticsearch/index/rankeval/RankEvalRequestIT.java b/modules/rank-eval/src/internalClusterTest/java/org/elasticsearch/index/rankeval/RankEvalRequestIT.java index 8dfc592b6f4a5..64a26c426ff55 100644 --- a/modules/rank-eval/src/internalClusterTest/java/org/elasticsearch/index/rankeval/RankEvalRequestIT.java +++ b/modules/rank-eval/src/internalClusterTest/java/org/elasticsearch/index/rankeval/RankEvalRequestIT.java @@ -291,7 +291,7 @@ public void testIndicesOptions() { // test expand_wildcards request = new RankEvalRequest(task, new String[] { "tes*" }); - request.indicesOptions(IndicesOptions.fromParameters("none", null, null, "false", SearchRequest.DEFAULT_INDICES_OPTIONS)); + request.indicesOptions(IndicesOptions.fromParameters("none", "true", null, "false", SearchRequest.DEFAULT_INDICES_OPTIONS)); response = client().execute(RankEvalAction.INSTANCE, request).actionGet(); details = (PrecisionAtK.Detail) response.getPartialResults().get("amsterdam_query").getMetricDetails(); assertEquals(0, details.getRetrieved()); diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.get_mapping/50_wildcard_expansion.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.get_mapping/50_wildcard_expansion.yml index 5069a85f16e69..c6ff83345c6a0 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.get_mapping/50_wildcard_expansion.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.get_mapping/50_wildcard_expansion.yml @@ -111,6 +111,7 @@ setup: indices.get_mapping: index: test-x* expand_wildcards: none + ignore_unavailable: true - match: { '': {} } --- @@ -121,6 +122,19 @@ setup: index: test-x* expand_wildcards: none allow_no_indices: false + ignore_unavailable: true +--- +"Get test-* with wildcard_expansion=none ignore_unavailable=false": + - skip: + version: " - 8.5.99" + reason: "bug fixed in 8.6" + - do: + catch: missing + indices.get_mapping: + index: test-x* + expand_wildcards: none + allow_no_indices: true + ignore_unavailable: false --- "Get test-* with wildcard_expansion=open,closed": diff --git a/server/src/internalClusterTest/java/org/elasticsearch/validate/SimpleValidateQueryIT.java b/server/src/internalClusterTest/java/org/elasticsearch/validate/SimpleValidateQueryIT.java index b3ff6688aa8c6..29cd365ff403c 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/validate/SimpleValidateQueryIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/validate/SimpleValidateQueryIT.java @@ -260,7 +260,7 @@ public void testValidateEmptyCluster() { client().admin().indices().prepareValidateQuery().get(); fail("Expected IndexNotFoundException"); } catch (IndexNotFoundException e) { - assertThat(e.getMessage(), is("no such index [null] and no indices exist")); + assertThat(e.getMessage(), is("no such index [_all] and no indices exist")); } } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java index 19fae95341396..b319aebe18caf 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java @@ -335,28 +335,12 @@ Index[] concreteIndices(Context context, String... indexExpressions) { } } } - // If only one index is specified then whether we fail a request if an index is missing depends on the allow_no_indices - // option. At some point we should change this, because there shouldn't be a reason why whether a single index - // or multiple indices are specified yield different behaviour. - final boolean failNoIndices = indexExpressions.length == 1 - ? options.allowNoIndices() == false - : options.ignoreUnavailable() == false; + final Collection expressions = resolveExpressions(Arrays.asList(indexExpressions), context); - if (expressions.isEmpty()) { + if (expressions.isEmpty() || (expressions.size() == 1 && expressions.iterator().next().equals(Metadata.ALL))) { if (options.allowNoIndices() == false) { - IndexNotFoundException infe; - if (indexExpressions.length == 1) { - if (indexExpressions[0].equals(Metadata.ALL)) { - infe = new IndexNotFoundException("no indices exist", (String) null); - } else { - infe = new IndexNotFoundException((String) null); - } - } else { - infe = new IndexNotFoundException((String) null); - } - infe.setResources("index_expression", indexExpressions); - throw infe; + throw notFoundException(indexExpressions); } else { return Index.EMPTY_ARRAY; } @@ -368,20 +352,15 @@ Index[] concreteIndices(Context context, String... indexExpressions) { for (String expression : expressions) { IndexAbstraction indexAbstraction = indicesLookup.get(expression); if (indexAbstraction == null) { - if (failNoIndices) { - IndexNotFoundException infe; - if (expression.equals(Metadata.ALL)) { - infe = new IndexNotFoundException("no indices exist", expression); - } else { - infe = new IndexNotFoundException(expression); - } - infe.setResources("index_expression", expression); - throw infe; + if (options.ignoreUnavailable() == false) { + assert options.expandWildcardExpressions() == false; + throw notFoundException(expression); } else { continue; } } else if (indexAbstraction.getType() == Type.ALIAS && context.getOptions().ignoreAliases()) { - if (failNoIndices) { + if (options.ignoreUnavailable() == false) { + assert options.expandWildcardExpressions() == false; throw aliasesNotSupportedException(expression); } else { continue; @@ -436,8 +415,7 @@ Index[] concreteIndices(Context context, String... indexExpressions) { } if (options.allowNoIndices() == false && concreteIndices.isEmpty()) { - IndexNotFoundException infe = new IndexNotFoundException((String) null); - infe.setResources("index_expression", indexExpressions); + IndexNotFoundException infe = notFoundException(indexExpressions); if (excludedDataStreams) { // Allows callers to handle IndexNotFoundException differently based on whether data streams were excluded. infe.addMetadata(EXCLUDED_DATA_STREAMS_KEY, "true"); @@ -448,6 +426,22 @@ Index[] concreteIndices(Context context, String... indexExpressions) { return concreteIndices.toArray(Index.EMPTY_ARRAY); } + private IndexNotFoundException notFoundException(String... indexExpressions) { + IndexNotFoundException infe; + if (indexExpressions.length == 1) { + if (indexExpressions[0].equals(Metadata.ALL)) { + infe = new IndexNotFoundException("no indices exist", indexExpressions[0]); + } else { + infe = new IndexNotFoundException(indexExpressions[0]); + } + infe.setResources("index_expression", indexExpressions[0]); + } else { + infe = new IndexNotFoundException((String) null); + infe.setResources("index_expression", indexExpressions); + } + return infe; + } + private void checkSystemIndexAccess(Context context, Set concreteIndices) { final Metadata metadata = context.getState().metadata(); final Predicate systemIndexAccessPredicate = context.getSystemIndexAccessPredicate().negate(); diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java index 0aa6bde7008b1..d08166eaf4118 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolverTests.java @@ -478,8 +478,9 @@ public void testConcreteIndexNamesExpandWildcards() { results = indexNameExpressionResolver.concreteIndexNames(context, Strings.EMPTY_ARRAY); assertThat(results, emptyArray()); - results = indexNameExpressionResolver.concreteIndexNames(context, "h*"); - assertThat(results, emptyArray()); + IndexNameExpressionResolver.Context context3 = context; + infe = expectThrows(IndexNotFoundException.class, () -> indexNameExpressionResolver.concreteIndexNames(context3, "h*")); + assertThat(infe.getResourceId().toString(), equalTo("[h*]")); results = indexNameExpressionResolver.concreteIndexNames(context, "hidden"); assertThat(results, arrayContainingInAnyOrder("hidden")); @@ -562,8 +563,11 @@ public void testConcreteIndexNamesNoExpandWildcards() { SystemIndexAccessLevel.NONE ); { - String[] results = indexNameExpressionResolver.concreteIndexNames(context, "baz*"); - assertThat(results, emptyArray()); + IndexNotFoundException infe = expectThrows( + IndexNotFoundException.class, + () -> indexNameExpressionResolver.concreteIndexNames(context, "baz*") + ); + assertThat(infe.getIndex().getName(), equalTo("baz*")); } { IndexNotFoundException infe = expectThrows( @@ -829,7 +833,7 @@ public void testConcreteIndicesNoIndicesErrorMessage() { IndexNotFoundException.class, () -> indexNameExpressionResolver.concreteIndices(context, new String[] {}) ); - assertThat(infe.getMessage(), is("no such index [null] and no indices exist")); + assertThat(infe.getMessage(), is("no such index [_all] and no indices exist")); } public void testConcreteIndicesNoIndicesErrorMessageNoExpand() { @@ -1292,13 +1296,14 @@ public void testConcreteIndicesWildcardNoMatch() { SystemIndexAccessLevel.NONE ); - // asking for non existing wildcard pattern should return empty list or exception - if (indicesOptions.allowNoIndices()) { + if (indicesOptions.allowNoIndices() == false + || indicesOptions.expandWildcardExpressions() == false && indicesOptions.ignoreUnavailable() == false) { + expectThrows(IndexNotFoundException.class, () -> indexNameExpressionResolver.concreteIndexNames(context, "Foo*")); + } else { + // asking for non existing wildcard pattern should return empty list or exception String[] concreteIndices = indexNameExpressionResolver.concreteIndexNames(context, "Foo*"); assertThat(concreteIndices, notNullValue()); assertThat(concreteIndices.length, equalTo(0)); - } else { - expectThrows(IndexNotFoundException.class, () -> indexNameExpressionResolver.concreteIndexNames(context, "Foo*")); } } } @@ -2471,7 +2476,7 @@ public void testDataStreams() { IndexNotFoundException.class, () -> indexNameExpressionResolver.concreteWriteIndex(state, indicesOptions, "my-data-stream", false, false) ); - assertThat(e.getMessage(), equalTo("no such index [null]")); + assertThat(e.getMessage(), equalTo("no such index [my-data-stream]")); } }