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.

5 files changed, 43 insertions, 37 deletions

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<IdsQueryBuilder> {
     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,33 +73,14 @@ public class IdsQueryBuilder extends AbstractQueryBuilder<IdsQueryBuilder> {
      * 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<String> 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<String> ids) {
-        return addIds(ids);
-    }
-
-    /**
      * Returns the ids for the query.
      */
     public Set<String> ids() {
@@ -100,13 +90,7 @@ public class IdsQueryBuilder extends AbstractQueryBuilder<IdsQueryBuilder> {
     @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<IdsQueryBuilder> {
              query = Queries.newMatchNoDocsQuery();
         } else {
             Collection<String> 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;
@@ -110,11 +109,18 @@ public abstract class QueryBuilders {
     }
 
     /**
+     * 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<IdsQueryBuilder>
 
         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()));
         }