From e177182248f440d3761ee4c534d33045f2427ad2 Mon Sep 17 00:00:00 2001 From: Ignacio Vera Date: Wed, 24 Jan 2024 13:05:04 +0100 Subject: [PATCH] Remove geohash field from GenericPointParser (#104677) None of the APIs support this field so it is not used. More over, there is not test exercising this logic except for GeoTileUtilsTest which is faking the structure, therefore let's remove it. --- .../common/geo/GenericPointParser.java | 44 ++++---------- .../elasticsearch/common/geo/GeoUtils.java | 20 +++---- .../search/geo/GeoPointParsingTests.java | 51 ++-------------- .../index/search/geo/GeoUtilsTests.java | 60 ++----------------- .../xpack/spatial/common/CartesianPoint.java | 8 +-- 5 files changed, 28 insertions(+), 155 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GenericPointParser.java b/server/src/main/java/org/elasticsearch/common/geo/GenericPointParser.java index 681849a9851ec..7c12b1f2cce29 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GenericPointParser.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GenericPointParser.java @@ -30,7 +30,6 @@ public abstract class GenericPointParser { private static final String X_FIELD = "x"; private static final String Y_FIELD = "y"; private static final String Z_FIELD = "z"; - private static final String GEOHASH = "geohash"; private static final String TYPE = "type"; private static final String COORDINATES = "coordinates"; private final Map> fields; @@ -112,9 +111,8 @@ List parseField(XContentSubParser subParser) throws IOException { * @param mapType whether the parser is for 'geo_point' or 'point' * @param xField the name of the first coordinate when constructing points (either 'x' or 'lat') * @param yField the name of the second coordinate when constructing points (either 'y' or 'lon') - * @param supportGeohash whether to support parsing geohash values (only geo_point supports this currently) */ - public GenericPointParser(String mapType, String xField, String yField, boolean supportGeohash) { + public GenericPointParser(String mapType, String xField, String yField) { this.mapType = mapType; this.xField = xField; this.yField = yField; @@ -124,9 +122,6 @@ public GenericPointParser(String mapType, String xField, String yField, boolean fields.put(Z_FIELD, new DoubleFieldParser(Z_FIELD, Z_FIELD)); fields.put(TYPE, new StringFieldParser(TYPE)); fields.put(COORDINATES, new DoubleArrayFieldParser(COORDINATES)); - if (supportGeohash) { - fields.put(GEOHASH, new StringFieldParser(GEOHASH)); - } } public abstract void assertZValue(boolean ignoreZValue, double zValue); @@ -142,11 +137,10 @@ public GenericPointParser(String mapType, String xField, String yField, boolean * @param ignoreZValue {@link XContentParser} to not throw an error if 3 dimensional data is provided * @return new Point parsed from the parser */ - public T parsePoint(XContentParser parser, boolean ignoreZValue, Function fromString, Function fromGeohash) - throws IOException, ElasticsearchParseException { + public T parsePoint(XContentParser parser, boolean ignoreZValue, Function fromString) throws IOException, + ElasticsearchParseException { double x = Double.NaN; double y = Double.NaN; - String geohash = null; String geojsonType = null; List coordinates = null; @@ -162,7 +156,6 @@ public T parsePoint(XContentParser parser, boolean ignoreZValue, Function x = (Double) fieldParser.parseField(subParser); case Y_FIELD -> y = (Double) fieldParser.parseField(subParser); case Z_FIELD -> assertZValue(ignoreZValue, (Double) fieldParser.parseField(subParser)); - case GEOHASH -> geohash = (String) fieldParser.parseField(subParser); case TYPE -> geojsonType = (String) fieldParser.parseField(subParser); case COORDINATES -> coordinates = ((DoubleArrayFieldParser) fieldParser).parseField(subParser); } @@ -175,16 +168,7 @@ public T parsePoint(XContentParser parser, boolean ignoreZValue, Function(); - if (geohash) found.add("geohash"); - if (xy) found.add(xField + "/" + yField); - if (geojson) found.add("GeoJSON"); - if (found.size() > 1) { - throw new ElasticsearchParseException("fields matching more than one point format found: {}", found); - } else if (geohash) { - if (x || y || type || coordinates) { - throw new ElasticsearchParseException(fieldError()); - } - } else if (found.size() == 0) { + private void assertOnlyOneFormat(boolean x, boolean y, boolean coordinates, boolean type) { + final boolean xy = x && y; + final boolean geojson = coordinates && type; + if (xy && geojson) { + throw new ElasticsearchParseException("fields matching more than one point format"); + } else if ((xy || geojson) == false) { if (x) { throw new ElasticsearchParseException("Required [{}]", yField); } else if (y) { diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index ceff7bf41c587..61f1641e9d312 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -410,18 +410,14 @@ public static GeoPoint parseGeoPoint(XContentParser parser, final boolean ignore */ public static GeoPoint parseGeoPoint(XContentParser parser, final boolean ignoreZValue, final EffectivePoint effectivePoint) throws IOException, ElasticsearchParseException { - return geoPointParser.parsePoint(parser, ignoreZValue, value -> { - GeoPoint point = new GeoPoint(); - point.resetFromString(value, ignoreZValue, effectivePoint); - return point; - }, value -> { - GeoPoint point = new GeoPoint(); - point.parseGeoHash(value, effectivePoint); - return point; - }); - } - - private static GenericPointParser geoPointParser = new GenericPointParser<>("geo_point", "lon", "lat", true) { + return geoPointParser.parsePoint( + parser, + ignoreZValue, + value -> new GeoPoint().resetFromString(value, ignoreZValue, effectivePoint) + ); + } + + private static final GenericPointParser geoPointParser = new GenericPointParser<>("geo_point", "lon", "lat") { @Override public void assertZValue(boolean ignoreZValue, double zValue) { diff --git a/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java b/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java index f70d40dcccd2b..0a6e31f44344c 100644 --- a/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java +++ b/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java @@ -21,7 +21,6 @@ import org.elasticsearch.xcontent.json.JsonXContent; import java.io.IOException; -import java.util.HashMap; import java.util.function.DoubleSupplier; import static org.elasticsearch.geometry.utils.Geohash.stringEncode; @@ -124,40 +123,12 @@ public void testInvalidPointEmbeddedObject() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field [location] not supported - must be one of: lon, lat, z, type, coordinates, geohash")); + assertThat(e.getMessage(), is("field [location] not supported - must be one of: lon, lat, z, type, coordinates")); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean())); - assertThat(e.getMessage(), is("field [location] not supported - must be one of: lon, lat, z, type, coordinates, geohash")); - } - } - - public void testInvalidPointHashMix() throws IOException { - HashMap otherFields = new HashMap<>(); - otherFields.put("lat", 0); - otherFields.put("lon", 0); - otherFields.put("type", "Point"); - otherFields.put("coordinates", new double[] { 0.0, 0.0 }); - for (String other : otherFields.keySet()) { - XContentBuilder content = JsonXContent.contentBuilder(); - content.startObject(); - content.field(other, otherFields.get(other)).field("geohash", stringEncode(0d, 0d)); - content.endObject(); - - try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { - parser.nextToken(); - Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either lat/lon, geohash string or type/coordinates")); - } - try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { - parser2.nextToken(); - Exception e = expectThrows( - ElasticsearchParseException.class, - () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean()) - ); - assertThat(e.getMessage(), is("field must be either lat/lon, geohash string or type/coordinates")); - } + assertThat(e.getMessage(), is("field [location] not supported - must be one of: lon, lat, z, type, coordinates")); } } @@ -170,27 +141,13 @@ public void testInvalidField() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field [test] not supported - must be one of: lon, lat, z, type, coordinates, geohash")); + assertThat(e.getMessage(), is("field [test] not supported - must be one of: lon, lat, z, type, coordinates")); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean())); - assertThat(e.getMessage(), is("field [test] not supported - must be one of: lon, lat, z, type, coordinates, geohash")); - } - } - - public void testInvalidGeoHash() throws IOException { - XContentBuilder content = JsonXContent.contentBuilder(); - content.startObject(); - content.field("geohash", "!!!!"); - content.endObject(); - - try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { - parser.nextToken(); - - Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("unsupported symbol [!] in geohash [!!!!]")); + assertThat(e.getMessage(), is("field [test] not supported - must be one of: lon, lat, z, type, coordinates")); } } diff --git a/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java b/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java index ba9e6b10150bc..09f0cf3ea8f26 100644 --- a/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java +++ b/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java @@ -11,7 +11,6 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; -import org.elasticsearch.geometry.utils.Geohash; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentParser; @@ -20,11 +19,9 @@ import java.io.IOException; import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder; -import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.closeTo; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.greaterThanOrEqualTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.not; @@ -503,45 +500,6 @@ public void testParseGeoPointArrayZValueError() throws IOException { } } - public void testParseGeoPointGeohash() throws IOException { - for (int i = 0; i < 100; i++) { - int geoHashLength = randomIntBetween(1, Geohash.PRECISION); - StringBuilder geohashBuilder = new StringBuilder(geoHashLength); - for (int j = 0; j < geoHashLength; j++) { - geohashBuilder.append(BASE_32[randomInt(BASE_32.length - 1)]); - } - XContentBuilder json = jsonBuilder().startObject().field("geohash", geohashBuilder.toString()).endObject(); - try (XContentParser parser = createParser(json)) { - parser.nextToken(); - GeoPoint point = GeoUtils.parseGeoPoint(parser); - assertThat(point.lat(), allOf(lessThanOrEqualTo(90.0), greaterThanOrEqualTo(-90.0))); - assertThat(point.lon(), allOf(lessThanOrEqualTo(180.0), greaterThanOrEqualTo(-180.0))); - assertThat(parser.currentToken(), is(Token.END_OBJECT)); - assertNull(parser.nextToken()); - } - json = jsonBuilder().startObject().field("geohash", geohashBuilder.toString()).endObject(); - try (XContentParser parser = createParser(json)) { - while (parser.currentToken() != Token.VALUE_STRING) { - parser.nextToken(); - } - GeoPoint point = GeoUtils.parseGeoPoint(parser); - assertThat(point.lat(), allOf(lessThanOrEqualTo(90.0), greaterThanOrEqualTo(-90.0))); - assertThat(point.lon(), allOf(lessThanOrEqualTo(180.0), greaterThanOrEqualTo(-180.0))); - } - } - } - - public void testParseGeoPointGeohashWrongType() throws IOException { - XContentBuilder json = jsonBuilder().startObject().field("geohash", 1.0).endObject(); - try (XContentParser parser = createParser(json)) { - parser.nextToken(); - Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), containsString("[geohash] must be a string")); - assertThat(parser.currentToken(), is(Token.END_OBJECT)); - assertNull(parser.nextToken()); - } - } - public void testParseGeoPointLatNoLon() throws IOException { double lat = 0.0; XContentBuilder json = jsonBuilder().startObject().field("lat", lat).endObject(); @@ -691,19 +649,7 @@ public void testParseGeoPointExtraField() throws IOException { try (XContentParser parser = createParser(json)) { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field [foo] not supported - must be one of: lon, lat, z, type, coordinates, geohash")); - } - } - - public void testParseGeoPointLonLatGeoHash() throws IOException { - double lat = 0.0; - double lon = 0.0; - String geohash = "abcd"; - XContentBuilder json = jsonBuilder().startObject().field("lat", lat).field("lon", lon).field("geohash", geohash).endObject(); - try (XContentParser parser = createParser(json)) { - parser.nextToken(); - Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), containsString("fields matching more than one point format found")); + assertThat(e.getMessage(), is("field [foo] not supported - must be one of: lon, lat, z, type, coordinates")); } } @@ -772,7 +718,9 @@ public void testParseGeoPointGeohashPositions() throws IOException { } private GeoPoint parseGeohash(String geohash, GeoUtils.EffectivePoint effectivePoint) throws IOException { - try (XContentParser parser = createParser(jsonBuilder().startObject().field("geohash", geohash).endObject())) { + try (XContentParser parser = createParser(jsonBuilder().startObject().field("location", geohash).endObject())) { + parser.nextToken(); + parser.nextToken(); parser.nextToken(); return GeoUtils.parseGeoPoint(parser, randomBoolean(), effectivePoint); } diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/common/CartesianPoint.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/common/CartesianPoint.java index c1caa3cad9096..5682ed9f9e981 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/common/CartesianPoint.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/common/CartesianPoint.java @@ -202,11 +202,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws */ public static CartesianPoint parsePoint(XContentParser parser, final boolean ignoreZValue) throws IOException, ElasticsearchParseException { - return cartesianPointParser.parsePoint(parser, ignoreZValue, value -> { - CartesianPoint point = new CartesianPoint(); - point.resetFromString(value, ignoreZValue); - return point; - }, value -> null); + return cartesianPointParser.parsePoint(parser, ignoreZValue, value -> new CartesianPoint().resetFromString(value, ignoreZValue)); } public static CartesianPoint parsePoint(Object value, boolean ignoreZValue) throws ElasticsearchParseException { @@ -244,7 +240,7 @@ public static void assertZValue(final boolean ignoreZValue, double zValue) { } } - private static GenericPointParser cartesianPointParser = new GenericPointParser<>("point", "x", "y", false) { + private static final GenericPointParser cartesianPointParser = new GenericPointParser<>("point", "x", "y") { @Override public void assertZValue(boolean ignoreZValue, double zValue) {