Skip to content

Commit

Permalink
Geo: Fix Empty Geometry Collection Handling (#37978)
Browse files Browse the repository at this point in the history 
Fixes handling empty geometry collection and re-enables
testParseGeometryCollection test.

Fixes #37894
  • Loading branch information
@imotov
imotov authored Jan 30, 2019
1 parent 53e80e9 commit 23805fa
Show file tree
Hide file tree
Showing 7 changed files with 65 additions and 20 deletions.
4 changes: 4 additions & 0 deletions libs/geo/src/main/java/org/elasticsearch/geo/utils/WellKnownText.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ public Void visit(MultiLine multiLine) {

@Override
public Void visit(MultiPoint multiPoint) {
if (multiPoint.isEmpty()) {
sb.append(EMPTY);
return null;
}
// walk through coordinates:
sb.append(LPAREN);
visitPoint(multiPoint.get(0).getLon(), multiPoint.get(0).getLat());
Expand Down
4 changes: 4 additions & 0 deletions server/src/main/java/org/elasticsearch/common/geo/builders/GeometryCollectionBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.geo.geometry.GeometryCollection;
import org.locationtech.spatial4j.shape.Shape;

import java.io.IOException;
Expand Down Expand Up @@ -186,6 +187,9 @@ public Shape buildS4J() {

@Override
public org.elasticsearch.geo.geometry.GeometryCollection<org.elasticsearch.geo.geometry.Geometry> buildGeometry() {
if (this.shapes.isEmpty()) {
return GeometryCollection.EMPTY;
}
List<org.elasticsearch.geo.geometry.Geometry> shapes = new ArrayList<>(this.shapes.size());

for (ShapeBuilder shape : this.shapes) {
Expand Down
3 changes: 3 additions & 0 deletions server/src/main/java/org/elasticsearch/common/geo/builders/MultiLineStringBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,9 @@ public JtsGeometry buildS4J() {

@Override
public org.elasticsearch.geo.geometry.Geometry buildGeometry() {
if (lines.isEmpty()) {
return MultiLine.EMPTY;
}
if (wrapdateline) {
List<org.elasticsearch.geo.geometry.Line> parts = new ArrayList<>();
for (LineStringBuilder line : lines) {
Expand Down
10 changes: 10 additions & 0 deletions server/src/main/java/org/elasticsearch/common/geo/builders/MultiPointBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ public MultiPointBuilder(List<Coordinate> coordinates) {
super(coordinates);
}

/**
* Creates a new empty MultiPoint builder
*/
public MultiPointBuilder() {
super();
}

/**
* Read from a stream.
*/
Expand Down Expand Up @@ -77,6 +84,9 @@ public XShapeCollection<Point> buildS4J() {

@Override
public MultiPoint buildGeometry() {
if (coordinates.isEmpty()) {
return MultiPoint.EMPTY;
}
return new MultiPoint(coordinates.stream().map(coord -> new org.elasticsearch.geo.geometry.Point(coord.y, coord.x))
.collect(Collectors.toList()));
}
Expand Down
3 changes: 3 additions & 0 deletions server/src/main/java/org/elasticsearch/common/geo/builders/MultiPolygonBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,9 @@ public MultiPolygon buildGeometry() {
shapes.add((org.elasticsearch.geo.geometry.Polygon)poly);
}
}
if (shapes.isEmpty()) {
return MultiPolygon.EMPTY;
}
return new MultiPolygon(shapes);
}

Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ private static MultiPointBuilder parseMultiPoint(StreamTokenizer stream, final b
throws IOException, ElasticsearchParseException {
String token = nextEmptyOrOpen(stream);
if (token.equals(EMPTY)) {
return null;
return new MultiPointBuilder();
}
return new MultiPointBuilder(parseCoordinateList(stream, ignoreZValue, coerce));
}
Expand Down Expand Up @@ -242,7 +242,7 @@ private static MultiLineStringBuilder parseMultiLine(StreamTokenizer stream, fin
throws IOException, ElasticsearchParseException {
String token = nextEmptyOrOpen(stream);
if (token.equals(EMPTY)) {
return null;
return new MultiLineStringBuilder();
}
MultiLineStringBuilder builder = new MultiLineStringBuilder();
builder.linestring(parseLine(stream, ignoreZValue, coerce));
Expand Down
57 changes: 39 additions & 18 deletions server/src/test/java/org/elasticsearch/common/geo/GeoWKTShapeParserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.geo.geometry.Geometry;
import org.elasticsearch.geo.geometry.Line;
import org.elasticsearch.geo.geometry.MultiLine;
import org.elasticsearch.geo.geometry.MultiPoint;
Expand Down Expand Up @@ -112,27 +113,39 @@ public void testParsePoint() throws IOException {

@Override
public void testParseMultiPoint() throws IOException {
int numPoints = randomIntBetween(2, 100);
int numPoints = randomIntBetween(0, 100);
List<Coordinate> coordinates = new ArrayList<>(numPoints);
for (int i = 0; i < numPoints; ++i) {
coordinates.add(new Coordinate(GeoTestUtil.nextLongitude(), GeoTestUtil.nextLatitude()));
}

Shape[] shapes = new Shape[numPoints];
List<org.elasticsearch.geo.geometry.Point> points = new ArrayList<>(numPoints);
for (int i = 0; i < numPoints; ++i) {
Coordinate c = coordinates.get(i);
shapes[i] = SPATIAL_CONTEXT.makePoint(c.x, c.y);
points.add(new org.elasticsearch.geo.geometry.Point(c.y, c.x));
}
ShapeCollection<?> expected = shapeCollection(shapes);
assertExpected(expected, new MultiPointBuilder(coordinates), true);

List<org.elasticsearch.geo.geometry.Point> points = new ArrayList<>(numPoints);
Geometry expectedGeom;
MultiPointBuilder actual;
if (numPoints == 0) {
expectedGeom = MultiPoint.EMPTY;
actual = new MultiPointBuilder();
} else {
expectedGeom = new MultiPoint(points);
actual = new MultiPointBuilder(coordinates);
}

assertExpected(expectedGeom, actual, false);
assertMalformed(actual);

assumeTrue("JTS test path cannot handle empty multipoints", numPoints > 1);
Shape[] shapes = new Shape[numPoints];
for (int i = 0; i < numPoints; ++i) {
Coordinate c = coordinates.get(i);
points.add(new org.elasticsearch.geo.geometry.Point(c.y, c.x));
shapes[i] = SPATIAL_CONTEXT.makePoint(c.x, c.y);
}
assertExpected(new MultiPoint(points), new MultiPointBuilder(coordinates), false);
assertMalformed(new MultiPointBuilder(coordinates));
ShapeCollection<?> expected = shapeCollection(shapes);
assertExpected(expected, new MultiPointBuilder(coordinates), true);
}

private List<Coordinate> randomLineStringCoords() {
Expand Down Expand Up @@ -163,7 +176,7 @@ public void testParseLineString() throws IOException {

@Override
public void testParseMultiLineString() throws IOException {
int numLineStrings = randomIntBetween(2, 8);
int numLineStrings = randomIntBetween(0, 8);
List<LineString> lineStrings = new ArrayList<>(numLineStrings);
MultiLineStringBuilder builder = new MultiLineStringBuilder();
for (int j = 0; j < numLineStrings; ++j) {
Expand All @@ -173,18 +186,27 @@ public void testParseMultiLineString() throws IOException {
builder.linestring(new LineStringBuilder(lsc));
}

MultiLineString expected = GEOMETRY_FACTORY.createMultiLineString(
lineStrings.toArray(new LineString[lineStrings.size()]));
assertExpected(jtsGeom(expected), builder, true);

List<Line> lines = new ArrayList<>(lineStrings.size());
for (int j = 0; j < lineStrings.size(); ++j) {
Coordinate[] c = lineStrings.get(j).getCoordinates();
lines.add(new Line(Arrays.stream(c).mapToDouble(i->i.y).toArray(),
Arrays.stream(c).mapToDouble(i->i.x).toArray()));
}
assertExpected(new MultiLine(lines), builder, false);
Geometry expectedGeom;
if (lines.isEmpty()) {
expectedGeom = MultiLine.EMPTY;
} else if (lines.size() == 1) {
expectedGeom = new Line(lines.get(0).getLats(), lines.get(0).getLons());
} else {
expectedGeom = new MultiLine(lines);
}
assertExpected(expectedGeom, builder, false);
assertMalformed(builder);

MultiLineString expected = GEOMETRY_FACTORY.createMultiLineString(
lineStrings.toArray(new LineString[lineStrings.size()]));
assumeTrue("JTS test path cannot handle empty multilinestrings", numLineStrings > 1);
assertExpected(jtsGeom(expected), builder, true);
}

@Override
Expand All @@ -201,7 +223,7 @@ public void testParsePolygon() throws IOException {

@Override
public void testParseMultiPolygon() throws IOException {
int numPolys = randomIntBetween(2, 8);
int numPolys = randomIntBetween(0, 8);
MultiPolygonBuilder builder = new MultiPolygonBuilder();
PolygonBuilder pb;
Coordinate[] coordinates;
Expand All @@ -214,7 +236,7 @@ public void testParseMultiPolygon() throws IOException {
shell = GEOMETRY_FACTORY.createLinearRing(coordinates);
shapes[i] = GEOMETRY_FACTORY.createPolygon(shell, null);
}

assumeTrue("JTS test path cannot handle empty multipolygon", numPolys > 1);
Shape expected = shapeCollection(shapes);
assertExpected(expected, builder, true);
assertMalformed(builder);
Expand Down Expand Up @@ -429,7 +451,6 @@ public void testInvalidGeometryType() throws IOException {
assertValidException(builder, IllegalArgumentException.class);
}

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/37894")
@Override
public void testParseGeometryCollection() throws IOException {
if (rarely()) {
Expand Down

0 comments on commit 23805fa

Please sign in to comment.