From d19081356a2ee658568a3127829ab1ee8772a6fb Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 14 Mar 2022 12:47:11 -0700 Subject: [PATCH 1/5] Bump woodstox-core from 6.1.1 to 6.2.8 in /plugins/repository-azure (#2456) * Bump woodstox-core from 6.1.1 to 6.2.8 in /plugins/repository-azure Bumps [woodstox-core](https://github.com/FasterXML/woodstox) from 6.1.1 to 6.2.8. - [Release notes](https://github.com/FasterXML/woodstox/releases) - [Commits](https://github.com/FasterXML/woodstox/compare/woodstox-core-6.1.1...woodstox-core-6.2.8) --- updated-dependencies: - dependency-name: com.fasterxml.woodstox:woodstox-core dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] * Updating SHAs Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] --- plugins/repository-azure/build.gradle | 2 +- plugins/repository-azure/licenses/woodstox-core-6.1.1.jar.sha1 | 1 - plugins/repository-azure/licenses/woodstox-core-6.2.8.jar.sha1 | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) delete mode 100644 plugins/repository-azure/licenses/woodstox-core-6.1.1.jar.sha1 create mode 100644 plugins/repository-azure/licenses/woodstox-core-6.2.8.jar.sha1 diff --git a/plugins/repository-azure/build.gradle b/plugins/repository-azure/build.gradle index c531cd390e7ee..3dc089ef8acb7 100644 --- a/plugins/repository-azure/build.gradle +++ b/plugins/repository-azure/build.gradle @@ -67,7 +67,7 @@ dependencies { api "com.fasterxml.jackson.dataformat:jackson-dataformat-xml:${versions.jackson}" api "com.fasterxml.jackson.module:jackson-module-jaxb-annotations:${versions.jackson}" api 'org.codehaus.woodstox:stax2-api:4.2.1' - implementation 'com.fasterxml.woodstox:woodstox-core:6.1.1' + implementation 'com.fasterxml.woodstox:woodstox-core:6.2.8' runtimeOnly 'com.google.guava:guava:31.1-jre' api 'org.apache.commons:commons-lang3:3.12.0' testImplementation project(':test:fixtures:azure-fixture') diff --git a/plugins/repository-azure/licenses/woodstox-core-6.1.1.jar.sha1 b/plugins/repository-azure/licenses/woodstox-core-6.1.1.jar.sha1 deleted file mode 100644 index f2ad1c80882d3..0000000000000 --- a/plugins/repository-azure/licenses/woodstox-core-6.1.1.jar.sha1 +++ /dev/null @@ -1 +0,0 @@ -989bb31963ed1758b95c7c4381a91592a9a8df61 \ No newline at end of file diff --git a/plugins/repository-azure/licenses/woodstox-core-6.2.8.jar.sha1 b/plugins/repository-azure/licenses/woodstox-core-6.2.8.jar.sha1 new file mode 100644 index 0000000000000..ae65cdebf26de --- /dev/null +++ b/plugins/repository-azure/licenses/woodstox-core-6.2.8.jar.sha1 @@ -0,0 +1 @@ +670748292899c53b1963730d9eb7f8ab71314e90 \ No newline at end of file From 5c0f9bc499c5c4a744d2f29fb5bd9eab4aabc004 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Mon, 14 Mar 2022 17:11:26 -0400 Subject: [PATCH 2/5] 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 51d0a4395127a..29845b39becf2 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 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 void testExplainTermsQueryWithLookup() throws Exception { .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 1fb293b200e51..1849b41ce707f 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 @@ protected void doExecute(Task task, ValidateQueryRequest request, ActionListener 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 @@ protected ShardValidateQueryResponse shardOperation(ShardValidateQueryRequest re 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 7c1033ecea3ad..1b301e85365ba 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 IndexSettings getIndexSettings() { * {@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 QueryShardContext newQueryShardContext(int shardId, IndexSearcher searche 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 ad1f02ce0265d..720ee077119d6 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 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 void onFailure(Exception e) { } } + 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 f67feadde4b41..bfc0490e507db 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 QueryShardContext( 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 QueryShardContext( indexSettings.getIndex().getUUID() ), allowExpensiveQueries, - valuesSourceRegistry + valuesSourceRegistry, + validate ); } @@ -175,7 +218,8 @@ public QueryShardContext(QueryShardContext source) { source.indexNameMatcher, source.fullyQualifiedIndex, source.allowExpensiveQueries, - source.valuesSourceRegistry + source.valuesSourceRegistry, + source.validate() ); } @@ -196,9 +240,10 @@ private QueryShardContext( 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 1c27946514a3d..80b792d750546 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 @@ protected MappedFieldType.Relation getRelation(QueryRewriteContext queryRewriteC } DateMathParser dateMathParser = getForceDateParser(); - return fieldType.isFieldWithinQuery( + final MappedFieldType.Relation relation = fieldType.isFieldWithinQuery( shardContext.getIndexReader(), from, to, @@ -462,6 +462,13 @@ protected MappedFieldType.Relation getRelation(QueryRewriteContext queryRewriteC 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 22ab5a9cd9c0b..5caafb0ce60d4 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 AliasFilter buildAliasFilter(ClusterState state, String index, Set context1.preProcess(false)); @@ -286,7 +291,8 @@ protected Engine.Searcher acquireSearcherInternal(String source) { timeout, null, false, - Version.CURRENT + Version.CURRENT, + false ); SliceBuilder sliceBuilder = mock(SliceBuilder.class); @@ -323,7 +329,8 @@ protected Engine.Searcher acquireSearcherInternal(String source) { timeout, null, false, - Version.CURRENT + Version.CURRENT, + false ); ParsedQuery parsedQuery = ParsedQuery.parsedMatchAllQuery(); context3.sliceBuilder(null).parsedQuery(parsedQuery).preProcess(false); @@ -352,7 +359,8 @@ protected Engine.Searcher acquireSearcherInternal(String source) { 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 void testClearQueryCancellationsOnClose() throws IOException { 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 @@ protected Engine.Searcher acquireSearcherInternal(String source) { timeout, null, false, - Version.CURRENT + Version.CURRENT, + false ); assertThat(context.searcher().hasCancellations(), is(false)); context.searcher().addQueryCancellation(() -> {}); From 2b68b14629ba8bb5ccb1db3ed3806f837b56df15 Mon Sep 17 00:00:00 2001 From: Suraj Singh <79435743+dreamer-89@users.noreply.github.com> Date: Mon, 14 Mar 2022 19:09:04 -0700 Subject: [PATCH 3/5] [Remove] Type from TermsLookUp (#2459) * [Remove] Type from TermsLookUp Signed-off-by: Suraj Singh * Fix unit test failure Signed-off-by: Suraj Singh --- .../search/150_rewrite_on_coordinator.yml | 4 +- .../search/query/SearchQueryIT.java | 36 ++++------- .../validate/SimpleValidateQueryIT.java | 2 +- .../index/query/TermsQueryBuilder.java | 4 -- .../org/opensearch/indices/TermsLookup.java | 62 ++++--------------- .../index/query/TermsQueryBuilderTests.java | 11 +--- .../opensearch/indices/TermsLookupTests.java | 55 ++-------------- 7 files changed, 32 insertions(+), 142 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml index be34e10ddcd74..77298cb4f61c3 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml @@ -39,7 +39,7 @@ search: rest_total_hits_as_int: true index: "search_index" - body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } } + body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "id": "1", "path": "followers"} } } } - do: indices.create: index: lookup_index @@ -64,7 +64,7 @@ search: rest_total_hits_as_int: true index: "search_index" - body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } } + body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "id": "1", "path": "followers"} } } } - match: { _shards.total: 5 } - match: { _shards.successful: 5 } diff --git a/server/src/internalClusterTest/java/org/opensearch/search/query/SearchQueryIT.java b/server/src/internalClusterTest/java/org/opensearch/search/query/SearchQueryIT.java index db87269c8ceae..c9bb746973226 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/query/SearchQueryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/query/SearchQueryIT.java @@ -1195,75 +1195,63 @@ public void testTermsLookupFilter() throws Exception { ); SearchResponse searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "1", "terms"))) + .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "1", "terms"))) .get(); assertHitCount(searchResponse, 2L); assertSearchHits(searchResponse, "1", "3"); // same as above, just on the _id... - searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("_id", new TermsLookup("lookup", "type", "1", "terms"))) - .get(); + searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("_id", new TermsLookup("lookup", "1", "terms"))).get(); assertHitCount(searchResponse, 2L); assertSearchHits(searchResponse, "1", "3"); // another search with same parameters... - searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "1", "terms"))) - .get(); + searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "1", "terms"))).get(); assertHitCount(searchResponse, 2L); assertSearchHits(searchResponse, "1", "3"); - searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "2", "terms"))) - .get(); + searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "2", "terms"))).get(); assertHitCount(searchResponse, 1L); assertFirstHit(searchResponse, hasId("2")); - searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "3", "terms"))) - .get(); + searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "3", "terms"))).get(); assertHitCount(searchResponse, 2L); assertSearchHits(searchResponse, "2", "4"); - searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "4", "terms"))) - .get(); + searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "4", "terms"))).get(); assertHitCount(searchResponse, 0L); searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "type", "1", "arr.term"))) + .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "1", "arr.term"))) .get(); assertHitCount(searchResponse, 2L); assertSearchHits(searchResponse, "1", "3"); searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "type", "2", "arr.term"))) + .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "2", "arr.term"))) .get(); assertHitCount(searchResponse, 1L); assertFirstHit(searchResponse, hasId("2")); searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "type", "3", "arr.term"))) + .setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "3", "arr.term"))) .get(); assertHitCount(searchResponse, 2L); assertSearchHits(searchResponse, "2", "4"); searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("not_exists", new TermsLookup("lookup2", "type", "3", "arr.term"))) + .setQuery(termsLookupQuery("not_exists", new TermsLookup("lookup2", "3", "arr.term"))) .get(); assertHitCount(searchResponse, 0L); // index "lookup" type "type" id "missing" document does not exist: ignore the lookup terms searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "missing", "terms"))) + .setQuery(termsLookupQuery("term", new TermsLookup("lookup", "missing", "terms"))) .get(); assertHitCount(searchResponse, 0L); // index "lookup3" type "type" has the source disabled: ignore the lookup terms - searchResponse = client().prepareSearch("test") - .setQuery(termsLookupQuery("term", new TermsLookup("lookup3", "type", "1", "terms"))) - .get(); + searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup3", "1", "terms"))).get(); assertHitCount(searchResponse, 0L); } diff --git a/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java b/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java index 29845b39becf2..30ab282bf3d44 100644 --- a/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java @@ -491,7 +491,7 @@ public void testExplainTermsQueryWithLookup() throws Exception { client().prepareIndex("twitter").setId("1").setSource("followers", new int[] { 1, 2, 3 }).get(); refresh(); - TermsQueryBuilder termsLookupQuery = QueryBuilders.termsLookupQuery("user", new TermsLookup("twitter", "_doc", "1", "followers")); + TermsQueryBuilder termsLookupQuery = QueryBuilders.termsLookupQuery("user", new TermsLookup("twitter", "1", "followers")); ValidateQueryResponse response = client().admin() .indices() .prepareValidateQuery("twitter") diff --git a/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java index e797730ac0dff..ac29cb2cf5201 100644 --- a/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java @@ -225,10 +225,6 @@ public TermsLookup termsLookup() { return this.termsLookup; } - public boolean isTypeless() { - return termsLookup == null || termsLookup.type() == null; - } - private static final Set> INTEGER_TYPES = new HashSet<>( Arrays.asList(Byte.class, Short.class, Integer.class, Long.class) ); diff --git a/server/src/main/java/org/opensearch/indices/TermsLookup.java b/server/src/main/java/org/opensearch/indices/TermsLookup.java index 1aa16ad5cd72c..bf6c024fa130e 100644 --- a/server/src/main/java/org/opensearch/indices/TermsLookup.java +++ b/server/src/main/java/org/opensearch/indices/TermsLookup.java @@ -32,8 +32,7 @@ package org.opensearch.indices; -import org.opensearch.LegacyESVersion; -import org.opensearch.common.Nullable; +import org.opensearch.Version; import org.opensearch.common.ParseField; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; @@ -42,13 +41,13 @@ import org.opensearch.common.xcontent.ToXContentFragment; import org.opensearch.common.xcontent.XContentBuilder; import org.opensearch.common.xcontent.XContentParser; +import org.opensearch.index.mapper.MapperService; import org.opensearch.index.query.TermsQueryBuilder; import java.io.IOException; import java.util.Objects; import static org.opensearch.common.xcontent.ConstructingObjectParser.constructorArg; -import static org.opensearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg; /** * Encapsulates the parameters needed to fetch terms. @@ -56,20 +55,11 @@ public class TermsLookup implements Writeable, ToXContentFragment { private final String index; - private @Nullable String type; private final String id; private final String path; private String routing; public TermsLookup(String index, String id, String path) { - this(index, null, id, path); - } - - /** - * @deprecated Types are in the process of being removed, use {@link TermsLookup(String, String, String)} instead. - */ - @Deprecated - public TermsLookup(String index, String type, String id, String path) { if (id == null) { throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the id."); } @@ -80,7 +70,6 @@ public TermsLookup(String index, String type, String id, String path) { throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the index."); } this.index = index; - this.type = type; this.id = id; this.path = path; } @@ -89,11 +78,8 @@ public TermsLookup(String index, String type, String id, String path) { * Read from a stream. */ public TermsLookup(StreamInput in) throws IOException { - if (in.getVersion().onOrAfter(LegacyESVersion.V_7_0_0)) { - type = in.readOptionalString(); - } else { - // Before 7.0, the type parameter was always non-null and serialized as a (non-optional) string. - type = in.readString(); + if (in.getVersion().before(Version.V_2_0_0)) { + in.readOptionalString(); } id = in.readString(); path = in.readString(); @@ -103,16 +89,8 @@ public TermsLookup(StreamInput in) throws IOException { @Override public void writeTo(StreamOutput out) throws IOException { - if (out.getVersion().onOrAfter(LegacyESVersion.V_7_0_0)) { - out.writeOptionalString(type); - } else { - if (type == null) { - throw new IllegalArgumentException( - "Typeless [terms] lookup queries are not supported if any " + "node is running a version before 7.0." - ); - - } - out.writeString(type); + if (out.getVersion().before(Version.V_2_0_0)) { + out.writeOptionalString(MapperService.SINGLE_MAPPING_NAME); } out.writeString(id); out.writeString(path); @@ -124,14 +102,6 @@ public String index() { return index; } - /** - * @deprecated Types are in the process of being removed. - */ - @Deprecated - public String type() { - return type; - } - public String id() { return id; } @@ -151,14 +121,12 @@ public TermsLookup routing(String routing) { private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>("terms_lookup", args -> { String index = (String) args[0]; - String type = (String) args[1]; - String id = (String) args[2]; - String path = (String) args[3]; - return new TermsLookup(index, type, id, path); + String id = (String) args[1]; + String path = (String) args[2]; + return new TermsLookup(index, id, path); }); static { PARSER.declareString(constructorArg(), new ParseField("index")); - PARSER.declareString(optionalConstructorArg(), new ParseField("type").withAllDeprecated()); PARSER.declareString(constructorArg(), new ParseField("id")); PARSER.declareString(constructorArg(), new ParseField("path")); PARSER.declareString(TermsLookup::routing, new ParseField("routing")); @@ -170,19 +138,12 @@ public static TermsLookup parseTermsLookup(XContentParser parser) throws IOExcep @Override public String toString() { - if (type == null) { - return index + "/" + id + "/" + path; - } else { - return index + "/" + type + "/" + id + "/" + path; - } + return index + "/" + id + "/" + path; } @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.field("index", index); - if (type != null) { - builder.field("type", type); - } builder.field("id", id); builder.field("path", path); if (routing != null) { @@ -193,7 +154,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws @Override public int hashCode() { - return Objects.hash(index, type, id, path, routing); + return Objects.hash(index, id, path, routing); } @Override @@ -206,7 +167,6 @@ public boolean equals(Object obj) { } TermsLookup other = (TermsLookup) obj; return Objects.equals(index, other.index) - && Objects.equals(type, other.type) && Objects.equals(id, other.id) && Objects.equals(path, other.path) && Objects.equals(routing, other.routing); diff --git a/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java index e37b4f1a1c39f..ea93d7a65b951 100644 --- a/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java @@ -119,9 +119,7 @@ protected TermsQueryBuilder doCreateTestQueryBuilder() { private TermsLookup randomTermsLookup() { // Randomly choose between a typeless terms lookup and one with an explicit type to make sure we are - TermsLookup lookup = maybeIncludeType && randomBoolean() - ? new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath) - : new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath); + TermsLookup lookup = new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath); // testing both cases. lookup.routing(randomBoolean() ? randomAlphaOfLength(10) : null); return lookup; @@ -379,13 +377,6 @@ protected QueryBuilder parseQuery(XContentParser parser) throws IOException { try { QueryBuilder query = super.parseQuery(parser); assertThat(query, CoreMatchers.instanceOf(TermsQueryBuilder.class)); - - TermsQueryBuilder termsQuery = (TermsQueryBuilder) query; - String deprecationWarning = "Deprecated field [type] used, this field is unused and will be removed entirely"; - if (termsQuery.isTypeless() == false && !assertedWarnings.contains(deprecationWarning)) { - assertWarnings(deprecationWarning); - assertedWarnings.add(deprecationWarning); - } return query; } finally { diff --git a/server/src/test/java/org/opensearch/indices/TermsLookupTests.java b/server/src/test/java/org/opensearch/indices/TermsLookupTests.java index fb1462b500ea9..661995a22c507 100644 --- a/server/src/test/java/org/opensearch/indices/TermsLookupTests.java +++ b/server/src/test/java/org/opensearch/indices/TermsLookupTests.java @@ -45,42 +45,36 @@ public class TermsLookupTests extends OpenSearchTestCase { public void testTermsLookup() { String index = randomAlphaOfLengthBetween(1, 10); - String type = randomAlphaOfLengthBetween(1, 10); String id = randomAlphaOfLengthBetween(1, 10); String path = randomAlphaOfLengthBetween(1, 10); String routing = randomAlphaOfLengthBetween(1, 10); - TermsLookup termsLookup = new TermsLookup(index, type, id, path); + TermsLookup termsLookup = new TermsLookup(index, id, path); termsLookup.routing(routing); assertEquals(index, termsLookup.index()); - assertEquals(type, termsLookup.type()); assertEquals(id, termsLookup.id()); assertEquals(path, termsLookup.path()); assertEquals(routing, termsLookup.routing()); } public void testIllegalArguments() { - String type = randomAlphaOfLength(5); String id = randomAlphaOfLength(5); String path = randomAlphaOfLength(5); String index = randomAlphaOfLength(5); - switch (randomIntBetween(0, 3)) { + switch (randomIntBetween(0, 2)) { case 0: - type = null; - break; - case 1: id = null; break; - case 2: + case 1: path = null; break; - case 3: + case 2: index = null; break; default: fail("unknown case"); } try { - new TermsLookup(index, type, id, path); + new TermsLookup(index, id, path); } catch (IllegalArgumentException e) { assertThat(e.getMessage(), containsString("[terms] query lookup element requires specifying")); } @@ -99,35 +93,6 @@ public void testSerialization() throws IOException { } } - public void testSerializationWithTypes() throws IOException { - TermsLookup termsLookup = randomTermsLookupWithTypes(); - try (BytesStreamOutput output = new BytesStreamOutput()) { - termsLookup.writeTo(output); - try (StreamInput in = output.bytes().streamInput()) { - TermsLookup deserializedLookup = new TermsLookup(in); - assertEquals(deserializedLookup, termsLookup); - assertEquals(deserializedLookup.hashCode(), termsLookup.hashCode()); - assertNotSame(deserializedLookup, termsLookup); - } - } - } - - public void testXContentParsingWithType() throws IOException { - XContentParser parser = createParser( - JsonXContent.jsonXContent, - "{ \"index\" : \"index\", \"id\" : \"id\", \"type\" : \"type\", \"path\" : \"path\", \"routing\" : \"routing\" }" - ); - - TermsLookup tl = TermsLookup.parseTermsLookup(parser); - assertEquals("index", tl.index()); - assertEquals("type", tl.type()); - assertEquals("id", tl.id()); - assertEquals("path", tl.path()); - assertEquals("routing", tl.routing()); - - assertWarnings("Deprecated field [type] used, this field is unused and will be removed entirely"); - } - public void testXContentParsing() throws IOException { XContentParser parser = createParser( JsonXContent.jsonXContent, @@ -136,7 +101,6 @@ public void testXContentParsing() throws IOException { TermsLookup tl = TermsLookup.parseTermsLookup(parser); assertEquals("index", tl.index()); - assertNull(tl.type()); assertEquals("id", tl.id()); assertEquals("path", tl.path()); assertEquals("routing", tl.routing()); @@ -147,13 +111,4 @@ public static TermsLookup randomTermsLookup() { randomBoolean() ? randomAlphaOfLength(10) : null ); } - - public static TermsLookup randomTermsLookupWithTypes() { - return new TermsLookup( - randomAlphaOfLength(10), - randomAlphaOfLength(10), - randomAlphaOfLength(10), - randomAlphaOfLength(10).replace('.', '_') - ).routing(randomBoolean() ? randomAlphaOfLength(10) : null); - } } From 02d000c514c6bab875d2985a2a77455a81576b41 Mon Sep 17 00:00:00 2001 From: Suraj Singh <79435743+dreamer-89@users.noreply.github.com> Date: Mon, 14 Mar 2022 20:46:23 -0700 Subject: [PATCH 4/5] [Remove] Type query (#2448) Signed-off-by: Suraj Singh --- .../index/mapper/DocumentMapper.java | 8 - .../index/query/TypeQueryBuilder.java | 158 ------------------ .../org/opensearch/search/SearchModule.java | 2 - .../index/query/TypeQueryBuilderTests.java | 90 ---------- .../opensearch/search/SearchModuleTests.java | 1 - 5 files changed, 259 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/index/query/TypeQueryBuilder.java delete mode 100644 server/src/test/java/org/opensearch/index/query/TypeQueryBuilderTests.java diff --git a/server/src/main/java/org/opensearch/index/mapper/DocumentMapper.java b/server/src/main/java/org/opensearch/index/mapper/DocumentMapper.java index 37e740ec33321..0ee0a3cb9a180 100644 --- a/server/src/main/java/org/opensearch/index/mapper/DocumentMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/DocumentMapper.java @@ -208,14 +208,6 @@ public T metadataMapper(Class type) { return mapping.metadataMapper(type); } - public IndexFieldMapper indexMapper() { - return metadataMapper(IndexFieldMapper.class); - } - - public TypeFieldMapper typeMapper() { - return metadataMapper(TypeFieldMapper.class); - } - public SourceFieldMapper sourceMapper() { return metadataMapper(SourceFieldMapper.class); } diff --git a/server/src/main/java/org/opensearch/index/query/TypeQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/TypeQueryBuilder.java deleted file mode 100644 index d1ffcb394ec06..0000000000000 --- a/server/src/main/java/org/opensearch/index/query/TypeQueryBuilder.java +++ /dev/null @@ -1,158 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -/* - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - -package org.opensearch.index.query; - -import org.apache.lucene.search.MatchNoDocsQuery; -import org.apache.lucene.search.Query; -import org.opensearch.common.ParseField; -import org.opensearch.common.ParsingException; -import org.opensearch.common.io.stream.StreamInput; -import org.opensearch.common.io.stream.StreamOutput; -import org.opensearch.common.logging.DeprecationLogger; -import org.opensearch.common.lucene.search.Queries; -import org.opensearch.common.xcontent.XContentBuilder; -import org.opensearch.common.xcontent.XContentParser; -import org.opensearch.index.mapper.DocumentMapper; - -import java.io.IOException; -import java.util.Objects; - -public class TypeQueryBuilder extends AbstractQueryBuilder { - public static final String NAME = "type"; - - private static final ParseField VALUE_FIELD = new ParseField("value"); - private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(TypeQueryBuilder.class); - static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Type queries are deprecated, " - + "prefer to filter on a field instead."; - - private final String type; - - public TypeQueryBuilder(String type) { - if (type == null) { - throw new IllegalArgumentException("[type] cannot be null"); - } - this.type = type; - } - - /** - * Read from a stream. - */ - public TypeQueryBuilder(StreamInput in) throws IOException { - super(in); - type = in.readString(); - } - - @Override - protected void doWriteTo(StreamOutput out) throws IOException { - out.writeString(type); - } - - public String type() { - return type; - } - - @Override - protected void doXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(NAME); - builder.field(VALUE_FIELD.getPreferredName(), type); - printBoostAndQueryName(builder); - builder.endObject(); - } - - public static TypeQueryBuilder fromXContent(XContentParser parser) throws IOException { - String type = null; - String queryName = null; - float boost = AbstractQueryBuilder.DEFAULT_BOOST; - String currentFieldName = null; - XContentParser.Token token; - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (token.isValue()) { - if (AbstractQueryBuilder.NAME_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - queryName = parser.text(); - } else if (AbstractQueryBuilder.BOOST_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - boost = parser.floatValue(); - } else if (VALUE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - type = parser.text(); - } else { - throw new ParsingException( - parser.getTokenLocation(), - "[" + TypeQueryBuilder.NAME + "] filter doesn't support [" + currentFieldName + "]" - ); - } - } else { - throw new ParsingException( - parser.getTokenLocation(), - "[" + TypeQueryBuilder.NAME + "] filter doesn't support [" + currentFieldName + "]" - ); - } - } - - if (type == null) { - throw new ParsingException( - parser.getTokenLocation(), - "[" + TypeQueryBuilder.NAME + "] filter needs to be provided with a value for the type" - ); - } - return new TypeQueryBuilder(type).boost(boost).queryName(queryName); - } - - @Override - public String getWriteableName() { - return NAME; - } - - @Override - protected Query doToQuery(QueryShardContext context) throws IOException { - deprecationLogger.deprecate("type_query", TYPES_DEPRECATION_MESSAGE); - // LUCENE 4 UPGRADE document mapper should use bytesref as well? - DocumentMapper documentMapper = context.getMapperService().documentMapper(); - if (documentMapper == null) { - // no type means no documents - return new MatchNoDocsQuery(); - } else { - return Queries.newNonNestedFilter(context.indexVersionCreated()); - } - } - - @Override - protected int doHashCode() { - return Objects.hash(type); - } - - @Override - protected boolean doEquals(TypeQueryBuilder other) { - return Objects.equals(type, other.type); - } -} diff --git a/server/src/main/java/org/opensearch/search/SearchModule.java b/server/src/main/java/org/opensearch/search/SearchModule.java index cdc2509bbcb00..c052f7f89e14e 100644 --- a/server/src/main/java/org/opensearch/search/SearchModule.java +++ b/server/src/main/java/org/opensearch/search/SearchModule.java @@ -89,7 +89,6 @@ import org.opensearch.index.query.TermQueryBuilder; import org.opensearch.index.query.TermsQueryBuilder; import org.opensearch.index.query.TermsSetQueryBuilder; -import org.opensearch.index.query.TypeQueryBuilder; import org.opensearch.index.query.WildcardQueryBuilder; import org.opensearch.index.query.WrapperQueryBuilder; import org.opensearch.index.query.functionscore.ExponentialDecayFunctionBuilder; @@ -1183,7 +1182,6 @@ private void registerQueryParsers(List plugins) { registerQuery( new QuerySpec<>(SimpleQueryStringBuilder.NAME, SimpleQueryStringBuilder::new, SimpleQueryStringBuilder::fromXContent) ); - registerQuery(new QuerySpec<>(TypeQueryBuilder.NAME, TypeQueryBuilder::new, TypeQueryBuilder::fromXContent)); registerQuery(new QuerySpec<>(ScriptQueryBuilder.NAME, ScriptQueryBuilder::new, ScriptQueryBuilder::fromXContent)); registerQuery(new QuerySpec<>(GeoDistanceQueryBuilder.NAME, GeoDistanceQueryBuilder::new, GeoDistanceQueryBuilder::fromXContent)); registerQuery( diff --git a/server/src/test/java/org/opensearch/index/query/TypeQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/TypeQueryBuilderTests.java deleted file mode 100644 index bf373ac180f04..0000000000000 --- a/server/src/test/java/org/opensearch/index/query/TypeQueryBuilderTests.java +++ /dev/null @@ -1,90 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -/* - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - -package org.opensearch.index.query; - -import org.apache.lucene.search.MatchNoDocsQuery; -import org.apache.lucene.search.Query; -import org.opensearch.common.lucene.search.Queries; -import org.opensearch.test.AbstractQueryTestCase; - -import java.io.IOException; - -import static org.hamcrest.Matchers.equalTo; - -public class TypeQueryBuilderTests extends AbstractQueryTestCase { - - @Override - protected TypeQueryBuilder doCreateTestQueryBuilder() { - return new TypeQueryBuilder("_doc"); - } - - @Override - protected void doAssertLuceneQuery(TypeQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException { - if (createShardContext().getMapperService().documentMapper() == null) { - assertEquals(new MatchNoDocsQuery(), query); - } else { - assertThat(query, equalTo(Queries.newNonNestedFilter(context.indexVersionCreated()))); - } - } - - public void testIllegalArgument() { - expectThrows(IllegalArgumentException.class, () -> new TypeQueryBuilder((String) null)); - } - - public void testFromJson() throws IOException { - String json = "{\n" + " \"type\" : {\n" + " \"value\" : \"my_type\",\n" + " \"boost\" : 1.0\n" + " }\n" + "}"; - - TypeQueryBuilder parsed = (TypeQueryBuilder) parseQuery(json); - checkGeneratedJson(json, parsed); - - assertEquals(json, "my_type", parsed.type()); - } - - @Override - public void testToQuery() throws IOException { - super.testToQuery(); - assertWarnings(TypeQueryBuilder.TYPES_DEPRECATION_MESSAGE); - } - - @Override - public void testMustRewrite() throws IOException { - super.testMustRewrite(); - assertWarnings(TypeQueryBuilder.TYPES_DEPRECATION_MESSAGE); - } - - @Override - public void testCacheability() throws IOException { - super.testCacheability(); - assertWarnings(TypeQueryBuilder.TYPES_DEPRECATION_MESSAGE); - } -} diff --git a/server/src/test/java/org/opensearch/search/SearchModuleTests.java b/server/src/test/java/org/opensearch/search/SearchModuleTests.java index 19b61275b8f62..05d4153949f9a 100644 --- a/server/src/test/java/org/opensearch/search/SearchModuleTests.java +++ b/server/src/test/java/org/opensearch/search/SearchModuleTests.java @@ -459,7 +459,6 @@ public List> getRescorers() { "term", "terms", "terms_set", - "type", "wildcard", "wrapper", "distance_feature" }; From 7df40ee1b098014cf3ef817acae303263ddc917f Mon Sep 17 00:00:00 2001 From: Nick Knize Date: Tue, 15 Mar 2022 01:05:58 -0500 Subject: [PATCH 5/5] [Remove] type from TaskResults index and IndexMetadata.getMappings (#2469) Removes types from the TaskResults internal index along with the getMappings method from IndexMetadata. This is needed to further remove types from CreateIndexRequest. Signed-off-by: Nicholas Walter Knize --- .../admin/cluster/node/tasks/TasksIT.java | 5 ++ .../gateway/GatewayIndexStateIT.java | 19 ++---- .../opensearch/gateway/MetadataNodesIT.java | 10 +-- .../opensearch/action/index/IndexRequest.java | 2 +- .../cluster/metadata/IndexMetadata.java | 34 +++++----- .../cluster/metadata/MappingMetadata.java | 62 +++++-------------- .../opensearch/cluster/metadata/Metadata.java | 2 +- .../index/mapper/MapperService.java | 5 +- .../opensearch/index/shard/StoreRecovery.java | 5 +- .../opensearch/tasks/TaskResultsService.java | 8 +-- .../opensearch/tasks/task-index-mapping.json | 2 +- .../metadata/MetadataMappingServiceTests.java | 2 +- 12 files changed, 50 insertions(+), 106 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/node/tasks/TasksIT.java b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/node/tasks/TasksIT.java index e1346492999be..fbac2f7dbff6e 100644 --- a/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/node/tasks/TasksIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/node/tasks/TasksIT.java @@ -840,6 +840,11 @@ public void testTaskStoringSuccessfulResult() throws Exception { GetTaskResponse getResponse = expectFinishedTask(taskId); assertEquals(result, getResponse.getTask().getResponseAsMap()); assertNull(getResponse.getTask().getError()); + + // run it again to check that the tasks index has been successfully created and can be re-used + client().execute(TestTaskPlugin.TestTaskAction.INSTANCE, request).get(); + events = findEvents(TestTaskPlugin.TestTaskAction.NAME, Tuple::v1); + assertEquals(2, events.size()); } public void testTaskStoringFailureResult() throws Exception { diff --git a/server/src/internalClusterTest/java/org/opensearch/gateway/GatewayIndexStateIT.java b/server/src/internalClusterTest/java/org/opensearch/gateway/GatewayIndexStateIT.java index 6fe22e2a8fde4..2138e24cc9b4c 100644 --- a/server/src/internalClusterTest/java/org/opensearch/gateway/GatewayIndexStateIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/gateway/GatewayIndexStateIT.java @@ -60,7 +60,6 @@ import org.opensearch.common.xcontent.XContentType; import org.opensearch.env.NodeEnvironment; import org.opensearch.index.mapper.MapperParsingException; -import org.opensearch.index.mapper.MapperService; import org.opensearch.indices.IndexClosedException; import org.opensearch.indices.ShardLimitValidator; import org.opensearch.test.OpenSearchIntegTestCase; @@ -123,9 +122,8 @@ public void testMappingMetadataParsed() throws Exception { .getState() .metadata() .index("test") - .getMappings() - .get(MapperService.SINGLE_MAPPING_NAME); - assertThat(mappingMd.routing().required(), equalTo(true)); + .mapping(); + assertThat(mappingMd.routingRequired(), equalTo(true)); logger.info("--> restarting nodes..."); internalCluster().fullRestart(); @@ -134,17 +132,8 @@ public void testMappingMetadataParsed() throws Exception { ensureYellow(); logger.info("--> verify meta _routing required exists"); - mappingMd = client().admin() - .cluster() - .prepareState() - .execute() - .actionGet() - .getState() - .metadata() - .index("test") - .getMappings() - .get(MapperService.SINGLE_MAPPING_NAME); - assertThat(mappingMd.routing().required(), equalTo(true)); + mappingMd = client().admin().cluster().prepareState().execute().actionGet().getState().metadata().index("test").mapping(); + assertThat(mappingMd.routingRequired(), equalTo(true)); } public void testSimpleOpenClose() throws Exception { diff --git a/server/src/internalClusterTest/java/org/opensearch/gateway/MetadataNodesIT.java b/server/src/internalClusterTest/java/org/opensearch/gateway/MetadataNodesIT.java index 2731eb9a290d6..c9807aa24e259 100644 --- a/server/src/internalClusterTest/java/org/opensearch/gateway/MetadataNodesIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/gateway/MetadataNodesIT.java @@ -153,11 +153,7 @@ public void testMetaWrittenWhenIndexIsClosedAndMetaUpdated() throws Exception { // make sure it was also written on red node although index is closed ImmutableOpenMap indicesMetadata = getIndicesMetadataOnNode(dataNode); - assertNotNull( - ((Map) (indicesMetadata.get(index).getMappings().get("_doc").getSourceAsMap().get("properties"))).get( - "integer_field" - ) - ); + assertNotNull(((Map) (indicesMetadata.get(index).mapping().getSourceAsMap().get("properties"))).get("integer_field")); assertThat(indicesMetadata.get(index).getState(), equalTo(IndexMetadata.State.CLOSE)); /* Try the same and see if this also works if node was just restarted. @@ -190,9 +186,7 @@ public void testMetaWrittenWhenIndexIsClosedAndMetaUpdated() throws Exception { // make sure it was also written on red node although index is closed indicesMetadata = getIndicesMetadataOnNode(dataNode); - assertNotNull( - ((Map) (indicesMetadata.get(index).getMappings().get("_doc").getSourceAsMap().get("properties"))).get("float_field") - ); + assertNotNull(((Map) (indicesMetadata.get(index).mapping().getSourceAsMap().get("properties"))).get("float_field")); assertThat(indicesMetadata.get(index).getState(), equalTo(IndexMetadata.State.CLOSE)); // finally check that meta data is also written of index opened again diff --git a/server/src/main/java/org/opensearch/action/index/IndexRequest.java b/server/src/main/java/org/opensearch/action/index/IndexRequest.java index ed77774bc01d3..7bf6b876fa652 100644 --- a/server/src/main/java/org/opensearch/action/index/IndexRequest.java +++ b/server/src/main/java/org/opensearch/action/index/IndexRequest.java @@ -615,7 +615,7 @@ public VersionType versionType() { public void process(Version indexCreatedVersion, @Nullable MappingMetadata mappingMd, String concreteIndex) { if (mappingMd != null) { // might as well check for routing here - if (mappingMd.routing().required() && routing == null) { + if (mappingMd.routingRequired() && routing == null) { throw new RoutingMissingException(concreteIndex, id); } } diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index a7f351a918ae5..6510c57060fe0 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -660,17 +660,6 @@ public ImmutableOpenMap getAliases() { return this.aliases; } - /** - * Return an object that maps each type to the associated mappings. - * The return value is never {@code null} but may be empty if the index - * has no mappings. - * @deprecated Use {@link #mapping()} instead now that indices have a single type - */ - @Deprecated - public ImmutableOpenMap getMappings() { - return mappings; - } - /** * Return the concrete mapping for this index or {@code null} if this index has no mappings at all. */ @@ -1175,7 +1164,10 @@ public Builder putMapping(String source) throws IOException { } public Builder putMapping(MappingMetadata mappingMd) { - mappings.put(mappingMd.type(), mappingMd); + mappings.clear(); + if (mappingMd != null) { + mappings.put(mappingMd.type(), mappingMd); + } return this; } @@ -1464,23 +1456,25 @@ public static void toXContent(IndexMetadata indexMetadata, XContentBuilder build if (context != Metadata.XContentContext.API) { builder.startArray(KEY_MAPPINGS); - for (ObjectObjectCursor cursor : indexMetadata.getMappings()) { + MappingMetadata mmd = indexMetadata.mapping(); + if (mmd != null) { if (binary) { - builder.value(cursor.value.source().compressed()); + builder.value(mmd.source().compressed()); } else { - builder.map(XContentHelper.convertToMap(cursor.value.source().uncompressed(), true).v2()); + builder.map(XContentHelper.convertToMap(mmd.source().uncompressed(), true).v2()); } } builder.endArray(); } else { builder.startObject(KEY_MAPPINGS); - for (ObjectObjectCursor cursor : indexMetadata.getMappings()) { - Map mapping = XContentHelper.convertToMap(cursor.value.source().uncompressed(), false).v2(); - if (mapping.size() == 1 && mapping.containsKey(cursor.key)) { + MappingMetadata mmd = indexMetadata.mapping(); + if (mmd != null) { + Map mapping = XContentHelper.convertToMap(mmd.source().uncompressed(), false).v2(); + if (mapping.size() == 1 && mapping.containsKey(mmd.type())) { // the type name is the root value, reduce it - mapping = (Map) mapping.get(cursor.key); + mapping = (Map) mapping.get(mmd.type()); } - builder.field(cursor.key); + builder.field(mmd.type()); builder.map(mapping); } builder.endObject(); diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.java index 66bca027d7cc4..620542f8f1bde 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.java @@ -50,6 +50,7 @@ import java.io.UncheckedIOException; import java.util.Collections; import java.util.Map; +import java.util.Objects; import static org.opensearch.common.xcontent.support.XContentMapValues.nodeBooleanValue; @@ -59,46 +60,16 @@ public class MappingMetadata extends AbstractDiffable { public static final MappingMetadata EMPTY_MAPPINGS = new MappingMetadata(MapperService.SINGLE_MAPPING_NAME, Collections.emptyMap()); - public static class Routing { - - public static final Routing EMPTY = new Routing(false); - - private final boolean required; - - public Routing(boolean required) { - this.required = required; - } - - public boolean required() { - return required; - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - - Routing routing = (Routing) o; - - return required == routing.required; - } - - @Override - public int hashCode() { - return getClass().hashCode() + (required ? 1 : 0); - } - } - private final String type; private final CompressedXContent source; - private final Routing routing; + private final boolean routingRequired; public MappingMetadata(DocumentMapper docMapper) { this.type = docMapper.type(); this.source = docMapper.mappingSource(); - this.routing = new Routing(docMapper.routingFieldMapper().required()); + this.routingRequired = docMapper.routingFieldMapper().required(); } @SuppressWarnings("unchecked") @@ -109,7 +80,7 @@ public MappingMetadata(CompressedXContent mapping) { throw new IllegalStateException("Can't derive type from mapping, no root type: " + mapping.string()); } this.type = mappingMap.keySet().iterator().next(); - this.routing = initRouting((Map) mappingMap.get(this.type)); + this.routingRequired = isRoutingRequired((Map) mappingMap.get(this.type)); } @SuppressWarnings("unchecked") @@ -125,13 +96,13 @@ public MappingMetadata(String type, Map mapping) { if (mapping.size() == 1 && mapping.containsKey(type)) { withoutType = (Map) mapping.get(type); } - this.routing = initRouting(withoutType); + this.routingRequired = isRoutingRequired(withoutType); } @SuppressWarnings("unchecked") - private Routing initRouting(Map withoutType) { + private boolean isRoutingRequired(Map withoutType) { + boolean required = false; if (withoutType.containsKey("_routing")) { - boolean required = false; Map routingNode = (Map) withoutType.get("_routing"); for (Map.Entry entry : routingNode.entrySet()) { String fieldName = entry.getKey(); @@ -147,10 +118,8 @@ private Routing initRouting(Map withoutType) { } } } - return new Routing(required); - } else { - return Routing.EMPTY; } + return required; } public String type() { @@ -180,8 +149,8 @@ public Map getSourceAsMap() throws OpenSearchParseException { return sourceAsMap(); } - public Routing routing() { - return this.routing; + public boolean routingRequired() { + return this.routingRequired; } @Override @@ -189,7 +158,7 @@ public void writeTo(StreamOutput out) throws IOException { out.writeString(type()); source().writeTo(out); // routing - out.writeBoolean(routing().required()); + out.writeBoolean(routingRequired); if (out.getVersion().before(LegacyESVersion.V_7_0_0)) { out.writeBoolean(false); // hasParentField } @@ -202,7 +171,7 @@ public boolean equals(Object o) { MappingMetadata that = (MappingMetadata) o; - if (!routing.equals(that.routing)) return false; + if (!Objects.equals(this.routingRequired, that.routingRequired)) return false; if (!source.equals(that.source)) return false; if (!type.equals(that.type)) return false; @@ -211,17 +180,14 @@ public boolean equals(Object o) { @Override public int hashCode() { - int result = type.hashCode(); - result = 31 * result + source.hashCode(); - result = 31 * result + routing.hashCode(); - return result; + return Objects.hash(type, source, routingRequired); } public MappingMetadata(StreamInput in) throws IOException { type = in.readString(); source = CompressedXContent.readCompressedString(in); // routing - routing = new Routing(in.readBoolean()); + routingRequired = in.readBoolean(); if (in.getVersion().before(LegacyESVersion.V_7_0_0)) { in.readBoolean(); // hasParentField } diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index b3503f64c53f3..6e9c30877f9c2 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -880,7 +880,7 @@ public boolean routingRequired(String concreteIndex) { if (indexMetadata != null) { MappingMetadata mappingMetadata = indexMetadata.mapping(); if (mappingMetadata != null) { - return mappingMetadata.routing().required(); + return mappingMetadata.routingRequired(); } } return false; diff --git a/server/src/main/java/org/opensearch/index/mapper/MapperService.java b/server/src/main/java/org/opensearch/index/mapper/MapperService.java index 1d4e49a6e6fee..a92647929ff08 100644 --- a/server/src/main/java/org/opensearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/opensearch/index/mapper/MapperService.java @@ -32,7 +32,6 @@ package org.opensearch.index.mapper; -import com.carrotsearch.hppc.cursors.ObjectCursor; import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.lucene.analysis.Analyzer; import org.apache.lucene.analysis.DelegatingAnalyzerWrapper; @@ -416,8 +415,8 @@ public DocumentMapper merge(String type, CompressedXContent mappingSource, Merge private synchronized Map internalMerge(IndexMetadata indexMetadata, MergeReason reason) { assert reason != MergeReason.MAPPING_UPDATE_PREFLIGHT; Map map = new LinkedHashMap<>(); - for (ObjectCursor cursor : indexMetadata.getMappings().values()) { - MappingMetadata mappingMetadata = cursor.value; + MappingMetadata mappingMetadata = indexMetadata.mapping(); + if (mappingMetadata != null) { map.put(mappingMetadata.type(), mappingMetadata.source()); } return internalMerge(map, reason); diff --git a/server/src/main/java/org/opensearch/index/shard/StoreRecovery.java b/server/src/main/java/org/opensearch/index/shard/StoreRecovery.java index 6cf6ad645ca00..20bb6e7060ca3 100644 --- a/server/src/main/java/org/opensearch/index/shard/StoreRecovery.java +++ b/server/src/main/java/org/opensearch/index/shard/StoreRecovery.java @@ -32,7 +32,6 @@ package org.opensearch.index.shard; -import com.carrotsearch.hppc.cursors.ObjectObjectCursor; import org.apache.logging.log4j.Logger; import org.apache.lucene.index.IndexWriter; import org.apache.lucene.index.IndexWriterConfig; @@ -132,8 +131,8 @@ void recoverFromLocalShards( throw new IllegalArgumentException("can't add shards from more than one index"); } IndexMetadata sourceMetadata = shards.get(0).getIndexMetadata(); - for (ObjectObjectCursor mapping : sourceMetadata.getMappings()) { - mappingUpdateConsumer.accept(mapping.value); + if (sourceMetadata.mapping() != null) { + mappingUpdateConsumer.accept(sourceMetadata.mapping()); } indexShard.mapperService().merge(sourceMetadata, MapperService.MergeReason.MAPPING_RECOVERY); // now that the mapping is merged we can validate the index sort configuration. diff --git a/server/src/main/java/org/opensearch/tasks/TaskResultsService.java b/server/src/main/java/org/opensearch/tasks/TaskResultsService.java index 60de452c3149e..e22793e057c6a 100644 --- a/server/src/main/java/org/opensearch/tasks/TaskResultsService.java +++ b/server/src/main/java/org/opensearch/tasks/TaskResultsService.java @@ -80,13 +80,11 @@ public class TaskResultsService { public static final String TASK_INDEX = ".tasks"; - public static final String TASK_TYPE = "task"; - public static final String TASK_RESULT_INDEX_MAPPING_FILE = "task-index-mapping.json"; public static final String TASK_RESULT_MAPPING_VERSION_META_FIELD = "version"; - public static final int TASK_RESULT_MAPPING_VERSION = 3; + public static final int TASK_RESULT_MAPPING_VERSION = 3; // must match version in task-index-mapping.json /** * The backoff policy to use when saving a task result fails. The total wait @@ -115,7 +113,7 @@ public void storeResult(TaskResult taskResult, ActionListener listener) { CreateIndexRequest createIndexRequest = new CreateIndexRequest(); createIndexRequest.settings(taskResultIndexSettings()); createIndexRequest.index(TASK_INDEX); - createIndexRequest.mapping(TASK_TYPE, taskResultIndexMapping(), XContentType.JSON); + createIndexRequest.mapping(taskResultIndexMapping()); createIndexRequest.cause("auto(task api)"); client.admin().indices().create(createIndexRequest, new ActionListener() { @@ -155,7 +153,7 @@ public void onFailure(Exception e) { } private int getTaskResultMappingVersion(IndexMetadata metadata) { - MappingMetadata mappingMetadata = metadata.getMappings().get(TASK_TYPE); + MappingMetadata mappingMetadata = metadata.mapping(); if (mappingMetadata == null) { return 0; } diff --git a/server/src/main/resources/org/opensearch/tasks/task-index-mapping.json b/server/src/main/resources/org/opensearch/tasks/task-index-mapping.json index 76b07bf3570f2..54e9d39902f03 100644 --- a/server/src/main/resources/org/opensearch/tasks/task-index-mapping.json +++ b/server/src/main/resources/org/opensearch/tasks/task-index-mapping.json @@ -1,5 +1,5 @@ { - "task" : { + "_doc" : { "_meta": { "version": 3 }, diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataMappingServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataMappingServiceTests.java index a87ec461e5dc8..94bf162303127 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataMappingServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataMappingServiceTests.java @@ -79,7 +79,7 @@ public void testMappingClusterStateUpdateDoesntChangeExistingIndices() throws Ex // the task really was a mapping update assertThat( indexService.mapperService().documentMapper().mappingSource(), - not(equalTo(result.resultingState.metadata().index("test").getMappings().get(MapperService.SINGLE_MAPPING_NAME).source())) + not(equalTo(result.resultingState.metadata().index("test").mapping().source())) ); // since we never committed the cluster state update, the in-memory state is unchanged assertThat(indexService.mapperService().documentMapper().mappingSource(), equalTo(currentMapping));