From bbc61bd830304b6ccaf1937c70f3271ec0cfc2d0 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Fri, 18 Mar 2022 17:59:53 +0100 Subject: [PATCH 1/8] Support GeoJSON for geo_point --- .../elasticsearch/common/geo/GeoPoint.java | 1 + .../elasticsearch/common/geo/GeoUtils.java | 35 +++++++ .../search/geo/GeoPointShapeQueryTests.java | 59 +++++++++++ .../geo/GeoPointShapeQueryTestCase.java | 99 +++++++++++++++++++ 4 files changed, 194 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java index 6d654f22f24aa..107d3e627cbf4 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java @@ -77,6 +77,7 @@ public GeoPoint resetFromString(String value) { } public GeoPoint resetFromString(String value, final boolean ignoreZValue, EffectivePoint effectivePoint) { + // TODO: Support GeoJSON if (value.toLowerCase(Locale.ROOT).contains("point")) { return resetFromWKT(value, ignoreZValue); } else if (value.contains(",")) { 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 78c5ae2f00c58..e391336fcd7c9 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -26,7 +26,9 @@ import org.elasticsearch.xcontent.support.MapXContentParser; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; +import java.util.Locale; public class GeoUtils { @@ -42,6 +44,8 @@ public class GeoUtils { public static final String LATITUDE = "lat"; public static final String LONGITUDE = "lon"; public static final String GEOHASH = "geohash"; + public static final String COORDINATES = "coordinates"; + public static final String TYPE = "type"; /** Earth ellipsoid major axis defined by WGS 84 in meters */ public static final double EARTH_SEMI_MAJOR_AXIS = 6378137.0; // meters (WGS 84) @@ -434,9 +438,12 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina */ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue, EffectivePoint effectivePoint) throws IOException, ElasticsearchParseException { + // TODO: Consider merging with GeoJSON parser double lat = Double.NaN; double lon = Double.NaN; String geohash = null; + String geojsonType = null; + ArrayList coordinates = null; NumberFormatException numberFormatException = null; if (parser.currentToken() == Token.START_OBJECT) { @@ -478,6 +485,26 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } else { throw new ElasticsearchParseException("geohash must be a string"); } + } else if (COORDINATES.equals(field)) { + subParser.nextToken(); + if (subParser.currentToken() == Token.START_ARRAY) { + coordinates = new ArrayList<>(); + subParser.nextToken(); + while (subParser.currentToken() != Token.END_ARRAY) { + switch (subParser.currentToken()) { + case VALUE_NUMBER -> coordinates.add(subParser.doubleValue()); + case VALUE_STRING -> coordinates.add(subParser.doubleValue(true)); + default -> throw new ElasticsearchParseException("GeoJSON coordinate values must be numbers"); + } + subParser.nextToken(); + } + } + } else if (TYPE.equals(field)) { + if (subParser.nextToken() == Token.VALUE_STRING) { + geojsonType = subParser.text(); + } else { + throw new ElasticsearchParseException("GeoJSON type must be a string"); + } } else { throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH); } @@ -492,6 +519,14 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } else { return point.parseGeoHash(geohash, effectivePoint); } + } else if (coordinates != null) { + if (geojsonType == null || geojsonType.toLowerCase(Locale.ROOT).equals("point") == false) { + throw new ElasticsearchParseException("GeoJSON type for geo_point can only be 'point'"); + } + if (coordinates.size() > 2) { + GeoPoint.assertZValue(ignoreZValue, coordinates.get(2)); + } + return point.reset(coordinates.get(1), coordinates.get(0)); } else if (numberFormatException != null) { throw new ElasticsearchParseException( "[{}] and [{}] must be valid double values", diff --git a/server/src/test/java/org/elasticsearch/search/geo/GeoPointShapeQueryTests.java b/server/src/test/java/org/elasticsearch/search/geo/GeoPointShapeQueryTests.java index 77f60c32e14f8..2885a636b4671 100644 --- a/server/src/test/java/org/elasticsearch/search/geo/GeoPointShapeQueryTests.java +++ b/server/src/test/java/org/elasticsearch/search/geo/GeoPointShapeQueryTests.java @@ -10,14 +10,18 @@ import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.geo.ShapeRelation; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.geo.GeometryTestUtils; import org.elasticsearch.geometry.Point; import org.elasticsearch.geometry.utils.WellKnownText; +import org.elasticsearch.index.query.QueryBuilders; +import org.elasticsearch.search.SearchHits; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; import java.io.IOException; +import java.util.HashMap; import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; import static org.elasticsearch.index.query.QueryBuilders.geoShapeQuery; @@ -67,4 +71,59 @@ public void testFieldAlias() throws IOException { SearchResponse response = client().prepareSearch(defaultIndexName).setQuery(geoShapeQuery("alias", point)).get(); assertEquals(1, response.getHits().getTotalHits().value); } + + public void testQueryPointFromMultiPointFormats() throws Exception { + createMapping(defaultIndexName, defaultGeoFieldName); + ensureGreen(); + + double[] geojsonDoubles = new double[] { 45.0, 35.0 }; + HashMap geojson = new HashMap<>(); + geojson.put("type", "Point"); + geojson.put("coordinates", geojsonDoubles); + double[] pointDoubles = new double[] { 35.0, 25.0 }; + Object[] points = new Object[] { "-35, -45", "POINT(-35 -25)", pointDoubles, geojson }; + client().prepareIndex(defaultIndexName) + .setId("1") + .setSource(jsonBuilder().startObject().field(defaultGeoFieldName, points).endObject()) + .setRefreshPolicy(IMMEDIATE) + .get(); + + Point pointA = new Point(-45, -35); + Point pointB = new Point(-35, -25); + Point pointC = new Point(35, 25); + Point pointD = new Point(45, 35); + Point pointInvalid = new Point(-35, -35); + for (Point point : new Point[] { pointA, pointB, pointC, pointD, pointInvalid }) { + int expectedDocs = point.equals(pointInvalid) ? 0 : 1; + int disjointDocs = point.equals(pointInvalid) ? 1 : 0; + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals("Doc matches %s" + point, expectedDocs, searchHits.getTotalHits().value); + } + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.WITHIN)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals("Doc WITHIN %s" + point, 0, searchHits.getTotalHits().value); + } + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.CONTAINS)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals("Doc CONTAINS %s" + point, expectedDocs, searchHits.getTotalHits().value); + } + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.DISJOINT)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals("Doc DISJOINT with %s" + point, disjointDocs, searchHits.getTotalHits().value); + } + } + } } diff --git a/test/framework/src/main/java/org/elasticsearch/search/geo/GeoPointShapeQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/geo/GeoPointShapeQueryTestCase.java index c860ac4b4cfcd..545ac4f30c062 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/geo/GeoPointShapeQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/geo/GeoPointShapeQueryTestCase.java @@ -9,6 +9,7 @@ package org.elasticsearch.search.geo; import org.elasticsearch.action.get.GetResponse; +import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.search.SearchAction; import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchRequestBuilder; @@ -38,6 +39,7 @@ import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; @@ -583,4 +585,101 @@ public void testQueryMultiPoint() throws Exception { assertEquals(0, searchHits.getTotalHits().value); } } + + public void testQueryPointFromGeoJSON() throws Exception { + createMapping(defaultIndexName, defaultGeoFieldName); + ensureGreen(); + + String doc1 = """ + { + "geo": { + "coordinates": [ -35, -25.0 ], + "type": "Point" + } + }"""; + client().index(new IndexRequest(defaultIndexName).id("1").source(doc1, XContentType.JSON).setRefreshPolicy(IMMEDIATE)).actionGet(); + + Point point = new Point(-35, -25); + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals(1, searchHits.getTotalHits().value); + } + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.WITHIN)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals(1, searchHits.getTotalHits().value); + } + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.CONTAINS)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals(1, searchHits.getTotalHits().value); + } + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.DISJOINT)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals(0, searchHits.getTotalHits().value); + } + } + + public void testQueryPointFromMultiPoint() throws Exception { + createMapping(defaultIndexName, defaultGeoFieldName); + ensureGreen(); + + double[] pointDoubles = new double[] { 35.0, 25.0 }; + HashMap geojson = new HashMap<>(); + geojson.put("type", "Point"); + geojson.put("coordinates", pointDoubles); + Object[] points = new Object[] { "POINT(-45 -35)", "POINT(-35 -25)", geojson }; + client().prepareIndex(defaultIndexName) + .setId("1") + .setSource(jsonBuilder().startObject().field(defaultGeoFieldName, points).endObject()) + .setRefreshPolicy(IMMEDIATE) + .get(); + + Point pointA = new Point(-45, -35); + Point pointB = new Point(-35, -25); + Point pointC = new Point(35, 25); + Point pointInvalid = new Point(-35, -35); + for (Point point : new Point[] { pointA, pointB, pointC, pointInvalid }) { + int expectedDocs = point.equals(pointInvalid) ? 0 : 1; + int disjointDocs = point.equals(pointInvalid) ? 1 : 0; + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals("Doc matches %s" + point, expectedDocs, searchHits.getTotalHits().value); + } + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.WITHIN)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals("Doc WITHIN %s" + point, 0, searchHits.getTotalHits().value); + } + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.CONTAINS)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals("Doc CONTAINS %s" + point, expectedDocs, searchHits.getTotalHits().value); + } + { + SearchResponse response = client().prepareSearch(defaultIndexName) + .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.DISJOINT)) + .get(); + SearchHits searchHits = response.getHits(); + assertEquals("Doc DISJOINT with %s" + point, disjointDocs, searchHits.getTotalHits().value); + } + } + } } From a4cb6de39f9bd1eb8617ca77332b6b119330a0f2 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Mon, 21 Mar 2022 17:20:07 +0100 Subject: [PATCH 2/8] Better error handling for GeoJSON parsing --- .../elasticsearch/common/geo/GeoUtils.java | 136 ++++++++++-------- .../index/mapper/GeoPointFieldTypeTests.java | 10 ++ 2 files changed, 87 insertions(+), 59 deletions(-) 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 e391336fcd7c9..cc9c3a157e13c 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -444,82 +444,88 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina String geohash = null; String geojsonType = null; ArrayList coordinates = null; - NumberFormatException numberFormatException = null; + class NumberFormatExceptionHandler { + NumberFormatException numberFormatException = null; + String field = null; + + public double doubleValue(XContentSubParser subParser, String field) throws IOException { + try { + return switch (subParser.currentToken()) { + case VALUE_NUMBER, VALUE_STRING -> subParser.doubleValue(true); + default -> throw new ElasticsearchParseException("[{}] must be a number", field); + }; + } catch (NumberFormatException e) { + numberFormatException = e; + this.field = field; + return Double.NaN; + } + } + + // TODO: Re-evaluate why this exception is not thrown earlier + public void assertNoException() { + if (numberFormatException != null) { + throw new ElasticsearchParseException("[{}] must be a valid double value", numberFormatException, field); + } + } + } + NumberFormatExceptionHandler numberFormatExceptionHandler = new NumberFormatExceptionHandler(); if (parser.currentToken() == Token.START_OBJECT) { try (XContentSubParser subParser = new XContentSubParser(parser)) { while (subParser.nextToken() != Token.END_OBJECT) { if (subParser.currentToken() == Token.FIELD_NAME) { String field = subParser.currentName(); + subParser.nextToken(); if (LATITUDE.equals(field)) { - subParser.nextToken(); - switch (subParser.currentToken()) { - case VALUE_NUMBER: - case VALUE_STRING: - try { - lat = subParser.doubleValue(true); - } catch (NumberFormatException e) { - numberFormatException = e; - } - break; - default: - throw new ElasticsearchParseException("latitude must be a number"); - } + lat = numberFormatExceptionHandler.doubleValue(subParser, field); } else if (LONGITUDE.equals(field)) { - subParser.nextToken(); - switch (subParser.currentToken()) { - case VALUE_NUMBER: - case VALUE_STRING: - try { - lon = subParser.doubleValue(true); - } catch (NumberFormatException e) { - numberFormatException = e; - } - break; - default: - throw new ElasticsearchParseException("longitude must be a number"); - } + lon = numberFormatExceptionHandler.doubleValue(subParser, field); } else if (GEOHASH.equals(field)) { - if (subParser.nextToken() == Token.VALUE_STRING) { + if (subParser.currentToken() == Token.VALUE_STRING) { geohash = subParser.text(); } else { throw new ElasticsearchParseException("geohash must be a string"); } } else if (COORDINATES.equals(field)) { - subParser.nextToken(); if (subParser.currentToken() == Token.START_ARRAY) { coordinates = new ArrayList<>(); - subParser.nextToken(); - while (subParser.currentToken() != Token.END_ARRAY) { - switch (subParser.currentToken()) { - case VALUE_NUMBER -> coordinates.add(subParser.doubleValue()); - case VALUE_STRING -> coordinates.add(subParser.doubleValue(true)); - default -> throw new ElasticsearchParseException("GeoJSON coordinate values must be numbers"); - } - subParser.nextToken(); + while (subParser.nextToken() != Token.END_ARRAY) { + coordinates.add(numberFormatExceptionHandler.doubleValue(subParser, field)); } } } else if (TYPE.equals(field)) { - if (subParser.nextToken() == Token.VALUE_STRING) { + if (subParser.currentToken() == Token.VALUE_STRING) { geojsonType = subParser.text(); } else { - throw new ElasticsearchParseException("GeoJSON type must be a string"); + throw new ElasticsearchParseException("GeoJSON 'type' must be a string"); } } else { - throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH); + throw new ElasticsearchParseException( + "field must be either [{}], [{}], [{}], [{}] or [{}]", + LATITUDE, + LONGITUDE, + GEOHASH, + COORDINATES, + TYPE + ); } } else { throw new ElasticsearchParseException("token [{}] not allowed", subParser.currentToken()); } } } + numberFormatExceptionHandler.assertNoException(); + assertOnlyOneFormat( + geohash != null, + Double.isNaN(lat) == false, + Double.isNaN(lon) == false, + coordinates != null, + geojsonType != null + ); if (geohash != null) { - if (Double.isNaN(lat) == false || Double.isNaN(lon) == false) { - throw new ElasticsearchParseException("field must be either lat/lon or geohash"); - } else { - return point.parseGeoHash(geohash, effectivePoint); - } - } else if (coordinates != null) { + return point.parseGeoHash(geohash, effectivePoint); + } + if (coordinates != null) { if (geojsonType == null || geojsonType.toLowerCase(Locale.ROOT).equals("point") == false) { throw new ElasticsearchParseException("GeoJSON type for geo_point can only be 'point'"); } @@ -527,20 +533,8 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina GeoPoint.assertZValue(ignoreZValue, coordinates.get(2)); } return point.reset(coordinates.get(1), coordinates.get(0)); - } else if (numberFormatException != null) { - throw new ElasticsearchParseException( - "[{}] and [{}] must be valid double values", - numberFormatException, - LATITUDE, - LONGITUDE - ); - } else if (Double.isNaN(lat)) { - throw new ElasticsearchParseException("field [{}] missing", LATITUDE); - } else if (Double.isNaN(lon)) { - throw new ElasticsearchParseException("field [{}] missing", LONGITUDE); - } else { - return point.reset(lat, lon); } + return point.reset(lat, lon); } else if (parser.currentToken() == Token.START_ARRAY) { try (XContentSubParser subParser = new XContentSubParser(parser)) { @@ -571,6 +565,30 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } } + private static void assertOnlyOneFormat(boolean geohash, boolean lat, boolean lon, boolean coordinates, boolean type) { + boolean latlon = lat && lon; + boolean geojson = coordinates && type; + var found = new ArrayList(); + if (geohash) found.add("geohash"); + if (latlon) found.add("lat/lon"); + if (geojson) found.add("GeoJSON"); + if (found.size() == 0) { + if (lat) { + throw new ElasticsearchParseException("field [{}] missing", LONGITUDE); + } else if (lon) { + throw new ElasticsearchParseException("field [{}] missing", LATITUDE); + } else if (coordinates) { + throw new ElasticsearchParseException("field [{}] missing", TYPE); + } else if (type) { + throw new ElasticsearchParseException("field [{}] missing", COORDINATES); + } else { + throw new ElasticsearchParseException("field must be either lat/lon, geohash string or type/coordinates"); + } + } else if (found.size() > 1) { + throw new ElasticsearchParseException("fields matching more than one point format found: {}", found); + } + } + /** * Parse a {@link GeoPoint} from a string. The string must have one of the following forms: * diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java index c38ffd0c61902..3d0fe92a0f532 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java @@ -71,6 +71,16 @@ public void testFetchSourceValue() throws IOException { assertEquals(List.of(), fetchSourceValue(mapper, sourceValue, null)); assertEquals(List.of(), fetchSourceValue(mapper, sourceValue, "wkt")); } + + // test single point in GeoJSON format + sourceValue = jsonPoint; + assertEquals(List.of(jsonPoint), fetchSourceValue(mapper, sourceValue, null)); + assertEquals(List.of(wktPoint), fetchSourceValue(mapper, sourceValue, "wkt")); + + // Test a list of points in GeoJSON format + sourceValue = List.of(jsonPoint, otherJsonPoint); + assertEquals(List.of(jsonPoint, otherJsonPoint), fetchSourceValue(mapper, sourceValue, null)); + assertEquals(List.of(wktPoint, otherWktPoint), fetchSourceValue(mapper, sourceValue, "wkt")); } public void testFetchVectorTile() throws IOException { From 318850e94f58815108597396bcdc8cd11479d82f Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Mon, 21 Mar 2022 19:51:38 +0100 Subject: [PATCH 3/8] Fixed failing tests --- .../elasticsearch/common/geo/GeoUtils.java | 31 ++++-- .../search/geo/GeoPointParsingTests.java | 16 +-- .../index/search/geo/GeoUtilsTests.java | 98 ++++++++++++++++++- 3 files changed, 126 insertions(+), 19 deletions(-) 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 cc9c3a157e13c..9d1dacdb77b0f 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -452,7 +452,7 @@ public double doubleValue(XContentSubParser subParser, String field) throws IOEx try { return switch (subParser.currentToken()) { case VALUE_NUMBER, VALUE_STRING -> subParser.doubleValue(true); - default -> throw new ElasticsearchParseException("[{}] must be a number", field); + default -> throw new ElasticsearchParseException("{} must be a number", field); }; } catch (NumberFormatException e) { numberFormatException = e; @@ -477,9 +477,9 @@ public void assertNoException() { String field = subParser.currentName(); subParser.nextToken(); if (LATITUDE.equals(field)) { - lat = numberFormatExceptionHandler.doubleValue(subParser, field); + lat = numberFormatExceptionHandler.doubleValue(subParser, "latitude"); } else if (LONGITUDE.equals(field)) { - lon = numberFormatExceptionHandler.doubleValue(subParser, field); + lon = numberFormatExceptionHandler.doubleValue(subParser, "longitude"); } else if (GEOHASH.equals(field)) { if (subParser.currentToken() == Token.VALUE_STRING) { geohash = subParser.text(); @@ -492,6 +492,8 @@ public void assertNoException() { while (subParser.nextToken() != Token.END_ARRAY) { coordinates.add(numberFormatExceptionHandler.doubleValue(subParser, field)); } + } else { + throw new ElasticsearchParseException("GeoJSON 'coordinates' must be an array"); } } else if (TYPE.equals(field)) { if (subParser.currentToken() == Token.VALUE_STRING) { @@ -527,11 +529,17 @@ public void assertNoException() { } if (coordinates != null) { if (geojsonType == null || geojsonType.toLowerCase(Locale.ROOT).equals("point") == false) { - throw new ElasticsearchParseException("GeoJSON type for geo_point can only be 'point'"); + throw new ElasticsearchParseException("GeoJSON 'type' for geo_point can only be 'Point'"); + } + if (coordinates.size() < 2) { + throw new ElasticsearchParseException("GeoJSON 'coordinates' must contain at least two values"); } - if (coordinates.size() > 2) { + if (coordinates.size() == 3) { GeoPoint.assertZValue(ignoreZValue, coordinates.get(2)); } + if (coordinates.size() > 3) { + throw new ElasticsearchParseException("[geo_point] field type does not accept > 3 dimensions"); + } return point.reset(coordinates.get(1), coordinates.get(0)); } return point.reset(lat, lon); @@ -566,13 +574,20 @@ public void assertNoException() { } private static void assertOnlyOneFormat(boolean geohash, boolean lat, boolean lon, boolean coordinates, boolean type) { + String invalidFieldsMessage = "field must be either lat/lon, geohash string or type/coordinates"; boolean latlon = lat && lon; boolean geojson = coordinates && type; var found = new ArrayList(); if (geohash) found.add("geohash"); if (latlon) found.add("lat/lon"); if (geojson) found.add("GeoJSON"); - if (found.size() == 0) { + if (found.size() > 1) { + throw new ElasticsearchParseException("fields matching more than one point format found: {}", found); + } else if (geohash) { + if (lat || lon || type || coordinates) { + throw new ElasticsearchParseException(invalidFieldsMessage); + } + } else if (found.size() == 0) { if (lat) { throw new ElasticsearchParseException("field [{}] missing", LONGITUDE); } else if (lon) { @@ -582,10 +597,8 @@ private static void assertOnlyOneFormat(boolean geohash, boolean lat, boolean lo } else if (type) { throw new ElasticsearchParseException("field [{}] missing", COORDINATES); } else { - throw new ElasticsearchParseException("field must be either lat/lon, geohash string or type/coordinates"); + throw new ElasticsearchParseException(invalidFieldsMessage); } - } else if (found.size() > 1) { - throw new ElasticsearchParseException("fields matching more than one point format found: {}", found); } } 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 216e643ecfca4..67dcc8dcb337a 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 @@ -118,12 +118,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 must be either [lat], [lon] or [geohash]")); + assertThat(e.getMessage(), is("field must be either [lat], [lon], [geohash], [coordinates] or [type]")); } 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] or [geohash]")); + assertThat(e.getMessage(), is("field must be either [lat], [lon], [geohash], [coordinates] or [type]")); } } @@ -136,12 +136,12 @@ public void testInvalidPointLatHashMix() 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 must be either lat/lon or geohash")); + 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 or geohash")); + assertThat(e.getMessage(), is("field must be either lat/lon, geohash string or type/coordinates")); } } @@ -155,12 +155,12 @@ public void testInvalidPointLonHashMix() throws IOException { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either lat/lon or geohash")); + 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 or geohash")); + assertThat(e.getMessage(), is("field must be either lat/lon, geohash string or type/coordinates")); } } @@ -173,13 +173,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 must be either [lat], [lon] or [geohash]")); + assertThat(e.getMessage(), is("field must be either [lat], [lon], [geohash], [coordinates] or [type]")); } 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] or [geohash]")); + assertThat(e.getMessage(), is("field must be either [lat], [lon], [geohash], [coordinates] or [type]")); } } 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 892f3bc9ddb26..16a713b7619ac 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 @@ -590,6 +590,100 @@ public void testParseGeoPointLatWrongType() throws IOException { } } + public void testParseGeoPointCoordinateNoType() throws IOException { + double[] coords = new double[] { 0.0, 0.0 }; + XContentBuilder json = jsonBuilder().startObject().field("coordinates", coords).endObject(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertThat(e.getMessage(), is("field [type] missing")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); + } + } + + public void testParseGeoPointTypeNoCoordinates() throws IOException { + XContentBuilder json = jsonBuilder().startObject().field("type", "Point").endObject(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertThat(e.getMessage(), is("field [coordinates] missing")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); + } + } + + public void testParseGeoPointTypeWrongValue() throws IOException { + double[] coords = new double[] { 0.0, 0.0 }; + XContentBuilder json = jsonBuilder().startObject().field("coordinates", coords).field("type", "LineString").endObject(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertThat(e.getMessage(), is("GeoJSON 'type' for geo_point can only be 'Point'")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); + } + } + + public void testParseGeoPointTypeWrongType() throws IOException { + double[] coords = new double[] { 0.0, 0.0 }; + XContentBuilder json = jsonBuilder().startObject().field("coordinates", coords).field("type", false).endObject(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertThat(e.getMessage(), is("GeoJSON 'type' must be a string")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); + } + } + + public void testParseGeoPointCoordinatesWrongType() throws IOException { + XContentBuilder json = jsonBuilder().startObject().field("coordinates", false).field("type", "Point").endObject(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertThat(e.getMessage(), is("GeoJSON 'coordinates' must be an array")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); + } + } + + public void testParseGeoPointCoordinatesTooShort() throws IOException { + double[] coords = new double[] { 0.0 }; + XContentBuilder json = jsonBuilder().startObject().field("coordinates", coords).field("type", "Point").endObject(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertThat(e.getMessage(), is("GeoJSON 'coordinates' must contain at least two values")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); + } + } + + public void testParseGeoPointCoordinatesTooLong() throws IOException { + double[] coords = new double[] { 0.0, 0.0, 0.0 }; + XContentBuilder json = jsonBuilder().startObject().field("coordinates", coords).field("type", "Point").endObject(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertThat(e.getMessage(), containsString("found Z value [0.0] but [ignore_z_value] parameter is [false]")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); + } + } + + public void testParseGeoPointCoordinatesWayTooLong() throws IOException { + double[] coords = new double[] { 0.0, 0.0, 0.0, 0.0 }; + XContentBuilder json = jsonBuilder().startObject().field("coordinates", coords).field("type", "Point").endObject(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertThat(e.getMessage(), is("[geo_point] field type does not accept > 3 dimensions")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); + } + } + public void testParseGeoPointExtraField() throws IOException { double lat = 0.0; double lon = 0.0; @@ -597,7 +691,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 must be either [lat], [lon] or [geohash]")); + assertThat(e.getMessage(), is("field must be either [lat], [lon], [geohash], [coordinates] or [type]")); } } @@ -609,7 +703,7 @@ public void testParseGeoPointLonLatGeoHash() throws IOException { try (XContentParser parser = createParser(json)) { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), containsString("field must be either lat/lon or geohash")); + assertThat(e.getMessage(), containsString("fields matching more than one point format found")); } } From ee6f1b00dd46d61b1339c39f765badecf57f7155 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 22 Mar 2022 10:33:08 +0100 Subject: [PATCH 4/8] Generalized test with only test data specialized --- .../search/geo/GeoPointShapeQueryTests.java | 73 ++++--------------- .../geo/GeoPointShapeQueryTestCase.java | 33 ++++++--- 2 files changed, 39 insertions(+), 67 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/geo/GeoPointShapeQueryTests.java b/server/src/test/java/org/elasticsearch/search/geo/GeoPointShapeQueryTests.java index 2885a636b4671..63c651107735b 100644 --- a/server/src/test/java/org/elasticsearch/search/geo/GeoPointShapeQueryTests.java +++ b/server/src/test/java/org/elasticsearch/search/geo/GeoPointShapeQueryTests.java @@ -10,18 +10,16 @@ import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.common.Strings; -import org.elasticsearch.common.geo.ShapeRelation; +import org.elasticsearch.common.geo.GeoJson; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.geo.GeometryTestUtils; import org.elasticsearch.geometry.Point; import org.elasticsearch.geometry.utils.WellKnownText; -import org.elasticsearch.index.query.QueryBuilders; -import org.elasticsearch.search.SearchHits; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; import java.io.IOException; -import java.util.HashMap; +import java.util.Map; import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; import static org.elasticsearch.index.query.QueryBuilders.geoShapeQuery; @@ -72,58 +70,19 @@ public void testFieldAlias() throws IOException { assertEquals(1, response.getHits().getTotalHits().value); } - public void testQueryPointFromMultiPointFormats() throws Exception { - createMapping(defaultIndexName, defaultGeoFieldName); - ensureGreen(); - - double[] geojsonDoubles = new double[] { 45.0, 35.0 }; - HashMap geojson = new HashMap<>(); - geojson.put("type", "Point"); - geojson.put("coordinates", geojsonDoubles); - double[] pointDoubles = new double[] { 35.0, 25.0 }; - Object[] points = new Object[] { "-35, -45", "POINT(-35 -25)", pointDoubles, geojson }; - client().prepareIndex(defaultIndexName) - .setId("1") - .setSource(jsonBuilder().startObject().field(defaultGeoFieldName, points).endObject()) - .setRefreshPolicy(IMMEDIATE) - .get(); - - Point pointA = new Point(-45, -35); - Point pointB = new Point(-35, -25); - Point pointC = new Point(35, 25); - Point pointD = new Point(45, 35); - Point pointInvalid = new Point(-35, -35); - for (Point point : new Point[] { pointA, pointB, pointC, pointD, pointInvalid }) { - int expectedDocs = point.equals(pointInvalid) ? 0 : 1; - int disjointDocs = point.equals(pointInvalid) ? 1 : 0; - { - SearchResponse response = client().prepareSearch(defaultIndexName) - .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point)) - .get(); - SearchHits searchHits = response.getHits(); - assertEquals("Doc matches %s" + point, expectedDocs, searchHits.getTotalHits().value); - } - { - SearchResponse response = client().prepareSearch(defaultIndexName) - .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.WITHIN)) - .get(); - SearchHits searchHits = response.getHits(); - assertEquals("Doc WITHIN %s" + point, 0, searchHits.getTotalHits().value); - } - { - SearchResponse response = client().prepareSearch(defaultIndexName) - .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.CONTAINS)) - .get(); - SearchHits searchHits = response.getHits(); - assertEquals("Doc CONTAINS %s" + point, expectedDocs, searchHits.getTotalHits().value); - } - { - SearchResponse response = client().prepareSearch(defaultIndexName) - .setQuery(QueryBuilders.geoShapeQuery(defaultGeoFieldName, point).relation(ShapeRelation.DISJOINT)) - .get(); - SearchHits searchHits = response.getHits(); - assertEquals("Doc DISJOINT with %s" + point, disjointDocs, searchHits.getTotalHits().value); - } - } + /** + * Produce an array of objects each representing a single point in a variety of + * supported point formats. For `geo_shape` we only support GeoJSON and WKT, + * while for `geo_point` we support a variety of additional special case formats. + * Therefor we define here sample data for double[]{lon,lat} as well as + * a string "lat,lon". + */ + @Override + protected Object[] samplePointDataMultiFormat(Point pointA, Point pointB, Point pointC, Point pointD) { + String str = "" + pointA.getLat() + ", " + pointA.getLon(); + String wkt = WellKnownText.toWKT(pointB); + double[] pointDoubles = new double[] { pointC.getLon(), pointC.getLat() }; + Map geojson = GeoJson.toMap(pointD); + return new Object[] { str, wkt, pointDoubles, geojson }; } } diff --git a/test/framework/src/main/java/org/elasticsearch/search/geo/GeoPointShapeQueryTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/geo/GeoPointShapeQueryTestCase.java index 545ac4f30c062..87859daaa0d8d 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/geo/GeoPointShapeQueryTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/geo/GeoPointShapeQueryTestCase.java @@ -15,6 +15,7 @@ import org.elasticsearch.action.search.SearchRequestBuilder; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.geo.GeoJson; import org.elasticsearch.common.geo.ShapeRelation; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.geometry.Circle; @@ -28,6 +29,7 @@ import org.elasticsearch.geometry.Polygon; import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.geometry.ShapeType; +import org.elasticsearch.geometry.utils.WellKnownText; import org.elasticsearch.index.query.GeoShapeQueryBuilder; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.plugins.Plugin; @@ -39,8 +41,8 @@ import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.List; +import java.util.Map; import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; @@ -630,26 +632,37 @@ public void testQueryPointFromGeoJSON() throws Exception { } } + /** + * Produce an array of objects each representing a single point in a variety of + * supported point formats. For `geo_shape` we only support GeoJSON and WKT, + * while for `geo_point` we support a variety of additional special case formats. + * This method is therefor overridden in the tests for `geo_point` (@see GeoPointShapeQueryTests). + */ + protected Object[] samplePointDataMultiFormat(Point pointA, Point pointB, Point pointC, Point pointD) { + String wktA = WellKnownText.toWKT(pointA); + String wktB = WellKnownText.toWKT(pointB); + Map geojsonC = GeoJson.toMap(pointC); + Map geojsonD = GeoJson.toMap(pointD); + return new Object[] { wktA, wktB, geojsonC, geojsonD }; + } + public void testQueryPointFromMultiPoint() throws Exception { createMapping(defaultIndexName, defaultGeoFieldName); ensureGreen(); - double[] pointDoubles = new double[] { 35.0, 25.0 }; - HashMap geojson = new HashMap<>(); - geojson.put("type", "Point"); - geojson.put("coordinates", pointDoubles); - Object[] points = new Object[] { "POINT(-45 -35)", "POINT(-35 -25)", geojson }; + Point pointA = new Point(-45, -35); + Point pointB = new Point(-35, -25); + Point pointC = new Point(35, 25); + Point pointD = new Point(45, 35); + Object[] points = samplePointDataMultiFormat(pointA, pointB, pointC, pointD); client().prepareIndex(defaultIndexName) .setId("1") .setSource(jsonBuilder().startObject().field(defaultGeoFieldName, points).endObject()) .setRefreshPolicy(IMMEDIATE) .get(); - Point pointA = new Point(-45, -35); - Point pointB = new Point(-35, -25); - Point pointC = new Point(35, 25); Point pointInvalid = new Point(-35, -35); - for (Point point : new Point[] { pointA, pointB, pointC, pointInvalid }) { + for (Point point : new Point[] { pointA, pointB, pointC, pointD, pointInvalid }) { int expectedDocs = point.equals(pointInvalid) ? 0 : 1; int disjointDocs = point.equals(pointInvalid) ? 1 : 0; { From f254ccdb95d220fd89cd36b5e51757fc4eed9737 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 22 Mar 2022 10:57:51 +0100 Subject: [PATCH 5/8] More point parsing tests --- .../search/geo/GeoPointParsingTests.java | 73 +++++++++---------- 1 file changed, 35 insertions(+), 38 deletions(-) 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 67dcc8dcb337a..4378e0184a775 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 @@ -10,8 +10,10 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.geo.GeoJson; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; +import org.elasticsearch.geometry.Point; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.geo.RandomGeoGenerator; import org.elasticsearch.xcontent.XContentBuilder; @@ -19,6 +21,7 @@ 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; @@ -84,25 +87,28 @@ public void testGeoPointParsing() throws IOException { GeoPoint point = GeoUtils.parseGeoPoint(objectLatLon(randomPt.lat(), randomPt.lon())); assertPointsEqual(point, randomPt); - GeoUtils.parseGeoPoint(toObject(objectLatLon(randomPt.lat(), randomPt.lon())), randomBoolean()); + point = GeoUtils.parseGeoPoint(toObject(objectLatLon(randomPt.lat(), randomPt.lon())), randomBoolean()); assertPointsEqual(point, randomPt); GeoUtils.parseGeoPoint(arrayLatLon(randomPt.lat(), randomPt.lon()), point); assertPointsEqual(point, randomPt); - GeoUtils.parseGeoPoint(toObject(arrayLatLon(randomPt.lat(), randomPt.lon())), randomBoolean()); + point = GeoUtils.parseGeoPoint(toObject(arrayLatLon(randomPt.lat(), randomPt.lon())), randomBoolean()); assertPointsEqual(point, randomPt); GeoUtils.parseGeoPoint(geohash(randomPt.lat(), randomPt.lon()), point); assertCloseTo(point, randomPt.lat(), randomPt.lon()); - GeoUtils.parseGeoPoint(toObject(geohash(randomPt.lat(), randomPt.lon())), randomBoolean()); + point = GeoUtils.parseGeoPoint(toObject(geohash(randomPt.lat(), randomPt.lon())), randomBoolean()); assertCloseTo(point, randomPt.lat(), randomPt.lon()); GeoUtils.parseGeoPoint(stringLatLon(randomPt.lat(), randomPt.lon()), point); assertCloseTo(point, randomPt.lat(), randomPt.lon()); - GeoUtils.parseGeoPoint(toObject(stringLatLon(randomPt.lat(), randomPt.lon())), randomBoolean()); + point = GeoUtils.parseGeoPoint(toObject(stringLatLon(randomPt.lat(), randomPt.lon())), randomBoolean()); + assertCloseTo(point, randomPt.lat(), randomPt.lon()); + + point = GeoUtils.parseGeoPoint(GeoJson.toMap(new Point(randomPt.lon(), randomPt.lat())), randomBoolean()); assertCloseTo(point, randomPt.lat(), randomPt.lon()); } @@ -127,40 +133,31 @@ public void testInvalidPointEmbeddedObject() throws IOException { } } - public void testInvalidPointLatHashMix() throws IOException { - XContentBuilder content = JsonXContent.contentBuilder(); - content.startObject(); - content.field("lat", 0).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")); - } - } - - public void testInvalidPointLonHashMix() throws IOException { - XContentBuilder content = JsonXContent.contentBuilder(); - content.startObject(); - content.field("lon", 0).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")); + 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")); + } } } From f654224777ffc6dd479dade66e23da572cbb953b Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 22 Mar 2022 11:36:18 +0100 Subject: [PATCH 6/8] Remove outdated TODOs --- server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java | 1 - server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java | 1 - 2 files changed, 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java index 107d3e627cbf4..6d654f22f24aa 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java @@ -77,7 +77,6 @@ public GeoPoint resetFromString(String value) { } public GeoPoint resetFromString(String value, final boolean ignoreZValue, EffectivePoint effectivePoint) { - // TODO: Support GeoJSON if (value.toLowerCase(Locale.ROOT).contains("point")) { return resetFromWKT(value, ignoreZValue); } else if (value.contains(",")) { 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 9d1dacdb77b0f..f4724e47b396b 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -438,7 +438,6 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina */ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue, EffectivePoint effectivePoint) throws IOException, ElasticsearchParseException { - // TODO: Consider merging with GeoJSON parser double lat = Double.NaN; double lon = Double.NaN; String geohash = null; From 425f34bf661da84b681c961a1ddf1e8396da2a74 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 22 Mar 2022 11:53:31 +0100 Subject: [PATCH 7/8] Update docs/changelog/85120.yaml --- docs/changelog/85120.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/85120.yaml diff --git a/docs/changelog/85120.yaml b/docs/changelog/85120.yaml new file mode 100644 index 0000000000000..4d23584325159 --- /dev/null +++ b/docs/changelog/85120.yaml @@ -0,0 +1,5 @@ +pr: 85120 +summary: Support GeoJSON for `geo_point` +area: Geo +type: enhancement +issues: [] From e6e9573f33f5015298ea7bbad5f35e2bb4cac857 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Tue, 22 Mar 2022 12:11:39 +0100 Subject: [PATCH 8/8] Simplify exception handling for parsing doubles --- .../elasticsearch/common/geo/GeoUtils.java | 43 ++++++------------- 1 file changed, 14 insertions(+), 29 deletions(-) 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 f4724e47b396b..fb7c6ec634aca 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -443,31 +443,6 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina String geohash = null; String geojsonType = null; ArrayList coordinates = null; - class NumberFormatExceptionHandler { - NumberFormatException numberFormatException = null; - String field = null; - - public double doubleValue(XContentSubParser subParser, String field) throws IOException { - try { - return switch (subParser.currentToken()) { - case VALUE_NUMBER, VALUE_STRING -> subParser.doubleValue(true); - default -> throw new ElasticsearchParseException("{} must be a number", field); - }; - } catch (NumberFormatException e) { - numberFormatException = e; - this.field = field; - return Double.NaN; - } - } - - // TODO: Re-evaluate why this exception is not thrown earlier - public void assertNoException() { - if (numberFormatException != null) { - throw new ElasticsearchParseException("[{}] must be a valid double value", numberFormatException, field); - } - } - } - NumberFormatExceptionHandler numberFormatExceptionHandler = new NumberFormatExceptionHandler(); if (parser.currentToken() == Token.START_OBJECT) { try (XContentSubParser subParser = new XContentSubParser(parser)) { @@ -476,9 +451,9 @@ public void assertNoException() { String field = subParser.currentName(); subParser.nextToken(); if (LATITUDE.equals(field)) { - lat = numberFormatExceptionHandler.doubleValue(subParser, "latitude"); + lat = parseValidDouble(subParser, "latitude"); } else if (LONGITUDE.equals(field)) { - lon = numberFormatExceptionHandler.doubleValue(subParser, "longitude"); + lon = parseValidDouble(subParser, "longitude"); } else if (GEOHASH.equals(field)) { if (subParser.currentToken() == Token.VALUE_STRING) { geohash = subParser.text(); @@ -489,7 +464,7 @@ public void assertNoException() { if (subParser.currentToken() == Token.START_ARRAY) { coordinates = new ArrayList<>(); while (subParser.nextToken() != Token.END_ARRAY) { - coordinates.add(numberFormatExceptionHandler.doubleValue(subParser, field)); + coordinates.add(parseValidDouble(subParser, field)); } } else { throw new ElasticsearchParseException("GeoJSON 'coordinates' must be an array"); @@ -515,7 +490,6 @@ public void assertNoException() { } } } - numberFormatExceptionHandler.assertNoException(); assertOnlyOneFormat( geohash != null, Double.isNaN(lat) == false, @@ -572,6 +546,17 @@ public void assertNoException() { } } + private static double parseValidDouble(XContentSubParser subParser, String field) throws IOException { + try { + return switch (subParser.currentToken()) { + case VALUE_NUMBER, VALUE_STRING -> subParser.doubleValue(true); + default -> throw new ElasticsearchParseException("{} must be a number", field); + }; + } catch (NumberFormatException e) { + throw new ElasticsearchParseException("[{}] must be a valid double value", e, field); + } + } + private static void assertOnlyOneFormat(boolean geohash, boolean lat, boolean lon, boolean coordinates, boolean type) { String invalidFieldsMessage = "field must be either lat/lon, geohash string or type/coordinates"; boolean latlon = lat && lon;