fixed some concerns by @colings86

qa/rolling-upgrade/src/test/resources/rest-api-spec/test/old_cluster/10_basic.yml

@@ -229,3 +229,15 @@
           - '{"location": "48.861111,2.336389", "name": "Musée du Louvre"}'
           - '{"index": {"_index": "geo_agg_index", "_type": "doc"}}'
           - '{"location": "48.860000,2.327000", "name": "Musée Orsay"}'
+  - do:
+      search:
+        index: geo_agg_index
+        body:
+          aggregations:
+            mygrid:
+              geohash_grid:
+                field : location
+                precision : 1
+  - match: { hits.total: 6 }
+  - match: { aggregations.mygrid.buckets.0.key: u }
+  - match: { aggregations.mygrid.buckets.0.doc_count: 6 }

server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java

@@ -557,7 +557,8 @@ public static int parsePrecisionString(String precision) {
         try {
             // we want to treat simple integer strings as precision levels, not distances
             return checkPrecisionRange(Integer.parseInt(precision));
-            // Do not catch IllegalArgumentException here
+            // checkPrecisionRange could also throw IllegalArgumentException, but let it through
+            // to keep errors somewhat consistent with how they were shown before this change
         } catch (NumberFormatException e) {
             // try to parse as a distance value
             final int parsedPrecision = GeoUtils.geoHashLevelsForPrecision(precision);

server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java

@@ -30,6 +30,7 @@
 import org.elasticsearch.search.aggregations.bucket.filter.FiltersAggregator.KeyedFilter;
 import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.geogrid.GeoHashGrid;
+import org.elasticsearch.search.aggregations.bucket.geogrid.GeoHashType;
 import org.elasticsearch.search.aggregations.bucket.global.Global;
 import org.elasticsearch.search.aggregations.bucket.global.GlobalAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
@@ -237,7 +238,7 @@ public static HistogramAggregationBuilder histogram(String name) {
      * Create a new {@link GeoHashGrid} aggregation with the given name.
      */
     public static GeoGridAggregationBuilder geohashGrid(String name) {
-        return new GeoGridAggregationBuilder(name);
+        return new GeoGridAggregationBuilder(name, GeoHashType.DEFAULT);
     }
 
     /**

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java

@@ -25,9 +25,9 @@
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.ObjectParser;
 import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentLocation;
 import org.elasticsearch.common.xcontent.XContentParseException;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.index.fielddata.AbstractSortingNumericDocValues;
@@ -54,78 +54,65 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
+
 public class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder<ValuesSource.GeoPoint, GeoGridAggregationBuilder>
-        implements MultiBucketAggregationBuilder {
+    implements MultiBucketAggregationBuilder {
     public static final String NAME = "geohash_grid";
     public static final int DEFAULT_MAX_NUM_CELLS = 10000;
 
-    private static final ObjectParser<GeoGridAggregationBuilder, Void> PARSER;
+    private static final ConstructingObjectParser<GeoGridAggregationBuilder, String> PARSER;
+
     static {
-        PARSER = new ObjectParser<>(GeoGridAggregationBuilder.NAME);
-        ValuesSourceParserHelper.declareGeoFields(PARSER, false, false);
-        PARSER.declareString(GeoGridAggregationBuilder::type, GeoHashGridParams.FIELD_TYPE);
-        PARSER.declareField(GeoGridAggregationBuilder::parsePrecision, GeoHashGridParams.FIELD_PRECISION, ObjectParser.ValueType.INT);
+        PARSER = new ConstructingObjectParser<>(GeoGridAggregationBuilder.NAME, false,
+            (a, name) -> new GeoGridAggregationBuilder(name, (String) a[0]));
+
+        PARSER.declareString(optionalConstructorArg(), GeoHashGridParams.FIELD_TYPE);
+        PARSER.declareField(
+            GeoGridAggregationBuilder::precisionRaw,
+            GeoGridAggregationBuilder::parsePrecision,
+            GeoHashGridParams.FIELD_PRECISION,
+            ObjectParser.ValueType.VALUE);
         PARSER.declareInt(GeoGridAggregationBuilder::size, GeoHashGridParams.FIELD_SIZE);
         PARSER.declareInt(GeoGridAggregationBuilder::shardSize, GeoHashGridParams.FIELD_SHARD_SIZE);
+
+        ValuesSourceParserHelper.declareGeoFields(PARSER, false, false);
     }
 
-    private static void parsePrecision(XContentParser parser, GeoGridAggregationBuilder builder, Void context)
-            throws IOException {
+    private static Object parsePrecision(XContentParser parser, String name)
+        throws IOException {
+        // Delay actual parsing until builder.precision()
         // In some cases, this value cannot be fully parsed until after we know the type
-        // Store precision as is until the end of parsing.
-        // ValueType.INT could be either a number or a string
-        builder.precisionLocation = parser.getTokenLocation();
-        if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) {
-            builder.precisionRaw = parser.intValue();
-        } else {
-            builder.precisionRaw = parser.text();
+        final XContentParser.Token token = parser.currentToken();
+        switch (token) {
+            case VALUE_NUMBER:
+                return parser.intValue();
+            case VALUE_STRING:
+                return parser.text();
+            default:
+                throw new XContentParseException(parser.getTokenLocation(),
+                    "[geohash_grid] failed to parse field [precision] in [" + name +
+                        "]. It must be either an integer or a string");
         }
     }
 
-    public static GeoGridAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException {
-        GeoGridAggregationBuilder builder = PARSER.parse(parser,
-            new GeoGridAggregationBuilder(aggregationName), null);
-
-        try {
-            // delayed precision validation
-            final GeoHashTypeProvider typeHandler = builder.type.getHandler();
-
-            if (builder.precisionRaw == null) {
-                builder.precision(typeHandler.getDefaultPrecision());
-            } else if (builder.precisionRaw instanceof String) {
-                builder.precision(typeHandler.parsePrecisionString((String) builder.precisionRaw));
-            } else {
-                builder.precision((int) builder.precisionRaw);
-            }
-
-            return builder;
-        } catch (Exception e) {
-            throw new XContentParseException(builder.precisionLocation,
-                "[geohash_grid] failed to parse field [precision]", e);
-        }
+    public static GeoGridAggregationBuilder parse(String aggregationName, XContentParser parser) {
+        return PARSER.apply(parser, aggregationName);
     }
 
-    private GeoHashType type = GeoHashType.DEFAULT;
-
-    /**
-     * Contains unparsed precision value during the parsing.
-     * This value will be converted into an integer precision at the end of parsing.
-     */
-    private Object precisionRaw = null;
-
-    /**
-     * Stores the location of the precision parameter, in case precision is
-     * incorrect and an error has to be reported to the user.  Only valid during parsing.
-     */
-    private XContentLocation precisionLocation = null;
-
-    private int precision = GeoHashType.DEFAULT.getHandler().getDefaultPrecision();
-
+    private final GeoHashType type;
+    private int precision;
     private int requiredSize = DEFAULT_MAX_NUM_CELLS;
     private int shardSize = -1;
 
-    public GeoGridAggregationBuilder(String name) {
+    public GeoGridAggregationBuilder(String name, String type) {
+        this(name, parseType(type, name));
+    }
+
+    public GeoGridAggregationBuilder(String name, GeoHashType type) {
         super(name, ValuesSourceType.GEOPOINT, ValueType.GEOPOINT);
+        this.type = type;
+        this.precision = type.getHandler().getDefaultPrecision();
     }
 
     protected GeoGridAggregationBuilder(GeoGridAggregationBuilder clone, Builder factoriesBuilder, Map<String, Object> metaData) {
@@ -160,9 +147,9 @@ protected void innerWriteTo(StreamOutput out) throws IOException {
         out.writeVInt(shardSize);
     }
 
-    public GeoGridAggregationBuilder type(String type) {
+    private static GeoHashType parseType(String type, String name) {
         try {
-            this.type = GeoHashType.forString(type);
+            return type == null ? GeoHashType.DEFAULT : GeoHashType.forString(type);
         } catch (IllegalArgumentException e) {
             throw new IllegalArgumentException(
                 "[type] is not valid. Allowed values: " +
@@ -171,16 +158,25 @@ public GeoGridAggregationBuilder type(String type) {
                         .collect(Collectors.joining(", ")) +
                     ". Found [" + type + "] in [" + name + "]");
         }
-        // Some tests bypass parsing steps, so keep the default precision consistent.
-        // Note that tests must set precision after setting the type
-        this.precision = this.type.getHandler().getDefaultPrecision();
-        return this;
     }
 
     public GeoHashType type() {
         return type;
     }
 
+    private GeoGridAggregationBuilder precisionRaw(Object precision) {
+        final GeoHashTypeProvider typeHandler = this.type.getHandler();
+
+        if (precision == null) {
+            this.precision(typeHandler.getDefaultPrecision());
+        } else if (precision instanceof String) {
+            this.precision(typeHandler.parsePrecisionString((String) precision));
+        } else {
+            this.precision((int) precision);
+        }
+        return this;
+    }
+
     public GeoGridAggregationBuilder precision(int precision) {
         this.precision = type.getHandler().validatePrecision(precision);
         return this;
@@ -193,7 +189,7 @@ public int precision() {
     public GeoGridAggregationBuilder size(int size) {
         if (size <= 0) {
             throw new IllegalArgumentException(
-                    "[size] must be greater than 0. Found [" + size + "] in [" + name + "]");
+                "[size] must be greater than 0. Found [" + size + "] in [" + name + "]");
         }
         this.requiredSize = size;
         return this;
@@ -206,7 +202,7 @@ public int size() {
     public GeoGridAggregationBuilder shardSize(int shardSize) {
         if (shardSize <= 0) {
             throw new IllegalArgumentException(
-                    "[shardSize] must be greater than 0. Found [" + shardSize + "] in [" + name + "]");
+                "[shardSize] must be greater than 0. Found [" + shardSize + "] in [" + name + "]");
         }
         this.shardSize = shardSize;
         return this;
@@ -218,8 +214,8 @@ public int shardSize() {
 
     @Override
     protected ValuesSourceAggregatorFactory<ValuesSource.GeoPoint, ?> innerBuild(SearchContext context,
-            ValuesSourceConfig<ValuesSource.GeoPoint> config, AggregatorFactory<?> parent, Builder subFactoriesBuilder)
-                    throws IOException {
+                                                                                 ValuesSourceConfig<ValuesSource.GeoPoint> config, AggregatorFactory<?> parent, Builder subFactoriesBuilder)
+        throws IOException {
         int shardSize = this.shardSize;
 
         int requiredSize = this.requiredSize;
@@ -232,14 +228,14 @@ public int shardSize() {
 
         if (requiredSize <= 0 || shardSize <= 0) {
             throw new ElasticsearchException(
-                    "parameters [required_size] and [shard_size] must be >0 in geohash_grid aggregation [" + name + "].");
+                "parameters [required_size] and [shard_size] must be >0 in geohash_grid aggregation [" + name + "].");
         }
 
         if (shardSize < requiredSize) {
             shardSize = requiredSize;
         }
         return new GeoHashGridAggregatorFactory(name, config, type, precision, requiredSize, shardSize, context, parent,
-                subFactoriesBuilder, metaData);
+            subFactoriesBuilder, metaData);
     }
 
     @Override

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashHandler.java

@@ -23,6 +23,9 @@
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoUtils;
 
+/**
+ * A simple wrapper for GeoUtils handling of the geohash hashing algorithm
+ */
 public class GeoHashHandler implements GeoHashTypeProvider {
     @Override
     public int getDefaultPrecision() {
@@ -50,7 +53,7 @@ public String hashAsString(long hash) {
     }
 
     @Override
-    public GeoPoint hashAsObject(long hash) {
+    public GeoPoint hashAsGeoPoint(long hash) {
         return GeoPoint.fromGeohash(hash);
     }
 }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashType.java

@@ -68,7 +68,7 @@ public void writeTo(StreamOutput out) throws IOException {
         if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
             out.writeEnum(this);
         } else if (this != DEFAULT) {
-            throw new UnsupportedOperationException("Geo aggregation type [" + this +
+            throw new UnsupportedOperationException("Geo aggregation type [" + toString() +
                 "] is not supported by the node version " + out.getVersion().toString());
         }
     }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashTypeProvider.java

@@ -60,5 +60,5 @@ public interface GeoHashTypeProvider {
      * @param hash as generated by the {@link #calculateHash}
      * @return center of the grid cell
      */
-    GeoPoint hashAsObject(long hash);
+    GeoPoint hashAsGeoPoint(long hash);
 }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/InternalGeoHashGrid.java

@@ -19,7 +19,6 @@
 package org.elasticsearch.search.aggregations.bucket.geogrid;
 
 import org.apache.lucene.util.PriorityQueue;
-import org.elasticsearch.common.geo.GeoHashUtils;
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -86,8 +85,7 @@ public String getKeyAsString() {
 
         @Override
         public GeoPoint getKey() {
-            // TODO/FIXME:  is it ok to change from GeoPoint to Object, and return different types?
-            return type.getHandler().hashAsObject(geohashAsLong);
+            return type.getHandler().hashAsGeoPoint(geohashAsLong);
         }
 
         @Override

server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java

@@ -59,10 +59,8 @@ public static int maxPrecision(GeoHashType type) {
     @Override
     protected GeoGridAggregationBuilder createTestAggregatorBuilder() {
         String name = randomAlphaOfLengthBetween(3, 20);
-        GeoGridAggregationBuilder factory = new GeoGridAggregationBuilder(name);
-        if (randomBoolean()) {
-            factory.type(randomType().toString());
-        }
+        String type = randomBoolean() ? randomType().toString() : null;
+        GeoGridAggregationBuilder factory = new GeoGridAggregationBuilder(name, type);
         if (randomBoolean()) {
             factory.precision(randomPrecision(factory.type()));
         }

server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java

@@ -116,8 +116,7 @@ private void testCase(Query query, String field, GeoHashType type, int precision
         IndexReader indexReader = DirectoryReader.open(directory);
         IndexSearcher indexSearcher = newSearcher(indexReader, true, true);
 
-        GeoGridAggregationBuilder aggregationBuilder = new GeoGridAggregationBuilder("_name").field(field);
-        aggregationBuilder.type(type.name());
+        GeoGridAggregationBuilder aggregationBuilder = new GeoGridAggregationBuilder("_name", type).field(field);
         aggregationBuilder.precision(precision);
         MappedFieldType fieldType = new GeoPointFieldMapper.GeoPointFieldType();
         fieldType.setHasDocValues(true);