From 3185eaece86365992e3e3ea3221b2400c22950c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Wed, 5 Jul 2017 09:50:10 +0200 Subject: [PATCH] QueryBuilders should implement ToXContentObject (#25530) All query builders written as self contained xContent objects, to we should mark them accordingly using ToXContentObject. This also makes it possible to use things like XContentHelper#toXContent to render query builders in tests. --- .../index/query/AbstractQueryBuilder.java | 1 - .../index/query/QueryBuilder.java | 4 +-- .../query/ConstantScoreQueryBuilderTests.java | 3 +-- .../index/query/WrapperQueryBuilderTests.java | 2 +- .../test/AbstractQueryTestCase.java | 27 +++++++------------ 5 files changed, 14 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java index 7db9fb3bb91..109ba42d28b 100644 --- a/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/AbstractQueryBuilder.java @@ -311,7 +311,6 @@ public abstract class AbstractQueryBuilder> } QueryBuilder result; try { - // TODO what can we pass in here result = parser.namedObject(QueryBuilder.class, queryName, null); } catch (UnknownNamedObjectException e) { // Preserve the error message from 5.0 until we have a compellingly better message so we don't break BWC. diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java index 197af655d54..7c6b332f4aa 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryBuilder.java @@ -21,11 +21,11 @@ package org.elasticsearch.index.query; import org.apache.lucene.search.Query; import org.elasticsearch.common.io.stream.NamedWriteable; -import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.ToXContentObject; import java.io.IOException; -public interface QueryBuilder extends NamedWriteable, ToXContent { +public interface QueryBuilder extends NamedWriteable, ToXContentObject { /** * Converts this QueryBuilder to a lucene {@link Query}. diff --git a/core/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java index d8925af70ae..3e4f0caabad 100644 --- a/core/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/ConstantScoreQueryBuilderTests.java @@ -23,7 +23,6 @@ import org.apache.lucene.search.ConstantScoreQuery; import org.apache.lucene.search.Query; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.xcontent.XContent; -import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.test.AbstractQueryTestCase; @@ -94,7 +93,7 @@ public class ConstantScoreQueryBuilderTests extends AbstractQueryTestCase parseQuery(json)); assertTrue(e.getMessage().contains("bogusField")); diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java index 509e6ba17e4..708809dbb3e 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractQueryTestCase.java @@ -40,6 +40,7 @@ import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.io.stream.BytesStreamOutput; @@ -226,7 +227,7 @@ public abstract class AbstractQueryTestCase> } @After - public void afterTest() throws IOException { + public void afterTest() { serviceHolder.clientInvocationHandler.delegate = null; } @@ -265,9 +266,10 @@ public abstract class AbstractQueryTestCase> public void testFromXContent() throws IOException { for (int runs = 0; runs < NUMBER_OF_TESTQUERIES; runs++) { QB testQuery = createTestQueryBuilder(); - XContentBuilder builder = toXContent(testQuery, randomFrom(XContentType.values())); - XContentBuilder shuffled = shuffleXContent(builder, shuffleProtectedFields()); - assertParsedQuery(createParser(shuffled), testQuery); + XContentType xContentType = randomFrom(XContentType.values()); + BytesReference shuffledXContent = toShuffledXContent(testQuery, xContentType, ToXContent.EMPTY_PARAMS, randomBoolean(), + shuffleProtectedFields()); + assertParsedQuery(createParser(xContentType.xContent(), shuffledXContent), testQuery); for (Map.Entry alternateVersion : getAlternateVersions().entrySet()) { String queryAsString = alternateVersion.getKey(); assertParsedQuery(createParser(JsonXContent.jsonXContent, queryAsString), alternateVersion.getValue()); @@ -283,22 +285,13 @@ public abstract class AbstractQueryTestCase> return Strings.EMPTY_ARRAY; } - protected static XContentBuilder toXContent(QueryBuilder query, XContentType contentType) throws IOException { - XContentBuilder builder = XContentFactory.contentBuilder(contentType); - if (randomBoolean()) { - builder.prettyPrint(); - } - query.toXContent(builder, ToXContent.EMPTY_PARAMS); - return builder; - } - /** * Test that unknown field trigger ParsingException. * To find the right position in the root query, we add a marker as `queryName` which * all query builders support. The added bogus field after that should trigger the exception. * Queries that allow arbitrary field names at this level need to override this test. */ - public void testUnknownField() throws IOException { + public void testUnknownField() { String marker = "#marker#"; QB testQuery; do { @@ -471,7 +464,7 @@ public abstract class AbstractQueryTestCase> * Test that wraps the randomly generated query into an array as follows: { "query_name" : [{}]} * This causes unexpected situations in parser code that may not be handled properly. */ - public final void testQueryWrappedInArray() throws IOException { + public final void testQueryWrappedInArray() { QB queryBuilder = createTestQueryBuilder(); String queryName = queryBuilder.getName(); String validQuery = queryBuilder.toString(); @@ -481,7 +474,7 @@ public abstract class AbstractQueryTestCase> } } - private void queryWrappedInArrayTest(String queryName, String validQuery) throws IOException { + private void queryWrappedInArrayTest(String queryName, String validQuery) { int i = validQuery.indexOf("\"" + queryName + "\""); assertThat(i, greaterThan(0)); @@ -727,7 +720,7 @@ public abstract class AbstractQueryTestCase> } } - public void testEqualsAndHashcode() throws IOException { + public void testEqualsAndHashcode() { for (int runs = 0; runs < NUMBER_OF_TESTQUERIES; runs++) { // TODO we only change name and boost, we should extend by any sub-test supplying a "mutate" method that randomly changes one // aspect of the object under test