From 8ae7cd748dc974641258a0e264a4170e65abaf74 Mon Sep 17 00:00:00 2001 From: Nikita_Glashchenko Date: Tue, 9 Jul 2019 12:51:57 +0400 Subject: [PATCH 1/9] Remove some redundant code in GeoPointFieldMapper --- .../index/mapper/GeoPointFieldMapper.java | 41 ++++++++----------- 1 file changed, 16 insertions(+), 25 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java index 1621f60b9b784..e55b0dd7d51db 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java @@ -301,34 +301,25 @@ public void parse(ParseContext context) throws IOException { XContentParser.Token token = context.parser().currentToken(); if (token == XContentParser.Token.START_ARRAY) { token = context.parser().nextToken(); - if (token == XContentParser.Token.START_ARRAY) { - // its an array of array of lon/lat [ [1.2, 1.3], [1.4, 1.5] ] - while (token != XContentParser.Token.END_ARRAY) { - parseGeoPointIgnoringMalformed(context, sparse); - token = context.parser().nextToken(); + if (token == XContentParser.Token.VALUE_NUMBER) { + double lon = context.parser().doubleValue(); + context.parser().nextToken(); + double lat = context.parser().doubleValue(); + token = context.parser().nextToken(); + if (token == XContentParser.Token.VALUE_NUMBER) { + GeoPoint.assertZValue(ignoreZValue.value(), context.parser().doubleValue()); + } else if (token != XContentParser.Token.END_ARRAY) { + throw new ElasticsearchParseException("[{}] field type does not accept > 3 dimensions", CONTENT_TYPE); } + parse(context, sparse.reset(lat, lon)); } else { - // its an array of other possible values - if (token == XContentParser.Token.VALUE_NUMBER) { - double lon = context.parser().doubleValue(); - context.parser().nextToken(); - double lat = context.parser().doubleValue(); - token = context.parser().nextToken(); - if (token == XContentParser.Token.VALUE_NUMBER) { - GeoPoint.assertZValue(ignoreZValue.value(), context.parser().doubleValue()); - } else if (token != XContentParser.Token.END_ARRAY) { - throw new ElasticsearchParseException("[{}] field type does not accept > 3 dimensions", CONTENT_TYPE); - } - parse(context, sparse.reset(lat, lon)); - } else { - while (token != XContentParser.Token.END_ARRAY) { - if (token == XContentParser.Token.VALUE_STRING) { - parseGeoPointStringIgnoringMalformed(context, sparse); - } else { - parseGeoPointIgnoringMalformed(context, sparse); - } - token = context.parser().nextToken(); + while (token != XContentParser.Token.END_ARRAY) { + if (token == XContentParser.Token.VALUE_STRING) { + parseGeoPointStringIgnoringMalformed(context, sparse); + } else { + parseGeoPointIgnoringMalformed(context, sparse); } + token = context.parser().nextToken(); } } } else if (token == XContentParser.Token.VALUE_STRING) { From 712b1e19514389085b3a74b80f8ea0e08bf25ed4 Mon Sep 17 00:00:00 2001 From: Nikita_Glashchenko Date: Tue, 9 Jul 2019 13:02:37 +0400 Subject: [PATCH 2/9] Fix not using `ignore_z_value` in some cases --- .../org/elasticsearch/index/mapper/GeoPointFieldMapper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java index e55b0dd7d51db..02b4062cb0e8e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java @@ -344,7 +344,7 @@ public void parse(ParseContext context) throws IOException { */ private void parseGeoPointIgnoringMalformed(ParseContext context, GeoPoint sparse) throws IOException { try { - parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse)); + parse(context, GeoUtils.parseGeoPoint(context.parser(), sparse, ignoreZValue.value())); } catch (ElasticsearchParseException e) { if (ignoreMalformed.value() == false) { throw e; From 42693d19d92413ab5014a09aca21ce7172b74b52 Mon Sep 17 00:00:00 2001 From: Nikita_Glashchenko Date: Tue, 9 Jul 2019 13:08:59 +0400 Subject: [PATCH 3/9] Fix GeoUtils.parseGeoPoint sometimes allowing more than 3 dimensions in point --- .../src/main/java/org/elasticsearch/common/geo/GeoUtils.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index f990a9750e0e1..01c8065ec911d 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -510,8 +510,10 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina lon = subParser.doubleValue(); } else if (element == 2) { lat = subParser.doubleValue(); - } else { + } else if (element == 3) { GeoPoint.assertZValue(ignoreZValue, subParser.doubleValue()); + } else { + throw new ElasticsearchParseException("[geo_point] field type does not accept > 3 dimensions"); } } else { throw new ElasticsearchParseException("numeric value expected"); From 3cadec56dfbd1132c1ba6ee2ed9a7aec234dba19 Mon Sep 17 00:00:00 2001 From: Nikita_Glashchenko Date: Tue, 9 Jul 2019 14:26:28 +0400 Subject: [PATCH 4/9] Support parsing of WKT POINT --- .../elasticsearch/common/geo/GeoPoint.java | 12 +++++++++++ .../common/geo/parsers/GeoWKTParser.java | 20 +++++++++++-------- .../search/geo/GeoPointParsingTests.java | 1 + 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java index b4039fdbd2825..0737247a6dc44 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java @@ -25,6 +25,8 @@ import org.apache.lucene.index.IndexableField; import org.apache.lucene.util.BitUtil; import org.apache.lucene.util.BytesRef; +import org.elasticsearch.common.geo.builders.PointBuilder; +import org.elasticsearch.common.geo.parsers.GeoWKTParser; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.ElasticsearchParseException; @@ -85,6 +87,8 @@ public GeoPoint resetFromString(String value) { public GeoPoint resetFromString(String value, final boolean ignoreZValue) { if (value.contains(",")) { return resetFromCoordinates(value, ignoreZValue); + } else if (value.contains("POINT")) { + return resetFromWKT(value, ignoreZValue); } return resetFromGeoHash(value); } @@ -114,6 +118,14 @@ public GeoPoint resetFromCoordinates(String value, final boolean ignoreZValue) { return reset(lat, lon); } + private GeoPoint resetFromWKT(String value, boolean ignoreZValue) { + try { + PointBuilder point = (PointBuilder)GeoWKTParser.parseExpectedType(value, GeoShapeType.POINT, ignoreZValue, false); + return reset(point.latitude(), point.longitude()); + } catch (IOException e) { + throw new ElasticsearchParseException("Invalid WKT format", e); + } + } public GeoPoint resetFromIndexHash(long hash) { lon = Geohash.decodeLongitude(hash); diff --git a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java index 2cffa417246fd..51260b940c89e 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java +++ b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java @@ -19,7 +19,6 @@ package org.elasticsearch.common.geo.parsers; import org.elasticsearch.ElasticsearchParseException; -import org.elasticsearch.common.Explicit; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoShapeType; import org.elasticsearch.common.geo.builders.CoordinatesBuilder; @@ -77,10 +76,15 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoShapeType shapeType, final BaseGeoShapeFieldMapper shapeMapper) throws IOException, ElasticsearchParseException { - try (StringReader reader = new StringReader(parser.text())) { - Explicit ignoreZValue = (shapeMapper == null) ? BaseGeoShapeFieldMapper.Defaults.IGNORE_Z_VALUE : - shapeMapper.ignoreZValue(); - Explicit coerce = (shapeMapper == null) ? BaseGeoShapeFieldMapper.Defaults.COERCE : shapeMapper.coerce(); + String text = parser.text(); + boolean ignoreZValue = ((shapeMapper == null) ? BaseGeoShapeFieldMapper.Defaults.IGNORE_Z_VALUE : + shapeMapper.ignoreZValue()).value(); + boolean coerce = ((shapeMapper == null) ? BaseGeoShapeFieldMapper.Defaults.COERCE : shapeMapper.coerce()).value(); + return parseExpectedType(text, shapeType, ignoreZValue, coerce); + } + + public static ShapeBuilder parseExpectedType(String text, GeoShapeType shapeType, boolean ignoreZValue, boolean coerce) throws IOException { + try (StringReader reader = new StringReader(text)) { // setup the tokenizer; configured to read words w/o numbers StreamTokenizer tokenizer = new StreamTokenizer(reader); tokenizer.resetSyntax(); @@ -93,7 +97,7 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha tokenizer.wordChars('.', '.'); tokenizer.whitespaceChars(0, ' '); tokenizer.commentChar('#'); - ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue.value(), coerce.value()); + ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue, coerce); checkEOF(tokenizer); return builder; } @@ -111,7 +115,7 @@ private static ShapeBuilder parseGeometry(StreamTokenizer stream, GeoShapeType s } switch (type) { case POINT: - return parsePoint(stream, ignoreZValue, coerce); + return parsePoint(stream, ignoreZValue); case MULTIPOINT: return parseMultiPoint(stream, ignoreZValue, coerce); case LINESTRING: @@ -146,7 +150,7 @@ private static EnvelopeBuilder parseBBox(StreamTokenizer stream) throws IOExcept return new EnvelopeBuilder(new Coordinate(minLon, maxLat), new Coordinate(maxLon, minLat)); } - private static PointBuilder parsePoint(StreamTokenizer stream, final boolean ignoreZValue, final boolean coerce) + public static PointBuilder parsePoint(StreamTokenizer stream, final boolean ignoreZValue) throws IOException, ElasticsearchParseException { if (nextEmptyOrOpen(stream).equals(EMPTY)) { return null; diff --git a/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java b/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java index 9af651119e642..1fd82f4dcce4a 100644 --- a/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java +++ b/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java @@ -57,6 +57,7 @@ public void testGeoPointReset() throws IOException { assertPointsEqual(point.reset(0, 0), point2.reset(0, 0)); assertPointsEqual(point.resetFromString(Double.toString(lat) + ", " + Double.toHexString(lon)), point2.reset(lat, lon)); assertPointsEqual(point.reset(0, 0), point2.reset(0, 0)); + assertPointsEqual(point.resetFromString("POINT(" + lon + " " + lat + ")"), point2.reset(lat, lon)); } public void testEqualsHashCodeContract() { From f909948a19ac4d00a69a60cb57986e91df9f6f2a Mon Sep 17 00:00:00 2001 From: Nikita_Glashchenko Date: Tue, 9 Jul 2019 14:35:51 +0400 Subject: [PATCH 5/9] Update docs --- docs/reference/mapping/types/geo-point.asciidoc | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/docs/reference/mapping/types/geo-point.asciidoc b/docs/reference/mapping/types/geo-point.asciidoc index 51e137fbc33b6..020a6e9917d5d 100644 --- a/docs/reference/mapping/types/geo-point.asciidoc +++ b/docs/reference/mapping/types/geo-point.asciidoc @@ -11,7 +11,7 @@ Fields of type `geo_point` accept latitude-longitude pairs, which can be used: * to integrate distance into a document's <>. * to <> documents by distance. -There are four ways that a geo-point may be specified, as demonstrated below: +There are five ways that a geo-point may be specified, as demonstrated below: [source,js] -------------------------------------------------- @@ -53,10 +53,16 @@ PUT my_index/_doc/4 "location": [ -71.34, 41.12 ] <4> } +PUT my_index/_doc/5 +{ + "text": "Geo-point as a WKT POINT primitive", + "location" : "POINT (-77.03653 38.897676)" <5> +} + GET my_index/_search { "query": { - "geo_bounding_box": { <5> + "geo_bounding_box": { <6> "location": { "top_left": { "lat": 42, @@ -76,7 +82,8 @@ GET my_index/_search <2> Geo-point expressed as a string with the format: `"lat,lon"`. <3> Geo-point expressed as a geohash. <4> Geo-point expressed as an array with the format: [ `lon`, `lat`] -<5> A geo-bounding box query which finds all geo-points that fall inside the box. +<5> Geo-point expressed as a Well-known text POINT with the format: `"POINT(lon lat)"` +<6> A geo-bounding box query which finds all geo-points that fall inside the box. [IMPORTANT] .Geo-points expressed as an array or string From f93ebe97e7445c712aa02c58f9928fe53f2fa9f0 Mon Sep 17 00:00:00 2001 From: Hohol Date: Tue, 9 Jul 2019 23:55:29 +0500 Subject: [PATCH 6/9] Fix style --- .../org/elasticsearch/common/geo/parsers/GeoWKTParser.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java index 51260b940c89e..b6ba3677144c7 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java +++ b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java @@ -83,7 +83,8 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha return parseExpectedType(text, shapeType, ignoreZValue, coerce); } - public static ShapeBuilder parseExpectedType(String text, GeoShapeType shapeType, boolean ignoreZValue, boolean coerce) throws IOException { + public static ShapeBuilder parseExpectedType(String text, GeoShapeType shapeType, + boolean ignoreZValue, boolean coerce) throws IOException { try (StringReader reader = new StringReader(text)) { // setup the tokenizer; configured to read words w/o numbers StreamTokenizer tokenizer = new StreamTokenizer(reader); @@ -150,7 +151,7 @@ private static EnvelopeBuilder parseBBox(StreamTokenizer stream) throws IOExcept return new EnvelopeBuilder(new Coordinate(minLon, maxLat), new Coordinate(maxLon, minLat)); } - public static PointBuilder parsePoint(StreamTokenizer stream, final boolean ignoreZValue) + private static PointBuilder parsePoint(StreamTokenizer stream, final boolean ignoreZValue) throws IOException, ElasticsearchParseException { if (nextEmptyOrOpen(stream).equals(EMPTY)) { return null; From af6d3a4808cf65581bd765484ffded6a1a5409f5 Mon Sep 17 00:00:00 2001 From: Nikita_Glashchenko Date: Wed, 10 Jul 2019 14:36:47 +0400 Subject: [PATCH 7/9] Fix review comments -fix parameters in docs -use WellKnownText parser -add negative tests --- .../mapping/types/geo-point.asciidoc | 2 +- .../elasticsearch/common/geo/GeoPoint.java | 29 +++++++++++++------ .../common/geo/parsers/GeoWKTParser.java | 21 +++++--------- .../search/geo/GeoPointParsingTests.java | 15 ++++++++++ 4 files changed, 44 insertions(+), 23 deletions(-) diff --git a/docs/reference/mapping/types/geo-point.asciidoc b/docs/reference/mapping/types/geo-point.asciidoc index 020a6e9917d5d..564b4c9010625 100644 --- a/docs/reference/mapping/types/geo-point.asciidoc +++ b/docs/reference/mapping/types/geo-point.asciidoc @@ -56,7 +56,7 @@ PUT my_index/_doc/4 PUT my_index/_doc/5 { "text": "Geo-point as a WKT POINT primitive", - "location" : "POINT (-77.03653 38.897676)" <5> + "location" : "POINT (-71.34 41.12)" <5> } GET my_index/_search diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java index 0737247a6dc44..0115fcf2c6e71 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java @@ -25,15 +25,19 @@ import org.apache.lucene.index.IndexableField; import org.apache.lucene.util.BitUtil; import org.apache.lucene.util.BytesRef; -import org.elasticsearch.common.geo.builders.PointBuilder; -import org.elasticsearch.common.geo.parsers.GeoWKTParser; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.geo.geometry.Geometry; +import org.elasticsearch.geo.geometry.Point; +import org.elasticsearch.geo.geometry.ShapeType; +import org.elasticsearch.geo.utils.GeographyValidator; import org.elasticsearch.geo.utils.Geohash; +import org.elasticsearch.geo.utils.WellKnownText; import java.io.IOException; import java.util.Arrays; +import java.util.Locale; import static org.elasticsearch.index.mapper.GeoPointFieldMapper.Names.IGNORE_Z_VALUE; @@ -85,10 +89,10 @@ public GeoPoint resetFromString(String value) { } public GeoPoint resetFromString(String value, final boolean ignoreZValue) { - if (value.contains(",")) { - return resetFromCoordinates(value, ignoreZValue); - } else if (value.contains("POINT")) { + if (value.toLowerCase(Locale.ROOT).contains("point")) { return resetFromWKT(value, ignoreZValue); + } else if (value.contains(",")) { + return resetFromCoordinates(value, ignoreZValue); } return resetFromGeoHash(value); } @@ -119,12 +123,19 @@ public GeoPoint resetFromCoordinates(String value, final boolean ignoreZValue) { } private GeoPoint resetFromWKT(String value, boolean ignoreZValue) { + Geometry geometry; try { - PointBuilder point = (PointBuilder)GeoWKTParser.parseExpectedType(value, GeoShapeType.POINT, ignoreZValue, false); - return reset(point.latitude(), point.longitude()); - } catch (IOException e) { - throw new ElasticsearchParseException("Invalid WKT format", e); + geometry = new WellKnownText(false, new GeographyValidator(ignoreZValue)) + .fromWKT(value); + } catch (Exception e) { + throw new ElasticsearchParseException("Invalid WKT format", e); + } + if (geometry.type() != ShapeType.POINT) { + throw new ElasticsearchParseException("[geo_point] supports only POINT among WKT primitives, " + + "but found " + geometry.type()); } + Point point = (Point) geometry; + return reset(point.getLat(), point.getLon()); } public GeoPoint resetFromIndexHash(long hash) { diff --git a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java index b6ba3677144c7..2cffa417246fd 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java +++ b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.geo.parsers; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.common.Explicit; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoShapeType; import org.elasticsearch.common.geo.builders.CoordinatesBuilder; @@ -76,16 +77,10 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoShapeType shapeType, final BaseGeoShapeFieldMapper shapeMapper) throws IOException, ElasticsearchParseException { - String text = parser.text(); - boolean ignoreZValue = ((shapeMapper == null) ? BaseGeoShapeFieldMapper.Defaults.IGNORE_Z_VALUE : - shapeMapper.ignoreZValue()).value(); - boolean coerce = ((shapeMapper == null) ? BaseGeoShapeFieldMapper.Defaults.COERCE : shapeMapper.coerce()).value(); - return parseExpectedType(text, shapeType, ignoreZValue, coerce); - } - - public static ShapeBuilder parseExpectedType(String text, GeoShapeType shapeType, - boolean ignoreZValue, boolean coerce) throws IOException { - try (StringReader reader = new StringReader(text)) { + try (StringReader reader = new StringReader(parser.text())) { + Explicit ignoreZValue = (shapeMapper == null) ? BaseGeoShapeFieldMapper.Defaults.IGNORE_Z_VALUE : + shapeMapper.ignoreZValue(); + Explicit coerce = (shapeMapper == null) ? BaseGeoShapeFieldMapper.Defaults.COERCE : shapeMapper.coerce(); // setup the tokenizer; configured to read words w/o numbers StreamTokenizer tokenizer = new StreamTokenizer(reader); tokenizer.resetSyntax(); @@ -98,7 +93,7 @@ public static ShapeBuilder parseExpectedType(String text, GeoShapeType shapeType tokenizer.wordChars('.', '.'); tokenizer.whitespaceChars(0, ' '); tokenizer.commentChar('#'); - ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue, coerce); + ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue.value(), coerce.value()); checkEOF(tokenizer); return builder; } @@ -116,7 +111,7 @@ private static ShapeBuilder parseGeometry(StreamTokenizer stream, GeoShapeType s } switch (type) { case POINT: - return parsePoint(stream, ignoreZValue); + return parsePoint(stream, ignoreZValue, coerce); case MULTIPOINT: return parseMultiPoint(stream, ignoreZValue, coerce); case LINESTRING: @@ -151,7 +146,7 @@ private static EnvelopeBuilder parseBBox(StreamTokenizer stream) throws IOExcept return new EnvelopeBuilder(new Coordinate(minLon, maxLat), new Coordinate(maxLon, minLat)); } - private static PointBuilder parsePoint(StreamTokenizer stream, final boolean ignoreZValue) + private static PointBuilder parsePoint(StreamTokenizer stream, final boolean ignoreZValue, final boolean coerce) throws IOException, ElasticsearchParseException { if (nextEmptyOrOpen(stream).equals(EMPTY)) { return null; diff --git a/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java b/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java index 1fd82f4dcce4a..0d7f5f5fef68e 100644 --- a/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java +++ b/server/src/test/java/org/elasticsearch/index/search/geo/GeoPointParsingTests.java @@ -60,6 +60,21 @@ public void testGeoPointReset() throws IOException { assertPointsEqual(point.resetFromString("POINT(" + lon + " " + lat + ")"), point2.reset(lat, lon)); } + public void testParseWktInvalid() { + GeoPoint point = new GeoPoint(0, 0); + Exception e = expectThrows( + ElasticsearchParseException.class, + () -> point.resetFromString("NOT A POINT(1 2)") + ); + assertEquals("Invalid WKT format", e.getMessage()); + + Exception e2 = expectThrows( + ElasticsearchParseException.class, + () -> point.resetFromString("MULTIPOINT(1 2, 3 4)") + ); + assertEquals("[geo_point] supports only POINT among WKT primitives, but found MULTIPOINT", e2.getMessage()); + } + public void testEqualsHashCodeContract() { // GeoPoint doesn't care about coordinate system bounds, this simply validates equality and hashCode. final DoubleSupplier randomDelta = () -> randomValueOtherThan(0.0, () -> randomDoubleBetween(-1000000, 1000000, true)); From 97ba38ac78bd049756e4508a3a7f4b79049a587b Mon Sep 17 00:00:00 2001 From: Nikita_Glashchenko Date: Wed, 10 Jul 2019 20:32:43 +0400 Subject: [PATCH 8/9] Fix code duplication in GeoUtils and GeoPointFieldMapper --- .../elasticsearch/common/geo/GeoPoint.java | 26 ++++++++++++-- .../elasticsearch/common/geo/GeoUtils.java | 36 ++----------------- .../index/mapper/GeoPointFieldMapper.java | 22 +----------- .../mapper/GeoPointFieldMapperTests.java | 17 +++++++++ 4 files changed, 44 insertions(+), 57 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java index 0115fcf2c6e71..1043c9115e8ef 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java @@ -25,11 +25,13 @@ import org.apache.lucene.index.IndexableField; import org.apache.lucene.util.BitUtil; import org.apache.lucene.util.BytesRef; +import org.elasticsearch.common.geo.GeoUtils.EffectivePoint; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.geo.geometry.Geometry; import org.elasticsearch.geo.geometry.Point; +import org.elasticsearch.geo.geometry.Rectangle; import org.elasticsearch.geo.geometry.ShapeType; import org.elasticsearch.geo.utils.GeographyValidator; import org.elasticsearch.geo.utils.Geohash; @@ -85,16 +87,16 @@ public GeoPoint resetLon(double lon) { } public GeoPoint resetFromString(String value) { - return resetFromString(value, false); + return resetFromString(value, false, EffectivePoint.BOTTOM_LEFT); } - public GeoPoint resetFromString(String value, final boolean ignoreZValue) { + public GeoPoint resetFromString(String value, final boolean ignoreZValue, EffectivePoint effectivePoint) { if (value.toLowerCase(Locale.ROOT).contains("point")) { return resetFromWKT(value, ignoreZValue); } else if (value.contains(",")) { return resetFromCoordinates(value, ignoreZValue); } - return resetFromGeoHash(value); + return parseGeoHash(value, effectivePoint); } @@ -138,6 +140,24 @@ private GeoPoint resetFromWKT(String value, boolean ignoreZValue) { return reset(point.getLat(), point.getLon()); } + GeoPoint parseGeoHash(String geohash, EffectivePoint effectivePoint) { + if (effectivePoint == EffectivePoint.BOTTOM_LEFT) { + return resetFromGeoHash(geohash); + } else { + Rectangle rectangle = Geohash.toBoundingBox(geohash); + switch (effectivePoint) { + case TOP_LEFT: + return reset(rectangle.getMaxLat(), rectangle.getMinLon()); + case TOP_RIGHT: + return reset(rectangle.getMaxLat(), rectangle.getMaxLon()); + case BOTTOM_RIGHT: + return reset(rectangle.getMinLat(), rectangle.getMaxLon()); + default: + throw new IllegalArgumentException("Unsupported effective point " + effectivePoint); + } + } + } + public GeoPoint resetFromIndexHash(long hash) { lon = Geohash.decodeLongitude(hash); lat = Geohash.decodeLatitude(hash); diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index 01c8065ec911d..e18f0673d87eb 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -33,8 +33,6 @@ import org.elasticsearch.common.xcontent.XContentSubParser; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.common.xcontent.support.XContentMapValues; -import org.elasticsearch.geo.geometry.Rectangle; -import org.elasticsearch.geo.utils.Geohash; import org.elasticsearch.index.fielddata.FieldData; import org.elasticsearch.index.fielddata.GeoPointValues; import org.elasticsearch.index.fielddata.MultiGeoPointValues; @@ -487,7 +485,7 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina if(!Double.isNaN(lat) || !Double.isNaN(lon)) { throw new ElasticsearchParseException("field must be either lat/lon or geohash"); } else { - return parseGeoHash(point, geohash, effectivePoint); + return point.parseGeoHash(geohash, effectivePoint); } } else if (numberFormatException != null) { throw new ElasticsearchParseException("[{}] and [{}] must be valid double values", numberFormatException, LATITUDE, @@ -523,35 +521,12 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina return point.reset(lat, lon); } else if(parser.currentToken() == Token.VALUE_STRING) { String val = parser.text(); - if (val.contains(",")) { - return point.resetFromString(val, ignoreZValue); - } else { - return parseGeoHash(point, val, effectivePoint); - } - + return point.resetFromString(val, ignoreZValue, effectivePoint); } else { throw new ElasticsearchParseException("geo_point expected"); } } - private static GeoPoint parseGeoHash(GeoPoint point, String geohash, EffectivePoint effectivePoint) { - if (effectivePoint == EffectivePoint.BOTTOM_LEFT) { - return point.resetFromGeoHash(geohash); - } else { - Rectangle rectangle = Geohash.toBoundingBox(geohash); - switch (effectivePoint) { - case TOP_LEFT: - return point.reset(rectangle.getMaxLat(), rectangle.getMinLon()); - case TOP_RIGHT: - return point.reset(rectangle.getMaxLat(), rectangle.getMaxLon()); - case BOTTOM_RIGHT: - return point.reset(rectangle.getMinLat(), rectangle.getMaxLon()); - default: - throw new IllegalArgumentException("Unsupported effective point " + effectivePoint); - } - } - } - /** * Parse a {@link GeoPoint} from a string. The string must have one of the following forms: * @@ -565,12 +540,7 @@ private static GeoPoint parseGeoHash(GeoPoint point, String geohash, EffectivePo */ public static GeoPoint parseFromString(String val) { GeoPoint point = new GeoPoint(); - boolean ignoreZValue = false; - if (val.contains(",")) { - return point.resetFromString(val, ignoreZValue); - } else { - return parseGeoHash(point, val, EffectivePoint.BOTTOM_LEFT); - } + return point.resetFromString(val, false, EffectivePoint.BOTTOM_LEFT); } /** diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java index 02b4062cb0e8e..bc3a4b08e273e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java @@ -314,16 +314,10 @@ public void parse(ParseContext context) throws IOException { parse(context, sparse.reset(lat, lon)); } else { while (token != XContentParser.Token.END_ARRAY) { - if (token == XContentParser.Token.VALUE_STRING) { - parseGeoPointStringIgnoringMalformed(context, sparse); - } else { - parseGeoPointIgnoringMalformed(context, sparse); - } + parseGeoPointIgnoringMalformed(context, sparse); token = context.parser().nextToken(); } } - } else if (token == XContentParser.Token.VALUE_STRING) { - parseGeoPointStringIgnoringMalformed(context, sparse); } else if (token == XContentParser.Token.VALUE_NULL) { if (fieldType.nullValue() != null) { parse(context, (GeoPoint) fieldType.nullValue()); @@ -353,20 +347,6 @@ private void parseGeoPointIgnoringMalformed(ParseContext context, GeoPoint spars } } - /** - * Parses geopoint represented as a string and ignores malformed geopoints if needed - */ - private void parseGeoPointStringIgnoringMalformed(ParseContext context, GeoPoint sparse) throws IOException { - try { - parse(context, sparse.resetFromString(context.parser().text(), ignoreZValue.value())); - } catch (ElasticsearchParseException e) { - if (ignoreMalformed.value() == false) { - throw e; - } - context.addIgnoredField(fieldType.name()); - } - } - @Override protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException { super.doXContentBody(builder, includeDefaults, params); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java index d80d51320403e..27da7cbe0592a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java @@ -75,6 +75,23 @@ public void testGeoHashValue() throws Exception { assertThat(doc.rootDoc().getField("point"), notNullValue()); } + public void testWKT() throws Exception { + XContentBuilder xContentBuilder = XContentFactory.jsonBuilder().startObject().startObject("type") + .startObject("properties").startObject("point").field("type", "geo_point"); + String mapping = Strings.toString(xContentBuilder.endObject().endObject().endObject().endObject()); + DocumentMapper defaultMapper = createIndex("test").mapperService().documentMapperParser() + .parse("type", new CompressedXContent(mapping)); + + ParsedDocument doc = defaultMapper.parse(new SourceToParse("test", "type", "1", + BytesReference.bytes(XContentFactory.jsonBuilder() + .startObject() + .field("point", "POINT (2 3)") + .endObject()), + XContentType.JSON)); + + assertThat(doc.rootDoc().getField("point"), notNullValue()); + } + public void testLatLonValuesStored() throws Exception { XContentBuilder xContentBuilder = XContentFactory.jsonBuilder().startObject().startObject("type") .startObject("properties").startObject("point").field("type", "geo_point"); From 98c9671d8189f9c1e69499e1ab30528cc5ccd0ca Mon Sep 17 00:00:00 2001 From: Nikita_Glashchenko Date: Fri, 12 Jul 2019 02:10:04 +0500 Subject: [PATCH 9/9] Add link to WKT specification in docs --- docs/reference/mapping/types/geo-point.asciidoc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/reference/mapping/types/geo-point.asciidoc b/docs/reference/mapping/types/geo-point.asciidoc index 564b4c9010625..f890cf92f89d2 100644 --- a/docs/reference/mapping/types/geo-point.asciidoc +++ b/docs/reference/mapping/types/geo-point.asciidoc @@ -82,7 +82,8 @@ GET my_index/_search <2> Geo-point expressed as a string with the format: `"lat,lon"`. <3> Geo-point expressed as a geohash. <4> Geo-point expressed as an array with the format: [ `lon`, `lat`] -<5> Geo-point expressed as a Well-known text POINT with the format: `"POINT(lon lat)"` +<5> Geo-point expressed as a http://docs.opengeospatial.org/is/12-063r5/12-063r5.html[Well-Known Text] +POINT with the format: `"POINT(lon lat)"` <6> A geo-bounding box query which finds all geo-points that fall inside the box. [IMPORTANT]