From 9f83deadf40441e43f6f8fefd56141fad9e6626f Mon Sep 17 00:00:00 2001 From: Suraj Singh <79435743+dreamer-89@users.noreply.github.com> Date: Thu, 17 Mar 2022 18:38:10 -0700 Subject: [PATCH] [Remove] Type from Percolate query API (#2490) * [Remove] Type from Percolator query API Signed-off-by: Suraj Singh * Address review comment Signed-off-by: Suraj Singh --- .../percolator/PercolatorQuerySearchIT.java | 14 +- .../percolator/PercolateQueryBuilder.java | 169 +++++------------- .../PercolateQueryBuilderTests.java | 60 +------ 3 files changed, 61 insertions(+), 182 deletions(-) diff --git a/modules/percolator/src/internalClusterTest/java/org/opensearch/percolator/PercolatorQuerySearchIT.java b/modules/percolator/src/internalClusterTest/java/org/opensearch/percolator/PercolatorQuerySearchIT.java index f78b74e272ebf..8d3c37bc9b039 100644 --- a/modules/percolator/src/internalClusterTest/java/org/opensearch/percolator/PercolatorQuerySearchIT.java +++ b/modules/percolator/src/internalClusterTest/java/org/opensearch/percolator/PercolatorQuerySearchIT.java @@ -397,14 +397,14 @@ public void testPercolatorQueryExistingDocument() throws Exception { logger.info("percolating empty doc"); SearchResponse response = client().prepareSearch() - .setQuery(new PercolateQueryBuilder("query", "test", "type", "1", null, null, null)) + .setQuery(new PercolateQueryBuilder("query", "test", "1", null, null, null)) .get(); assertHitCount(response, 1); assertThat(response.getHits().getAt(0).getId(), equalTo("1")); logger.info("percolating doc with 1 field"); response = client().prepareSearch() - .setQuery(new PercolateQueryBuilder("query", "test", "type", "5", null, null, null)) + .setQuery(new PercolateQueryBuilder("query", "test", "5", null, null, null)) .addSort("id", SortOrder.ASC) .get(); assertHitCount(response, 2); @@ -413,7 +413,7 @@ public void testPercolatorQueryExistingDocument() throws Exception { logger.info("percolating doc with 2 fields"); response = client().prepareSearch() - .setQuery(new PercolateQueryBuilder("query", "test", "type", "6", null, null, null)) + .setQuery(new PercolateQueryBuilder("query", "test", "6", null, null, null)) .addSort("id", SortOrder.ASC) .get(); assertHitCount(response, 3); @@ -438,7 +438,7 @@ public void testPercolatorQueryExistingDocumentSourceDisabled() throws Exception logger.info("percolating empty doc with source disabled"); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> { client().prepareSearch().setQuery(new PercolateQueryBuilder("query", "test", "type", "1", null, null, null)).get(); } + () -> { client().prepareSearch().setQuery(new PercolateQueryBuilder("query", "test", "1", null, null, null)).get(); } ); assertThat(e.getMessage(), containsString("source disabled")); } @@ -1193,10 +1193,10 @@ public void testPercolatorQueryViaMultiSearch() throws Exception { ) ) ) - .add(client().prepareSearch("test").setQuery(new PercolateQueryBuilder("query", "test", "type", "5", null, null, null))) + .add(client().prepareSearch("test").setQuery(new PercolateQueryBuilder("query", "test", "5", null, null, null))) .add( client().prepareSearch("test") // non existing doc, so error element - .setQuery(new PercolateQueryBuilder("query", "test", "type", "6", null, null, null)) + .setQuery(new PercolateQueryBuilder("query", "test", "6", null, null, null)) ) .get(); @@ -1228,7 +1228,7 @@ public void testPercolatorQueryViaMultiSearch() throws Exception { item = response.getResponses()[5]; assertThat(item.getResponse(), nullValue()); assertThat(item.getFailureMessage(), notNullValue()); - assertThat(item.getFailureMessage(), containsString("[test/type/6] couldn't be found")); + assertThat(item.getFailureMessage(), containsString("[test/6] couldn't be found")); } public void testDisallowExpensiveQueries() throws IOException { diff --git a/modules/percolator/src/main/java/org/opensearch/percolator/PercolateQueryBuilder.java b/modules/percolator/src/main/java/org/opensearch/percolator/PercolateQueryBuilder.java index 87f08e2ff50fc..b2130eca3bb02 100644 --- a/modules/percolator/src/main/java/org/opensearch/percolator/PercolateQueryBuilder.java +++ b/modules/percolator/src/main/java/org/opensearch/percolator/PercolateQueryBuilder.java @@ -67,7 +67,6 @@ import org.opensearch.common.io.stream.NamedWriteableRegistry; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; -import org.opensearch.common.logging.DeprecationLogger; import org.opensearch.common.xcontent.ConstructingObjectParser; import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.common.xcontent.NamedXContentRegistry; @@ -111,19 +110,11 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder { public static final String NAME = "percolate"; - private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(ParseField.class); - static final String DOCUMENT_TYPE_DEPRECATION_MESSAGE = "[types removal] Types are deprecated in [percolate] queries. " - + "The [document_type] should no longer be specified."; - static final String TYPE_DEPRECATION_MESSAGE = "[types removal] Types are deprecated in [percolate] queries. " - + "The [type] of the indexed document should no longer be specified."; - static final ParseField DOCUMENT_FIELD = new ParseField("document"); static final ParseField DOCUMENTS_FIELD = new ParseField("documents"); private static final ParseField NAME_FIELD = new ParseField("name"); private static final ParseField QUERY_FIELD = new ParseField("field"); - private static final ParseField DOCUMENT_TYPE_FIELD = new ParseField("document_type"); private static final ParseField INDEXED_DOCUMENT_FIELD_INDEX = new ParseField("index"); - private static final ParseField INDEXED_DOCUMENT_FIELD_TYPE = new ParseField("type"); private static final ParseField INDEXED_DOCUMENT_FIELD_ID = new ParseField("id"); private static final ParseField INDEXED_DOCUMENT_FIELD_ROUTING = new ParseField("routing"); private static final ParseField INDEXED_DOCUMENT_FIELD_PREFERENCE = new ParseField("preference"); @@ -131,29 +122,16 @@ public class PercolateQueryBuilder extends AbstractQueryBuilder documents; private final XContentType documentXContentType; private final String indexedDocumentIndex; - @Deprecated - private final String indexedDocumentType; private final String indexedDocumentId; private final String indexedDocumentRouting; private final String indexedDocumentPreference; private final Long indexedDocumentVersion; private final Supplier documentSupplier; - /** - * @deprecated use {@link #PercolateQueryBuilder(String, BytesReference, XContentType)} with the document content - * type to avoid autodetection. - */ - @Deprecated - public PercolateQueryBuilder(String field, String documentType, BytesReference document) { - this(field, documentType, Collections.singletonList(document), XContentHelper.xContentType(document)); - } - /** * Creates a percolator query builder instance for percolating a provided document. * @@ -162,7 +140,7 @@ public PercolateQueryBuilder(String field, String documentType, BytesReference d * @param documentXContentType The content type of the binary blob containing the document to percolate */ public PercolateQueryBuilder(String field, BytesReference document, XContentType documentXContentType) { - this(field, null, Collections.singletonList(document), documentXContentType); + this(field, Collections.singletonList(document), documentXContentType); } /** @@ -173,11 +151,6 @@ public PercolateQueryBuilder(String field, BytesReference document, XContentType * @param documentXContentType The content type of the binary blob containing the document to percolate */ public PercolateQueryBuilder(String field, List documents, XContentType documentXContentType) { - this(field, null, documents, documentXContentType); - } - - @Deprecated - public PercolateQueryBuilder(String field, String documentType, List documents, XContentType documentXContentType) { if (field == null) { throw new IllegalArgumentException("[field] is a required argument"); } @@ -185,11 +158,9 @@ public PercolateQueryBuilder(String field, String documentType, List documentSupplier) { - if (field == null) { - throw new IllegalArgumentException("[field] is a required argument"); - } - this.field = field; - this.documentType = documentType; - this.documents = Collections.emptyList(); - this.documentXContentType = null; - this.documentSupplier = documentSupplier; - indexedDocumentIndex = null; - indexedDocumentType = null; - indexedDocumentId = null; - indexedDocumentRouting = null; - indexedDocumentPreference = null; - indexedDocumentVersion = null; - } - /** * Creates a percolator query builder instance for percolating a document in a remote index. * * @param field The field that contains the percolator query * @param indexedDocumentIndex The index containing the document to percolate - * @param indexedDocumentType The type containing the document to percolate * @param indexedDocumentId The id of the document to percolate * @param indexedDocumentRouting The routing value for the document to percolate * @param indexedDocumentPreference The preference to use when fetching the document to percolate @@ -228,30 +181,6 @@ protected PercolateQueryBuilder(String field, String documentType, Supplier documentSupplier) { + if (field == null) { + throw new IllegalArgumentException("[field] is a required argument"); + } + this.field = field; + this.documents = Collections.emptyList(); + this.documentXContentType = null; + this.documentSupplier = documentSupplier; + indexedDocumentIndex = null; + indexedDocumentId = null; + indexedDocumentRouting = null; + indexedDocumentPreference = null; + indexedDocumentVersion = null; + } + /** * Read from a stream. */ @@ -286,9 +228,20 @@ public PercolateQueryBuilder( super(in); field = in.readString(); name = in.readOptionalString(); - documentType = in.readOptionalString(); + if (in.getVersion().before(Version.V_2_0_0)) { + String documentType = in.readOptionalString(); + if (documentType != null) { + throw new IllegalStateException("documentType must be null"); + } + } indexedDocumentIndex = in.readOptionalString(); - indexedDocumentType = in.readOptionalString(); + if (in.getVersion().before(Version.V_2_0_0)) { + String indexedDocumentType = in.readOptionalString(); + if (indexedDocumentType != null) { + throw new IllegalStateException("indexedDocumentType must be null"); + } + } + indexedDocumentId = in.readOptionalString(); indexedDocumentRouting = in.readOptionalString(); indexedDocumentPreference = in.readOptionalString(); @@ -322,9 +275,15 @@ protected void doWriteTo(StreamOutput out) throws IOException { } out.writeString(field); out.writeOptionalString(name); - out.writeOptionalString(documentType); + if (out.getVersion().before(Version.V_2_0_0)) { + // In 7x, typeless percolate queries are represented by null documentType values + out.writeOptionalString(null); + } out.writeOptionalString(indexedDocumentIndex); - out.writeOptionalString(indexedDocumentType); + if (out.getVersion().before(Version.V_2_0_0)) { + // In 7x, typeless percolate queries are represented by null indexedDocumentType values + out.writeOptionalString(null); + } out.writeOptionalString(indexedDocumentId); out.writeOptionalString(indexedDocumentRouting); out.writeOptionalString(indexedDocumentPreference); @@ -346,7 +305,6 @@ protected void doWriteTo(StreamOutput out) throws IOException { @Override protected void doXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(NAME); - builder.field(DOCUMENT_TYPE_FIELD.getPreferredName(), documentType); builder.field(QUERY_FIELD.getPreferredName(), field); if (name != null) { builder.field(NAME_FIELD.getPreferredName(), name); @@ -367,13 +325,10 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep } builder.endArray(); } - if (indexedDocumentIndex != null || indexedDocumentType != null || indexedDocumentId != null) { + if (indexedDocumentIndex != null || indexedDocumentId != null) { if (indexedDocumentIndex != null) { builder.field(INDEXED_DOCUMENT_FIELD_INDEX.getPreferredName(), indexedDocumentIndex); } - if (indexedDocumentType != null) { - builder.field(INDEXED_DOCUMENT_FIELD_TYPE.getPreferredName(), indexedDocumentType); - } if (indexedDocumentId != null) { builder.field(INDEXED_DOCUMENT_FIELD_ID.getPreferredName(), indexedDocumentId); } @@ -401,23 +356,12 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep String indexDocRouting = (String) args[5]; String indexDocPreference = (String) args[6]; Long indexedDocVersion = (Long) args[7]; - String indexedDocType = (String) args[8]; - String docType = (String) args[9]; if (indexedDocId != null) { - return new PercolateQueryBuilder( - field, - docType, - indexedDocIndex, - indexedDocType, - indexedDocId, - indexDocRouting, - indexDocPreference, - indexedDocVersion - ); + return new PercolateQueryBuilder(field, indexedDocIndex, indexedDocId, indexDocRouting, indexDocPreference, indexedDocVersion); } else if (document != null) { - return new PercolateQueryBuilder(field, docType, Collections.singletonList(document), XContentType.JSON); + return new PercolateQueryBuilder(field, Collections.singletonList(document), XContentType.JSON); } else { - return new PercolateQueryBuilder(field, docType, documents, XContentType.JSON); + return new PercolateQueryBuilder(field, documents, XContentType.JSON); } }); static { @@ -429,8 +373,6 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep PARSER.declareString(optionalConstructorArg(), INDEXED_DOCUMENT_FIELD_ROUTING); PARSER.declareString(optionalConstructorArg(), INDEXED_DOCUMENT_FIELD_PREFERENCE); PARSER.declareLong(optionalConstructorArg(), INDEXED_DOCUMENT_FIELD_VERSION); - PARSER.declareStringOrNull(optionalConstructorArg(), INDEXED_DOCUMENT_FIELD_TYPE); - PARSER.declareStringOrNull(optionalConstructorArg(), DOCUMENT_TYPE_FIELD); PARSER.declareString(PercolateQueryBuilder::setName, NAME_FIELD); PARSER.declareString(PercolateQueryBuilder::queryName, AbstractQueryBuilder.NAME_FIELD); PARSER.declareFloat(PercolateQueryBuilder::boost, BOOST_FIELD); @@ -461,10 +403,8 @@ public static PercolateQueryBuilder fromXContent(XContentParser parser) throws I @Override protected boolean doEquals(PercolateQueryBuilder other) { return Objects.equals(field, other.field) - && Objects.equals(documentType, other.documentType) && Objects.equals(documents, other.documents) && Objects.equals(indexedDocumentIndex, other.indexedDocumentIndex) - && Objects.equals(indexedDocumentType, other.indexedDocumentType) && Objects.equals(documentSupplier, other.documentSupplier) && Objects.equals(indexedDocumentId, other.indexedDocumentId); @@ -472,7 +412,7 @@ protected boolean doEquals(PercolateQueryBuilder other) { @Override protected int doHashCode() { - return Objects.hash(field, documentType, documents, indexedDocumentIndex, indexedDocumentType, indexedDocumentId, documentSupplier); + return Objects.hash(field, documents, indexedDocumentIndex, indexedDocumentId, documentSupplier); } @Override @@ -491,7 +431,6 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) { } else { PercolateQueryBuilder rewritten = new PercolateQueryBuilder( field, - documentType, Collections.singletonList(source), XContentHelper.xContentType(source) ); @@ -513,20 +452,14 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) { client.get(getRequest, ActionListener.wrap(getResponse -> { if (getResponse.isExists() == false) { throw new ResourceNotFoundException( - "indexed document [{}{}/{}] couldn't be found", + "indexed document [{}/{}] couldn't be found", indexedDocumentIndex, - indexedDocumentType == null ? "" : "/" + indexedDocumentType, indexedDocumentId ); } if (getResponse.isSourceEmpty()) { throw new IllegalArgumentException( - "indexed document [" - + indexedDocumentIndex - + (indexedDocumentType == null ? "" : "/" + indexedDocumentType) - + "/" - + indexedDocumentId - + "] source disabled" + "indexed document [" + indexedDocumentIndex + "/" + indexedDocumentId + "] source disabled" ); } documentSupplier.set(getResponse.getSourceAsBytesRef()); @@ -534,7 +467,7 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) { }, listener::onFailure)); }); - PercolateQueryBuilder rewritten = new PercolateQueryBuilder(field, documentType, documentSupplier::get); + PercolateQueryBuilder rewritten = new PercolateQueryBuilder(field, documentSupplier::get); if (name != null) { rewritten.setName(name); } @@ -576,14 +509,6 @@ protected Query doToQuery(QueryShardContext context) throws IOException { final DocumentMapper docMapper; final MapperService mapperService = context.getMapperService(); String type = mapperService.documentMapper().type(); - if (documentType != null) { - deprecationLogger.deprecate("percolate_with_document_type", DOCUMENT_TYPE_DEPRECATION_MESSAGE); - if (documentType.equals(type) == false) { - throw new IllegalArgumentException( - "specified document_type [" + documentType + "] is not equal to the actual type [" + type + "]" - ); - } - } docMapper = mapperService.documentMapper(); for (BytesReference document : documents) { docs.add(docMapper.parse(new SourceToParse(context.index().getName(), "_temp_id", document, documentXContentType))); @@ -631,10 +556,6 @@ public String getField() { return field; } - public String getDocumentType() { - return documentType; - } - public List getDocuments() { return documents; } diff --git a/modules/percolator/src/test/java/org/opensearch/percolator/PercolateQueryBuilderTests.java b/modules/percolator/src/test/java/org/opensearch/percolator/PercolateQueryBuilderTests.java index 44d8d64086091..87aa28a3346bc 100644 --- a/modules/percolator/src/test/java/org/opensearch/percolator/PercolateQueryBuilderTests.java +++ b/modules/percolator/src/test/java/org/opensearch/percolator/PercolateQueryBuilderTests.java @@ -148,16 +148,14 @@ private PercolateQueryBuilder doCreateTestQueryBuilder(boolean indexedDocument) indexedDocumentVersion = (long) randomIntBetween(0, Integer.MAX_VALUE); queryBuilder = new PercolateQueryBuilder( queryField, - null, indexedDocumentIndex, - null, indexedDocumentId, indexedDocumentRouting, indexedDocumentPreference, indexedDocumentVersion ); } else { - queryBuilder = new PercolateQueryBuilder(queryField, null, documentSource, XContentType.JSON); + queryBuilder = new PercolateQueryBuilder(queryField, documentSource, XContentType.JSON); } if (randomBoolean()) { queryBuilder.setName(randomAlphaOfLength(4)); @@ -217,7 +215,6 @@ protected GetResponse executeGet(GetRequest getRequest) { protected void doAssertLuceneQuery(PercolateQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException { assertThat(query, Matchers.instanceOf(PercolateQuery.class)); PercolateQuery percolateQuery = (PercolateQuery) query; - assertNull(queryBuilder.getDocumentType()); assertThat(percolateQuery.getDocuments(), Matchers.equalTo(documentSource)); } @@ -227,12 +224,7 @@ public void testMustRewrite() throws IOException { IllegalStateException e = expectThrows(IllegalStateException.class, () -> pqb.toQuery(createShardContext())); assertThat(e.getMessage(), equalTo("query builder must be rewritten first")); QueryBuilder rewrite = rewriteAndFetch(pqb, createShardContext()); - PercolateQueryBuilder geoShapeQueryBuilder = new PercolateQueryBuilder( - pqb.getField(), - pqb.getDocumentType(), - documentSource, - XContentType.JSON - ); + PercolateQueryBuilder geoShapeQueryBuilder = new PercolateQueryBuilder(pqb.getField(), documentSource, XContentType.JSON); assertEquals(geoShapeQueryBuilder, rewrite); } @@ -259,25 +251,19 @@ public void testRequiredParameters() { ); assertThat(e.getMessage(), equalTo("[field] is a required argument")); - e = expectThrows(IllegalArgumentException.class, () -> new PercolateQueryBuilder("_field", "_document_type", null, null)); - assertThat(e.getMessage(), equalTo("[document] is a required argument")); - e = expectThrows( IllegalArgumentException.class, - () -> { new PercolateQueryBuilder(null, null, "_index", "_type", "_id", null, null, null); } + () -> new PercolateQueryBuilder("_field", (List) null, XContentType.JSON) ); + assertThat(e.getMessage(), equalTo("[document] is a required argument")); + + e = expectThrows(IllegalArgumentException.class, () -> { new PercolateQueryBuilder(null, "_index", "_id", null, null, null); }); assertThat(e.getMessage(), equalTo("[field] is a required argument")); - e = expectThrows( - IllegalArgumentException.class, - () -> { new PercolateQueryBuilder("_field", "_document_type", null, "_type", "_id", null, null, null); } - ); + e = expectThrows(IllegalArgumentException.class, () -> { new PercolateQueryBuilder("_field", null, "_id", null, null, null); }); assertThat(e.getMessage(), equalTo("[index] is a required argument")); - e = expectThrows( - IllegalArgumentException.class, - () -> { new PercolateQueryBuilder("_field", "_document_type", "_index", "_type", null, null, null, null); } - ); + e = expectThrows(IllegalArgumentException.class, () -> { new PercolateQueryBuilder("_field", "_index", null, null, null, null); }); assertThat(e.getMessage(), equalTo("[id] is a required argument")); } @@ -287,15 +273,6 @@ public void testFromJsonNoDocumentType() throws IOException { queryBuilder.toQuery(queryShardContext); } - public void testFromJsonWithDocumentType() throws IOException { - QueryShardContext queryShardContext = createShardContext(); - QueryBuilder queryBuilder = parseQuery( - "{\"percolate\" : { \"document\": {}, \"document_type\":\"" + docType + "\", \"field\":\"" + queryField + "\"}}" - ); - queryBuilder.toQuery(queryShardContext); - assertWarnings(PercolateQueryBuilder.DOCUMENT_TYPE_DEPRECATION_MESSAGE); - } - public void testFromJsonNoType() throws IOException { indexedDocumentIndex = randomAlphaOfLength(4); indexedDocumentId = randomAlphaOfLength(4); @@ -315,25 +292,6 @@ public void testFromJsonNoType() throws IOException { rewriteAndFetch(queryBuilder, queryShardContext).toQuery(queryShardContext); } - public void testFromJsonWithType() throws IOException { - indexedDocumentIndex = randomAlphaOfLength(4); - indexedDocumentId = randomAlphaOfLength(4); - indexedDocumentVersion = Versions.MATCH_ANY; - documentSource = Collections.singletonList(randomSource(new HashSet<>())); - - QueryShardContext queryShardContext = createShardContext(); - QueryBuilder queryBuilder = parseQuery( - "{\"percolate\" : { \"index\": \"" - + indexedDocumentIndex - + "\", \"type\": \"_doc\", \"id\": \"" - + indexedDocumentId - + "\", \"field\":\"" - + queryField - + "\"}}" - ); - rewriteAndFetch(queryBuilder, queryShardContext).toQuery(queryShardContext); - } - public void testBothDocumentAndDocumentsSpecified() { IllegalArgumentException e = expectThrows( IllegalArgumentException.class, @@ -426,7 +384,7 @@ public void testSettingNameWhileRewritingWhenDocumentSupplierAndSourceNotNull() Supplier supplier = () -> new BytesArray("{\"test\": \"test\"}"); String testName = "name1"; QueryShardContext shardContext = createShardContext(); - PercolateQueryBuilder percolateQueryBuilder = new PercolateQueryBuilder(queryField, null, supplier); + PercolateQueryBuilder percolateQueryBuilder = new PercolateQueryBuilder(queryField, supplier); percolateQueryBuilder.setName(testName); QueryBuilder rewrittenQueryBuilder = percolateQueryBuilder.doRewrite(shardContext);