From e8653f51569580f23ca28a91106d90b07e989167 Mon Sep 17 00:00:00 2001 From: javanna Date: Mon, 5 Oct 2015 13:44:20 +0200 Subject: Java api: IdsQueryBuilder to accept only non null ids and types Types are still optional, but if you do provide them, they can't be null. Split the existing constructor that accepted nnull into two, one that accepts no arguments, and another one that accepts the types argument, which must be not null. Also trimmed down different ways of setting ids, some were misleading as they would always add the ids to the existing ones and not set them, the add prefix makes that clear. Left `addIds` method that accepts a varargs argument. Added check for ids not be null. --- .../elasticsearch/index/query/IdsQueryBuilder.java | 50 ++++++++-------------- .../elasticsearch/index/query/QueryBuilders.java | 10 ++++- .../index/query/IdsQueryBuilderTests.java | 16 +++++++ .../search/child/ChildQuerySearchIT.java | 2 +- .../search/compress/SearchSourceCompressTests.java | 2 +- 5 files changed, 43 insertions(+), 37 deletions(-) (limited to 'core') diff --git a/core/src/main/java/org/elasticsearch/index/query/IdsQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/IdsQueryBuilder.java index b85db4b66b..1de8db2e80 100644 --- a/core/src/main/java/org/elasticsearch/index/query/IdsQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/IdsQueryBuilder.java @@ -22,7 +22,6 @@ package org.elasticsearch.index.query; import org.apache.lucene.queries.TermsQuery; import org.apache.lucene.search.Query; import org.elasticsearch.cluster.metadata.MetaData; -import org.elasticsearch.common.Nullable; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.lucene.search.Queries; @@ -47,9 +46,19 @@ public class IdsQueryBuilder extends AbstractQueryBuilder { static final IdsQueryBuilder PROTOTYPE = new IdsQueryBuilder(); /** - * Creates a new IdsQueryBuilder by optionally providing the types of the documents to look for + * Creates a new IdsQueryBuilder without providing the types of the documents to look for */ - public IdsQueryBuilder(@Nullable String... types) { + public IdsQueryBuilder() { + this.types = new String[0]; + } + + /** + * Creates a new IdsQueryBuilder by providing the types of the documents to look for + */ + public IdsQueryBuilder(String... types) { + if (types == null) { + throw new IllegalArgumentException("[ids] types cannot be null"); + } this.types = types; } @@ -64,32 +73,13 @@ public class IdsQueryBuilder extends AbstractQueryBuilder { * Adds ids to the query. */ public IdsQueryBuilder addIds(String... ids) { + if (ids == null) { + throw new IllegalArgumentException("[ids] ids cannot be null"); + } Collections.addAll(this.ids, ids); return this; } - /** - * Adds ids to the query. - */ - public IdsQueryBuilder addIds(Collection ids) { - this.ids.addAll(ids); - return this; - } - - /** - * Adds ids to the filter. - */ - public IdsQueryBuilder ids(String... ids) { - return addIds(ids); - } - - /** - * Adds ids to the filter. - */ - public IdsQueryBuilder ids(Collection ids) { - return addIds(ids); - } - /** * Returns the ids for the query. */ @@ -100,13 +90,7 @@ public class IdsQueryBuilder extends AbstractQueryBuilder { @Override protected void doXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(NAME); - if (types != null) { - if (types.length == 1) { - builder.field("type", types[0]); - } else { - builder.array("types", types); - } - } + builder.array("types", types); builder.startArray("values"); for (String value : ids) { builder.value(value); @@ -128,7 +112,7 @@ public class IdsQueryBuilder extends AbstractQueryBuilder { query = Queries.newMatchNoDocsQuery(); } else { Collection typesForQuery; - if (types == null || types.length == 0) { + if (types.length == 0) { typesForQuery = context.queryTypes(); } else if (types.length == 1 && MetaData.ALL.equals(types[0])) { typesForQuery = context.mapperService().types(); diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java b/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java index df823e166f..67e654cb0d 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryBuilders.java @@ -19,7 +19,6 @@ package org.elasticsearch.index.query; -import org.elasticsearch.common.Nullable; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.ShapeRelation; @@ -109,12 +108,19 @@ public abstract class QueryBuilders { return new DisMaxQueryBuilder(); } + /** + * Constructs a query that will match only specific ids within all types. + */ + public static IdsQueryBuilder idsQuery() { + return new IdsQueryBuilder(); + } + /** * Constructs a query that will match only specific ids within types. * * @param types The mapping/doc type */ - public static IdsQueryBuilder idsQuery(@Nullable String... types) { + public static IdsQueryBuilder idsQuery(String... types) { return new IdsQueryBuilder(types); } diff --git a/core/src/test/java/org/elasticsearch/index/query/IdsQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/IdsQueryBuilderTests.java index 7ec3e3f8d2..177caf0871 100644 --- a/core/src/test/java/org/elasticsearch/index/query/IdsQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/IdsQueryBuilderTests.java @@ -121,4 +121,20 @@ public class IdsQueryBuilderTests extends AbstractQueryTestCase return alternateVersions; } + + public void testIllegalArguments() { + try { + new IdsQueryBuilder((String[])null); + fail("must be not null"); + } catch(IllegalArgumentException e) { + //all good + } + + try { + new IdsQueryBuilder().addIds((String[])null); + fail("must be not null"); + } catch(IllegalArgumentException e) { + //all good + } + } } diff --git a/core/src/test/java/org/elasticsearch/search/child/ChildQuerySearchIT.java b/core/src/test/java/org/elasticsearch/search/child/ChildQuerySearchIT.java index 335b584e12..09f33f60ed 100644 --- a/core/src/test/java/org/elasticsearch/search/child/ChildQuerySearchIT.java +++ b/core/src/test/java/org/elasticsearch/search/child/ChildQuerySearchIT.java @@ -181,7 +181,7 @@ public class ChildQuerySearchIT extends ESIntegTestCase { refresh(); // TEST FETCHING _parent from child - SearchResponse searchResponse = client().prepareSearch("test").setQuery(idsQuery("child").ids("c1")).addFields("_parent").execute() + SearchResponse searchResponse = client().prepareSearch("test").setQuery(idsQuery("child").addIds("c1")).addFields("_parent").execute() .actionGet(); assertNoFailures(searchResponse); assertThat(searchResponse.getHits().totalHits(), equalTo(1l)); diff --git a/core/src/test/java/org/elasticsearch/search/compress/SearchSourceCompressTests.java b/core/src/test/java/org/elasticsearch/search/compress/SearchSourceCompressTests.java index b6d335318c..d3b5160db4 100644 --- a/core/src/test/java/org/elasticsearch/search/compress/SearchSourceCompressTests.java +++ b/core/src/test/java/org/elasticsearch/search/compress/SearchSourceCompressTests.java @@ -84,7 +84,7 @@ public class SearchSourceCompressTests extends ESSingleNodeTestCase { assertThat(getResponse.getSourceAsBytes(), equalTo(buildSource(10000).bytes().toBytes())); for (int i = 1; i < 100; i++) { - SearchResponse searchResponse = client().prepareSearch().setQuery(QueryBuilders.idsQuery("type1").ids(Integer.toString(i))).execute().actionGet(); + SearchResponse searchResponse = client().prepareSearch().setQuery(QueryBuilders.idsQuery("type1").addIds(Integer.toString(i))).execute().actionGet(); assertThat(searchResponse.getHits().getTotalHits(), equalTo(1l)); assertThat(searchResponse.getHits().getAt(0).source(), equalTo(buildSource(i).bytes().toBytes())); } -- cgit v1.2.3