diff --git a/docs/changelog/134134.yaml b/docs/changelog/134134.yaml new file mode 100644 index 0000000000000..cbaff5bce55f6 --- /dev/null +++ b/docs/changelog/134134.yaml @@ -0,0 +1,6 @@ +pr: 134134 +summary: Prevent field caps from failing due to can match failure +area: Search +type: bug +issues: + - 116106 diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/CCSFieldCapabilitiesIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/CCSFieldCapabilitiesIT.java index e727f5fdfced4..240a13bc601b8 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/CCSFieldCapabilitiesIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/CCSFieldCapabilitiesIT.java @@ -14,15 +14,12 @@ import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse; import org.elasticsearch.client.internal.Client; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.plugins.Plugin; -import org.elasticsearch.search.fieldcaps.FieldCapabilitiesIT.ExceptionOnRewriteQueryBuilder; -import org.elasticsearch.search.fieldcaps.FieldCapabilitiesIT.ExceptionOnRewriteQueryPlugin; +import org.elasticsearch.index.shard.IllegalIndexShardStateException; import org.elasticsearch.test.AbstractMultiClustersTestCase; import org.elasticsearch.transport.RemoteTransportException; -import java.util.ArrayList; +import java.io.IOException; import java.util.Arrays; -import java.util.Collection; import java.util.List; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; @@ -46,14 +43,7 @@ protected boolean reuseClusters() { return false; } - @Override - protected Collection> nodePlugins(String clusterAlias) { - final List> plugins = new ArrayList<>(super.nodePlugins(clusterAlias)); - plugins.add(ExceptionOnRewriteQueryPlugin.class); - return plugins; - } - - public void testFailuresFromRemote() { + public void testFailuresFromRemote() throws IOException { Settings indexSettings = Settings.builder().put("index.number_of_replicas", 0).build(); final Client localClient = client(LOCAL_CLUSTER); final Client remoteClient = client("remote_cluster"); @@ -71,11 +61,10 @@ public void testFailuresFromRemote() { FieldCapabilitiesResponse response = client().prepareFieldCaps("*", "remote_cluster:*").setFields("*").get(); assertThat(Arrays.asList(response.getIndices()), containsInAnyOrder(localIndex, "remote_cluster:" + remoteErrorIndex)); - // adding an index filter so remote call should fail - response = client().prepareFieldCaps("*", "remote_cluster:*") - .setFields("*") - .setIndexFilter(new ExceptionOnRewriteQueryBuilder()) - .get(); + // Closed shards will result to index error because shards must be in readable state + FieldCapabilitiesIT.closeShards(cluster("remote_cluster"), remoteErrorIndex); + + response = client().prepareFieldCaps("*", "remote_cluster:*").setFields("*").get(); assertThat(response.getIndices()[0], equalTo(localIndex)); assertThat(response.getFailedIndicesCount(), equalTo(1)); FieldCapabilitiesFailure failure = response.getFailures() @@ -86,25 +75,22 @@ public void testFailuresFromRemote() { Exception ex = failure.getException(); assertEquals(RemoteTransportException.class, ex.getClass()); Throwable cause = ExceptionsHelper.unwrapCause(ex); - assertEquals(IllegalArgumentException.class, cause.getClass()); - assertEquals("I throw because I choose to.", cause.getMessage()); + assertEquals(IllegalIndexShardStateException.class, cause.getClass()); + assertEquals( + "CurrentState[CLOSED] operations only allowed when shard state is one of [POST_RECOVERY, STARTED]", + cause.getMessage() + ); // if we only query the remote we should get back an exception only - ex = expectThrows( - IllegalArgumentException.class, - client().prepareFieldCaps("remote_cluster:*").setFields("*").setIndexFilter(new ExceptionOnRewriteQueryBuilder()) - ); - assertEquals("I throw because I choose to.", ex.getMessage()); + ex = expectThrows(IllegalIndexShardStateException.class, client().prepareFieldCaps("remote_cluster:*").setFields("*")); + assertEquals("CurrentState[CLOSED] operations only allowed when shard state is one of [POST_RECOVERY, STARTED]", ex.getMessage()); // add an index that doesn't fail to the remote assertAcked(remoteClient.admin().indices().prepareCreate("okay_remote_index")); remoteClient.prepareIndex("okay_remote_index").setId("2").setSource("foo", "bar").get(); remoteClient.admin().indices().prepareRefresh("okay_remote_index").get(); - response = client().prepareFieldCaps("*", "remote_cluster:*") - .setFields("*") - .setIndexFilter(new ExceptionOnRewriteQueryBuilder()) - .get(); + response = client().prepareFieldCaps("*", "remote_cluster:*").setFields("*").get(); assertThat(Arrays.asList(response.getIndices()), containsInAnyOrder(localIndex, "remote_cluster:okay_remote_index")); assertThat(response.getFailedIndicesCount(), equalTo(1)); failure = response.getFailures() @@ -113,8 +99,8 @@ public void testFailuresFromRemote() { .findFirst() .get(); ex = failure.getException(); - assertEquals(IllegalArgumentException.class, ex.getClass()); - assertEquals("I throw because I choose to.", ex.getMessage()); + assertEquals(IllegalIndexShardStateException.class, ex.getClass()); + assertEquals("CurrentState[CLOSED] operations only allowed when shard state is one of [POST_RECOVERY, STARTED]", ex.getMessage()); } public void testFailedToConnectToRemoteCluster() throws Exception { diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/FieldCapabilitiesIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/FieldCapabilitiesIT.java index 05b6bf1f9166d..b9fcec556213a 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/FieldCapabilitiesIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/search/fieldcaps/FieldCapabilitiesIT.java @@ -47,7 +47,7 @@ import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.query.QueryRewriteContext; -import org.elasticsearch.index.query.SearchExecutionContext; +import org.elasticsearch.index.shard.IllegalIndexShardStateException; import org.elasticsearch.index.shard.IndexShard; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.indices.IndexClosedException; @@ -59,6 +59,7 @@ import org.elasticsearch.search.DummyQueryBuilder; import org.elasticsearch.tasks.TaskInfo; import org.elasticsearch.test.ESIntegTestCase; +import org.elasticsearch.test.InternalTestCluster; import org.elasticsearch.test.MockLog; import org.elasticsearch.test.junit.annotations.TestLogging; import org.elasticsearch.test.transport.MockTransportService; @@ -78,6 +79,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.CancellationException; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -87,7 +89,6 @@ import java.util.function.Predicate; import java.util.stream.IntStream; -import static java.util.Collections.singletonList; import static org.elasticsearch.action.support.ActionTestUtils.wrapAsRestResponseListener; import static org.elasticsearch.index.shard.IndexShardTestCase.closeShardNoCheck; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; @@ -193,7 +194,7 @@ public void setUp() throws Exception { @Override protected Collection> nodePlugins() { - return List.of(TestMapperPlugin.class, ExceptionOnRewriteQueryPlugin.class, BlockingOnRewriteQueryPlugin.class); + return List.of(TestMapperPlugin.class, BlockingOnRewriteQueryPlugin.class); } @Override @@ -445,30 +446,36 @@ public void testFieldMetricsAndDimensions() { assertThat(response.get().get("some_dimension").get("keyword").nonDimensionIndices(), array(equalTo("new_index"))); } - public void testFailures() throws InterruptedException { + public void testFailures() throws IOException { // in addition to the existing "old_index" and "new_index", create two where the test query throws an error on rewrite assertAcked(prepareCreate("index1-error"), prepareCreate("index2-error")); ensureGreen("index1-error", "index2-error"); - FieldCapabilitiesResponse response = client().prepareFieldCaps() + + // Closed shards will result to index error because shards must be in readable state + closeShards(internalCluster(), "index1-error", "index2-error"); + + FieldCapabilitiesResponse response = client().prepareFieldCaps("old_index", "new_index", "index1-error", "index2-error") .setFields("*") - .setIndexFilter(new ExceptionOnRewriteQueryBuilder()) .get(); assertEquals(1, response.getFailures().size()); assertEquals(2, response.getFailedIndicesCount()); assertThat(response.getFailures().get(0).getIndices(), arrayContainingInAnyOrder("index1-error", "index2-error")); Exception failure = response.getFailures().get(0).getException(); - assertEquals(IllegalArgumentException.class, failure.getClass()); - assertEquals("I throw because I choose to.", failure.getMessage()); + assertEquals(IllegalIndexShardStateException.class, failure.getClass()); + assertEquals( + "CurrentState[CLOSED] operations only allowed when shard state is one of [POST_RECOVERY, STARTED]", + failure.getMessage() + ); // the "indices" section should not include failed ones assertThat(Arrays.asList(response.getIndices()), containsInAnyOrder("old_index", "new_index")); // if all requested indices failed, we fail the request by throwing the exception - IllegalArgumentException ex = expectThrows( - IllegalArgumentException.class, - client().prepareFieldCaps("index1-error", "index2-error").setFields("*").setIndexFilter(new ExceptionOnRewriteQueryBuilder()) + IllegalIndexShardStateException ex = expectThrows( + IllegalIndexShardStateException.class, + client().prepareFieldCaps("index1-error", "index2-error").setFields("*") ); - assertEquals("I throw because I choose to.", ex.getMessage()); + assertEquals("CurrentState[CLOSED] operations only allowed when shard state is one of [POST_RECOVERY, STARTED]", ex.getMessage()); } private void populateTimeRangeIndices() throws Exception { @@ -913,45 +920,17 @@ private void assertIndices(FieldCapabilitiesResponse response, String... indices assertArrayEquals(indices, response.getIndices()); } - /** - * Adds an "exception" query that throws on rewrite if the index name contains the string "error" - */ - public static class ExceptionOnRewriteQueryPlugin extends Plugin implements SearchPlugin { - - public ExceptionOnRewriteQueryPlugin() {} - - @Override - public List> getQueries() { - return singletonList( - new QuerySpec<>("exception", ExceptionOnRewriteQueryBuilder::new, p -> new ExceptionOnRewriteQueryBuilder()) - ); - } - } - - static class ExceptionOnRewriteQueryBuilder extends DummyQueryBuilder { - - public static final String NAME = "exception"; - - ExceptionOnRewriteQueryBuilder() {} - - ExceptionOnRewriteQueryBuilder(StreamInput in) throws IOException { - super(in); - } - - @Override - protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws IOException { - SearchExecutionContext searchExecutionContext = queryRewriteContext.convertToSearchExecutionContext(); - if (searchExecutionContext != null) { - if (searchExecutionContext.indexMatches("*error*")) { - throw new IllegalArgumentException("I throw because I choose to."); + static void closeShards(InternalTestCluster cluster, String... indices) throws IOException { + final Set indicesToClose = Set.of(indices); + for (String node : cluster.getNodeNames()) { + final IndicesService indicesService = cluster.getInstance(IndicesService.class, node); + for (IndexService indexService : indicesService) { + if (indicesToClose.contains(indexService.getMetadata().getIndex().getName())) { + for (IndexShard indexShard : indexService) { + closeShardNoCheck(indexShard); + } } } - return this; - } - - @Override - public String getWriteableName() { - return NAME; } } diff --git a/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesFetcher.java b/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesFetcher.java index aee7858d84b13..3cb759250802d 100644 --- a/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesFetcher.java +++ b/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesFetcher.java @@ -249,12 +249,17 @@ private static boolean canMatchShard( QueryBuilder indexFilter, long nowInMillis, SearchExecutionContext searchExecutionContext - ) throws IOException { + ) { assert alwaysMatches(indexFilter) == false : "should not be called for always matching [" + indexFilter + "]"; assert nowInMillis != 0L; ShardSearchRequest searchRequest = new ShardSearchRequest(shardId, nowInMillis, AliasFilter.EMPTY); searchRequest.source(new SearchSourceBuilder().query(indexFilter)); - return SearchService.queryStillMatchesAfterRewrite(searchRequest, searchExecutionContext); + try { + return SearchService.queryStillMatchesAfterRewrite(searchRequest, searchExecutionContext); + } catch (Exception e) { + // treat as if shard is still a potential match + return true; + } } private static boolean alwaysMatches(QueryBuilder indexFilter) { diff --git a/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceFeatures.java b/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceFeatures.java index d5f8cebba3167..f6b5a8760aed0 100644 --- a/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceFeatures.java +++ b/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceFeatures.java @@ -84,7 +84,8 @@ public Set getTestFeatures() { SEMANTIC_TEXT_HIGHLIGHTING_FLAT, SEMANTIC_TEXT_SPARSE_VECTOR_INDEX_OPTIONS, SEMANTIC_TEXT_FIELDS_CHUNKS_FORMAT, - SemanticQueryBuilder.SEMANTIC_QUERY_MULTIPLE_INFERENCE_IDS + SemanticQueryBuilder.SEMANTIC_QUERY_MULTIPLE_INFERENCE_IDS, + SemanticQueryBuilder.SEMANTIC_QUERY_FILTER_FIELD_CAPS_FIX ) ); if (RERANK_SNIPPETS.isEnabled()) { diff --git a/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java b/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java index 71eb3494aa8ce..cf90c3e93fce8 100644 --- a/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java +++ b/x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/queries/SemanticQueryBuilder.java @@ -56,6 +56,7 @@ public class SemanticQueryBuilder extends AbstractQueryBuilder