From 94bde37bcfea3cdf5c69857a7a7f3ef6d4e8fa0b Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Fri, 19 Oct 2018 13:53:54 -0400 Subject: [PATCH] Geo: Don't flip longitude of envelopes crossing dateline (#34535) When a envelope that crosses the dateline is specified as a part of geo_shape query is parsed it shouldn't have its left and right points flipped. Fixes #34418 --- .../mapping/types/geo-shape.asciidoc | 2 +- .../migration/migrate_7_0/search.asciidoc | 4 ++ .../common/geo/GeoShapeType.java | 5 -- .../common/geo/GeoJsonShapeParserTests.java | 4 +- .../search/geo/GeoShapeQueryTests.java | 72 +++++++++++++++++++ 5 files changed, 79 insertions(+), 8 deletions(-) diff --git a/docs/reference/mapping/types/geo-shape.asciidoc b/docs/reference/mapping/types/geo-shape.asciidoc index 5a39c4b117e..2f51465d110 100644 --- a/docs/reference/mapping/types/geo-shape.asciidoc +++ b/docs/reference/mapping/types/geo-shape.asciidoc @@ -567,7 +567,7 @@ POST /example/doc Elasticsearch supports an `envelope` type, which consists of coordinates for upper left and lower right points of the shape to represent a -bounding rectangle: +bounding rectangle in the format [[minLon, maxLat],[maxLon, minLat]]: [source,js] -------------------------------------------------- diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index 44e22c97f2f..afb2251fb1b 100644 --- a/docs/reference/migration/migrate_7_0/search.asciidoc +++ b/docs/reference/migration/migrate_7_0/search.asciidoc @@ -20,6 +20,10 @@ * Attempts to generate multi-term phrase queries against non-text fields with a custom analyzer will now throw an exception +* An `envelope` crossing the dateline in a `geo_shape `query is now processed + correctly when specified using REST API instead of having its left and + right corners flipped. + [float] ==== Adaptive replica selection enabled by default diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoShapeType.java b/server/src/main/java/org/elasticsearch/common/geo/GeoShapeType.java index 1b918f72413..5022f66550c 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoShapeType.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoShapeType.java @@ -223,11 +223,6 @@ public enum GeoShapeType { // verify coordinate bounds, correct if necessary Coordinate uL = coordinates.children.get(0).coordinate; Coordinate lR = coordinates.children.get(1).coordinate; - if (((lR.x < uL.x) || (uL.y < lR.y))) { - Coordinate uLtmp = uL; - uL = new Coordinate(Math.min(uL.x, lR.x), Math.max(uL.y, lR.y)); - lR = new Coordinate(Math.max(uLtmp.x, lR.x), Math.min(uLtmp.y, lR.y)); - } return new EnvelopeBuilder(uL, lR); } diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java index 17f25d1556d..57cb6b62623 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java @@ -175,7 +175,7 @@ public class GeoJsonShapeParserTests extends BaseGeoParsingTestCase { Rectangle expected = SPATIAL_CONTEXT.makeRectangle(-50, 50, -30, 30); assertGeometryEquals(expected, multilinesGeoJson); - // test #2: envelope with agnostic coordinate order (TopRight, BottomLeft) + // test #2: envelope that spans dateline multilinesGeoJson = XContentFactory.jsonBuilder().startObject().field("type", "envelope") .startArray("coordinates") .startArray().value(50).value(30).endArray() @@ -183,7 +183,7 @@ public class GeoJsonShapeParserTests extends BaseGeoParsingTestCase { .endArray() .endObject(); - expected = SPATIAL_CONTEXT.makeRectangle(-50, 50, -30, 30); + expected = SPATIAL_CONTEXT.makeRectangle(50, -50, -30, 30); assertGeometryEquals(expected, multilinesGeoJson); // test #3: "envelope" (actually a triangle) with invalid number of coordinates (TopRight, BottomLeft, BottomRight) diff --git a/server/src/test/java/org/elasticsearch/search/geo/GeoShapeQueryTests.java b/server/src/test/java/org/elasticsearch/search/geo/GeoShapeQueryTests.java index 6f204796e41..d0d05f6a369 100644 --- a/server/src/test/java/org/elasticsearch/search/geo/GeoShapeQueryTests.java +++ b/server/src/test/java/org/elasticsearch/search/geo/GeoShapeQueryTests.java @@ -20,7 +20,9 @@ package org.elasticsearch.search.geo; import org.elasticsearch.action.get.GetResponse; +import org.elasticsearch.action.index.IndexRequest; import org.elasticsearch.action.search.SearchResponse; +import org.elasticsearch.common.CheckedSupplier; import org.elasticsearch.common.Strings; import org.elasticsearch.common.geo.ShapeRelation; import org.elasticsearch.common.geo.builders.CoordinatesBuilder; @@ -32,6 +34,7 @@ import org.elasticsearch.common.geo.builders.ShapeBuilder; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.mapper.MapperParsingException; import org.elasticsearch.index.query.GeoShapeQueryBuilder; @@ -531,4 +534,73 @@ public class GeoShapeQueryTests extends ESSingleNodeTestCase { .execute().actionGet(); assertEquals(1, response.getHits().getTotalHits()); } + + // Test for issue #34418 + public void testEnvelopeSpanningDateline() throws IOException { + XContentBuilder mapping = XContentFactory.jsonBuilder().startObject() + .startObject("doc") + .startObject("properties") + .startObject("geo").field("type", "geo_shape").endObject() + .endObject() + .endObject() + .endObject(); + + createIndex("test", Settings.builder().put("index.number_of_shards", 1).build(), "doc", mapping); + + String doc1 = "{\"geo\": {\r\n" + "\"coordinates\": [\r\n" + "-33.918711,\r\n" + "18.847685\r\n" + "],\r\n" + + "\"type\": \"Point\"\r\n" + "}}"; + client().index(new IndexRequest("test", "doc", "1").source(doc1, XContentType.JSON).setRefreshPolicy(IMMEDIATE)).actionGet(); + + String doc2 = "{\"geo\": {\r\n" + "\"coordinates\": [\r\n" + "-49.0,\r\n" + "18.847685\r\n" + "],\r\n" + + "\"type\": \"Point\"\r\n" + "}}"; + client().index(new IndexRequest("test", "doc", "2").source(doc2, XContentType.JSON).setRefreshPolicy(IMMEDIATE)).actionGet(); + + String doc3 = "{\"geo\": {\r\n" + "\"coordinates\": [\r\n" + "49.0,\r\n" + "18.847685\r\n" + "],\r\n" + + "\"type\": \"Point\"\r\n" + "}}"; + client().index(new IndexRequest("test", "doc", "3").source(doc3, XContentType.JSON).setRefreshPolicy(IMMEDIATE)).actionGet(); + + @SuppressWarnings("unchecked") CheckedSupplier querySupplier = randomFrom( + () -> QueryBuilders.geoShapeQuery( + "geo", + new EnvelopeBuilder(new Coordinate(-21, 44), new Coordinate(-39, 9)) + ).relation(ShapeRelation.WITHIN), + () -> { + XContentBuilder builder = XContentFactory.jsonBuilder().startObject() + .startObject("geo") + .startObject("shape") + .field("type", "envelope") + .startArray("coordinates") + .startArray().value(-21).value(44).endArray() + .startArray().value(-39).value(9).endArray() + .endArray() + .endObject() + .field("relation", "within") + .endObject() + .endObject(); + try (XContentParser parser = createParser(builder)){ + parser.nextToken(); + return GeoShapeQueryBuilder.fromXContent(parser); + } + }, + () -> { + XContentBuilder builder = XContentFactory.jsonBuilder().startObject() + .startObject("geo") + .field("shape", "BBOX (-21, -39, 44, 9)") + .field("relation", "within") + .endObject() + .endObject(); + try (XContentParser parser = createParser(builder)){ + parser.nextToken(); + return GeoShapeQueryBuilder.fromXContent(parser); + } + } + ); + + SearchResponse response = client().prepareSearch("test") + .setQuery(querySupplier.get()) + .execute().actionGet(); + assertEquals(2, response.getHits().getTotalHits()); + assertNotEquals("1", response.getHits().getAt(0).getId()); + assertNotEquals("1", response.getHits().getAt(1).getId()); + } }