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 <andriy.redko@aiven.io>

* Moved the validate() check later into the flow to allow range validation to trigger first

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

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));
+
+    }
 }

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);

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
         );
     }
 

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<BiConsumer<Client, ActionListener<?>>> 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;
+    }
 }

server/src/main/java/org/opensearch/index/query/QueryShardContext.java

@@ -132,6 +132,48 @@ public class QueryShardContext extends QueryRewriteContext {
         Predicate<String> 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<MappedFieldType, String, Supplier<SearchLookup>, IndexFieldData<?>> indexFieldDataLookup,
+        MapperService mapperService,
+        SimilarityService similarityService,
+        ScriptService scriptService,
+        NamedXContentRegistry xContentRegistry,
+        NamedWriteableRegistry namedWriteableRegistry,
+        Client client,
+        IndexSearcher searcher,
+        LongSupplier nowInMillis,
+        String clusterAlias,
+        Predicate<String> 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<String> 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;

server/src/main/java/org/opensearch/index/query/RangeQueryBuilder.java

@@ -452,7 +452,7 @@ public class RangeQueryBuilder extends AbstractQueryBuilder<RangeQueryBuilder> 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<RangeQueryBuilder> 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.

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);
     }
 
     /**

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;

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;
     }

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(() -> {});