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.

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,32 +73,13 @@ 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.
      */
@@ -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.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();

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);
     }
 

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
+        }
+    }
 }

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));

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()));
         }

docs/reference/migration/migrate_3_0.asciidoc

@@ -262,3 +262,7 @@ of string values: see `FilterFunctionScoreQuery.ScoreMode` and `CombineFunction`
 
 `CombineFunction.MULT` has been renamed to `MULTIPLY`.
 
+==== IdsQueryBuilder
+
+For simplicity, only one way of adding the ids to the existing list (empty by default)  is left: `addIds(String...)`
+

plugins/lang-groovy/src/test/java/org/elasticsearch/messy/tests/SearchQueryTests.java

@@ -28,6 +28,7 @@ import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.action.search.SearchType;
 import org.elasticsearch.action.search.ShardSearchFailure;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.index.mapper.MapperParsingException;
@@ -677,25 +678,25 @@ public class SearchQueryTests extends ESIntegTestCase {
                 client().prepareIndex("test", "type1", "2").setSource("field1", "value2"),
                 client().prepareIndex("test", "type1", "3").setSource("field1", "value3"));
 
-        SearchResponse searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery("type1").ids("1", "3"))).get();
+        SearchResponse searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery("type1").addIds("1", "3"))).get();
         assertHitCount(searchResponse, 2l);
         assertSearchHits(searchResponse, "1", "3");
 
         // no type
-        searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery().ids("1", "3"))).get();
+        searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery().addIds("1", "3"))).get();
         assertHitCount(searchResponse, 2l);
         assertSearchHits(searchResponse, "1", "3");
 
-        searchResponse = client().prepareSearch().setQuery(idsQuery("type1").ids("1", "3")).get();
+        searchResponse = client().prepareSearch().setQuery(idsQuery("type1").addIds("1", "3")).get();
         assertHitCount(searchResponse, 2l);
         assertSearchHits(searchResponse, "1", "3");
 
         // no type
-        searchResponse = client().prepareSearch().setQuery(idsQuery().ids("1", "3")).get();
+        searchResponse = client().prepareSearch().setQuery(idsQuery().addIds("1", "3")).get();
         assertHitCount(searchResponse, 2l);
         assertSearchHits(searchResponse, "1", "3");
 
-        searchResponse = client().prepareSearch().setQuery(idsQuery("type1").ids("7", "10")).get();
+        searchResponse = client().prepareSearch().setQuery(idsQuery("type1").addIds("7", "10")).get();
         assertHitCount(searchResponse, 0l);
 
         // repeat..., with terms
@@ -1292,52 +1293,6 @@ public class SearchQueryTests extends ESIntegTestCase {
         assertHitCount(searchResponse, 0l);
     }
 
-    @Test
-    public void testBasicFilterById() throws Exception {
-        createIndex("test");
-
-        client().prepareIndex("test", "type1", "1").setSource("field1", "value1").get();
-        client().prepareIndex("test", "type2", "2").setSource("field1", "value2").get();
-        refresh();
-
-        SearchResponse searchResponse = client().prepareSearch().setQuery(matchAllQuery()).setPostFilter(idsQuery("type1").ids("1")).get();
-        assertHitCount(searchResponse, 1l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(1));
-
-        searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery("type1", "type2").ids("1", "2"))).get();
-        assertHitCount(searchResponse, 2l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(2));
-
-        searchResponse = client().prepareSearch().setQuery(matchAllQuery()).setPostFilter(idsQuery().ids("1")).get();
-        assertHitCount(searchResponse, 1l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(1));
-
-        searchResponse = client().prepareSearch().setQuery(matchAllQuery()).setPostFilter(idsQuery().ids("1", "2")).get();
-        assertHitCount(searchResponse, 2l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(2));
-
-        searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery().ids("1", "2"))).get();
-        assertHitCount(searchResponse, 2l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(2));
-
-        searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery("type1").ids("1", "2"))).get();
-        assertHitCount(searchResponse, 1l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(1));
-
-        searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery().ids("1"))).get();
-        assertHitCount(searchResponse, 1l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(1));
-
-        // TODO: why do we even support passing null??
-        searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery((String[])null).ids("1"))).get();
-        assertHitCount(searchResponse, 1l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(1));
-
-        searchResponse = client().prepareSearch().setQuery(constantScoreQuery(idsQuery("type1", "type2", "type3").ids("1", "2", "3", "4"))).get();
-        assertHitCount(searchResponse, 2l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(2));
-    }
-
     @Test
     public void testBasicQueryById() throws Exception {
         createIndex("test");
@@ -1346,32 +1301,27 @@ public class SearchQueryTests extends ESIntegTestCase {
         client().prepareIndex("test", "type2", "2").setSource("field1", "value2").get();
         refresh();
 
-        SearchResponse searchResponse = client().prepareSearch().setQuery(idsQuery("type1", "type2").ids("1", "2")).get();
+        SearchResponse searchResponse = client().prepareSearch().setQuery(idsQuery("type1", "type2").addIds("1", "2")).get();
         assertHitCount(searchResponse, 2l);
         assertThat(searchResponse.getHits().hits().length, equalTo(2));
 
-        searchResponse = client().prepareSearch().setQuery(idsQuery().ids("1")).get();
+        searchResponse = client().prepareSearch().setQuery(idsQuery().addIds("1")).get();
         assertHitCount(searchResponse, 1l);
         assertThat(searchResponse.getHits().hits().length, equalTo(1));
 
-        searchResponse = client().prepareSearch().setQuery(idsQuery().ids("1", "2")).get();
+        searchResponse = client().prepareSearch().setQuery(idsQuery().addIds("1", "2")).get();
         assertHitCount(searchResponse, 2l);
         assertThat(searchResponse.getHits().hits().length, equalTo(2));
 
-
+        searchResponse = client().prepareSearch().setQuery(idsQuery("type1").addIds("1", "2")).get();
-        searchResponse = client().prepareSearch().setQuery(idsQuery("type1").ids("1", "2")).get();
         assertHitCount(searchResponse, 1l);
         assertThat(searchResponse.getHits().hits().length, equalTo(1));
 
-        searchResponse = client().prepareSearch().setQuery(idsQuery().ids("1")).get();
+        searchResponse = client().prepareSearch().setQuery(idsQuery(Strings.EMPTY_ARRAY).addIds("1")).get();
         assertHitCount(searchResponse, 1l);
         assertThat(searchResponse.getHits().hits().length, equalTo(1));
 
-        searchResponse = client().prepareSearch().setQuery(idsQuery((String[])null).ids("1")).get();
+        searchResponse = client().prepareSearch().setQuery(idsQuery("type1", "type2", "type3").addIds("1", "2", "3", "4")).get();
-        assertHitCount(searchResponse, 1l);
-        assertThat(searchResponse.getHits().hits().length, equalTo(1));
-
-        searchResponse = client().prepareSearch().setQuery(idsQuery("type1", "type2", "type3").ids("1", "2", "3", "4")).get();
         assertHitCount(searchResponse, 2l);
         assertThat(searchResponse.getHits().hits().length, equalTo(2));
     }