diff --git a/presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/EsriGeometrySerde.java b/presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/EsriGeometrySerde.java index 536c81089b0e2..3dc1d3e06ee63 100644 --- a/presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/EsriGeometrySerde.java +++ b/presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/EsriGeometrySerde.java @@ -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; @@ -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; @@ -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; @@ -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); + } } private static OGCGeometry readGeometry(BasicSliceInput input, Slice inputSlice, GeometrySerializationType type, int length) diff --git a/presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/JtsGeometrySerde.java b/presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/JtsGeometrySerde.java index 420748c48d267..4c97a4f4b2524 100644 --- a/presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/JtsGeometrySerde.java +++ b/presto-geospatial-toolkit/src/main/java/com/facebook/presto/geospatial/serde/JtsGeometrySerde.java @@ -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; @@ -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; @@ -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) @@ -171,25 +179,26 @@ private static Geometry readPolygon(SliceInput input, boolean multitype) LinearRing shell = null; List holes = new ArrayList<>(); List 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])); diff --git a/presto-geospatial-toolkit/src/test/java/com/facebook/presto/geospatial/serde/TestGeometrySerialization.java b/presto-geospatial-toolkit/src/test/java/com/facebook/presto/geospatial/serde/TestGeometrySerialization.java index fe90756fe84dd..50182d367bb23 100644 --- a/presto-geospatial-toolkit/src/test/java/com/facebook/presto/geospatial/serde/TestGeometrySerialization.java +++ b/presto-geospatial-toolkit/src/test/java/com/facebook/presto/geospatial/serde/TestGeometrySerialization.java @@ -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; @@ -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 @@ -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); @@ -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); @@ -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) @@ -263,4 +291,14 @@ private static void ensureEnvelopeLoaded(OGCGeometry geometry) { geometry.envelope(); } + + private static void assertThrowsPrestoException(String argument, Consumer function, StandardErrorCode errorCode) + { + try { + function.accept(argument); + } + catch (PrestoException e) { + assertEquals(e.getErrorCode(), errorCode.toErrorCode()); + } + } } diff --git a/presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeoFunctions.java b/presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeoFunctions.java index 2fd9f44f73d94..42a3408f226b4 100644 --- a/presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeoFunctions.java +++ b/presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeoFunctions.java @@ -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; @@ -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 diff --git a/presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeometryType.java b/presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeometryType.java index 724f855382a02..d4f0aac6b83bc 100644 --- a/presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeometryType.java +++ b/presto-geospatial/src/main/java/com/facebook/presto/plugin/geospatial/GeometryType.java @@ -13,7 +13,9 @@ */ 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; @@ -21,6 +23,7 @@ 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 @@ -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 { + return deserialize(slice).asText(); + } + catch (GeometryException e) { + throw new PrestoException(INVALID_FUNCTION_ARGUMENT, e.getMessage(), e); + } } } diff --git a/presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java b/presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java index 4d671e27ebadb..cd1741965e884 100644 --- a/presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java +++ b/presto-geospatial/src/test/java/com/facebook/presto/plugin/geospatial/TestGeoFunctions.java @@ -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)