From 5c0f9bc499c5c4a744d2f29fb5bd9eab4aabc004 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Mon, 14 Mar 2022 17:11:26 -0400 Subject: [PATCH] Discrepancy in result from _validate/query API and actual query validity (#2416) * Discrepancy in result from _validate/query API and actual query validity Signed-off-by: Andriy Redko * Moved the validate() check later into the flow to allow range validation to trigger first Signed-off-by: Andriy Redko --- .../validate/SimpleValidateQueryIT.java | 97 +++++++++++++++++++ .../query/TransportValidateQueryAction.java | 4 +- .../org/opensearch/index/IndexService.java | 19 +++- .../index/query/QueryRewriteContext.java | 15 +++ .../index/query/QueryShardContext.java | 53 +++++++++- .../index/query/RangeQueryBuilder.java | 9 +- .../opensearch/indices/IndicesService.java | 16 ++- .../search/DefaultSearchContext.java | 6 +- .../org/opensearch/search/SearchService.java | 26 ++++- .../search/DefaultSearchContextTests.java | 27 ++++-- 10 files changed, 248 insertions(+), 24 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java b/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java index 51d0a439512..29845b39bec 100644 --- a/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java @@ -62,6 +62,7 @@ import java.util.List; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_NUMBER_OF_SHARDS; import static org.opensearch.index.query.QueryBuilders.queryStringQuery; +import static org.opensearch.index.query.QueryBuilders.rangeQuery; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertNoFailures; import static org.hamcrest.Matchers.allOf; @@ -500,4 +501,100 @@ public class SimpleValidateQueryIT extends OpenSearchIntegTestCase { .actionGet(); assertThat(response.isValid(), is(true)); } + + // Issue: https://github.com/opensearch-project/OpenSearch/issues/2036 + public void testValidateDateRangeInQueryString() throws IOException { + assertAcked(prepareCreate("test").setSettings(Settings.builder().put(indexSettings()).put("index.number_of_shards", 1))); + + assertAcked( + client().admin() + .indices() + .preparePutMapping("test") + .setSource( + XContentFactory.jsonBuilder() + .startObject() + .startObject(MapperService.SINGLE_MAPPING_NAME) + .startObject("properties") + .startObject("name") + .field("type", "keyword") + .endObject() + .startObject("timestamp") + .field("type", "date") + .endObject() + .endObject() + .endObject() + .endObject() + ) + ); + + client().prepareIndex("test").setId("1").setSource("name", "username", "timestamp", 200).get(); + refresh(); + + ValidateQueryResponse response = client().admin() + .indices() + .prepareValidateQuery() + .setQuery( + QueryBuilders.boolQuery() + .must(rangeQuery("timestamp").gte(0).lte(100)) + .must(queryStringQuery("username").allowLeadingWildcard(false)) + ) + .setRewrite(true) + .get(); + + assertNoFailures(response); + assertThat(response.isValid(), is(true)); + + // Use wildcard and date outside the range + response = client().admin() + .indices() + .prepareValidateQuery() + .setQuery( + QueryBuilders.boolQuery() + .must(rangeQuery("timestamp").gte(0).lte(100)) + .must(queryStringQuery("*erna*").allowLeadingWildcard(false)) + ) + .setRewrite(true) + .get(); + + assertNoFailures(response); + assertThat(response.isValid(), is(false)); + + // Use wildcard and date inside the range + response = client().admin() + .indices() + .prepareValidateQuery() + .setQuery( + QueryBuilders.boolQuery() + .must(rangeQuery("timestamp").gte(0).lte(1000)) + .must(queryStringQuery("*erna*").allowLeadingWildcard(false)) + ) + .setRewrite(true) + .get(); + + assertNoFailures(response); + assertThat(response.isValid(), is(false)); + + // Use wildcard and date inside the range (allow leading wildcard) + response = client().admin() + .indices() + .prepareValidateQuery() + .setQuery(QueryBuilders.boolQuery().must(rangeQuery("timestamp").gte(0).lte(1000)).must(queryStringQuery("*erna*"))) + .setRewrite(true) + .get(); + + assertNoFailures(response); + assertThat(response.isValid(), is(true)); + + // Use invalid date range + response = client().admin() + .indices() + .prepareValidateQuery() + .setQuery(QueryBuilders.boolQuery().must(rangeQuery("timestamp").gte("aaa").lte(100))) + .setRewrite(true) + .get(); + + assertNoFailures(response); + assertThat(response.isValid(), is(false)); + + } } diff --git a/server/src/main/java/org/opensearch/action/admin/indices/validate/query/TransportValidateQueryAction.java b/server/src/main/java/org/opensearch/action/admin/indices/validate/query/TransportValidateQueryAction.java index 1fb293b200e..1849b41ce70 100644 --- a/server/src/main/java/org/opensearch/action/admin/indices/validate/query/TransportValidateQueryAction.java +++ b/server/src/main/java/org/opensearch/action/admin/indices/validate/query/TransportValidateQueryAction.java @@ -131,7 +131,7 @@ public class TransportValidateQueryAction extends TransportBroadcastAction< if (request.query() == null) { rewriteListener.onResponse(request.query()); } else { - Rewriteable.rewriteAndFetch(request.query(), searchService.getRewriteContext(timeProvider), rewriteListener); + Rewriteable.rewriteAndFetch(request.query(), searchService.getValidationRewriteContext(timeProvider), rewriteListener); } } @@ -225,7 +225,7 @@ public class TransportValidateQueryAction extends TransportBroadcastAction< request.nowInMillis(), request.filteringAliases() ); - SearchContext searchContext = searchService.createSearchContext(shardSearchLocalRequest, SearchService.NO_TIMEOUT); + SearchContext searchContext = searchService.createValidationContext(shardSearchLocalRequest, SearchService.NO_TIMEOUT); try { ParsedQuery parsedQuery = searchContext.getQueryShardContext().toQuery(request.query()); searchContext.parsedQuery(parsedQuery); diff --git a/server/src/main/java/org/opensearch/index/IndexService.java b/server/src/main/java/org/opensearch/index/IndexService.java index 7c1033ecea3..1b301e85365 100644 --- a/server/src/main/java/org/opensearch/index/IndexService.java +++ b/server/src/main/java/org/opensearch/index/IndexService.java @@ -630,6 +630,22 @@ public class IndexService extends AbstractIndexComponent implements IndicesClust * {@link IndexReader}-specific optimizations, such as rewriting containing range queries. */ public QueryShardContext newQueryShardContext(int shardId, IndexSearcher searcher, LongSupplier nowInMillis, String clusterAlias) { + return newQueryShardContext(shardId, searcher, nowInMillis, clusterAlias, false); + } + + /** + * Creates a new QueryShardContext. + * + * Passing a {@code null} {@link IndexSearcher} will return a valid context, however it won't be able to make + * {@link IndexReader}-specific optimizations, such as rewriting containing range queries. + */ + public QueryShardContext newQueryShardContext( + int shardId, + IndexSearcher searcher, + LongSupplier nowInMillis, + String clusterAlias, + boolean validate + ) { final SearchIndexNameMatcher indexNameMatcher = new SearchIndexNameMatcher( index().getName(), clusterAlias, @@ -653,7 +669,8 @@ public class IndexService extends AbstractIndexComponent implements IndicesClust clusterAlias, indexNameMatcher, allowExpensiveQueries, - valuesSourceRegistry + valuesSourceRegistry, + validate ); } diff --git a/server/src/main/java/org/opensearch/index/query/QueryRewriteContext.java b/server/src/main/java/org/opensearch/index/query/QueryRewriteContext.java index ad1f02ce026..720ee077119 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryRewriteContext.java +++ b/server/src/main/java/org/opensearch/index/query/QueryRewriteContext.java @@ -52,6 +52,7 @@ public class QueryRewriteContext { protected final Client client; protected final LongSupplier nowInMillis; private final List>> asyncActions = new ArrayList<>(); + private final boolean validate; public QueryRewriteContext( NamedXContentRegistry xContentRegistry, @@ -59,11 +60,22 @@ public class QueryRewriteContext { Client client, LongSupplier nowInMillis ) { + this(xContentRegistry, writeableRegistry, client, nowInMillis, false); + } + + public QueryRewriteContext( + NamedXContentRegistry xContentRegistry, + NamedWriteableRegistry writeableRegistry, + Client client, + LongSupplier nowInMillis, + boolean validate + ) { this.xContentRegistry = xContentRegistry; this.writeableRegistry = writeableRegistry; this.client = client; this.nowInMillis = nowInMillis; + this.validate = validate; } /** @@ -140,4 +152,7 @@ public class QueryRewriteContext { } } + public boolean validate() { + return validate; + } } diff --git a/server/src/main/java/org/opensearch/index/query/QueryShardContext.java b/server/src/main/java/org/opensearch/index/query/QueryShardContext.java index f67feadde4b..bfc0490e507 100644 --- a/server/src/main/java/org/opensearch/index/query/QueryShardContext.java +++ b/server/src/main/java/org/opensearch/index/query/QueryShardContext.java @@ -132,6 +132,48 @@ public class QueryShardContext extends QueryRewriteContext { Predicate indexNameMatcher, BooleanSupplier allowExpensiveQueries, ValuesSourceRegistry valuesSourceRegistry + ) { + this( + shardId, + indexSettings, + bigArrays, + bitsetFilterCache, + indexFieldDataLookup, + mapperService, + similarityService, + scriptService, + xContentRegistry, + namedWriteableRegistry, + client, + searcher, + nowInMillis, + clusterAlias, + indexNameMatcher, + allowExpensiveQueries, + valuesSourceRegistry, + false + ); + } + + public QueryShardContext( + int shardId, + IndexSettings indexSettings, + BigArrays bigArrays, + BitsetFilterCache bitsetFilterCache, + TriFunction, IndexFieldData> indexFieldDataLookup, + MapperService mapperService, + SimilarityService similarityService, + ScriptService scriptService, + NamedXContentRegistry xContentRegistry, + NamedWriteableRegistry namedWriteableRegistry, + Client client, + IndexSearcher searcher, + LongSupplier nowInMillis, + String clusterAlias, + Predicate indexNameMatcher, + BooleanSupplier allowExpensiveQueries, + ValuesSourceRegistry valuesSourceRegistry, + boolean validate ) { this( shardId, @@ -153,7 +195,8 @@ public class QueryShardContext extends QueryRewriteContext { indexSettings.getIndex().getUUID() ), allowExpensiveQueries, - valuesSourceRegistry + valuesSourceRegistry, + validate ); } @@ -175,7 +218,8 @@ public class QueryShardContext extends QueryRewriteContext { source.indexNameMatcher, source.fullyQualifiedIndex, source.allowExpensiveQueries, - source.valuesSourceRegistry + source.valuesSourceRegistry, + source.validate() ); } @@ -196,9 +240,10 @@ public class QueryShardContext extends QueryRewriteContext { Predicate indexNameMatcher, Index fullyQualifiedIndex, BooleanSupplier allowExpensiveQueries, - ValuesSourceRegistry valuesSourceRegistry + ValuesSourceRegistry valuesSourceRegistry, + boolean validate ) { - super(xContentRegistry, namedWriteableRegistry, client, nowInMillis); + super(xContentRegistry, namedWriteableRegistry, client, nowInMillis, validate); this.shardId = shardId; this.similarityService = similarityService; this.mapperService = mapperService; diff --git a/server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java index 1c27946514a..80b792d7505 100644 --- a/server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java @@ -452,7 +452,7 @@ public class RangeQueryBuilder extends AbstractQueryBuilder i } DateMathParser dateMathParser = getForceDateParser(); - return fieldType.isFieldWithinQuery( + final MappedFieldType.Relation relation = fieldType.isFieldWithinQuery( shardContext.getIndexReader(), from, to, @@ -462,6 +462,13 @@ public class RangeQueryBuilder extends AbstractQueryBuilder i dateMathParser, queryRewriteContext ); + + // For validation, always assume that there is an intersection + if (relation == MappedFieldType.Relation.DISJOINT && shardContext.validate()) { + return MappedFieldType.Relation.INTERSECTS; + } + + return relation; } // Not on the shard, we have no way to know what the relation is. diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 22ab5a9cd9c..5caafb0ce60 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -1632,7 +1632,21 @@ public class IndicesService extends AbstractLifecycleComponent * Returns a new {@link QueryRewriteContext} with the given {@code now} provider */ public QueryRewriteContext getRewriteContext(LongSupplier nowInMillis) { - return new QueryRewriteContext(xContentRegistry, namedWriteableRegistry, client, nowInMillis); + return getRewriteContext(nowInMillis, false); + } + + /** + * Returns a new {@link QueryRewriteContext} for query validation with the given {@code now} provider + */ + public QueryRewriteContext getValidationRewriteContext(LongSupplier nowInMillis) { + return getRewriteContext(nowInMillis, true); + } + + /** + * Returns a new {@link QueryRewriteContext} with the given {@code now} provider + */ + private QueryRewriteContext getRewriteContext(LongSupplier nowInMillis, boolean validate) { + return new QueryRewriteContext(xContentRegistry, namedWriteableRegistry, client, nowInMillis, validate); } /** diff --git a/server/src/main/java/org/opensearch/search/DefaultSearchContext.java b/server/src/main/java/org/opensearch/search/DefaultSearchContext.java index 2df62d60786..a641f2e625e 100644 --- a/server/src/main/java/org/opensearch/search/DefaultSearchContext.java +++ b/server/src/main/java/org/opensearch/search/DefaultSearchContext.java @@ -176,7 +176,8 @@ final class DefaultSearchContext extends SearchContext { TimeValue timeout, FetchPhase fetchPhase, boolean lowLevelCancellation, - Version minNodeVersion + Version minNodeVersion, + boolean validate ) throws IOException { this.readerContext = readerContext; this.request = request; @@ -206,7 +207,8 @@ final class DefaultSearchContext extends SearchContext { request.shardId().id(), this.searcher, request::nowInMillis, - shardTarget.getClusterAlias() + shardTarget.getClusterAlias(), + validate ); queryBoost = request.indexBoost(); this.lowLevelCancellation = lowLevelCancellation; diff --git a/server/src/main/java/org/opensearch/search/SearchService.java b/server/src/main/java/org/opensearch/search/SearchService.java index eda91533810..de4586efd60 100644 --- a/server/src/main/java/org/opensearch/search/SearchService.java +++ b/server/src/main/java/org/opensearch/search/SearchService.java @@ -816,7 +816,7 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv SearchShardTask task, boolean includeAggregations ) throws IOException { - final DefaultSearchContext context = createSearchContext(readerContext, request, defaultSearchTimeout); + final DefaultSearchContext context = createSearchContext(readerContext, request, defaultSearchTimeout, false); try { if (request.scroll() != null) { context.scrollContext().scroll = request.scroll(); @@ -842,19 +842,27 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv return context; } - public DefaultSearchContext createSearchContext(ShardSearchRequest request, TimeValue timeout) throws IOException { + public DefaultSearchContext createSearchContext(ShardSearchRequest request, TimeValue timeout, boolean validate) throws IOException { final IndexService indexService = indicesService.indexServiceSafe(request.shardId().getIndex()); final IndexShard indexShard = indexService.getShard(request.shardId().getId()); final Engine.SearcherSupplier reader = indexShard.acquireSearcherSupplier(); final ShardSearchContextId id = new ShardSearchContextId(sessionId, idGenerator.incrementAndGet()); try (ReaderContext readerContext = new ReaderContext(id, indexService, indexShard, reader, -1L, true)) { - DefaultSearchContext searchContext = createSearchContext(readerContext, request, timeout); + DefaultSearchContext searchContext = createSearchContext(readerContext, request, timeout, validate); searchContext.addReleasable(readerContext.markAsUsed(0L)); return searchContext; } } - private DefaultSearchContext createSearchContext(ReaderContext reader, ShardSearchRequest request, TimeValue timeout) + public DefaultSearchContext createValidationContext(ShardSearchRequest request, TimeValue timeout) throws IOException { + return createSearchContext(request, timeout, true); + } + + public DefaultSearchContext createSearchContext(ShardSearchRequest request, TimeValue timeout) throws IOException { + return createSearchContext(request, timeout, false); + } + + private DefaultSearchContext createSearchContext(ReaderContext reader, ShardSearchRequest request, TimeValue timeout, boolean validate) throws IOException { boolean success = false; DefaultSearchContext searchContext = null; @@ -875,7 +883,8 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv timeout, fetchPhase, lowLevelCancellation, - clusterService.state().nodes().getMinNodeVersion() + clusterService.state().nodes().getMinNodeVersion(), + validate ); // we clone the query shard context here just for rewriting otherwise we // might end up with incorrect state since we are using now() or script services @@ -1349,6 +1358,13 @@ public class SearchService extends AbstractLifecycleComponent implements IndexEv return indicesService.getRewriteContext(nowInMillis); } + /** + * Returns a new {@link QueryRewriteContext} for query validation with the given {@code now} provider + */ + public QueryRewriteContext getValidationRewriteContext(LongSupplier nowInMillis) { + return indicesService.getValidationRewriteContext(nowInMillis); + } + public IndicesService getIndicesService() { return indicesService; } diff --git a/server/src/test/java/org/opensearch/search/DefaultSearchContextTests.java b/server/src/test/java/org/opensearch/search/DefaultSearchContextTests.java index 2bd8c697adb..48c4717f664 100644 --- a/server/src/test/java/org/opensearch/search/DefaultSearchContextTests.java +++ b/server/src/test/java/org/opensearch/search/DefaultSearchContextTests.java @@ -84,6 +84,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.mockito.Mockito.any; import static org.mockito.Mockito.anyString; +import static org.mockito.Mockito.anyBoolean; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.nullable; import static org.mockito.Mockito.mock; @@ -122,7 +123,9 @@ public class DefaultSearchContextTests extends OpenSearchTestCase { when(indexCache.query()).thenReturn(queryCache); when(indexService.cache()).thenReturn(indexCache); QueryShardContext queryShardContext = mock(QueryShardContext.class); - when(indexService.newQueryShardContext(eq(shardId.id()), any(), any(), nullable(String.class))).thenReturn(queryShardContext); + when(indexService.newQueryShardContext(eq(shardId.id()), any(), any(), nullable(String.class), anyBoolean())).thenReturn( + queryShardContext + ); MapperService mapperService = mock(MapperService.class); when(mapperService.hasNested()).thenReturn(randomBoolean()); when(indexService.mapperService()).thenReturn(mapperService); @@ -178,7 +181,8 @@ public class DefaultSearchContextTests extends OpenSearchTestCase { timeout, null, false, - Version.CURRENT + Version.CURRENT, + false ); contextWithoutScroll.from(300); contextWithoutScroll.close(); @@ -218,7 +222,8 @@ public class DefaultSearchContextTests extends OpenSearchTestCase { timeout, null, false, - Version.CURRENT + Version.CURRENT, + false ); context1.from(300); exception = expectThrows(IllegalArgumentException.class, () -> context1.preProcess(false)); @@ -286,7 +291,8 @@ public class DefaultSearchContextTests extends OpenSearchTestCase { timeout, null, false, - Version.CURRENT + Version.CURRENT, + false ); SliceBuilder sliceBuilder = mock(SliceBuilder.class); @@ -323,7 +329,8 @@ public class DefaultSearchContextTests extends OpenSearchTestCase { timeout, null, false, - Version.CURRENT + Version.CURRENT, + false ); ParsedQuery parsedQuery = ParsedQuery.parsedMatchAllQuery(); context3.sliceBuilder(null).parsedQuery(parsedQuery).preProcess(false); @@ -352,7 +359,8 @@ public class DefaultSearchContextTests extends OpenSearchTestCase { timeout, null, false, - Version.CURRENT + Version.CURRENT, + false ); context4.sliceBuilder(new SliceBuilder(1, 2)).parsedQuery(parsedQuery).preProcess(false); Query query1 = context4.query(); @@ -380,7 +388,9 @@ public class DefaultSearchContextTests extends OpenSearchTestCase { IndexService indexService = mock(IndexService.class); QueryShardContext queryShardContext = mock(QueryShardContext.class); - when(indexService.newQueryShardContext(eq(shardId.id()), any(), any(), nullable(String.class))).thenReturn(queryShardContext); + when(indexService.newQueryShardContext(eq(shardId.id()), any(), any(), nullable(String.class), anyBoolean())).thenReturn( + queryShardContext + ); BigArrays bigArrays = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService()); @@ -429,7 +439,8 @@ public class DefaultSearchContextTests extends OpenSearchTestCase { timeout, null, false, - Version.CURRENT + Version.CURRENT, + false ); assertThat(context.searcher().hasCancellations(), is(false)); context.searcher().addQueryCancellation(() -> {});