From ee300071b6de07aa9a225c1092053cc3fbcec2ca Mon Sep 17 00:00:00 2001 From: Heemin Kim Date: Mon, 19 Sep 2022 14:56:40 -0700 Subject: [PATCH 1/5] Support of GeoJson Point for GeoPoint field See https://github.com/opensearch-project/geospatial/issues/152 Signed-off-by: Heemin Kim --- CHANGELOG.md | 1 + .../org/opensearch/common/geo/GeoPoint.java | 6 +- .../org/opensearch/common/geo/GeoUtils.java | 131 +++------- .../org/opensearch/common/geo/PointUtils.java | 227 ++++++++++++++++++ .../AbstractPointGeometryFieldMapper.java | 29 ++- .../common/geo/PointUtilsTests.java | 216 +++++++++++++++++ .../search/geo/GeoPointParsingTests.java | 16 +- .../index/search/geo/GeoUtilsTests.java | 8 +- 8 files changed, 509 insertions(+), 125 deletions(-) create mode 100644 server/src/main/java/org/opensearch/common/geo/PointUtils.java create mode 100644 server/src/test/java/org/opensearch/common/geo/PointUtilsTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 37e16aecff69a..e16125001f074 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Added precommit support for MacOS ([#4682](https://github.com/opensearch-project/OpenSearch/pull/4682)) - Recommission API changes for service layer ([#4320](https://github.com/opensearch-project/OpenSearch/pull/4320)) - Update GeoGrid base class access modifier to support extensibility ([#4572](https://github.com/opensearch-project/OpenSearch/pull/4572)) +- Add support for GeoJson Point type in GeoPoint field ([#4597](https://github.com/opensearch-project/OpenSearch/pull/4597)) ### Dependencies - Bumps `log4j-core` from 2.18.0 to 2.19.0 diff --git a/server/src/main/java/org/opensearch/common/geo/GeoPoint.java b/server/src/main/java/org/opensearch/common/geo/GeoPoint.java index a2b06dccded8c..f6b049b46789d 100644 --- a/server/src/main/java/org/opensearch/common/geo/GeoPoint.java +++ b/server/src/main/java/org/opensearch/common/geo/GeoPoint.java @@ -119,7 +119,11 @@ public GeoPoint resetFromString(String value, final boolean ignoreZValue, Effect public GeoPoint resetFromCoordinates(String value, final boolean ignoreZValue) { String[] vals = value.split(","); if (vals.length > 3) { - throw new OpenSearchParseException("failed to parse [{}], expected 2 or 3 coordinates " + "but found: [{}]", vals.length); + throw new OpenSearchParseException( + "failed to parse [{}], expected 2 or 3 coordinates " + "but found: [{}]", + vals.length, + vals.length + ); } final double lat; final double lon; diff --git a/server/src/main/java/org/opensearch/common/geo/GeoUtils.java b/server/src/main/java/org/opensearch/common/geo/GeoUtils.java index 5534967d559d6..6a7a319197daf 100644 --- a/server/src/main/java/org/opensearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/opensearch/common/geo/GeoUtils.java @@ -40,10 +40,10 @@ import org.opensearch.common.xcontent.LoggingDeprecationHandler; import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentParser; -import org.opensearch.common.xcontent.XContentParser.Token; import org.opensearch.common.xcontent.XContentSubParser; import org.opensearch.common.xcontent.support.MapXContentParser; import org.opensearch.common.xcontent.support.XContentMapValues; +import org.opensearch.geometry.Point; import org.opensearch.index.fielddata.FieldData; import org.opensearch.index.fielddata.GeoPointValues; import org.opensearch.index.fielddata.MultiGeoPointValues; @@ -95,6 +95,12 @@ public class GeoUtils { /** rounding error for quantized latitude and longitude values */ public static final double TOLERANCE = 1E-6; + public static final PointUtils POINT_PARSER; + + static { + POINT_PARSER = new PointUtils(LONGITUDE, LATITUDE, true); + } + /** Returns true if latitude is actually a valid latitude value.*/ public static boolean isValidLatitude(double latitude) { if (Double.isNaN(latitude) || Double.isInfinite(latitude) || latitude < GeoUtils.MIN_LAT || latitude > GeoUtils.MAX_LAT) { @@ -444,113 +450,44 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina * Parse a {@link GeoPoint} with a {@link XContentParser}. A geopoint has one of the following forms: * * * + * * @param parser {@link XContentParser} to parse the value from * @param point A {@link GeoPoint} that will be reset by the values parsed + * @param ignoreZValue tells to ignore z value or throw exception when there is a z value + * @param effectivePoint tells which point to use for GeoHash form * @return new {@link GeoPoint} parsed from the parse */ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue, EffectivePoint effectivePoint) throws IOException, OpenSearchParseException { - double lat = Double.NaN; - double lon = Double.NaN; - String geohash = null; - NumberFormatException numberFormatException = null; - - 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(); - 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 OpenSearchParseException("latitude must be a number"); - } - } 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 OpenSearchParseException("longitude must be a number"); - } - } else if (GEOHASH.equals(field)) { - if (subParser.nextToken() == Token.VALUE_STRING) { - geohash = subParser.text(); - } else { - throw new OpenSearchParseException("geohash must be a string"); - } - } else { - throw new OpenSearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH); - } - } else { - throw new OpenSearchParseException("token [{}] not allowed", subParser.currentToken()); - } + switch (parser.currentToken()) { + case START_OBJECT: + try (XContentSubParser subParser = new XContentSubParser(parser)) { + Point pointInObject = POINT_PARSER.parseObject(subParser, ignoreZValue, effectivePoint); + point.reset(pointInObject.getLat(), pointInObject.getLon()); } - } - if (geohash != null) { - if (!Double.isNaN(lat) || !Double.isNaN(lon)) { - throw new OpenSearchParseException("field must be either lat/lon or geohash"); - } else { - return point.parseGeoHash(geohash, effectivePoint); - } - } else if (numberFormatException != null) { - throw new OpenSearchParseException("[{}] and [{}] must be valid double values", numberFormatException, LATITUDE, LONGITUDE); - } else if (Double.isNaN(lat)) { - throw new OpenSearchParseException("field [{}] missing", LATITUDE); - } else if (Double.isNaN(lon)) { - throw new OpenSearchParseException("field [{}] missing", LONGITUDE); - } else { - return point.reset(lat, lon); - } - - } else if (parser.currentToken() == Token.START_ARRAY) { - try (XContentSubParser subParser = new XContentSubParser(parser)) { - int element = 0; - while (subParser.nextToken() != Token.END_ARRAY) { - if (subParser.currentToken() == Token.VALUE_NUMBER) { - element++; - if (element == 1) { - lon = subParser.doubleValue(); - } else if (element == 2) { - lat = subParser.doubleValue(); - } else if (element == 3) { - GeoPoint.assertZValue(ignoreZValue, subParser.doubleValue()); - } else { - throw new OpenSearchParseException("[geo_point] field type does not accept > 3 dimensions"); - } - } else { - throw new OpenSearchParseException("numeric value expected"); - } + break; + case START_ARRAY: + try (XContentSubParser subParser = new XContentSubParser(parser)) { + Point pointInArray = POINT_PARSER.parseFromArray(subParser, ignoreZValue, "geo_point"); + point.reset(pointInArray.getLat(), pointInArray.getLon()); } - } - return point.reset(lat, lon); - } else if (parser.currentToken() == Token.VALUE_STRING) { - String val = parser.text(); - return point.resetFromString(val, ignoreZValue, effectivePoint); - } else { - throw new OpenSearchParseException("geo_point expected"); + break; + case VALUE_STRING: + String val = parser.text(); + point.resetFromString(val, ignoreZValue, effectivePoint); + break; + default: + throw new OpenSearchParseException("geo_point expected"); } + return point; } /** diff --git a/server/src/main/java/org/opensearch/common/geo/PointUtils.java b/server/src/main/java/org/opensearch/common/geo/PointUtils.java new file mode 100644 index 0000000000000..d5d823af05b1f --- /dev/null +++ b/server/src/main/java/org/opensearch/common/geo/PointUtils.java @@ -0,0 +1,227 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.common.geo; + +import org.opensearch.OpenSearchParseException; +import org.opensearch.common.xcontent.XContentParser; +import org.opensearch.geometry.Point; + +import java.io.IOException; +import java.util.HashMap; +import java.util.Locale; + +/** + * A utility class with a point parser methods supporting different point representation formats + * + * @opensearch.internal + */ +public class PointUtils { + private static final String FN_GEOJSON_TYPE = "type"; + private static final String FV_GEOJSON_TYPE_POINT = "Point"; + private static final String FN_GEOJSON_COORDS = "coordinates"; + + private static final String FN_GEOHASH = "geohash"; + private static final String ERR_MSG_INVALID_TOKEN = "token [{}] not allowed"; + + private String fieldNameX; + private String fieldNameY; + private boolean supportGeoHash; + + private String invalidFieldErrMsg; + + public PointUtils(final String fieldNameX, final String fieldNameY, final boolean supportGeoHash) { + this.fieldNameX = fieldNameX; + this.fieldNameY = fieldNameY; + this.supportGeoHash = supportGeoHash; + this.invalidFieldErrMsg = supportGeoHash + ? String.format( + Locale.ROOT, + "field must be either %s/%s, %s/%s, or %s", + fieldNameX, + fieldNameY, + FN_GEOJSON_TYPE, + FN_GEOJSON_COORDS, + FN_GEOHASH + ) + : String.format( + Locale.ROOT, + "field must be either %s/%s, or %s/%s", + fieldNameX, + fieldNameY, + FN_GEOJSON_TYPE, + FN_GEOJSON_COORDS + ); + } + + public Point parseObject(final XContentParser parser, final boolean ignoreZValue, final GeoUtils.EffectivePoint effectivePoint) + throws IOException { + if (parser.currentToken() != XContentParser.Token.START_OBJECT) { + throw new OpenSearchParseException("object is expected"); + } + + parser.nextToken(); + + if (parser.currentToken() == XContentParser.Token.END_OBJECT) { + throw new OpenSearchParseException(invalidFieldErrMsg); + } + + if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { + throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, parser.currentToken()); + } + + Point point = null; + String field = parser.currentName(); + if (fieldNameX.equals(field) || fieldNameY.equals(field)) { + point = parseBasicFields(parser); + } else if (supportGeoHash && FN_GEOHASH.equals(field)) { + point = parseGeoHashFields(parser, effectivePoint); + } else if (FN_GEOJSON_TYPE.equals(field) || FN_GEOJSON_COORDS.equals(field)) { + point = parseGeoJsonFields(parser, ignoreZValue); + } else { + throw new OpenSearchParseException(invalidFieldErrMsg); + } + + if (parser.currentToken() != XContentParser.Token.END_OBJECT) { + throw new OpenSearchParseException(invalidFieldErrMsg); + } + + return point; + } + + private Point parseGeoHashFields(final XContentParser parser, final GeoUtils.EffectivePoint effectivePoint) throws IOException { + if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { + throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, parser.currentToken()); + } + + String field = parser.currentName(); + if (!FN_GEOHASH.equals(field)) { + throw new OpenSearchParseException(invalidFieldErrMsg); + } + + parser.nextToken(); + + if (parser.currentToken() != XContentParser.Token.VALUE_STRING) { + throw new OpenSearchParseException("{} must be a string", FN_GEOHASH); + } + + String geoHash = parser.text(); + GeoPoint geoPoint = new GeoPoint(); + geoPoint = geoPoint.parseGeoHash(geoHash, effectivePoint); + + parser.nextToken(); + return new Point(geoPoint.lon(), geoPoint.lat()); + } + + private Point parseBasicFields(final XContentParser parser) throws IOException { + HashMap xy = new HashMap<>(); + for (int i = 0; i < 2; i++) { + if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { + break; + } + + String field = parser.currentName(); + if (!fieldNameX.equals(field) && !fieldNameY.equals(field)) { + throw new OpenSearchParseException(invalidFieldErrMsg); + } + parser.nextToken(); + switch (parser.currentToken()) { + case VALUE_NUMBER: + case VALUE_STRING: + try { + xy.put(field, parser.doubleValue(true)); + } catch (NumberFormatException e) { + throw new OpenSearchParseException("[{}] and [{}] must be valid double values", e, fieldNameX, fieldNameY); + } + break; + default: + throw new OpenSearchParseException("{} must be a number", field); + } + parser.nextToken(); + } + + if (xy.get(fieldNameX) == null) { + throw new OpenSearchParseException("field [{}] missing", fieldNameX); + } + if (xy.get(fieldNameY) == null) { + throw new OpenSearchParseException("field [{}] missing", fieldNameY); + } + + return new Point(xy.get(fieldNameX), xy.get(fieldNameY)); + } + + private Point parseGeoJsonFields(final XContentParser parser, final boolean ignoreZValue) throws IOException { + boolean isTypePoint = false; + Point point = null; + for (int i = 0; i < 2; i++) { + if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { + throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, parser.currentToken()); + } + + String field = parser.currentName(); + parser.nextToken(); + if (FN_GEOJSON_TYPE.equals(field)) { + if (parser.currentToken() != XContentParser.Token.VALUE_STRING) { + throw new OpenSearchParseException("{} must be a string", FN_GEOJSON_TYPE); + } + + if (!FV_GEOJSON_TYPE_POINT.equals(parser.text())) { + throw new OpenSearchParseException("{} must be {}", FN_GEOJSON_TYPE, FV_GEOJSON_TYPE_POINT); + } + isTypePoint = true; + } else if (FN_GEOJSON_COORDS.equals(field)) { + if (parser.currentToken() != XContentParser.Token.START_ARRAY) { + throw new OpenSearchParseException("{} must be an array", FN_GEOJSON_COORDS); + } + point = parseFromArray(parser, ignoreZValue, FN_GEOJSON_COORDS); + } else { + throw new OpenSearchParseException(invalidFieldErrMsg); + } + parser.nextToken(); + } + + if (!isTypePoint) { + throw new OpenSearchParseException("field [{}] missing", FN_GEOJSON_TYPE); + } + + if (point == null) { + throw new OpenSearchParseException("field [{}] missing", FN_GEOJSON_COORDS); + } + + return point; + } + + public Point parseFromArray(final XContentParser parser, final boolean ignoreZValue, final String fieldName) throws IOException { + double x = Double.NaN; + double y = Double.NaN; + + if (parser.currentToken() == XContentParser.Token.START_ARRAY) { + parser.nextToken(); + } + + int element = 0; + while (parser.currentToken() != XContentParser.Token.END_ARRAY) { + if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { + element++; + if (element == 1) { + x = parser.doubleValue(); + } else if (element == 2) { + y = parser.doubleValue(); + } else if (element == 3) { + GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); + } else { + throw new OpenSearchParseException("[{}] field type does not accept > 3 dimensions", fieldName); + } + } else { + throw new OpenSearchParseException("numeric value expected"); + } + parser.nextToken(); + } + if (element < 2) { + throw new OpenSearchParseException("[{}] field type should have at least two dimensions", fieldName); + } + return new Point(x, y); + } +} diff --git a/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java index b546eeca1ec0a..7db3559357ce6 100644 --- a/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java @@ -281,39 +281,38 @@ private P process(P in) { @Override public List

parse(XContentParser parser) throws IOException, ParseException { - if (parser.currentToken() == XContentParser.Token.START_ARRAY) { - XContentParser.Token token = parser.nextToken(); - P point = pointSupplier.get(); - ArrayList

points = new ArrayList<>(); - if (token == XContentParser.Token.VALUE_NUMBER) { + parser.nextToken(); + if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { double x = parser.doubleValue(); parser.nextToken(); double y = parser.doubleValue(); - token = parser.nextToken(); - if (token == XContentParser.Token.VALUE_NUMBER) { + parser.nextToken(); + if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); - } else if (token != XContentParser.Token.END_ARRAY) { + } else if (parser.currentToken() != XContentParser.Token.END_ARRAY) { throw new OpenSearchParseException("field type does not accept > 3 dimensions"); } - + P point = pointSupplier.get(); point.resetCoords(x, y); - points.add(process(point)); + return Collections.singletonList(process(point)); } else { - while (token != XContentParser.Token.END_ARRAY) { - points.add(process(objectParser.apply(parser, point))); - point = pointSupplier.get(); - token = parser.nextToken(); + ArrayList

points = new ArrayList<>(); + while (parser.currentToken() != XContentParser.Token.END_ARRAY) { + points.add(process(objectParser.apply(parser, pointSupplier.get()))); + parser.nextToken(); } + return points; } - return points; } else if (parser.currentToken() == XContentParser.Token.VALUE_NULL) { + // Null value if (nullValue == null) { return null; } else { return Collections.singletonList(nullValue); } } else { + // Single point return Collections.singletonList(process(objectParser.apply(parser, pointSupplier.get()))); } } diff --git a/server/src/test/java/org/opensearch/common/geo/PointUtilsTests.java b/server/src/test/java/org/opensearch/common/geo/PointUtilsTests.java new file mode 100644 index 0000000000000..d8102ae485f26 --- /dev/null +++ b/server/src/test/java/org/opensearch/common/geo/PointUtilsTests.java @@ -0,0 +1,216 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.common.geo; + +import org.opensearch.OpenSearchParseException; +import org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.common.xcontent.XContentParser; +import org.opensearch.geometry.Point; +import org.opensearch.test.OpenSearchTestCase; + +import java.io.IOException; +import java.util.Locale; + +public class PointUtilsTests extends OpenSearchTestCase { + private static final PointUtils PARSER; + private static final PointUtils PARSER_WITHOUT_GEOHASH; + private static final String FIELD_NAME_X = "x"; + private static final String FIELD_NAME_Y = "y"; + private static final String FN_GEOHASH = "geohash"; + private static final String FN_GEOJSON_TYPE = "type"; + private static final String FV_GEOJSON_TYPE_POINT = "Point"; + private static final String FN_GEOJSON_COORDS = "coordinates"; + + private static final String FN_IN_ERR_MSG = "test error message"; + + static { + PARSER = new PointUtils(FIELD_NAME_X, FIELD_NAME_Y, true); + PARSER_WITHOUT_GEOHASH = new PointUtils(FIELD_NAME_X, FIELD_NAME_Y, false); + } + + public void testParseFromObjectOfBasicForm() throws IOException { + double x = 74.00; + double y = 40.71; + XContentBuilder pointInJson = XContentFactory.jsonBuilder().startObject().field(FIELD_NAME_X, x).field(FIELD_NAME_Y, y).endObject(); + try (XContentParser parser = createParser(pointInJson)) { + parser.nextToken(); + Point point = PARSER.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT); + assertEquals(x, point.getX(), 0.001); + assertEquals(y, point.getY(), 0.001); + } + } + + public void testParseFromObjectOfBasicFormMissingValue() throws IOException { + double x = 74.00; + XContentBuilder pointInJson = XContentFactory.jsonBuilder().startObject().field(FIELD_NAME_X, x).endObject(); + try (XContentParser parser = createParser(pointInJson)) { + parser.nextToken(); + OpenSearchParseException ex = expectThrows( + OpenSearchParseException.class, + () -> PARSER.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT) + ); + assertEquals("field [y] missing", ex.getMessage()); + } + } + + public void testParseFromObjectOfBasicFormAdditionalField() throws IOException { + double x = 74.00; + double y = 40.71; + XContentBuilder pointInJson = XContentFactory.jsonBuilder() + .startObject() + .field(FIELD_NAME_X, x) + .field(FIELD_NAME_Y, y) + .field("InvalidField", 10.1) + .endObject(); + try (XContentParser parser = createParser(pointInJson)) { + parser.nextToken(); + OpenSearchParseException ex = expectThrows( + OpenSearchParseException.class, + () -> PARSER.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT) + ); + assertEquals("field must be either x/y, type/coordinates, or geohash", ex.getMessage()); + } + } + + public void testParseFromObjectOfGeoHash() throws IOException { + double longitude = 74.00; + double latitude = 40.71; + String geohash = "txhxegj0uyp3"; + XContentBuilder pointInJson = XContentFactory.jsonBuilder().startObject().field(FN_GEOHASH, geohash).endObject(); + try (XContentParser parser = createParser(pointInJson)) { + parser.nextToken(); + Point point = PARSER.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT); + assertEquals(longitude, point.getLon(), 0.001); + assertEquals(latitude, point.getLat(), 0.001); + } + } + + public void testParseFromObjectOfGeoHashWithNoGeoHashSupport() throws IOException { + String geohash = "txhxegj0uyp3"; + XContentBuilder pointInJson = XContentFactory.jsonBuilder().startObject().field(FN_GEOHASH, geohash).endObject(); + try (XContentParser parser = createParser(pointInJson)) { + parser.nextToken(); + OpenSearchParseException ex = expectThrows( + OpenSearchParseException.class, + () -> PARSER_WITHOUT_GEOHASH.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT) + ); + assertEquals("field must be either x/y, or type/coordinates", ex.getMessage()); + } + } + + public void testParseFromObjectOfGeoJson() throws IOException { + double x = 41.12; + double y = -71.34; + XContentBuilder pointInJson = XContentFactory.jsonBuilder() + .startObject() + .field(FN_GEOJSON_TYPE, FV_GEOJSON_TYPE_POINT) + .array(FN_GEOJSON_COORDS, new double[] { x, y }) + .endObject(); + try (XContentParser parser = createParser(pointInJson)) { + parser.nextToken(); + Point point = PARSER_WITHOUT_GEOHASH.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT); + assertEquals(x, point.getX(), 0.001); + assertEquals(y, point.getY(), 0.001); + } + } + + public void testParseFromArrayOne() throws IOException { + double x = 41.12; + XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).endArray(); + try (XContentParser parser = createParser(pointArray)) { + parser.nextToken(); + // parser -> "[41.12]" + OpenSearchParseException ex = expectThrows( + OpenSearchParseException.class, + () -> PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG) + ); + assertEquals(String.format(Locale.ROOT, "[%s] field type should have at least two dimensions", FN_IN_ERR_MSG), ex.getMessage()); + } + } + + public void testParseFromArrayTwo() throws IOException { + double x = 41.12; + double y = -71.34; + XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).endArray(); + try (XContentParser parser = createParser(pointArray)) { + parser.nextToken(); + // parser -> "[41.12, -71.34]" + Point point = PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG); + assertEquals(x, point.getX(), 0.001); + assertEquals(y, point.getY(), 0.001); + } + } + + public void testParseFromArrayThree() throws IOException { + double x = 41.12; + double y = -71.34; + double z = 12.12; + XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).value(z).endArray(); + try (XContentParser parser = createParser(pointArray)) { + parser.nextToken(); + // parser -> "[41.12, -71.34, 12.12]" + OpenSearchParseException ex = expectThrows( + OpenSearchParseException.class, + () -> PARSER.parseFromArray(parser, false, FN_IN_ERR_MSG) + ); + assertEquals( + String.format( + Locale.ROOT, + "Exception parsing coordinates: found Z value [%.2f] but [ignore_z_value] parameter is [false]", + z + ), + ex.getMessage() + ); + } + } + + public void testParseFromArrayThreeIgnore() throws IOException { + double x = 41.12; + double y = -71.34; + double z = 12.12; + XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).value(z).endArray(); + try (XContentParser parser = createParser(pointArray)) { + parser.nextToken(); + // parser -> "[41.12, -71.34, 12.12]" + Point point = PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG); + assertEquals(x, point.getX(), 0.001); + assertEquals(y, point.getY(), 0.001); + } + } + + public void testParseFromArrayFourIgnore() throws IOException { + double x = 41.12; + double y = -71.34; + double z = 12.12; + double a = 33.12; + XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).value(z).value(a).endArray(); + try (XContentParser parser = createParser(pointArray)) { + parser.nextToken(); + // parser -> "[41.12, -71.34, 12.12, 33.12]" + OpenSearchParseException ex = expectThrows( + OpenSearchParseException.class, + () -> PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG) + ); + assertEquals(String.format(Locale.ROOT, "[%s] field type does not accept > 3 dimensions", FN_IN_ERR_MSG), ex.getMessage()); + } + } + + // "41.12, -71.34]" + public void testParseFromArrayPartialForm() throws IOException { + double x = 41.12; + double y = -71.34; + XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).endArray(); + try (XContentParser parser = createParser(pointArray)) { + parser.nextToken(); + parser.nextToken(); + // parser -> "41.12, -71.34]" + Point point = PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG); + assertEquals(x, point.getX(), 0.001); + assertEquals(y, point.getY(), 0.001); + } + } +} diff --git a/server/src/test/java/org/opensearch/index/search/geo/GeoPointParsingTests.java b/server/src/test/java/org/opensearch/index/search/geo/GeoPointParsingTests.java index 6b937ad881f00..06f36435608bd 100644 --- a/server/src/test/java/org/opensearch/index/search/geo/GeoPointParsingTests.java +++ b/server/src/test/java/org/opensearch/index/search/geo/GeoPointParsingTests.java @@ -142,12 +142,12 @@ public void testInvalidPointEmbeddedObject() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either [lat], [lon] or [geohash]")); + assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(OpenSearchParseException.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 lon/lat, type/coordinates, or geohash")); } } @@ -160,12 +160,12 @@ public void testInvalidPointLatHashMix() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either lat/lon or geohash")); + assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(OpenSearchParseException.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 lon/lat, type/coordinates, or geohash")); } } @@ -179,12 +179,12 @@ public void testInvalidPointLonHashMix() throws IOException { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either lat/lon or geohash")); + assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(OpenSearchParseException.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 lon/lat, type/coordinates, or geohash")); } } @@ -197,13 +197,13 @@ public void testInvalidField() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either [lat], [lon] or [geohash]")); + assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(OpenSearchParseException.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 lon/lat, type/coordinates, or geohash")); } } diff --git a/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java b/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java index bb4b057d01414..e10d8b21b695a 100644 --- a/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java +++ b/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java @@ -601,7 +601,7 @@ public void testParseGeoPointLonWrongType() throws IOException { try (XContentParser parser = createParser(json)) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("longitude must be a number")); + assertThat(e.getMessage(), is("lon must be a number")); assertThat(parser.currentToken(), is(Token.END_OBJECT)); assertNull(parser.nextToken()); } @@ -613,7 +613,7 @@ public void testParseGeoPointLatWrongType() throws IOException { try (XContentParser parser = createParser(json)) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("latitude must be a number")); + assertThat(e.getMessage(), is("lat must be a number")); assertThat(parser.currentToken(), is(Token.END_OBJECT)); assertNull(parser.nextToken()); } @@ -626,7 +626,7 @@ public void testParseGeoPointExtraField() throws IOException { try (XContentParser parser = createParser(json)) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either [lat], [lon] or [geohash]")); + assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); } } @@ -638,7 +638,7 @@ public void testParseGeoPointLonLatGeoHash() throws IOException { try (XContentParser parser = createParser(json)) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), containsString("field must be either lat/lon or geohash")); + assertThat(e.getMessage(), containsString("field must be either lon/lat, type/coordinates, or geohash")); } } From d0bf31cbe809275330295e53e0556a74aa15f5cb Mon Sep 17 00:00:00 2001 From: Heemin Kim Date: Fri, 30 Sep 2022 19:31:33 -0700 Subject: [PATCH 2/5] Refactored code based on comments Signed-off-by: Heemin Kim --- .../org/opensearch/common/geo/GeoPoint.java | 2 +- .../org/opensearch/common/geo/GeoUtils.java | 192 +++++++++++++-- .../org/opensearch/common/geo/PointUtils.java | 227 ------------------ .../AbstractPointGeometryFieldMapper.java | 58 +++-- .../common/geo/PointUtilsTests.java | 216 ----------------- .../search/geo/GeoPointParsingTests.java | 17 +- .../index/search/geo/GeoUtilsTests.java | 69 +++++- 7 files changed, 291 insertions(+), 490 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/common/geo/PointUtils.java delete mode 100644 server/src/test/java/org/opensearch/common/geo/PointUtilsTests.java diff --git a/server/src/main/java/org/opensearch/common/geo/GeoPoint.java b/server/src/main/java/org/opensearch/common/geo/GeoPoint.java index f6b049b46789d..c59a9046e0318 100644 --- a/server/src/main/java/org/opensearch/common/geo/GeoPoint.java +++ b/server/src/main/java/org/opensearch/common/geo/GeoPoint.java @@ -121,7 +121,7 @@ public GeoPoint resetFromCoordinates(String value, final boolean ignoreZValue) { if (vals.length > 3) { throw new OpenSearchParseException( "failed to parse [{}], expected 2 or 3 coordinates " + "but found: [{}]", - vals.length, + value, vals.length ); } diff --git a/server/src/main/java/org/opensearch/common/geo/GeoUtils.java b/server/src/main/java/org/opensearch/common/geo/GeoUtils.java index 6a7a319197daf..8e1f6c7094c8d 100644 --- a/server/src/main/java/org/opensearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/opensearch/common/geo/GeoUtils.java @@ -43,7 +43,6 @@ import org.opensearch.common.xcontent.XContentSubParser; import org.opensearch.common.xcontent.support.MapXContentParser; import org.opensearch.common.xcontent.support.XContentMapValues; -import org.opensearch.geometry.Point; import org.opensearch.index.fielddata.FieldData; import org.opensearch.index.fielddata.GeoPointValues; import org.opensearch.index.fielddata.MultiGeoPointValues; @@ -53,6 +52,7 @@ import java.io.IOException; import java.util.Collections; +import java.util.HashMap; /** * Useful geo utilities @@ -60,7 +60,8 @@ * @opensearch.internal */ public class GeoUtils { - + private static final String ERR_MSG_INVALID_TOKEN = "token [{}] not allowed"; + private static final String ERR_MSG_INVALID_FIELDS = "field must be either [lon|lat], [type|coordinates], or [geohash]"; /** Maximum valid latitude in degrees. */ public static final double MAX_LAT = 90.0; /** Minimum valid latitude in degrees. */ @@ -74,6 +75,9 @@ public class GeoUtils { public static final String LONGITUDE = "lon"; public static final String GEOHASH = "geohash"; + public static final String GEOJSON_TYPE = "type"; + public static final String GEOJSON_TYPE_POINT = "Point"; + public static final String GEOJSON_COORDS = "coordinates"; /** Earth ellipsoid major axis defined by WGS 84 in meters */ public static final double EARTH_SEMI_MAJOR_AXIS = 6378137.0; // meters (WGS 84) @@ -95,12 +99,6 @@ public class GeoUtils { /** rounding error for quantized latitude and longitude values */ public static final double TOLERANCE = 1E-6; - public static final PointUtils POINT_PARSER; - - static { - POINT_PARSER = new PointUtils(LONGITUDE, LATITUDE, true); - } - /** Returns true if latitude is actually a valid latitude value.*/ public static boolean isValidLatitude(double latitude) { if (Double.isNaN(latitude) || Double.isInfinite(latitude) || latitude < GeoUtils.MIN_LAT || latitude > GeoUtils.MAX_LAT) { @@ -465,20 +463,18 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina * @param effectivePoint tells which point to use for GeoHash form * @return new {@link GeoPoint} parsed from the parse */ - public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue, EffectivePoint effectivePoint) - throws IOException, OpenSearchParseException { + public static GeoPoint parseGeoPoint( + final XContentParser parser, + final GeoPoint point, + final boolean ignoreZValue, + final EffectivePoint effectivePoint + ) throws IOException, OpenSearchParseException { switch (parser.currentToken()) { case START_OBJECT: - try (XContentSubParser subParser = new XContentSubParser(parser)) { - Point pointInObject = POINT_PARSER.parseObject(subParser, ignoreZValue, effectivePoint); - point.reset(pointInObject.getLat(), pointInObject.getLon()); - } + parseGeoPointObject(parser, point, ignoreZValue, effectivePoint); break; case START_ARRAY: - try (XContentSubParser subParser = new XContentSubParser(parser)) { - Point pointInArray = POINT_PARSER.parseFromArray(subParser, ignoreZValue, "geo_point"); - point.reset(pointInArray.getLat(), pointInArray.getLon()); - } + parseGeoPointArray(parser, point, ignoreZValue); break; case VALUE_STRING: String val = parser.text(); @@ -490,6 +486,166 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina return point; } + private static GeoPoint parseGeoPointObject( + final XContentParser parser, + final GeoPoint point, + final boolean ignoreZValue, + final GeoUtils.EffectivePoint effectivePoint + ) throws IOException { + try (XContentSubParser subParser = new XContentSubParser(parser)) { + if (subParser.nextToken() != XContentParser.Token.FIELD_NAME) { + throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, subParser.currentToken()); + } + + String field = subParser.currentName(); + if (LONGITUDE.equals(field) || LATITUDE.equals(field)) { + parseGeoPointObjectBasicFields(subParser, point); + } else if (GEOHASH.equals(field)) { + parseGeoHashFields(subParser, point, effectivePoint); + } else if (GEOJSON_TYPE.equals(field) || GEOJSON_COORDS.equals(field)) { + parseGeoJsonFields(subParser, point, ignoreZValue); + } else { + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); + } + + if (subParser.nextToken() != XContentParser.Token.END_OBJECT) { + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); + } + + return point; + } + } + + private static GeoPoint parseGeoPointObjectBasicFields(final XContentParser parser, final GeoPoint point) throws IOException { + HashMap data = new HashMap<>(); + for (int i = 0; i < 2; i++) { + if (i != 0) { + parser.nextToken(); + } + + if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { + break; + } + + String field = parser.currentName(); + if (!LONGITUDE.equals(field) && !LATITUDE.equals(field)) { + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); + } + switch (parser.nextToken()) { + case VALUE_NUMBER: + case VALUE_STRING: + try { + data.put(field, parser.doubleValue(true)); + } catch (NumberFormatException e) { + throw new OpenSearchParseException("[{}] and [{}] must be valid double values", e, LONGITUDE, LATITUDE); + } + break; + default: + throw new OpenSearchParseException("{} must be a number", field); + } + } + + if (data.get(LONGITUDE) == null) { + throw new OpenSearchParseException("field [{}] missing", LONGITUDE); + } + if (data.get(LATITUDE) == null) { + throw new OpenSearchParseException("field [{}] missing", LATITUDE); + } + + return point.reset(data.get(LATITUDE), data.get(LONGITUDE)); + } + + private static GeoPoint parseGeoHashFields( + final XContentParser parser, + final GeoPoint point, + final GeoUtils.EffectivePoint effectivePoint + ) throws IOException { + if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { + throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, parser.currentToken()); + } + + if (!GEOHASH.equals(parser.currentName())) { + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); + } + + if (parser.nextToken() != XContentParser.Token.VALUE_STRING) { + throw new OpenSearchParseException("{} must be a string", GEOHASH); + } + + return point.parseGeoHash(parser.text(), effectivePoint); + } + + private static GeoPoint parseGeoJsonFields(final XContentParser parser, final GeoPoint point, final boolean ignoreZValue) + throws IOException { + boolean hasTypePoint = false; + boolean hasCoordinates = false; + for (int i = 0; i < 2; i++) { + if (i != 0) { + parser.nextToken(); + } + + if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { + if (!hasTypePoint) { + throw new OpenSearchParseException("field [{}] missing", GEOJSON_TYPE); + } + if (!hasCoordinates) { + throw new OpenSearchParseException("field [{}] missing", GEOJSON_COORDS); + } + } + + if (GEOJSON_TYPE.equals(parser.currentName())) { + if (parser.nextToken() != XContentParser.Token.VALUE_STRING) { + throw new OpenSearchParseException("{} must be a string", GEOJSON_TYPE); + } + + if (!GEOJSON_TYPE_POINT.equals(parser.text())) { + throw new OpenSearchParseException("{} must be {}", GEOJSON_TYPE, GEOJSON_TYPE_POINT); + } + hasTypePoint = true; + } else if (GEOJSON_COORDS.equals(parser.currentName())) { + if (parser.nextToken() != XContentParser.Token.START_ARRAY) { + throw new OpenSearchParseException("{} must be an array", GEOJSON_COORDS); + } + parseGeoPointArray(parser, point, ignoreZValue); + hasCoordinates = true; + } else { + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); + } + } + + return point; + } + + private static GeoPoint parseGeoPointArray(final XContentParser parser, final GeoPoint point, final boolean ignoreZValue) + throws IOException { + try (XContentSubParser subParser = new XContentSubParser(parser)) { + double x = Double.NaN; + double y = Double.NaN; + + int element = 0; + while (subParser.nextToken() != XContentParser.Token.END_ARRAY) { + if (parser.currentToken() != XContentParser.Token.VALUE_NUMBER) { + throw new OpenSearchParseException("numeric value expected"); + } + element++; + if (element == 1) { + x = parser.doubleValue(); + } else if (element == 2) { + y = parser.doubleValue(); + } else if (element == 3) { + GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); + } else { + throw new OpenSearchParseException("[geo_point] field type does not accept more than 3 values"); + } + } + + if (element < 2) { + throw new OpenSearchParseException("[geo_point] field type should have at least two dimensions"); + } + return point.reset(y, x); + } + } + /** * Parse a {@link GeoPoint} from a string. The string must have one of the following forms: * diff --git a/server/src/main/java/org/opensearch/common/geo/PointUtils.java b/server/src/main/java/org/opensearch/common/geo/PointUtils.java deleted file mode 100644 index d5d823af05b1f..0000000000000 --- a/server/src/main/java/org/opensearch/common/geo/PointUtils.java +++ /dev/null @@ -1,227 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.common.geo; - -import org.opensearch.OpenSearchParseException; -import org.opensearch.common.xcontent.XContentParser; -import org.opensearch.geometry.Point; - -import java.io.IOException; -import java.util.HashMap; -import java.util.Locale; - -/** - * A utility class with a point parser methods supporting different point representation formats - * - * @opensearch.internal - */ -public class PointUtils { - private static final String FN_GEOJSON_TYPE = "type"; - private static final String FV_GEOJSON_TYPE_POINT = "Point"; - private static final String FN_GEOJSON_COORDS = "coordinates"; - - private static final String FN_GEOHASH = "geohash"; - private static final String ERR_MSG_INVALID_TOKEN = "token [{}] not allowed"; - - private String fieldNameX; - private String fieldNameY; - private boolean supportGeoHash; - - private String invalidFieldErrMsg; - - public PointUtils(final String fieldNameX, final String fieldNameY, final boolean supportGeoHash) { - this.fieldNameX = fieldNameX; - this.fieldNameY = fieldNameY; - this.supportGeoHash = supportGeoHash; - this.invalidFieldErrMsg = supportGeoHash - ? String.format( - Locale.ROOT, - "field must be either %s/%s, %s/%s, or %s", - fieldNameX, - fieldNameY, - FN_GEOJSON_TYPE, - FN_GEOJSON_COORDS, - FN_GEOHASH - ) - : String.format( - Locale.ROOT, - "field must be either %s/%s, or %s/%s", - fieldNameX, - fieldNameY, - FN_GEOJSON_TYPE, - FN_GEOJSON_COORDS - ); - } - - public Point parseObject(final XContentParser parser, final boolean ignoreZValue, final GeoUtils.EffectivePoint effectivePoint) - throws IOException { - if (parser.currentToken() != XContentParser.Token.START_OBJECT) { - throw new OpenSearchParseException("object is expected"); - } - - parser.nextToken(); - - if (parser.currentToken() == XContentParser.Token.END_OBJECT) { - throw new OpenSearchParseException(invalidFieldErrMsg); - } - - if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { - throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, parser.currentToken()); - } - - Point point = null; - String field = parser.currentName(); - if (fieldNameX.equals(field) || fieldNameY.equals(field)) { - point = parseBasicFields(parser); - } else if (supportGeoHash && FN_GEOHASH.equals(field)) { - point = parseGeoHashFields(parser, effectivePoint); - } else if (FN_GEOJSON_TYPE.equals(field) || FN_GEOJSON_COORDS.equals(field)) { - point = parseGeoJsonFields(parser, ignoreZValue); - } else { - throw new OpenSearchParseException(invalidFieldErrMsg); - } - - if (parser.currentToken() != XContentParser.Token.END_OBJECT) { - throw new OpenSearchParseException(invalidFieldErrMsg); - } - - return point; - } - - private Point parseGeoHashFields(final XContentParser parser, final GeoUtils.EffectivePoint effectivePoint) throws IOException { - if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { - throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, parser.currentToken()); - } - - String field = parser.currentName(); - if (!FN_GEOHASH.equals(field)) { - throw new OpenSearchParseException(invalidFieldErrMsg); - } - - parser.nextToken(); - - if (parser.currentToken() != XContentParser.Token.VALUE_STRING) { - throw new OpenSearchParseException("{} must be a string", FN_GEOHASH); - } - - String geoHash = parser.text(); - GeoPoint geoPoint = new GeoPoint(); - geoPoint = geoPoint.parseGeoHash(geoHash, effectivePoint); - - parser.nextToken(); - return new Point(geoPoint.lon(), geoPoint.lat()); - } - - private Point parseBasicFields(final XContentParser parser) throws IOException { - HashMap xy = new HashMap<>(); - for (int i = 0; i < 2; i++) { - if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { - break; - } - - String field = parser.currentName(); - if (!fieldNameX.equals(field) && !fieldNameY.equals(field)) { - throw new OpenSearchParseException(invalidFieldErrMsg); - } - parser.nextToken(); - switch (parser.currentToken()) { - case VALUE_NUMBER: - case VALUE_STRING: - try { - xy.put(field, parser.doubleValue(true)); - } catch (NumberFormatException e) { - throw new OpenSearchParseException("[{}] and [{}] must be valid double values", e, fieldNameX, fieldNameY); - } - break; - default: - throw new OpenSearchParseException("{} must be a number", field); - } - parser.nextToken(); - } - - if (xy.get(fieldNameX) == null) { - throw new OpenSearchParseException("field [{}] missing", fieldNameX); - } - if (xy.get(fieldNameY) == null) { - throw new OpenSearchParseException("field [{}] missing", fieldNameY); - } - - return new Point(xy.get(fieldNameX), xy.get(fieldNameY)); - } - - private Point parseGeoJsonFields(final XContentParser parser, final boolean ignoreZValue) throws IOException { - boolean isTypePoint = false; - Point point = null; - for (int i = 0; i < 2; i++) { - if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { - throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, parser.currentToken()); - } - - String field = parser.currentName(); - parser.nextToken(); - if (FN_GEOJSON_TYPE.equals(field)) { - if (parser.currentToken() != XContentParser.Token.VALUE_STRING) { - throw new OpenSearchParseException("{} must be a string", FN_GEOJSON_TYPE); - } - - if (!FV_GEOJSON_TYPE_POINT.equals(parser.text())) { - throw new OpenSearchParseException("{} must be {}", FN_GEOJSON_TYPE, FV_GEOJSON_TYPE_POINT); - } - isTypePoint = true; - } else if (FN_GEOJSON_COORDS.equals(field)) { - if (parser.currentToken() != XContentParser.Token.START_ARRAY) { - throw new OpenSearchParseException("{} must be an array", FN_GEOJSON_COORDS); - } - point = parseFromArray(parser, ignoreZValue, FN_GEOJSON_COORDS); - } else { - throw new OpenSearchParseException(invalidFieldErrMsg); - } - parser.nextToken(); - } - - if (!isTypePoint) { - throw new OpenSearchParseException("field [{}] missing", FN_GEOJSON_TYPE); - } - - if (point == null) { - throw new OpenSearchParseException("field [{}] missing", FN_GEOJSON_COORDS); - } - - return point; - } - - public Point parseFromArray(final XContentParser parser, final boolean ignoreZValue, final String fieldName) throws IOException { - double x = Double.NaN; - double y = Double.NaN; - - if (parser.currentToken() == XContentParser.Token.START_ARRAY) { - parser.nextToken(); - } - - int element = 0; - while (parser.currentToken() != XContentParser.Token.END_ARRAY) { - if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { - element++; - if (element == 1) { - x = parser.doubleValue(); - } else if (element == 2) { - y = parser.doubleValue(); - } else if (element == 3) { - GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); - } else { - throw new OpenSearchParseException("[{}] field type does not accept > 3 dimensions", fieldName); - } - } else { - throw new OpenSearchParseException("numeric value expected"); - } - parser.nextToken(); - } - if (element < 2) { - throw new OpenSearchParseException("[{}] field type should have at least two dimensions", fieldName); - } - return new Point(x, y); - } -} diff --git a/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java index 7db3559357ce6..305351ca63b9a 100644 --- a/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/AbstractPointGeometryFieldMapper.java @@ -36,11 +36,14 @@ import org.opensearch.common.CheckedBiFunction; import org.opensearch.common.Explicit; import org.opensearch.common.ParseField; -import org.opensearch.common.geo.GeoPoint; +import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.geo.GeometryFormat; import org.opensearch.common.geo.GeometryParser; +import org.opensearch.common.xcontent.DeprecationHandler; import org.opensearch.common.xcontent.LoggingDeprecationHandler; +import org.opensearch.common.xcontent.NamedXContentRegistry; import org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.common.xcontent.XContentFactory; import org.opensearch.common.xcontent.XContentParser; import org.opensearch.geometry.Geometry; import org.opensearch.geometry.Point; @@ -57,7 +60,7 @@ import static org.opensearch.index.mapper.TypeParsers.parseField; /** - * Base class for for spatial fields that only support indexing points + * Base class for spatial fields that only support indexing points * * @opensearch.internal */ @@ -284,18 +287,16 @@ public List

parse(XContentParser parser) throws IOException, ParseException { if (parser.currentToken() == XContentParser.Token.START_ARRAY) { parser.nextToken(); if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { - double x = parser.doubleValue(); - parser.nextToken(); - double y = parser.doubleValue(); - parser.nextToken(); - if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { - GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); - } else if (parser.currentToken() != XContentParser.Token.END_ARRAY) { - throw new OpenSearchParseException("field type does not accept > 3 dimensions"); + XContentBuilder xContentBuilder = reConstructArrayXContent(parser); + try ( + XContentParser subParser = createParser( + parser.getXContentRegistry(), + parser.getDeprecationHandler(), + xContentBuilder + ); + ) { + return Collections.singletonList(process(objectParser.apply(subParser, pointSupplier.get()))); } - P point = pointSupplier.get(); - point.resetCoords(x, y); - return Collections.singletonList(process(point)); } else { ArrayList

points = new ArrayList<>(); while (parser.currentToken() != XContentParser.Token.END_ARRAY) { @@ -305,18 +306,45 @@ public List

parse(XContentParser parser) throws IOException, ParseException { return points; } } else if (parser.currentToken() == XContentParser.Token.VALUE_NULL) { - // Null value if (nullValue == null) { return null; } else { return Collections.singletonList(nullValue); } } else { - // Single point return Collections.singletonList(process(objectParser.apply(parser, pointSupplier.get()))); } } + private XContentParser createParser( + NamedXContentRegistry namedXContentRegistry, + DeprecationHandler deprecationHandler, + XContentBuilder xContentBuilder + ) throws IOException { + XContentParser subParser = xContentBuilder.contentType() + .xContent() + .createParser(namedXContentRegistry, deprecationHandler, BytesReference.bytes(xContentBuilder).streamInput()); + subParser.nextToken(); + return subParser; + } + + private XContentBuilder reConstructArrayXContent(XContentParser parser) throws IOException { + XContentBuilder builder = XContentFactory.jsonBuilder().startArray(); + int count = 0; + while (parser.currentToken() != XContentParser.Token.END_ARRAY) { + if (++count > 3) { + break; + } + if (parser.currentToken() != XContentParser.Token.VALUE_NUMBER) { + throw new OpenSearchParseException("numeric value expected"); + } + builder.value(parser.doubleValue()); + parser.nextToken(); + } + builder.endArray(); + return builder; + } + @Override public Object format(List

points, String format) { List result = new ArrayList<>(); diff --git a/server/src/test/java/org/opensearch/common/geo/PointUtilsTests.java b/server/src/test/java/org/opensearch/common/geo/PointUtilsTests.java deleted file mode 100644 index d8102ae485f26..0000000000000 --- a/server/src/test/java/org/opensearch/common/geo/PointUtilsTests.java +++ /dev/null @@ -1,216 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.common.geo; - -import org.opensearch.OpenSearchParseException; -import org.opensearch.common.xcontent.XContentBuilder; -import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.common.xcontent.XContentParser; -import org.opensearch.geometry.Point; -import org.opensearch.test.OpenSearchTestCase; - -import java.io.IOException; -import java.util.Locale; - -public class PointUtilsTests extends OpenSearchTestCase { - private static final PointUtils PARSER; - private static final PointUtils PARSER_WITHOUT_GEOHASH; - private static final String FIELD_NAME_X = "x"; - private static final String FIELD_NAME_Y = "y"; - private static final String FN_GEOHASH = "geohash"; - private static final String FN_GEOJSON_TYPE = "type"; - private static final String FV_GEOJSON_TYPE_POINT = "Point"; - private static final String FN_GEOJSON_COORDS = "coordinates"; - - private static final String FN_IN_ERR_MSG = "test error message"; - - static { - PARSER = new PointUtils(FIELD_NAME_X, FIELD_NAME_Y, true); - PARSER_WITHOUT_GEOHASH = new PointUtils(FIELD_NAME_X, FIELD_NAME_Y, false); - } - - public void testParseFromObjectOfBasicForm() throws IOException { - double x = 74.00; - double y = 40.71; - XContentBuilder pointInJson = XContentFactory.jsonBuilder().startObject().field(FIELD_NAME_X, x).field(FIELD_NAME_Y, y).endObject(); - try (XContentParser parser = createParser(pointInJson)) { - parser.nextToken(); - Point point = PARSER.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT); - assertEquals(x, point.getX(), 0.001); - assertEquals(y, point.getY(), 0.001); - } - } - - public void testParseFromObjectOfBasicFormMissingValue() throws IOException { - double x = 74.00; - XContentBuilder pointInJson = XContentFactory.jsonBuilder().startObject().field(FIELD_NAME_X, x).endObject(); - try (XContentParser parser = createParser(pointInJson)) { - parser.nextToken(); - OpenSearchParseException ex = expectThrows( - OpenSearchParseException.class, - () -> PARSER.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT) - ); - assertEquals("field [y] missing", ex.getMessage()); - } - } - - public void testParseFromObjectOfBasicFormAdditionalField() throws IOException { - double x = 74.00; - double y = 40.71; - XContentBuilder pointInJson = XContentFactory.jsonBuilder() - .startObject() - .field(FIELD_NAME_X, x) - .field(FIELD_NAME_Y, y) - .field("InvalidField", 10.1) - .endObject(); - try (XContentParser parser = createParser(pointInJson)) { - parser.nextToken(); - OpenSearchParseException ex = expectThrows( - OpenSearchParseException.class, - () -> PARSER.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT) - ); - assertEquals("field must be either x/y, type/coordinates, or geohash", ex.getMessage()); - } - } - - public void testParseFromObjectOfGeoHash() throws IOException { - double longitude = 74.00; - double latitude = 40.71; - String geohash = "txhxegj0uyp3"; - XContentBuilder pointInJson = XContentFactory.jsonBuilder().startObject().field(FN_GEOHASH, geohash).endObject(); - try (XContentParser parser = createParser(pointInJson)) { - parser.nextToken(); - Point point = PARSER.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT); - assertEquals(longitude, point.getLon(), 0.001); - assertEquals(latitude, point.getLat(), 0.001); - } - } - - public void testParseFromObjectOfGeoHashWithNoGeoHashSupport() throws IOException { - String geohash = "txhxegj0uyp3"; - XContentBuilder pointInJson = XContentFactory.jsonBuilder().startObject().field(FN_GEOHASH, geohash).endObject(); - try (XContentParser parser = createParser(pointInJson)) { - parser.nextToken(); - OpenSearchParseException ex = expectThrows( - OpenSearchParseException.class, - () -> PARSER_WITHOUT_GEOHASH.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT) - ); - assertEquals("field must be either x/y, or type/coordinates", ex.getMessage()); - } - } - - public void testParseFromObjectOfGeoJson() throws IOException { - double x = 41.12; - double y = -71.34; - XContentBuilder pointInJson = XContentFactory.jsonBuilder() - .startObject() - .field(FN_GEOJSON_TYPE, FV_GEOJSON_TYPE_POINT) - .array(FN_GEOJSON_COORDS, new double[] { x, y }) - .endObject(); - try (XContentParser parser = createParser(pointInJson)) { - parser.nextToken(); - Point point = PARSER_WITHOUT_GEOHASH.parseObject(parser, true, GeoUtils.EffectivePoint.BOTTOM_LEFT); - assertEquals(x, point.getX(), 0.001); - assertEquals(y, point.getY(), 0.001); - } - } - - public void testParseFromArrayOne() throws IOException { - double x = 41.12; - XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).endArray(); - try (XContentParser parser = createParser(pointArray)) { - parser.nextToken(); - // parser -> "[41.12]" - OpenSearchParseException ex = expectThrows( - OpenSearchParseException.class, - () -> PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG) - ); - assertEquals(String.format(Locale.ROOT, "[%s] field type should have at least two dimensions", FN_IN_ERR_MSG), ex.getMessage()); - } - } - - public void testParseFromArrayTwo() throws IOException { - double x = 41.12; - double y = -71.34; - XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).endArray(); - try (XContentParser parser = createParser(pointArray)) { - parser.nextToken(); - // parser -> "[41.12, -71.34]" - Point point = PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG); - assertEquals(x, point.getX(), 0.001); - assertEquals(y, point.getY(), 0.001); - } - } - - public void testParseFromArrayThree() throws IOException { - double x = 41.12; - double y = -71.34; - double z = 12.12; - XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).value(z).endArray(); - try (XContentParser parser = createParser(pointArray)) { - parser.nextToken(); - // parser -> "[41.12, -71.34, 12.12]" - OpenSearchParseException ex = expectThrows( - OpenSearchParseException.class, - () -> PARSER.parseFromArray(parser, false, FN_IN_ERR_MSG) - ); - assertEquals( - String.format( - Locale.ROOT, - "Exception parsing coordinates: found Z value [%.2f] but [ignore_z_value] parameter is [false]", - z - ), - ex.getMessage() - ); - } - } - - public void testParseFromArrayThreeIgnore() throws IOException { - double x = 41.12; - double y = -71.34; - double z = 12.12; - XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).value(z).endArray(); - try (XContentParser parser = createParser(pointArray)) { - parser.nextToken(); - // parser -> "[41.12, -71.34, 12.12]" - Point point = PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG); - assertEquals(x, point.getX(), 0.001); - assertEquals(y, point.getY(), 0.001); - } - } - - public void testParseFromArrayFourIgnore() throws IOException { - double x = 41.12; - double y = -71.34; - double z = 12.12; - double a = 33.12; - XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).value(z).value(a).endArray(); - try (XContentParser parser = createParser(pointArray)) { - parser.nextToken(); - // parser -> "[41.12, -71.34, 12.12, 33.12]" - OpenSearchParseException ex = expectThrows( - OpenSearchParseException.class, - () -> PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG) - ); - assertEquals(String.format(Locale.ROOT, "[%s] field type does not accept > 3 dimensions", FN_IN_ERR_MSG), ex.getMessage()); - } - } - - // "41.12, -71.34]" - public void testParseFromArrayPartialForm() throws IOException { - double x = 41.12; - double y = -71.34; - XContentBuilder pointArray = XContentFactory.jsonBuilder().startArray().value(x).value(y).endArray(); - try (XContentParser parser = createParser(pointArray)) { - parser.nextToken(); - parser.nextToken(); - // parser -> "41.12, -71.34]" - Point point = PARSER.parseFromArray(parser, true, FN_IN_ERR_MSG); - assertEquals(x, point.getX(), 0.001); - assertEquals(y, point.getY(), 0.001); - } - } -} diff --git a/server/src/test/java/org/opensearch/index/search/geo/GeoPointParsingTests.java b/server/src/test/java/org/opensearch/index/search/geo/GeoPointParsingTests.java index 06f36435608bd..ea8d660b78157 100644 --- a/server/src/test/java/org/opensearch/index/search/geo/GeoPointParsingTests.java +++ b/server/src/test/java/org/opensearch/index/search/geo/GeoPointParsingTests.java @@ -50,6 +50,7 @@ import static org.hamcrest.Matchers.is; public class GeoPointParsingTests extends OpenSearchTestCase { + private static final String ERR_MSG_INVALID_FIELDS = "field must be either [lon|lat], [type|coordinates], or [geohash]"; private static final double TOLERANCE = 1E-5; public void testGeoPointReset() throws IOException { @@ -142,12 +143,12 @@ public void testInvalidPointEmbeddedObject() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), is(ERR_MSG_INVALID_FIELDS)); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean())); - assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), is(ERR_MSG_INVALID_FIELDS)); } } @@ -160,12 +161,12 @@ public void testInvalidPointLatHashMix() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), is(ERR_MSG_INVALID_FIELDS)); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean())); - assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), is(ERR_MSG_INVALID_FIELDS)); } } @@ -179,12 +180,12 @@ public void testInvalidPointLonHashMix() throws IOException { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), is(ERR_MSG_INVALID_FIELDS)); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean())); - assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), is(ERR_MSG_INVALID_FIELDS)); } } @@ -197,13 +198,13 @@ public void testInvalidField() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), is(ERR_MSG_INVALID_FIELDS)); } try (XContentParser parser2 = createParser(JsonXContent.jsonXContent, BytesReference.bytes(content))) { parser2.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(toObject(parser2), randomBoolean())); - assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), is(ERR_MSG_INVALID_FIELDS)); } } diff --git a/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java b/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java index e10d8b21b695a..f9f6bea077dd6 100644 --- a/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java +++ b/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java @@ -35,6 +35,8 @@ import org.apache.lucene.spatial.prefix.tree.Cell; import org.apache.lucene.spatial.prefix.tree.GeohashPrefixTree; import org.apache.lucene.spatial.prefix.tree.QuadPrefixTree; +import org.locationtech.spatial4j.context.SpatialContext; +import org.locationtech.spatial4j.distance.DistanceUtils; import org.opensearch.OpenSearchParseException; import org.opensearch.common.geo.GeoPoint; import org.opensearch.common.geo.GeoUtils; @@ -43,12 +45,10 @@ import org.opensearch.common.xcontent.XContentParser.Token; import org.opensearch.geometry.utils.Geohash; import org.opensearch.test.OpenSearchTestCase; -import org.locationtech.spatial4j.context.SpatialContext; -import org.locationtech.spatial4j.distance.DistanceUtils; +import org.opensearch.test.geo.RandomGeoGenerator; import java.io.IOException; -import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.closeTo; import static org.hamcrest.Matchers.containsString; @@ -57,8 +57,10 @@ import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.not; +import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; public class GeoUtilsTests extends OpenSearchTestCase { + private static final String ERR_MSG_INVALID_FIELDS = "field must be either [lon|lat], [type|coordinates], or [geohash]"; private static final char[] BASE_32 = { '0', '1', @@ -626,7 +628,7 @@ public void testParseGeoPointExtraField() throws IOException { try (XContentParser parser = createParser(json)) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), is("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), is(ERR_MSG_INVALID_FIELDS)); } } @@ -638,7 +640,7 @@ public void testParseGeoPointLonLatGeoHash() throws IOException { try (XContentParser parser = createParser(json)) { parser.nextToken(); Exception e = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); - assertThat(e.getMessage(), containsString("field must be either lon/lat, type/coordinates, or geohash")); + assertThat(e.getMessage(), containsString(ERR_MSG_INVALID_FIELDS)); } } @@ -698,6 +700,63 @@ public void testParseGeoPointInvalidType() throws IOException { } } + public void testParserGeoPointGeoJson() throws IOException { + GeoPoint geoPoint = RandomGeoGenerator.randomPoint(random()); + double[] coordinates = { geoPoint.getLon(), geoPoint.getLat() }; + XContentBuilder json = jsonBuilder().startObject().field("type", "Point").array("coordinates", coordinates).endObject(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + GeoPoint paredPoint = GeoUtils.parseGeoPoint(parser); + assertEquals(geoPoint, paredPoint); + } + } + + public void testParserGeoPointGeoJsonMissingField() throws IOException { + GeoPoint geoPoint = RandomGeoGenerator.randomPoint(random()); + double[] coordinates = { geoPoint.getLon(), geoPoint.getLat() }; + XContentBuilder missingType = jsonBuilder().startObject().array("coordinates", coordinates).endObject(); + expectParseException(missingType, "field [type] missing"); + + XContentBuilder missingCoordinates = jsonBuilder().startObject().field("type", "Point").endObject(); + expectParseException(missingCoordinates, "field [coordinates] missing"); + } + + public void testParserGeoPointGeoJsonUnknownField() throws IOException { + GeoPoint geoPoint = RandomGeoGenerator.randomPoint(random()); + double[] coordinates = { geoPoint.getLon(), geoPoint.getLat() }; + XContentBuilder unknownField = jsonBuilder().startObject() + .field("type", "Point") + .array("coordinates", coordinates) + .field("unknown", "value") + .endObject(); + expectParseException(unknownField, "field must be either [lon|lat], [type|coordinates], or [geohash]"); + } + + public void testParserGeoPointGeoJsonInvalidValue() throws IOException { + GeoPoint geoPoint = RandomGeoGenerator.randomPoint(random()); + double[] coordinates = { geoPoint.getLon(), geoPoint.getLat() }; + XContentBuilder invalidGeoJsonType = jsonBuilder().startObject() + .field("type", "point") + .array("coordinates", coordinates) + .endObject(); + expectParseException(invalidGeoJsonType, "type must be Point"); + + String[] coordinatesInString = { String.valueOf(geoPoint.getLon()), String.valueOf(geoPoint.getLat()) }; + XContentBuilder invalideCoordinatesType = jsonBuilder().startObject() + .field("type", "Point") + .array("coordinates", coordinatesInString) + .endObject(); + expectParseException(invalideCoordinatesType, "numeric value expected"); + } + + private void expectParseException(XContentBuilder content, String errMsg) throws IOException { + try (XContentParser parser = createParser(content)) { + parser.nextToken(); + OpenSearchParseException ex = expectThrows(OpenSearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); + assertEquals(errMsg, ex.getMessage()); + } + } + public void testPrefixTreeCellSizes() { assertThat(GeoUtils.EARTH_SEMI_MAJOR_AXIS, equalTo(DistanceUtils.EARTH_EQUATORIAL_RADIUS_KM * 1000)); assertThat(GeoUtils.quadTreeCellWidth(0), lessThanOrEqualTo(GeoUtils.EARTH_EQUATOR)); From ee99065e8ece0323df425aa2d087af5f93fb06eb Mon Sep 17 00:00:00 2001 From: Heemin Kim Date: Mon, 3 Oct 2022 14:23:38 -0700 Subject: [PATCH 3/5] Add unit tests for GeoJson support Signed-off-by: Heemin Kim --- .../mapper/GeoPointFieldMapperTests.java | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/server/src/test/java/org/opensearch/index/mapper/GeoPointFieldMapperTests.java b/server/src/test/java/org/opensearch/index/mapper/GeoPointFieldMapperTests.java index 6a6e6b190bff3..1b4c95d9ceb8f 100644 --- a/server/src/test/java/org/opensearch/index/mapper/GeoPointFieldMapperTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/GeoPointFieldMapperTests.java @@ -352,6 +352,59 @@ public void testInvalidGeopointValuesIgnored() throws Exception { ); } + public void testGeoJsonSingleValue() throws Exception { + DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); + ParsedDocument doc = mapper.parse( + source(b -> b.startObject("field").field("type", "Point").array("coordinates", new double[] { 1.1, 1.2 }).endObject()) + ); + assertThat(doc.rootDoc().getField("field"), notNullValue()); + } + + public void testGeoJsonArray() throws Exception { + DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping)); + ParsedDocument doc = mapper.parse( + source( + b -> b.startArray("field") + .startObject() + .field("type", "Point") + .array("coordinates", new double[] { 1.1, 1.2 }) + .endObject() + .startObject() + .field("type", "Point") + .array("coordinates", new double[] { 1.3, 1.4 }) + .endObject() + .endArray() + ) + ); + assertThat(doc.rootDoc().getField("field"), notNullValue()); + assertThat(doc.rootDoc().getFields("field"), arrayWithSize(4)); + } + + public void testGeoJsonIgnoreZValue() throws Exception { + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "geo_point").field("ignore_z_value", true))); + ParsedDocument doc = mapper.parse( + source(b -> b.startObject("field").field("type", "Point").array("coordinates", new double[] { 1.1, 1.2, 1.3 }).endObject()) + ); + assertThat(doc.rootDoc().getField("field"), notNullValue()); + } + + public void testGeoJsonZValueException() throws Exception { + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "geo_point").field("ignore_z_value", false))); + Exception e = expectThrows( + MapperParsingException.class, + () -> mapper.parse( + source(b -> b.startObject("field").field("type", "Point").array("coordinates", new double[] { 1.1, 1.2, 1.3 }).endObject()) + ) + ); + assertThat(e.getCause().getMessage(), containsString("but [ignore_z_value] parameter is [false]")); + } + + public void testGeoJsonIgnoreInvalidForm() throws Exception { + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> b.field("type", "geo_point").field("ignore_malformed", "true"))); + ParsedDocument doc = mapper.parse(source(b -> b.startObject("field").array("coordinates", new double[] { 1.1, 1.2 }).endObject())); + assertThat(doc.rootDoc().getField("field"), nullValue()); + } + @Override protected GeoPointFieldMapper.Builder newBuilder() { return new GeoPointFieldMapper.Builder("geo"); From 7a00141fd798794be4a08087d694cd751cd788e4 Mon Sep 17 00:00:00 2001 From: Heemin Kim Date: Mon, 10 Oct 2022 10:02:35 -0700 Subject: [PATCH 4/5] Resolve comments * Remove negation expression * Ignore case for GeoJson Point type to be consistent with geo_shape parsing Signed-off-by: Heemin Kim --- .../java/org/opensearch/common/geo/GeoUtils.java | 15 ++++++++------- .../index/search/geo/GeoUtilsTests.java | 13 ++++++++++--- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/geo/GeoUtils.java b/server/src/main/java/org/opensearch/common/geo/GeoUtils.java index 8e1f6c7094c8d..96be418c85179 100644 --- a/server/src/main/java/org/opensearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/opensearch/common/geo/GeoUtils.java @@ -43,6 +43,7 @@ import org.opensearch.common.xcontent.XContentSubParser; import org.opensearch.common.xcontent.support.MapXContentParser; import org.opensearch.common.xcontent.support.XContentMapValues; +import org.opensearch.geometry.ShapeType; import org.opensearch.index.fielddata.FieldData; import org.opensearch.index.fielddata.GeoPointValues; import org.opensearch.index.fielddata.MultiGeoPointValues; @@ -76,7 +77,6 @@ public class GeoUtils { public static final String GEOHASH = "geohash"; public static final String GEOJSON_TYPE = "type"; - public static final String GEOJSON_TYPE_POINT = "Point"; public static final String GEOJSON_COORDS = "coordinates"; /** Earth ellipsoid major axis defined by WGS 84 in meters */ public static final double EARTH_SEMI_MAJOR_AXIS = 6378137.0; // meters (WGS 84) @@ -528,7 +528,7 @@ private static GeoPoint parseGeoPointObjectBasicFields(final XContentParser pars } String field = parser.currentName(); - if (!LONGITUDE.equals(field) && !LATITUDE.equals(field)) { + if (LONGITUDE.equals(field) == false && LATITUDE.equals(field) == false) { throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); } switch (parser.nextToken()) { @@ -564,7 +564,7 @@ private static GeoPoint parseGeoHashFields( throw new OpenSearchParseException(ERR_MSG_INVALID_TOKEN, parser.currentToken()); } - if (!GEOHASH.equals(parser.currentName())) { + if (GEOHASH.equals(parser.currentName()) == false) { throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); } @@ -585,10 +585,10 @@ private static GeoPoint parseGeoJsonFields(final XContentParser parser, final Ge } if (parser.currentToken() != XContentParser.Token.FIELD_NAME) { - if (!hasTypePoint) { + if (hasTypePoint == false) { throw new OpenSearchParseException("field [{}] missing", GEOJSON_TYPE); } - if (!hasCoordinates) { + if (hasCoordinates == false) { throw new OpenSearchParseException("field [{}] missing", GEOJSON_COORDS); } } @@ -598,8 +598,9 @@ private static GeoPoint parseGeoJsonFields(final XContentParser parser, final Ge throw new OpenSearchParseException("{} must be a string", GEOJSON_TYPE); } - if (!GEOJSON_TYPE_POINT.equals(parser.text())) { - throw new OpenSearchParseException("{} must be {}", GEOJSON_TYPE, GEOJSON_TYPE_POINT); + // To be consistent with geo_shape parsing, ignore case here as well. + if (ShapeType.POINT.name().equalsIgnoreCase(parser.text()) == false) { + throw new OpenSearchParseException("{} must be Point", GEOJSON_TYPE); } hasTypePoint = true; } else if (GEOJSON_COORDS.equals(parser.currentName())) { diff --git a/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java b/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java index f9f6bea077dd6..f5e15b0342028 100644 --- a/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java +++ b/server/src/test/java/org/opensearch/index/search/geo/GeoUtilsTests.java @@ -703,8 +703,15 @@ public void testParseGeoPointInvalidType() throws IOException { public void testParserGeoPointGeoJson() throws IOException { GeoPoint geoPoint = RandomGeoGenerator.randomPoint(random()); double[] coordinates = { geoPoint.getLon(), geoPoint.getLat() }; - XContentBuilder json = jsonBuilder().startObject().field("type", "Point").array("coordinates", coordinates).endObject(); - try (XContentParser parser = createParser(json)) { + XContentBuilder json1 = jsonBuilder().startObject().field("type", "Point").array("coordinates", coordinates).endObject(); + try (XContentParser parser = createParser(json1)) { + parser.nextToken(); + GeoPoint paredPoint = GeoUtils.parseGeoPoint(parser); + assertEquals(geoPoint, paredPoint); + } + + XContentBuilder json2 = jsonBuilder().startObject().field("type", "PoInT").array("coordinates", coordinates).endObject(); + try (XContentParser parser = createParser(json2)) { parser.nextToken(); GeoPoint paredPoint = GeoUtils.parseGeoPoint(parser); assertEquals(geoPoint, paredPoint); @@ -736,7 +743,7 @@ public void testParserGeoPointGeoJsonInvalidValue() throws IOException { GeoPoint geoPoint = RandomGeoGenerator.randomPoint(random()); double[] coordinates = { geoPoint.getLon(), geoPoint.getLat() }; XContentBuilder invalidGeoJsonType = jsonBuilder().startObject() - .field("type", "point") + .field("type", "invalid") .array("coordinates", coordinates) .endObject(); expectParseException(invalidGeoJsonType, "type must be Point"); From c530d8e6a7fa768d36970c2b3a6193b9ca6e408c Mon Sep 17 00:00:00 2001 From: Heemin Kim Date: Tue, 11 Oct 2022 10:25:55 -0700 Subject: [PATCH 5/5] Added yaml test for geopoint Signed-off-by: Heemin Kim --- .../rest-api-spec/test/index/80_geo_point.yml | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/index/80_geo_point.yml diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/index/80_geo_point.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/index/80_geo_point.yml new file mode 100644 index 0000000000000..d15529af0041a --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/index/80_geo_point.yml @@ -0,0 +1,145 @@ +setup: + - do: + indices.create: + index: test_1 + body: + settings: + number_of_replicas: 0 + mappings: + properties: + location: + type: geo_point + +--- +"Single point test": + - do: + bulk: + refresh: true + body: + - index: + _index: test_1 + _id: 1 + - location: + lon: 52.374081 + lat: 4.912350 + - index: + _index: test_1 + _id: 2 + - location: "4.901618,52.369219" + - index: + _index: test_1 + _id: 3 + - location: [ 52.371667, 4.914722 ] + - index: + _index: test_1 + _id: 4 + - location: "POINT (52.371667 4.914722)" + - index: + _index: test_1 + _id: 5 + - location: "t0v5zsq1gpzf" + - index: + _index: test_1 + _id: 6 + - location: + type: Point + coordinates: [ 52.371667, 4.914722 ] + + - do: + search: + index: test_1 + rest_total_hits_as_int: true + body: + query: + geo_shape: + location: + shape: + type: "envelope" + coordinates: [ [ 51, 5 ], [ 53, 3 ] ] + + - match: { hits.total: 6 } + + - do: + search: + index: test_1 + rest_total_hits_as_int: true + body: + query: + geo_shape: + location: + shape: + type: "envelope" + coordinates: [ [ 151, 15 ], [ 153, 13 ] ] + + - match: { hits.total: 0 } + +--- +"Multi points test": + - do: + bulk: + refresh: true + body: + - index: + _index: test_1 + _id: 1 + - location: + - {lon: 52.374081, lat: 4.912350} + - {lon: 152.374081, lat: 14.912350} + - index: + _index: test_1 + _id: 2 + - location: + - "4.901618,52.369219" + - "14.901618,152.369219" + - index: + _index: test_1 + _id: 3 + - location: + - [ 52.371667, 4.914722 ] + - [ 152.371667, 14.914722 ] + - index: + _index: test_1 + _id: 4 + - location: + - "POINT (52.371667 4.914722)" + - "POINT (152.371667 14.914722)" + - index: + _index: test_1 + _id: 5 + - location: + - "t0v5zsq1gpzf" + - "x6skg0zbhnum" + - index: + _index: test_1 + _id: 6 + - location: + - {type: Point, coordinates: [ 52.371667, 4.914722 ]} + - {type: Point, coordinates: [ 152.371667, 14.914722 ]} + + - do: + search: + index: test_1 + rest_total_hits_as_int: true + body: + query: + geo_shape: + location: + shape: + type: "envelope" + coordinates: [ [ 51, 5 ], [ 53, 3 ] ] + + - match: { hits.total: 6 } + + - do: + search: + index: test_1 + rest_total_hits_as_int: true + body: + query: + geo_shape: + location: + shape: + type: "envelope" + coordinates: [ [ 151, 15 ], [ 153, 13 ] ] + + - match: { hits.total: 6 }