Convert geo field mappers to Parametrized form (#63836)

Relates to #62988
elastic · Oct 22, 2020 · bfaf304 · bfaf304
1 parent 3efcc55
commit bfaf304
Show file tree

Hide file tree

Showing 43 changed files with 962 additions and 1,333 deletions.
diff --git a/docs/reference/migration/migrate_8_0/mappings.asciidoc b/docs/reference/migration/migrate_8_0/mappings.asciidoc
@@ -95,3 +95,21 @@ For a detailed migration guide, see the {ref}/migrate-to-java-time.html[Java
 time migration guide].
 ====
 // end::notable-breaking-changes[]
+
+[[geo-shape-strategy]]
+.The `strategy` parameter on `geo_shape` mappings now rejects unknown strategies
+[%collapsible]
+====
+*Details* +
+The only permissible values for the `strategy` parameter on `geo_shape` mappings
+are `term` and `recursive`. In 7.x if a non-permissible value was used as a
+parameter here, the mapping would silently fall back to using `recursive`.  The
+mapping will now be rejected instead.
+
+*Impact* +
+This will have no impact on existing mappings created with non-permissible
+strategy values, as they will already be serializing themselves as if they
+had been configured as `recursive`.  New indexes will need to use one of the
+permissible strategies, or preferably not define a strategy at all and use
+the far more efficient BKD index.
+====
diff --git a/modules/geo/src/main/java/org/elasticsearch/geo/GeoPlugin.java b/modules/geo/src/main/java/org/elasticsearch/geo/GeoPlugin.java
@@ -31,6 +31,6 @@ public class GeoPlugin extends Plugin implements MapperPlugin {
 
     @Override
     public Map<String, Mapper.TypeParser> getMappers() {
-        return Collections.singletonMap(GeoShapeFieldMapper.CONTENT_TYPE, new GeoShapeFieldMapper.TypeParser());
+        return Collections.singletonMap(GeoShapeFieldMapper.CONTENT_TYPE, GeoShapeFieldMapper.PARSER);
     }
 }
diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/geo/GeoShapeIntegrationIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/geo/GeoShapeIntegrationIT.java
@@ -154,7 +154,7 @@ public void testMappingUpdate() throws Exception {
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> client().admin().indices()
             .preparePutMapping("test")
             .setSource(update, XContentType.JSON).get());
-        assertThat(e.getMessage(), containsString("using [BKD] strategy cannot be merged with"));
+        assertThat(e.getMessage(), containsString("mapper [shape] of type [geo_shape] cannot change strategy from [BKD] to [recursive]"));
     }
 
     /**

diff --git a/server/src/main/java/org/elasticsearch/common/Strings.java b/server/src/main/java/org/elasticsearch/common/Strings.java
@@ -411,6 +411,25 @@ public static String[] toStringArray(Collection<String> collection) {
         return collection.toArray(new String[collection.size()]);
     }
 
+    /**
+     * Concatenate two string arrays into a third
+     */
+    public static String[] concatStringArrays(String[] first, String[] second) {
+        if (first == null && second == null) {
+            return Strings.EMPTY_ARRAY;
+        }
+        if (first == null || first.length == 0) {
+            return second;
+        }
+        if (second == null || second.length == 0) {
+            return first;
+        }
+        String[] concat = new String[first.length + second.length];
+        System.arraycopy(first, 0, concat, 0, first.length);
+        System.arraycopy(second, 0, concat, first.length, second.length);
+        return concat;
+    }
+
     /**
      * Tokenize the specified string by commas to a set, trimming whitespace and ignoring empty tokens.
      *
@@ -879,7 +898,7 @@ public static String padStart(String s, int minimumLength, char c) {
             return sb.toString();
         }
     }
-    
+
     public static String toLowercaseAscii(String in) {
         StringBuilder out = new StringBuilder();
         Iterator<Integer> iter = in.codePoints().iterator();
@@ -892,5 +911,5 @@ public static String toLowercaseAscii(String in) {
             }
         }
         return out.toString();
-    }    
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java
@@ -25,10 +25,10 @@
 import org.apache.lucene.index.IndexableField;
 import org.apache.lucene.util.BitUtil;
 import org.apache.lucene.util.BytesRef;
+import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.geo.GeoUtils.EffectivePoint;
 import org.elasticsearch.common.xcontent.ToXContentFragment;
 import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.geometry.Geometry;
 import org.elasticsearch.geometry.Point;
 import org.elasticsearch.geometry.Rectangle;
@@ -41,8 +41,6 @@
 import java.util.Arrays;
 import java.util.Locale;
 
-import static org.elasticsearch.index.mapper.AbstractPointGeometryFieldMapper.Names.IGNORE_Z_VALUE;
-
 public class GeoPoint implements ToXContentFragment {
 
     protected double lat;
@@ -265,8 +263,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
 
     public static double assertZValue(final boolean ignoreZValue, double zValue) {
         if (ignoreZValue == false) {
-            throw new ElasticsearchParseException("Exception parsing coordinates: found Z value [{}] but [{}] "
-                + "parameter is [{}]", zValue, IGNORE_Z_VALUE, ignoreZValue);
+            throw new ElasticsearchParseException("Exception parsing coordinates: found Z value [{}] but [ignore_z_value] "
+                + "parameter is [{}]", zValue, ignoreZValue);
         }
         return zValue;
     }

diff --git a/server/src/main/java/org/elasticsearch/common/geo/SpatialStrategy.java b/server/src/main/java/org/elasticsearch/common/geo/SpatialStrategy.java
@@ -54,6 +54,6 @@ public static SpatialStrategy fromString(String strategyName) {
                 return strategy;
             }
         }
-        return null;
+        throw new IllegalArgumentException("Unknown strategy [" + strategyName + "]");
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java
@@ -19,7 +19,6 @@
 package org.elasticsearch.common.geo.parsers;
 
 import org.elasticsearch.ElasticsearchParseException;
-import org.elasticsearch.common.Explicit;
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoShapeType;
 import org.elasticsearch.common.geo.builders.CircleBuilder;
@@ -50,14 +49,10 @@ protected static ShapeBuilder parse(XContentParser parser, AbstractShapeGeometry
         GeometryCollectionBuilder geometryCollections = null;
 
         Orientation orientation = (shapeMapper == null)
-            ? AbstractShapeGeometryFieldMapper.Defaults.ORIENTATION.value()
+            ? Orientation.RIGHT
             : shapeMapper.orientation();
-        Explicit<Boolean> coerce = (shapeMapper == null)
-            ? AbstractShapeGeometryFieldMapper.Defaults.COERCE
-            : shapeMapper.coerce();
-        Explicit<Boolean> ignoreZValue = (shapeMapper == null)
-            ? AbstractShapeGeometryFieldMapper.Defaults.IGNORE_Z_VALUE
-            : shapeMapper.ignoreZValue();
+        boolean coerce = shapeMapper != null && shapeMapper.coerce();
+        boolean ignoreZValue = shapeMapper == null || shapeMapper.ignoreZValue();
 
         String malformedException = null;
 
@@ -78,7 +73,7 @@ protected static ShapeBuilder parse(XContentParser parser, AbstractShapeGeometry
                         }
                     } else if (ShapeParser.FIELD_COORDINATES.match(fieldName, subParser.getDeprecationHandler())) {
                         subParser.nextToken();
-                        CoordinateNode tempNode = parseCoordinates(subParser, ignoreZValue.value());
+                        CoordinateNode tempNode = parseCoordinates(subParser, ignoreZValue);
                         if (coordinateNode != null && tempNode.numDimensions() != coordinateNode.numDimensions()) {
                             throw new ElasticsearchParseException("Exception parsing coordinates: " +
                                 "number of dimensions do not match");
@@ -134,7 +129,7 @@ protected static ShapeBuilder parse(XContentParser parser, AbstractShapeGeometry
             return geometryCollections;
         }
 
-        return shapeType.getBuilder(coordinateNode, radius, orientation, coerce.value());
+        return shapeType.getBuilder(coordinateNode, radius, orientation, coerce);
     }
 
     /**

diff --git a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java
@@ -19,7 +19,6 @@
 package org.elasticsearch.common.geo.parsers;
 
 import org.elasticsearch.ElasticsearchParseException;
-import org.elasticsearch.common.Explicit;
 import org.elasticsearch.common.geo.GeoPoint;
 import org.elasticsearch.common.geo.GeoShapeType;
 import org.elasticsearch.common.geo.builders.CoordinatesBuilder;
@@ -78,9 +77,8 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha
                                                  final AbstractShapeGeometryFieldMapper shapeMapper)
             throws IOException, ElasticsearchParseException {
         try (StringReader reader = new StringReader(parser.text())) {
-            Explicit<Boolean> ignoreZValue = (shapeMapper == null) ? AbstractShapeGeometryFieldMapper.Defaults.IGNORE_Z_VALUE :
-                shapeMapper.ignoreZValue();
-            Explicit<Boolean> coerce = (shapeMapper == null) ? AbstractShapeGeometryFieldMapper.Defaults.COERCE : shapeMapper.coerce();
+            boolean coerce = shapeMapper != null && shapeMapper.coerce();
+            boolean ignoreZValue = shapeMapper == null || shapeMapper.ignoreZValue();
             // setup the tokenizer; configured to read words w/o numbers
             StreamTokenizer tokenizer = new StreamTokenizer(reader);
             tokenizer.resetSyntax();
@@ -93,7 +91,7 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha
             tokenizer.wordChars('.', '.');
             tokenizer.whitespaceChars(0, ' ');
             tokenizer.commentChar('#');
-            ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue.value(), coerce.value());
+            ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue, coerce);
             checkEOF(tokenizer);
             return builder;
         }
@@ -258,7 +256,7 @@ private static PolygonBuilder parsePolygon(StreamTokenizer stream, final boolean
             return null;
         }
         PolygonBuilder builder = new PolygonBuilder(parseLinearRing(stream, ignoreZValue, coerce),
-            AbstractShapeGeometryFieldMapper.Defaults.ORIENTATION.value());
+            ShapeBuilder.Orientation.RIGHT);
         while (nextCloserOrComma(stream).equals(COMMA)) {
             builder.hole(parseLinearRing(stream, ignoreZValue, coerce));
         }