Skip to content

Commit

Permalink
[7.15] Remove InvalidShapeException from GeoPolygonDecomposer (#77613) (
Browse files Browse the repository at this point in the history
#77740)

fixes a bug where InvalidShapeException are not properly handle so even if the mappings defined with
ignore_malformed set to true, the exception leaks and the setting is ignored
  • Loading branch information
iverase authored Sep 15, 2021
1 parent 7c17e3f commit 54e1d24
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import org.elasticsearch.geometry.MultiPolygon;
import org.elasticsearch.geometry.Point;
import org.elasticsearch.geometry.Polygon;
import org.locationtech.spatial4j.exception.InvalidShapeException;

import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -192,7 +191,7 @@ private static Edge[] ring(int component, boolean direction, boolean handedness,
}
if (signedArea == 0) {
// Points are collinear or self-intersection
throw new InvalidShapeException("Cannot determine orientation: signed area equal to 0." +
throw new IllegalArgumentException("Cannot determine orientation: signed area equal to 0." +
" Points are collinear or polygon self-intersects.");
}
boolean orientation = signedArea < 0;
Expand Down Expand Up @@ -339,7 +338,7 @@ private static Edge[] concat(int component, boolean direction, Point[] points, f
edges[edgeOffset + i - 1].next = edges[edgeOffset + i] = new Edge(nextPoint, null);
edges[edgeOffset + i - 1].component = component;
} else {
throw new InvalidShapeException("Provided shape has duplicate consecutive coordinates at: (" + nextPoint + ")");
throw new IllegalArgumentException("Provided shape has duplicate consecutive coordinates at: (" + nextPoint + ")");
}
}

Expand Down Expand Up @@ -464,7 +463,7 @@ private static void assign(Edge[] holes, Point[][] points, int numHoles, Edge[]
if (intersections == 0) {
// There were no edges that intersect the line of longitude through
// holes[i].coordinate, so there's no way this hole is within the polygon.
throw new InvalidShapeException("Invalid shape: Hole is not within polygon");
throw new IllegalArgumentException("Invalid shape: Hole is not within polygon");
}

// Next we do a binary search to find the position of holes[i].coordinate in the array.
Expand All @@ -480,7 +479,7 @@ private static void assign(Edge[] holes, Point[][] points, int numHoles, Edge[]
// and it didn't match after all.

// TODO Can this actually happen? Needs a test to exercise it, or else needs to be removed.
throw new InvalidShapeException("Invalid shape: Hole is not within polygon");
throw new IllegalArgumentException("Invalid shape: Hole is not within polygon");
}

final int index;
Expand Down Expand Up @@ -546,7 +545,7 @@ private static int component(final Edge edge, final int id, final ArrayList<Edge
partitionPoint[1] = current.coordinate.getY();
partitionPoint[2] = current.coordinate.getZ();
if (connectedComponents > 0 && current.next != edge) {
throw new InvalidShapeException("Shape contains more than one shared point");
throw new IllegalArgumentException("Shape contains more than one shared point");
}

// a negative id flags the edge as visited for the edges(...) method.
Expand Down Expand Up @@ -594,10 +593,10 @@ private static Point[] coordinates(Edge component, Point[] coordinates, double[]
// First and last coordinates must be equal
if (coordinates[0].equals(coordinates[coordinates.length - 1]) == false) {
if (Double.isNaN(partitionPoint[2])) {
throw new InvalidShapeException("Self-intersection at or near point ["
throw new IllegalArgumentException("Self-intersection at or near point ["
+ partitionPoint[0] + "," + partitionPoint[1] + "]");
} else {
throw new InvalidShapeException("Self-intersection at or near point ["
throw new IllegalArgumentException("Self-intersection at or near point ["
+ partitionPoint[0] + "," + partitionPoint[1] + "," + partitionPoint[2] + "]");
}
}
Expand Down Expand Up @@ -728,7 +727,7 @@ void setNext(Edge next) {
if (next != null) {
// self-loop throws an invalid shape
if (this.coordinate.equals(next.coordinate)) {
throw new InvalidShapeException("Provided shape has duplicate consecutive coordinates at: " + this.coordinate);
throw new IllegalArgumentException("Provided shape has duplicate consecutive coordinates at: " + this.coordinate);
}
this.next = next;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import org.elasticsearch.geometry.ShapeType;
import org.elasticsearch.index.mapper.GeoShapeIndexer;
import org.elasticsearch.test.ESTestCase;
import org.locationtech.spatial4j.exception.InvalidShapeException;

import java.io.IOException;
import java.text.ParseException;
Expand Down Expand Up @@ -431,7 +430,7 @@ public void testInvalidSelfCrossingPolygon() {
Polygon polygon = new Polygon(new LinearRing(
new double[]{0, 0, 1, 0.5, 1.5, 1, 2, 2, 0}, new double[]{0, 2, 1.9, 1.8, 1.8, 1.9, 2, 0, 0}
));
Exception e = expectThrows(InvalidShapeException.class, () -> indexer.prepareForIndexing(polygon));
Exception e = expectThrows(IllegalArgumentException.class, () -> indexer.prepareForIndexing(polygon));
assertThat(e.getMessage(), containsString("Self-intersection at or near point ["));
assertThat(e.getMessage(), not(containsString("NaN")));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -834,9 +834,6 @@ public void testInvalidSelfCrossingPolygon() {
Exception e = expectThrows(InvalidShapeException.class, () -> builder.close().buildS4J());
assertThat(e.getMessage(), containsString("Self-intersection at or near point ["));
assertThat(e.getMessage(), not(containsString("NaN")));
e = expectThrows(InvalidShapeException.class, () -> buildGeometry(builder.close()));
assertThat(e.getMessage(), containsString("Self-intersection at or near point ["));
assertThat(e.getMessage(), not(containsString("NaN")));
}

public Object buildGeometry(ShapeBuilder<?, ?, ?> builder) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.index.mapper.DocumentMapper;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.Mapper;
import org.elasticsearch.index.mapper.MapperParsingException;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.mapper.MapperTestCase;
import org.elasticsearch.index.mapper.ParsedDocument;
Expand All @@ -38,6 +39,7 @@
import java.util.Collection;
import java.util.Collections;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.instanceOf;
Expand Down Expand Up @@ -225,6 +227,44 @@ public void testIgnoreMalformedParsing() throws IOException {
assertThat(ignoreMalformed, equalTo(false));
}

public void testIgnoreMalformedValues() throws IOException {

DocumentMapper ignoreMapper = createDocumentMapper(fieldMapping(b -> {
b.field("type", "geo_shape");
b.field("ignore_malformed", true);
}));
DocumentMapper failMapper = createDocumentMapper(fieldMapping(b -> {
b.field("type", "geo_shape");
b.field("ignore_malformed", false);
}));

{
BytesReference arrayedDoc = BytesReference.bytes(XContentFactory.jsonBuilder()
.startObject()
.field("field", "Bad shape")
.endObject()
);
SourceToParse sourceToParse = new SourceToParse("test", "_doc", "1", arrayedDoc, XContentType.JSON);
ParsedDocument document = ignoreMapper.parse(sourceToParse);
assertThat(document.docs().get(0).getFields("field").length, equalTo(0));
MapperParsingException exception = expectThrows(MapperParsingException.class, () -> failMapper.parse(sourceToParse));
assertThat(exception.getCause().getMessage(), containsString("Unknown geometry type: bad"));
}
{
BytesReference arrayedDoc = BytesReference.bytes(XContentFactory.jsonBuilder()
.startObject()
.field("field", "POLYGON ((18.9401790919516 -33.9681188869036, 18.9401790919516 -33.9681188869037, 18.9401790919517 " +
"-33.9681188869037, 18.9401790919517 -33.9681188869036, 18.9401790919516 -33.9681188869036))")
.endObject()
);
SourceToParse sourceToParse = new SourceToParse("test", "_doc", "1", arrayedDoc, XContentType.JSON);
ParsedDocument document = ignoreMapper.parse(sourceToParse);
assertThat(document.docs().get(0).getFields("field").length, equalTo(0));
MapperParsingException exception = expectThrows(MapperParsingException.class, () -> failMapper.parse(sourceToParse));
assertThat(exception.getCause().getMessage(), containsString("Cannot determine orientation"));
}
}

/**
* Test that doc_values parameter correctly parses
*/
Expand Down

0 comments on commit 54e1d24

Please sign in to comment.