Skip to content

Improve error handling for geometry deserialization edge cases #13922

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

Merged
mbasmanova merged 2 commits into from
Jan 10, 2020
Merged

Improve error handling for geometry deserialization edge cases #13922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
155871f
Use helper method jtsGeometryFromWkt
jagill Jan 3, 2020
28b3d8e
Capture geometry deserialization failures
jagill Jan 3, 2020
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
10 changes: 9 additions & 1 deletion ...spatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/EsriGeometrySerde.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import com.esri.core.geometry.Envelope;
import com.esri.core.geometry.Geometry;
import com.esri.core.geometry.GeometryException;
import com.esri.core.geometry.MultiPoint;
import com.esri.core.geometry.OperatorImportFromESRIShape;
import com.esri.core.geometry.Point;
Expand All @@ -31,6 +32,7 @@
import com.esri.core.geometry.ogc.OGCPoint;
import com.esri.core.geometry.ogc.OGCPolygon;
import com.facebook.presto.geospatial.GeometryType;
import com.facebook.presto.spi.PrestoException;
import com.google.common.annotations.VisibleForTesting;
import io.airlift.slice.BasicSliceInput;
import io.airlift.slice.DynamicSliceOutput;
Expand All @@ -46,6 +48,7 @@
import static com.esri.core.geometry.Geometry.Type.Unknown;
import static com.esri.core.geometry.GeometryEngine.geometryToEsriShape;
import static com.facebook.presto.geospatial.GeometryUtils.isEsriNaN;
import static com.facebook.presto.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;
import static com.google.common.base.Verify.verify;
import static java.lang.Double.NaN;
import static java.lang.Double.isNaN;
Expand Down Expand Up @@ -171,7 +174,12 @@ public static OGCGeometry deserialize(Slice shape)
verify(input.available() > 0);
int length = input.available() - 1;
GeometrySerializationType type = GeometrySerializationType.getForCode(input.readByte());
return readGeometry(input, shape, type, length);
try {
return readGeometry(input, shape, type, length);
}
catch (GeometryException e) {
throw new PrestoException(INVALID_FUNCTION_ARGUMENT, e.getMessage(), e);
Copy link
Contributor

@aweisberg aweisberg Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I ran TestGeometrySerialization I didn't see this exercised in the debugger.

}
}

private static OGCGeometry readGeometry(BasicSliceInput input, Slice inputSlice, GeometrySerializationType type, int length)
Expand Down
39 changes: 24 additions & 15 deletions ...ospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/JtsGeometrySerde.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package com.facebook.presto.geospatial.serde;

import com.facebook.presto.spi.PrestoException;
import io.airlift.slice.BasicSliceInput;
import io.airlift.slice.DynamicSliceOutput;
import io.airlift.slice.Slice;
Expand All @@ -27,12 +28,14 @@
import org.locationtech.jts.geom.MultiPoint;
import org.locationtech.jts.geom.Point;
import org.locationtech.jts.geom.Polygon;
import org.locationtech.jts.geom.TopologyException;

import java.util.ArrayList;
import java.util.List;

import static com.facebook.presto.geospatial.GeometryUtils.isEsriNaN;
import static com.facebook.presto.geospatial.GeometryUtils.translateToAVNaN;
import static com.facebook.presto.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;
import static com.google.common.base.Verify.verify;
import static com.google.common.collect.Iterables.getOnlyElement;
import static io.airlift.slice.SizeOf.SIZE_OF_DOUBLE;
Expand All @@ -53,7 +56,12 @@ public static Geometry deserialize(Slice shape)
BasicSliceInput input = shape.getInput();
verify(input.available() > 0);
GeometrySerializationType type = GeometrySerializationType.getForCode(input.readByte());
return readGeometry(input, type);
try {
return readGeometry(input, type);
}
catch (TopologyException e) {
throw new PrestoException(INVALID_FUNCTION_ARGUMENT, e.getMessage(), e);
}
}

private static Geometry readGeometry(BasicSliceInput input, GeometrySerializationType type)
Expand Down Expand Up @@ -171,25 +179,26 @@ private static Geometry readPolygon(SliceInput input, boolean multitype)
LinearRing shell = null;
List<LinearRing> holes = new ArrayList<>();
List<Polygon> polygons = new ArrayList<>();
for (int i = 0; i < partCount; i++) {
Coordinate[] coordinates = readCoordinates(input, partLengths[i]);
if (isClockwise(coordinates)) {
// next polygon has started
if (shell != null) {
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
holes.clear();
try {
for (int i = 0; i < partCount; i++) {
Coordinate[] coordinates = readCoordinates(input, partLengths[i]);
if (isClockwise(coordinates)) {
// next polygon has started
if (shell != null) {
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
holes.clear();
}
shell = GEOMETRY_FACTORY.createLinearRing(coordinates);
}
else {
verify(holes.isEmpty(), "shell is null but holes found");
holes.add(GEOMETRY_FACTORY.createLinearRing(coordinates));
}
shell = GEOMETRY_FACTORY.createLinearRing(coordinates);
}
else {
verify(shell != null, "shell is null but hole found");
holes.add(GEOMETRY_FACTORY.createLinearRing(coordinates));
}
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));
}
catch (IllegalArgumentException e) {
throw new TopologyException("Error constructing Polygon: " + e.getMessage());
}
polygons.add(GEOMETRY_FACTORY.createPolygon(shell, holes.toArray(new LinearRing[0])));

if (multitype) {
return GEOMETRY_FACTORY.createMultiPolygon(polygons.toArray(new Polygon[0]));
Expand Down
64 changes: 51 additions & 13 deletions ...toolkit/src/test/java/com/facebook/presto/geospatial/serde/TestGeometrySerialization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@

import com.esri.core.geometry.Envelope;
import com.esri.core.geometry.ogc.OGCGeometry;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.StandardErrorCode;
import io.airlift.slice.Slice;
import org.locationtech.jts.geom.Geometry;
import org.locationtech.jts.io.ParseException;
import org.locationtech.jts.io.WKTReader;
import org.testng.annotations.Test;

import java.util.function.Consumer;

import static com.facebook.presto.geospatial.GeometryUtils.jtsGeometryFromWkt;
import static com.facebook.presto.geospatial.serde.EsriGeometrySerde.createFromEsriGeometry;
import static com.facebook.presto.geospatial.serde.EsriGeometrySerde.deserialize;
import static com.facebook.presto.geospatial.serde.EsriGeometrySerde.deserializeEnvelope;
Expand All @@ -34,6 +37,7 @@
import static com.facebook.presto.geospatial.serde.GeometrySerializationType.MULTI_POLYGON;
import static com.facebook.presto.geospatial.serde.GeometrySerializationType.POINT;
import static com.facebook.presto.geospatial.serde.GeometrySerializationType.POLYGON;
import static com.facebook.presto.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;
import static org.testng.Assert.assertEquals;

public class TestGeometrySerialization
Expand Down Expand Up @@ -181,6 +185,28 @@ public void testDeserializeType()
assertEquals(deserializeType(serialize(new Envelope(1, 2, 3, 4))), ENVELOPE);
}

@Test
public void testInvalidSerializations()
{
String wkt = "LINESTRING (0 0)";
testEsriSerialization(wkt);
assertThrowsPrestoException(wkt, TestGeometrySerialization::testJtsSerialization, INVALID_FUNCTION_ARGUMENT);
assertThrowsPrestoException(wkt, TestGeometrySerialization::tryDeserializeEsriFromJts, INVALID_FUNCTION_ARGUMENT);
tryDeserializeJtsFromEsri(wkt);

wkt = "POLYGON ((0 0, 1 1))";
testEsriSerialization(wkt);
assertThrowsPrestoException(wkt, TestGeometrySerialization::testJtsSerialization, INVALID_FUNCTION_ARGUMENT);
assertThrowsPrestoException(wkt, TestGeometrySerialization::tryDeserializeEsriFromJts, INVALID_FUNCTION_ARGUMENT);
assertThrowsPrestoException(wkt, TestGeometrySerialization::tryDeserializeJtsFromEsri, INVALID_FUNCTION_ARGUMENT);

wkt = "POLYGON ((0 0, 1 1, 0 1, 1 0, 0 0))";
testEsriSerialization(wkt);
assertThrowsPrestoException(wkt, TestGeometrySerialization::testJtsSerialization, INVALID_FUNCTION_ARGUMENT);
tryDeserializeEsriFromJts(wkt);
assertThrowsPrestoException(wkt, TestGeometrySerialization::tryDeserializeJtsFromEsri, INVALID_FUNCTION_ARGUMENT);
}

private static void testSerialization(String wkt)
{
testEsriSerialization(wkt);
Expand All @@ -197,14 +223,14 @@ private static void testEsriSerialization(String wkt)

private static void testJtsSerialization(String wkt)
{
Geometry expected = createJtsGeometry(wkt);
Geometry expected = jtsGeometryFromWkt(wkt);
Geometry actual = JtsGeometrySerde.deserialize(JtsGeometrySerde.serialize(expected));
assertGeometryEquals(actual, expected);
}

private static void testCrossSerialization(String wkt)
{
Geometry jtsGeometry = createJtsGeometry(wkt);
Geometry jtsGeometry = jtsGeometryFromWkt(wkt);
OGCGeometry esriGeometry = OGCGeometry.fromText(wkt);

Slice jtsSerialized = JtsGeometrySerde.serialize(jtsGeometry);
Expand All @@ -216,19 +242,21 @@ private static void testCrossSerialization(String wkt)
assertGeometryEquals(jtsFromEsri, jtsGeometry);
}

private static Slice geometryFromText(String wkt)
private static void tryDeserializeEsriFromJts(String wkt)
{
return serialize(OGCGeometry.fromText(wkt));
Geometry jtsGeometry = jtsGeometryFromWkt(wkt);
EsriGeometrySerde.deserialize(JtsGeometrySerde.serialize(jtsGeometry));
}

private static Geometry createJtsGeometry(String wkt)
private static void tryDeserializeJtsFromEsri(String wkt)
{
try {
return new WKTReader().read(wkt);
}
catch (ParseException e) {
throw new RuntimeException(e);
}
OGCGeometry esriGeometry = OGCGeometry.fromText(wkt);
JtsGeometrySerde.deserialize(EsriGeometrySerde.serialize(esriGeometry));
}

private static Slice geometryFromText(String wkt)
{
return serialize(OGCGeometry.fromText(wkt));
}

private static void assertGeometryEquals(Geometry actual, Geometry expected)
Expand Down Expand Up @@ -263,4 +291,14 @@ private static void ensureEnvelopeLoaded(OGCGeometry geometry)
{
geometry.envelope();
}

private static void assertThrowsPrestoException(String argument, Consumer<String> function, StandardErrorCode errorCode)
{
try {
function.accept(argument);
}
catch (PrestoException e) {
assertEquals(e.getErrorCode(), errorCode.toErrorCode());
}
}
}
8 changes: 7 additions & 1 deletion presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeoFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import com.esri.core.geometry.Envelope;
import com.esri.core.geometry.GeometryCursor;
import com.esri.core.geometry.GeometryException;
import com.esri.core.geometry.ListeningGeometryCursor;
import com.esri.core.geometry.MultiPath;
import com.esri.core.geometry.MultiVertexGeometry;
Expand Down Expand Up @@ -325,7 +326,12 @@ public static Slice stAsText(@SqlType(GEOMETRY_TYPE_NAME) Slice input)
@SqlType(VARBINARY)
public static Slice stAsBinary(@SqlType(GEOMETRY_TYPE_NAME) Slice input)
{
return wrappedBuffer(EsriGeometrySerde.deserialize(input).asBinary());
try {
return wrappedBuffer(EsriGeometrySerde.deserialize(input).asBinary());
}
catch (GeometryException e) {
throw new PrestoException(INVALID_FUNCTION_ARGUMENT, "Invalid geometry: " + e.getMessage(), e);
}
}

@SqlNullable
Expand Down
10 changes: 9 additions & 1 deletion presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeometryType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,17 @@
*/
package com.facebook.presto.plugin.geospatial;

import com.esri.core.geometry.GeometryException;
import com.facebook.presto.spi.ConnectorSession;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.block.Block;
import com.facebook.presto.spi.block.BlockBuilder;
import com.facebook.presto.spi.type.AbstractVariableWidthType;
import com.facebook.presto.spi.type.TypeSignature;
import io.airlift.slice.Slice;

import static com.facebook.presto.geospatial.serde.EsriGeometrySerde.deserialize;
import static com.facebook.presto.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT;

public class GeometryType
extends AbstractVariableWidthType
Expand Down Expand Up @@ -83,6 +86,11 @@ public Object getObjectValue(ConnectorSession session, Block block, int position
return null;
}
Slice slice = block.getSlice(position, 0, block.getSliceLength(position));
return deserialize(slice).asText();
try {
Copy link
Contributor

@aweisberg aweisberg Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this error path tested exercised by any test? Looks like deserialize catches the GeometryException and wraps it so is this necessary?

return deserialize(slice).asText();
}
catch (GeometryException e) {
throw new PrestoException(INVALID_FUNCTION_ARGUMENT, e.getMessage(), e);
}
}
}
1 change: 1 addition & 0 deletions presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,6 +1187,7 @@ public void testSTGeometryFromBinary()

// invalid binary
assertInvalidFunction("ST_GeomFromBinary(from_hex('deadbeef'))", "Invalid WKB");
assertInvalidFunction("ST_AsBinary(ST_GeometryFromText('POLYGON ((0 0, 1 1, 0 1, 1 0, 0 0))'))", INVALID_FUNCTION_ARGUMENT, "Invalid geometry: corrupted geometry");
}

private void assertGeomFromBinary(String wkt)
Expand Down