Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Geo: Fix Empty Geometry Collection Handling #37978

Merged
merged 1 commit into from
Jan 30, 2019
Merged

Geo: Fix Empty Geometry Collection Handling #37978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions libs/geo/src/main/java/org/elasticsearch/geo/utils/WellKnownText.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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