[Remove] Type mappings from GeoShapeQueryBuilder (opensearch-project#…

…2322) * Remove type end-points from GeoShapeBuilder Signed-off-by: Suraj Singh <[email protected]> * Fix integration test failures Signed-off-by: Suraj Singh <[email protected]>
kartg · Mar 5, 2022 · d5e58a2 · d5e58a2
1 parent be64af2
commit d5e58a2
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 153 deletions.
diff --git a/server/src/internalClusterTest/java/org/opensearch/search/geo/GeoShapeIntegrationIT.java b/server/src/internalClusterTest/java/org/opensearch/search/geo/GeoShapeIntegrationIT.java
@@ -240,7 +240,7 @@ public void testIndexShapeRouting() throws Exception {
         indexRandom(true, client().prepareIndex("test").setId("0").setSource(source, XContentType.JSON).setRouting("ABC"));
 
         SearchResponse searchResponse = client().prepareSearch("test")
-            .setQuery(geoShapeQuery("shape", "0", "doc").indexedShapeIndex("test").indexedShapeRouting("ABC"))
+            .setQuery(geoShapeQuery("shape", "0").indexedShapeIndex("test").indexedShapeRouting("ABC"))
             .get();
 
         assertThat(searchResponse.getHits().getTotalHits().value, equalTo(1L));

diff --git a/...r/src/internalClusterTest/java/org/opensearch/search/geo/LegacyGeoShapeIntegrationIT.java b/...r/src/internalClusterTest/java/org/opensearch/search/geo/LegacyGeoShapeIntegrationIT.java
@@ -218,7 +218,7 @@ public void testIndexShapeRouting() throws Exception {
         indexRandom(true, client().prepareIndex("test").setId("0").setSource(source, XContentType.JSON).setRouting("ABC"));
 
         SearchResponse searchResponse = client().prepareSearch("test")
-            .setQuery(geoShapeQuery("shape", "0", "doc").indexedShapeIndex("test").indexedShapeRouting("ABC"))
+            .setQuery(geoShapeQuery("shape", "0").indexedShapeIndex("test").indexedShapeRouting("ABC"))
             .get();
 
         assertThat(searchResponse.getHits().getTotalHits().value, equalTo(1L));

diff --git a/server/src/main/java/org/opensearch/index/query/AbstractGeometryQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/AbstractGeometryQueryBuilder.java
@@ -35,11 +35,11 @@
 import org.apache.lucene.search.MatchNoDocsQuery;
 import org.apache.lucene.search.Query;
 import org.apache.lucene.util.SetOnce;
+import org.opensearch.Version;
 import org.opensearch.action.ActionListener;
 import org.opensearch.action.get.GetRequest;
 import org.opensearch.action.get.GetResponse;
 import org.opensearch.client.Client;
-import org.opensearch.common.Nullable;
 import org.opensearch.common.ParseField;
 import org.opensearch.common.ParsingException;
 import org.opensearch.common.geo.GeoJson;
@@ -56,6 +56,7 @@
 import org.opensearch.common.xcontent.XContentParser;
 import org.opensearch.geometry.Geometry;
 import org.opensearch.index.mapper.MappedFieldType;
+import org.opensearch.index.mapper.MapperService;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -66,9 +67,6 @@
  */
 public abstract class AbstractGeometryQueryBuilder<QB extends AbstractGeometryQueryBuilder<QB>> extends AbstractQueryBuilder<QB> {
 
-    static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Types are deprecated in [geo_shape] queries. "
-        + "The type should no longer be specified in the [indexed_shape] section.";
-
     public static final String DEFAULT_SHAPE_INDEX_NAME = "shapes";
     public static final String DEFAULT_SHAPE_FIELD_NAME = "shape";
     public static final ShapeRelation DEFAULT_SHAPE_RELATION = ShapeRelation.INTERSECTS;
@@ -80,7 +78,6 @@ public abstract class AbstractGeometryQueryBuilder<QB extends AbstractGeometryQu
     protected static final ParseField RELATION_FIELD = new ParseField("relation");
     protected static final ParseField INDEXED_SHAPE_FIELD = new ParseField("indexed_shape");
     protected static final ParseField SHAPE_ID_FIELD = new ParseField("id");
-    protected static final ParseField SHAPE_TYPE_FIELD = new ParseField("type");
     protected static final ParseField SHAPE_INDEX_FIELD = new ParseField("index");
     protected static final ParseField SHAPE_PATH_FIELD = new ParseField("path");
     protected static final ParseField SHAPE_ROUTING_FIELD = new ParseField("routing");
@@ -90,7 +87,6 @@ public abstract class AbstractGeometryQueryBuilder<QB extends AbstractGeometryQu
     protected final Supplier<Geometry> supplier;
 
     protected final String indexedShapeId;
-    protected final String indexedShapeType;
 
     protected Geometry shape;
     protected String indexedShapeIndex = DEFAULT_SHAPE_INDEX_NAME;
@@ -113,7 +109,7 @@ public abstract class AbstractGeometryQueryBuilder<QB extends AbstractGeometryQu
      */
     @Deprecated
     protected AbstractGeometryQueryBuilder(String fieldName, ShapeBuilder shape) {
-        this(fieldName, shape == null ? null : shape.buildGeometry(), null, null);
+        this(fieldName, shape == null ? null : shape.buildGeometry(), null);
     }
 
     /**
@@ -126,7 +122,7 @@ protected AbstractGeometryQueryBuilder(String fieldName, ShapeBuilder shape) {
      *            Shape used in the Query
      */
     public AbstractGeometryQueryBuilder(String fieldName, Geometry shape) {
-        this(fieldName, shape, null, null);
+        this(fieldName, shape, null);
     }
 
     /**
@@ -139,28 +135,10 @@ public AbstractGeometryQueryBuilder(String fieldName, Geometry shape) {
      *            ID of the indexed Shape that will be used in the Query
      */
     protected AbstractGeometryQueryBuilder(String fieldName, String indexedShapeId) {
-        this(fieldName, (Geometry) null, indexedShapeId, null);
+        this(fieldName, (Geometry) null, indexedShapeId);
     }
 
-    /**
-     * Creates a new AbstractGeometryQueryBuilder whose Query will be against the given
-     * field name and will use the Shape found with the given ID in the given
-     * type
-     *
-     * @param fieldName
-     *            Name of the field that will be filtered
-     * @param indexedShapeId
-     *            ID of the indexed Shape that will be used in the Query
-     * @param indexedShapeType
-     *            Index type of the indexed Shapes
-     * @deprecated use {@link #AbstractGeometryQueryBuilder(String, String)} instead
-     */
-    @Deprecated
-    protected AbstractGeometryQueryBuilder(String fieldName, String indexedShapeId, String indexedShapeType) {
-        this(fieldName, (Geometry) null, indexedShapeId, indexedShapeType);
-    }
-
-    protected AbstractGeometryQueryBuilder(String fieldName, Geometry shape, String indexedShapeId, @Nullable String indexedShapeType) {
+    protected AbstractGeometryQueryBuilder(String fieldName, Geometry shape, String indexedShapeId) {
         if (fieldName == null) {
             throw new IllegalArgumentException("fieldName is required");
         }
@@ -170,21 +148,21 @@ protected AbstractGeometryQueryBuilder(String fieldName, Geometry shape, String
         this.fieldName = fieldName;
         this.shape = shape;
         this.indexedShapeId = indexedShapeId;
-        this.indexedShapeType = indexedShapeType;
         this.supplier = null;
     }
 
-    protected AbstractGeometryQueryBuilder(
-        String fieldName,
-        Supplier<Geometry> supplier,
-        String indexedShapeId,
-        @Nullable String indexedShapeType
-    ) {
+    protected AbstractGeometryQueryBuilder(String fieldName, Supplier<Geometry> supplier, String indexedShapeId) {
+        if (fieldName == null) {
+            throw new IllegalArgumentException("fieldName is required");
+        }
+        if (supplier == null && indexedShapeId == null) {
+            throw new IllegalArgumentException("either shape or indexedShapeId is required");
+        }
+
         this.fieldName = fieldName;
         this.shape = null;
         this.supplier = supplier;
         this.indexedShapeId = indexedShapeId;
-        this.indexedShapeType = indexedShapeType;
     }
 
     /**
@@ -196,11 +174,13 @@ protected AbstractGeometryQueryBuilder(StreamInput in) throws IOException {
         if (in.readBoolean()) {
             shape = GeometryIO.readGeometry(in);
             indexedShapeId = null;
-            indexedShapeType = null;
         } else {
             shape = null;
             indexedShapeId = in.readOptionalString();
-            indexedShapeType = in.readOptionalString();
+            if (in.getVersion().before(Version.V_2_0_0)) {
+                String type = in.readOptionalString();
+                assert MapperService.SINGLE_MAPPING_NAME.equals(type) : "Expected type [_doc], got [" + type + "]";
+            }
             indexedShapeIndex = in.readOptionalString();
             indexedShapePath = in.readOptionalString();
             indexedShapeRouting = in.readOptionalString();
@@ -222,7 +202,9 @@ protected void doWriteTo(StreamOutput out) throws IOException {
             GeometryIO.writeGeometry(out, shape);
         } else {
             out.writeOptionalString(indexedShapeId);
-            out.writeOptionalString(indexedShapeType);
+            if (out.getVersion().before(Version.V_2_0_0)) {
+                out.writeOptionalString(MapperService.SINGLE_MAPPING_NAME);
+            }
             out.writeOptionalString(indexedShapeIndex);
             out.writeOptionalString(indexedShapePath);
             out.writeOptionalString(indexedShapeRouting);
@@ -266,17 +248,6 @@ public String indexedShapeId() {
         return indexedShapeId;
     }
 
-    /**
-     * @return the document type of the indexed Shape that will be used in the
-     *         Query
-     *
-     * @deprecated Types are in the process of being removed.
-     */
-    @Deprecated
-    public String indexedShapeType() {
-        return indexedShapeType;
-    }
-
     /**
      * Sets the name of the index where the indexed Shape can be found
      *
@@ -382,12 +353,11 @@ public boolean ignoreUnmapped() {
     /** creates a new ShapeQueryBuilder from the provided field name and shape builder */
     protected abstract AbstractGeometryQueryBuilder<QB> newShapeQueryBuilder(String fieldName, Geometry shape);
 
-    /** creates a new ShapeQueryBuilder from the provided field name, supplier, indexed shape id, and indexed shape type */
+    /** creates a new ShapeQueryBuilder from the provided field name, supplier, indexed shape id */
     protected abstract AbstractGeometryQueryBuilder<QB> newShapeQueryBuilder(
         String fieldName,
         Supplier<Geometry> shapeSupplier,
-        String indexedShapeId,
-        String indexedShapeType
+        String indexedShapeId
     );
 
     @Override
@@ -480,9 +450,6 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
             GeoJson.toXContent(shape, builder, params);
         } else {
             builder.startObject(INDEXED_SHAPE_FIELD.getPreferredName()).field(SHAPE_ID_FIELD.getPreferredName(), indexedShapeId);
-            if (indexedShapeType != null) {
-                builder.field(SHAPE_TYPE_FIELD.getPreferredName(), indexedShapeType);
-            }
             if (indexedShapeIndex != null) {
                 builder.field(SHAPE_INDEX_FIELD.getPreferredName(), indexedShapeIndex);
             }
@@ -514,7 +481,6 @@ protected boolean doEquals(AbstractGeometryQueryBuilder other) {
             && Objects.equals(indexedShapeId, other.indexedShapeId)
             && Objects.equals(indexedShapeIndex, other.indexedShapeIndex)
             && Objects.equals(indexedShapePath, other.indexedShapePath)
-            && Objects.equals(indexedShapeType, other.indexedShapeType)
             && Objects.equals(indexedShapeRouting, other.indexedShapeRouting)
             && Objects.equals(relation, other.relation)
             && Objects.equals(shape, other.shape)
@@ -529,7 +495,6 @@ protected int doHashCode() {
             indexedShapeId,
             indexedShapeIndex,
             indexedShapePath,
-            indexedShapeType,
             indexedShapeRouting,
             relation,
             shape,
@@ -552,7 +517,7 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws
                     listener.onResponse(null);
                 }, listener::onFailure));
             });
-            return newShapeQueryBuilder(this.fieldName, supplier::get, this.indexedShapeId, this.indexedShapeType).relation(relation);
+            return newShapeQueryBuilder(this.fieldName, supplier::get, this.indexedShapeId).relation(relation);
         }
         return this;
     }
@@ -564,7 +529,6 @@ protected abstract static class ParsedGeometryQueryParams {
         public ShapeBuilder shape;
 
         public String id = null;
-        public String type = null;
         public String index = null;
         public String shapePath = null;
         public String shapeRouting = null;
@@ -608,8 +572,6 @@ public static ParsedGeometryQueryParams parsedParamsFromXContent(XContentParser
                                 } else if (token.isValue()) {
                                     if (SHAPE_ID_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
                                         params.id = parser.text();
-                                    } else if (SHAPE_TYPE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                                        params.type = parser.text();
                                     } else if (SHAPE_INDEX_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
                                         params.index = parser.text();
                                     } else if (SHAPE_PATH_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {

diff --git a/server/src/main/java/org/opensearch/index/query/GeoShapeQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/GeoShapeQueryBuilder.java
@@ -34,7 +34,6 @@
 
 import org.apache.lucene.search.ConstantScoreQuery;
 import org.apache.lucene.search.Query;
-import org.opensearch.common.Nullable;
 import org.opensearch.common.ParseField;
 import org.opensearch.common.ParsingException;
 import org.opensearch.common.geo.ShapeRelation;
@@ -43,7 +42,6 @@
 import org.opensearch.common.geo.parsers.ShapeParser;
 import org.opensearch.common.io.stream.StreamInput;
 import org.opensearch.common.io.stream.StreamOutput;
-import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.xcontent.XContentBuilder;
 import org.opensearch.common.xcontent.XContentParser;
 import org.opensearch.geometry.Geometry;
@@ -62,8 +60,6 @@
  */
 public class GeoShapeQueryBuilder extends AbstractGeometryQueryBuilder<GeoShapeQueryBuilder> {
     public static final String NAME = "geo_shape";
-    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(GeoShapeQueryBuilder.class);
-
     protected static final ParseField STRATEGY_FIELD = new ParseField("strategy");
 
     private SpatialStrategy strategy;
@@ -97,31 +93,8 @@ public GeoShapeQueryBuilder(String fieldName, ShapeBuilder shape) {
         super(fieldName, shape);
     }
 
-    public GeoShapeQueryBuilder(
-        String fieldName,
-        Supplier<Geometry> shapeSupplier,
-        String indexedShapeId,
-        @Nullable String indexedShapeType
-    ) {
-        super(fieldName, shapeSupplier, indexedShapeId, indexedShapeType);
-    }
-
-    /**
-     * Creates a new GeoShapeQueryBuilder whose Query will be against the given
-     * field name and will use the Shape found with the given ID in the given
-     * type
-     *
-     * @param fieldName
-     *            Name of the field that will be filtered
-     * @param indexedShapeId
-     *            ID of the indexed Shape that will be used in the Query
-     * @param indexedShapeType
-     *            Index type of the indexed Shapes
-     * @deprecated use {@link #GeoShapeQueryBuilder(String, String)} instead
-     */
-    @Deprecated
-    public GeoShapeQueryBuilder(String fieldName, String indexedShapeId, String indexedShapeType) {
-        super(fieldName, indexedShapeId, indexedShapeType);
+    public GeoShapeQueryBuilder(String fieldName, Supplier<Geometry> shapeSupplier, String indexedShapeId) {
+        super(fieldName, shapeSupplier, indexedShapeId);
     }
 
     /**
@@ -223,13 +196,8 @@ protected GeoShapeQueryBuilder newShapeQueryBuilder(String fieldName, Geometry s
     }
 
     @Override
-    protected GeoShapeQueryBuilder newShapeQueryBuilder(
-        String fieldName,
-        Supplier<Geometry> shapeSupplier,
-        String indexedShapeId,
-        String indexedShapeType
-    ) {
-        return new GeoShapeQueryBuilder(fieldName, shapeSupplier, indexedShapeId, indexedShapeType);
+    protected GeoShapeQueryBuilder newShapeQueryBuilder(String fieldName, Supplier<Geometry> shapeSupplier, String indexedShapeId) {
+        return new GeoShapeQueryBuilder(fieldName, shapeSupplier, indexedShapeId);
     }
 
     @Override
@@ -291,14 +259,11 @@ public static GeoShapeQueryBuilder fromXContent(XContentParser parser) throws IO
         );
 
         GeoShapeQueryBuilder builder;
-        if (pgsqp.type != null) {
-            deprecationLogger.deprecate("geo_share_query_with_types", TYPES_DEPRECATION_MESSAGE);
-        }
 
         if (pgsqp.shape != null) {
             builder = new GeoShapeQueryBuilder(pgsqp.fieldName, pgsqp.shape);
         } else {
-            builder = new GeoShapeQueryBuilder(pgsqp.fieldName, pgsqp.id, pgsqp.type);
+            builder = new GeoShapeQueryBuilder(pgsqp.fieldName, pgsqp.id);
         }
 
         if (pgsqp.index != null) {