diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e78f266c4c02..d564e115e453c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Introduce experimental searchable snapshot API ([#4680](https://github.com/opensearch-project/OpenSearch/pull/4680)) - Introduce Remote translog feature flag([#4158](https://github.com/opensearch-project/OpenSearch/pull/4158)) - Add groupId value propagation tests for ZIP publication task ([#4848](https://github.com/opensearch-project/OpenSearch/pull/4848)) +- Add support for GeoJson Point type in GeoPoint field ([#4597](https://github.com/opensearch-project/OpenSearch/pull/4597)) + ### Dependencies - Bumps `com.diffplug.spotless` from 6.9.1 to 6.10.0 - Bumps `xmlbeans` from 5.1.0 to 5.1.1 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..7d6c2b835f1f7 --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/index/80_geo_point.yml @@ -0,0 +1,151 @@ +setup: + - do: + indices.create: + index: test_1 + body: + settings: + number_of_replicas: 0 + mappings: + properties: + location: + type: geo_point + +--- +"Single point test": + - skip: + version: " - 2.3.99" + reason: "geojson format is supported in 2.4 and above" + - 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": + - skip: + version: " - 2.3.99" + reason: "geojson format is supported in 2.4 and above" + - 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 } 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..c59a9046e0318 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: [{}]", + value, + 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..96be418c85179 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.ShapeType; import org.opensearch.index.fielddata.FieldData; import org.opensearch.index.fielddata.GeoPointValues; import org.opensearch.index.fielddata.MultiGeoPointValues; @@ -53,6 +53,7 @@ import java.io.IOException; import java.util.Collections; +import java.util.HashMap; /** * Useful geo utilities @@ -60,7 +61,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 +76,8 @@ 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_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) @@ -444,112 +448,202 @@ 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()); + 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: + parseGeoPointObject(parser, point, ignoreZValue, effectivePoint); + break; + case START_ARRAY: + parseGeoPointArray(parser, point, ignoreZValue); + break; + case VALUE_STRING: + String val = parser.text(); + point.resetFromString(val, ignoreZValue, effectivePoint); + break; + default: + throw new OpenSearchParseException("geo_point expected"); + } + 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) == false && LATITUDE.equals(field) == false) { + 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()) == false) { + 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 == false) { + throw new OpenSearchParseException("field [{}] missing", GEOJSON_TYPE); + } + if (hasCoordinates == false) { + throw new OpenSearchParseException("field [{}] missing", GEOJSON_COORDS); } } - 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); + + if (GEOJSON_TYPE.equals(parser.currentName())) { + if (parser.nextToken() != XContentParser.Token.VALUE_STRING) { + throw new OpenSearchParseException("{} must be a string", GEOJSON_TYPE); } - } 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); + + // 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())) { + if (parser.nextToken() != XContentParser.Token.START_ARRAY) { + throw new OpenSearchParseException("{} must be an array", GEOJSON_COORDS); + } + parseGeoPointArray(parser, point, ignoreZValue); + hasCoordinates = true; } else { - return point.reset(lat, lon); + throw new OpenSearchParseException(ERR_MSG_INVALID_FIELDS); } + } - } 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"); - } + 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"); } } - 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"); + + if (element < 2) { + throw new OpenSearchParseException("[geo_point] field type should have at least two dimensions"); + } + return point.reset(y, x); } } 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..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 */ @@ -281,32 +284,27 @@ 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) { - double x = parser.doubleValue(); - parser.nextToken(); - double y = parser.doubleValue(); - token = parser.nextToken(); - if (token == XContentParser.Token.VALUE_NUMBER) { - GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); - } else if (token != XContentParser.Token.END_ARRAY) { - throw new OpenSearchParseException("field type does not accept > 3 dimensions"); + parser.nextToken(); + if (parser.currentToken() == XContentParser.Token.VALUE_NUMBER) { + XContentBuilder xContentBuilder = reConstructArrayXContent(parser); + try ( + XContentParser subParser = createParser( + parser.getXContentRegistry(), + parser.getDeprecationHandler(), + xContentBuilder + ); + ) { + return Collections.singletonList(process(objectParser.apply(subParser, pointSupplier.get()))); } - - point.resetCoords(x, y); - points.add(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) { if (nullValue == null) { return null; @@ -318,6 +316,35 @@ public List

parse(XContentParser parser) throws IOException, ParseException { } } + 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/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"); 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..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 [lat], [lon] 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 [lat], [lon] 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 lat/lon 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 lat/lon 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 lat/lon 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 lat/lon 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 [lat], [lon] 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 [lat], [lon] 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 bb4b057d01414..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 @@ -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', @@ -601,7 +603,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 +615,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 +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 [lat], [lon] 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 lat/lon or geohash")); + assertThat(e.getMessage(), containsString(ERR_MSG_INVALID_FIELDS)); } } @@ -698,6 +700,70 @@ public void testParseGeoPointInvalidType() throws IOException { } } + public void testParserGeoPointGeoJson() throws IOException { + GeoPoint geoPoint = RandomGeoGenerator.randomPoint(random()); + double[] coordinates = { geoPoint.getLon(), geoPoint.getLat() }; + 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); + } + } + + 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", "invalid") + .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));