Improve testing of geo_shape field in the spatial plugin (elastic#77687…

…) (elastic#77834) Adds more integration test to the spatial plugin.
martijnvg · Sep 16, 2021 · 751bbdd · 751bbdd
1 parent ae233a7
commit 751bbdd
Show file tree

Hide file tree

Showing 18 changed files with 613 additions and 74 deletions.
diff --git a/...nalClusterTest/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryLegacyGeoShapeIT.java b/...nalClusterTest/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryLegacyGeoShapeIT.java
@@ -10,11 +10,25 @@
 
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.plugins.Plugin;
+import org.elasticsearch.test.TestLegacyGeoShapeFieldMapperPlugin;
 
 import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
 
 public class GeoBoundingBoxQueryLegacyGeoShapeIT extends GeoBoundingBoxQueryIntegTestCase {
 
+    @Override
+    protected boolean addMockGeoShapeFieldMapper() {
+        return false;
+    }
+
+    @Override
+    protected Collection<Class<? extends Plugin>> nodePlugins() {
+        return Collections.singleton(TestLegacyGeoShapeFieldMapperPlugin.class);
+    }
+
     @Override
     public XContentBuilder getMapping() throws IOException {
         return XContentFactory.jsonBuilder().startObject().startObject("type1")

diff --git a/server/src/internalClusterTest/java/org/elasticsearch/search/geo/LegacyGeoShapeIT.java b/server/src/internalClusterTest/java/org/elasticsearch/search/geo/LegacyGeoShapeIT.java
@@ -11,18 +11,30 @@
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.geometry.Circle;
+import org.elasticsearch.plugins.Plugin;
+import org.elasticsearch.test.TestLegacyGeoShapeFieldMapperPlugin;
 
 import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
 
 import static org.elasticsearch.index.query.QueryBuilders.geoShapeQuery;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
-import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 
 public class LegacyGeoShapeIT extends GeoShapeIntegTestCase {
 
+    @Override
+    protected boolean addMockGeoShapeFieldMapper() {
+        return false;
+    }
+
+    @Override
+    protected Collection<Class<? extends Plugin>> nodePlugins() {
+        return Collections.singleton(TestLegacyGeoShapeFieldMapperPlugin.class);
+    }
+
     @Override
     protected void getGeoShapeMapping(XContentBuilder b) throws IOException {
         b.field("type", "geo_shape");
@@ -34,26 +46,6 @@ protected boolean allowExpensiveQueries() {
         return false;
     }
 
-    public void testMappingUpdate() {
-        // create index
-        assertAcked(client().admin().indices().prepareCreate("test")
-            .addMapping("shape", "shape", "type=geo_shape,strategy=recursive").get());
-        ensureGreen();
-
-        String update ="{\n" +
-            "  \"properties\": {\n" +
-            "    \"shape\": {\n" +
-            "      \"type\": \"geo_shape\"" +
-            "    }\n" +
-            "  }\n" +
-            "}";
-
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> client().admin().indices()
-            .preparePutMapping("test").setType("shape")
-            .setSource(update, XContentType.JSON).get());
-        assertThat(e.getMessage(), containsString("mapper [shape] of type [geo_shape] cannot change strategy from [recursive] to [BKD]"));
-    }
-
     /**
      * Test that the circle is still supported for the legacy shapes
      */

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java
@@ -16,8 +16,8 @@
 import org.elasticsearch.common.geo.GeoShapeUtils;
 import org.elasticsearch.common.geo.GeometryFormatterFactory;
 import org.elasticsearch.common.geo.GeometryParser;
-import org.elasticsearch.common.geo.ShapeRelation;
 import org.elasticsearch.common.geo.Orientation;
+import org.elasticsearch.common.geo.ShapeRelation;
 import org.elasticsearch.common.logging.DeprecationCategory;
 import org.elasticsearch.common.logging.DeprecationLogger;
 import org.elasticsearch.geometry.Geometry;
@@ -145,21 +145,11 @@ protected Function<List<Geometry>, List<Object>> getFormatter(String format) {
         }
     }
 
-    @SuppressWarnings("deprecation")
+    @Deprecated
     public static Mapper.TypeParser PARSER = (name, node, parserContext) -> {
-        FieldMapper.Builder builder;
         boolean ignoreMalformedByDefault = IGNORE_MALFORMED_SETTING.get(parserContext.getSettings());
         boolean coerceByDefault = COERCE_SETTING.get(parserContext.getSettings());
-        if (parserContext.indexVersionCreated().before(Version.V_6_6_0)
-            || LegacyGeoShapeFieldMapper.containsDeprecatedParameter(node.keySet())) {
-            builder = new LegacyGeoShapeFieldMapper.Builder(
-                name,
-                parserContext.indexVersionCreated(),
-                ignoreMalformedByDefault,
-                coerceByDefault);
-        } else {
-            builder = new Builder(name, ignoreMalformedByDefault, coerceByDefault);
-        }
+        FieldMapper.Builder builder = new Builder(name, ignoreMalformedByDefault, coerceByDefault);
         builder.parse(name, parserContext, node);
         return builder;
     };
@@ -193,17 +183,6 @@ public FieldMapper.Builder getMergeBuilder() {
         ).init(this);
     }
 
-    @Override
-    @SuppressWarnings("deprecation")
-    protected void checkIncomingMergeType(FieldMapper mergeWith) {
-        if (mergeWith instanceof LegacyGeoShapeFieldMapper) {
-            String strategy = ((LegacyGeoShapeFieldMapper)mergeWith).strategy();
-            throw new IllegalArgumentException("mapper [" + name()
-                + "] of type [geo_shape] cannot change strategy from [BKD] to [" + strategy + "]");
-        }
-        super.checkIncomingMergeType(mergeWith);
-    }
-
     @Override
     protected void index(DocumentParserContext context, Geometry geometry) throws IOException {
         if (geometry == null) {

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java
@@ -300,6 +300,19 @@ public LegacyGeoShapeFieldMapper build(MapperBuilderContext context) {
         }
     }
 
+    @Deprecated
+    public static Mapper.TypeParser PARSER = (name, node, parserContext) -> {
+        boolean ignoreMalformedByDefault = IGNORE_MALFORMED_SETTING.get(parserContext.getSettings());
+        boolean coerceByDefault = COERCE_SETTING.get(parserContext.getSettings());
+        FieldMapper.Builder builder = new LegacyGeoShapeFieldMapper.Builder(
+                name,
+                parserContext.indexVersionCreated(),
+                ignoreMalformedByDefault,
+                coerceByDefault);
+        builder.parse(name, parserContext, node);
+        return builder;
+    };
+
     private static class LegacyGeoShapeParser extends Parser<ShapeBuilder<?, ?, ?>> {
 
         private LegacyGeoShapeParser() {
@@ -507,9 +520,10 @@ public FieldMapper.Builder getMergeBuilder() {
 
     @Override
     protected void checkIncomingMergeType(FieldMapper mergeWith) {
-        if (mergeWith instanceof GeoShapeFieldMapper) {
+        if (mergeWith instanceof AbstractShapeGeometryFieldMapper<?>
+            && (mergeWith instanceof LegacyGeoShapeFieldMapper) == false) {
             throw new IllegalArgumentException("mapper [" + name()
-                + "] of type [geo_shape] cannot change strategy from [" + strategy() + "] to [BKD]");
+                + "] of type [geo_shape] cannot change strategy from [recursive] to [BKD]");
         }
         super.checkIncomingMergeType(mergeWith);
     }

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldFilterMapperPluginTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldFilterMapperPluginTests.java
@@ -294,9 +294,7 @@ public Function<String, Predicate<String>> getFieldFilter() {
             "              \"type\": \"geo_point\"\n" +
             "            },\n" +
             "            \"area_visible\": {\n" +
-            "              \"type\": \"geo_shape\",  \n" +
-            "              \"tree\": \"quadtree\",\n" +
-            "              \"precision\": \"1m\"\n" +
+            "              \"type\": \"geo_shape\"\n" +
             "            }\n" +
             "          }\n" +
             "        },\n" +

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java
@@ -180,23 +180,6 @@ public void testGeoShapeMapperMerge() throws Exception {
         assertThat(geoShapeFieldMapper.fieldType().orientation(), equalTo(Orientation.CW));
     }
 
-    public void testGeoShapeLegacyMerge() throws Exception {
-        MapperService m = createMapperService(fieldMapping(b -> b.field("type", "geo_shape")));
-        Exception e = expectThrows(IllegalArgumentException.class,
-            () -> merge(m, fieldMapping(b -> b.field("type", "geo_shape").field("strategy", "recursive"))));
-
-        assertThat(e.getMessage(),
-            containsString("mapper [field] of type [geo_shape] cannot change strategy from [BKD] to [recursive]"));
-        assertFieldWarnings("strategy");
-
-        MapperService lm = createMapperService(fieldMapping(b -> b.field("type", "geo_shape").field("strategy", "recursive")));
-        e = expectThrows(IllegalArgumentException.class,
-            () -> merge(lm, fieldMapping(b -> b.field("type", "geo_shape"))));
-        assertThat(e.getMessage(),
-            containsString("mapper [field] of type [geo_shape] cannot change strategy from [recursive] to [BKD]"));
-        assertFieldWarnings("strategy");
-    }
-
     public void testSerializeDefaults() throws Exception {
         DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping));
         assertThat(

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapperTests.java
@@ -25,7 +25,7 @@
 import org.elasticsearch.geometry.Point;
 import org.elasticsearch.index.query.SearchExecutionContext;
 import org.elasticsearch.plugins.Plugin;
-import org.elasticsearch.test.TestGeoShapeFieldMapperPlugin;
+import org.elasticsearch.test.TestLegacyGeoShapeFieldMapperPlugin;
 
 import java.io.IOException;
 import java.util.Collection;
@@ -95,7 +95,7 @@ protected void registerParameters(ParameterChecker checker) throws IOException {
 
     @Override
     protected Collection<? extends Plugin> getPlugins() {
-        return List.of(new TestGeoShapeFieldMapperPlugin());
+        return List.of(new TestLegacyGeoShapeFieldMapperPlugin());
     }
 
     @Override
@@ -595,10 +595,10 @@ public void testDisallowExpensiveQueries() throws IOException {
         LegacyGeoShapeFieldMapper geoShapeFieldMapper = (LegacyGeoShapeFieldMapper) fieldMapper;
 
         ElasticsearchException e = expectThrows(ElasticsearchException.class,
-                () -> geoShapeFieldMapper.fieldType().geoShapeQuery(
-                        new Point(-10, 10), "location", SpatialStrategy.TERM, ShapeRelation.INTERSECTS, searchExecutionContext));
+            () -> geoShapeFieldMapper.fieldType().geoShapeQuery(
+                new Point(-10, 10), "location", SpatialStrategy.TERM, ShapeRelation.INTERSECTS, searchExecutionContext));
         assertEquals("[geo-shape] queries on [PrefixTree geo shapes] cannot be executed when " +
-                        "'search.allow_expensive_queries' is set to false.", e.getMessage());
+            "'search.allow_expensive_queries' is set to false.", e.getMessage());
         assertFieldWarnings("tree", "strategy");
     }
 

diff --git a/server/src/test/java/org/elasticsearch/index/query/LegacyGeoShapeFieldQueryTests.java b/server/src/test/java/org/elasticsearch/index/query/LegacyGeoShapeFieldQueryTests.java
@@ -16,12 +16,22 @@
 import org.elasticsearch.geo.GeometryTestUtils;
 import org.elasticsearch.geometry.Geometry;
 import org.elasticsearch.geometry.ShapeType;
+import org.elasticsearch.plugins.Plugin;
+import org.elasticsearch.test.TestLegacyGeoShapeFieldMapperPlugin;
 import org.elasticsearch.test.VersionUtils;
 
 import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
 
 public class LegacyGeoShapeFieldQueryTests extends GeoShapeQueryBuilderTests {
 
+    @SuppressWarnings("deprecation")
+    @Override
+    protected Collection<Class<? extends Plugin>> getPlugins() {
+        return Collections.singletonList(TestLegacyGeoShapeFieldMapperPlugin.class);
+    }
+
     @Override
     protected String fieldName() {
         return GEO_SHAPE_FIELD_NAME;

diff --git a/server/src/test/java/org/elasticsearch/search/geo/LegacyGeoShapeQueryTests.java b/server/src/test/java/org/elasticsearch/search/geo/LegacyGeoShapeQueryTests.java
@@ -21,8 +21,12 @@
 import org.elasticsearch.geometry.Point;
 import org.elasticsearch.index.mapper.LegacyGeoShapeFieldMapper;
 import org.elasticsearch.index.mapper.MapperParsingException;
+import org.elasticsearch.plugins.Plugin;
+import org.elasticsearch.test.TestLegacyGeoShapeFieldMapperPlugin;
 
 import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
 
 import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
@@ -38,6 +42,11 @@ public class LegacyGeoShapeQueryTests extends GeoShapeQueryTestCase {
         LegacyGeoShapeFieldMapper.PrefixTrees.QUADTREE
     };
 
+    @Override
+    protected Collection<Class<? extends Plugin>> getPlugins() {
+        return Collections.singleton(TestLegacyGeoShapeFieldMapperPlugin.class);
+    }
+
     @Override
     protected void createMapping(String indexName, String type, String fieldName, Settings settings) throws Exception {
         final XContentBuilder xcb = XContentFactory.jsonBuilder().startObject()

diff --git a/test/framework/src/main/java/org/elasticsearch/test/TestLegacyGeoShapeFieldMapperPlugin.java b/test/framework/src/main/java/org/elasticsearch/test/TestLegacyGeoShapeFieldMapperPlugin.java
@@ -0,0 +1,34 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+package org.elasticsearch.test;
+
+import org.elasticsearch.index.mapper.LegacyGeoShapeFieldMapper;
+import org.elasticsearch.index.mapper.Mapper;
+import org.elasticsearch.plugins.MapperPlugin;
+import org.elasticsearch.plugins.Plugin;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+/**
+ * Some tests depend on the {@link LegacyGeoShapeFieldMapper}.
+ * This mapper is registered in the spatial-extras module, but used in some integration
+ * tests in server code. The goal is to migrate all of the spatial/geo pieces to the spatial-extras
+ * module such that no tests in server depend on this test plugin
+ */
+@Deprecated
+public class TestLegacyGeoShapeFieldMapperPlugin extends Plugin implements MapperPlugin {
+
+    @Override
+    public Map<String, Mapper.TypeParser> getMappers() {
+        Map<String, Mapper.TypeParser> mappers = new LinkedHashMap<>();
+        mappers.put(LegacyGeoShapeFieldMapper.CONTENT_TYPE, LegacyGeoShapeFieldMapper.PARSER);
+        return Collections.unmodifiableMap(mappers);
+    }
+}
diff --git a/...va/org/elasticsearch/xpack/spatial/search/GeoBoundingBoxQueryGeoShapeWithDocValuesIT.java b/...va/org/elasticsearch/xpack/spatial/search/GeoBoundingBoxQueryGeoShapeWithDocValuesIT.java
@@ -0,0 +1,40 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+package org.elasticsearch.xpack.spatial.search;
+
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.plugins.Plugin;
+import org.elasticsearch.search.geo.GeoBoundingBoxQueryIntegTestCase;
+import org.elasticsearch.xpack.spatial.LocalStateSpatialPlugin;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+
+public class GeoBoundingBoxQueryGeoShapeWithDocValuesIT extends GeoBoundingBoxQueryIntegTestCase {
+
+    @Override
+    protected boolean addMockGeoShapeFieldMapper() {
+        return false;
+    }
+
+    @Override
+    protected Collection<Class<? extends Plugin>>  nodePlugins() {
+        return Collections.singleton(LocalStateSpatialPlugin.class);
+    }
+
+    @Override
+    public XContentBuilder getMapping() throws IOException {
+        XContentBuilder xContentBuilder = XContentFactory.jsonBuilder().startObject().startObject("type1")
+            .startObject("properties").startObject("location").field("type", "geo_shape");
+        xContentBuilder.endObject().endObject().endObject().endObject();
+        return xContentBuilder;
+    }
+}
+