diff options
author | Emily Wang <emily.wang@mongodb.com> | 2022-06-22 21:26:59 +0000 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2022-06-22 23:18:48 +0000 |
commit | ccfb3a019e514e5a02f68a6f3e3fb4579cd14f46 (patch) | |
tree | 0f704d15a7fe93e1f7a52880dde75d57dfae63e0 | |
parent | f53f11d70e44ca9649a2819a606da4936492b739 (diff) | |
download | mongo-ccfb3a019e514e5a02f68a6f3e3fb4579cd14f46.tar.gz |
SERVER-23146 More verbose error messages for geoparser
-rw-r--r-- | jstests/core/geo_parse_err.js | 124 | ||||
-rw-r--r-- | src/mongo/db/geo/geoparser.cpp | 121 |
2 files changed, 201 insertions, 44 deletions
diff --git a/jstests/core/geo_parse_err.js b/jstests/core/geo_parse_err.js new file mode 100644 index 00000000000..73bc451bd7c --- /dev/null +++ b/jstests/core/geo_parse_err.js @@ -0,0 +1,124 @@ +/** + * Test the error messages users get when creating geo objects. For example: + * - Do we get the error message we expect when: + * - We insert something of a different type than an array of doubles for coordinates? + * - When the number of loops in a simple polygon exceeds 1? + * @tags: [ + * multiversion_incompatible + * ] + */ + +(function() { +"use strict"; +let t = db.geo_parse_err; +t.drop(); + +const indexname = "2dsphere"; +const bigCRS = { + type: "name", + properties: {name: "urn:x-mongodb:crs:strictwinding:EPSG:4326"} +}; + +t.createIndex({loc: indexname}); + +// parseFlatPoint +let err = t.insert({loc: {type: "Point", coordinates: "hello"}}); +assert.includes(err.getWriteError().errmsg, + 'Point must be an array or object, instead got type string'); + +err = t.insert({loc: {type: "Point", coordinates: ["hello", 5]}}); +assert.includes(err.getWriteError().errmsg, + "Point must only contain numeric elements, instead got type string"); + +err = t.insert({loc: {type: "Point", coordinates: [5 / 0, 5]}}); +assert.includes(err.getWriteError().errmsg, "Point coordinates must be finite numbers"); + +// parseGeoJSONCoordinate +err = t.insert({loc: {type: "LineString", coordinates: [5, 5]}}); +assert.includes(err.getWriteError().errmsg, + "GeoJSON coordinates must be an array, instead got type double"); + +// parseArrayOfCoordinates +err = t.insert({loc: {type: "LineString", coordinates: 5}}); +assert.includes(err.getWriteError().errmsg, + "GeoJSON coordinates must be an array of coordinates, instead got type double"); +// isLoopClosed +err = t.insert({loc: {type: "Polygon", coordinates: [[[0, 0], [1, 2], [2, 3]]]}}); +assert.includes(err.getWriteError().errmsg, + "Loop is not closed, first vertex does not equal last vertex:"); + +// parseGeoJSONPolygonCoordinates +err = t.insert({loc: {type: "Polygon", coordinates: "hi"}}); +assert.includes(err.getWriteError().errmsg, + "Polygon coordinates must be an array, instead got type string"); + +err = t.insert({loc: {type: "Polygon", coordinates: [[[0, 0], [1, 2], [0, 0]]]}}); +assert.includes(err.getWriteError().errmsg, + "Loop must have at least 3 different vertices, 2 unique vertices were provided:"); + +// parseBigSimplePolygonCoordinates +err = t.insert({loc: {type: "Polygon", coordinates: "", crs: bigCRS}}); +assert.includes(err.getWriteError().errmsg, + "Coordinates of polygon must be an array, instead got type string"); + +err = t.insert({ + loc: { + type: "Polygon", + coordinates: + [[[10.0, 10.0], [-10.0, 10.0], [-10.0, -10.0], [10.0, -10.0], [10.0, 10.0]], []], + crs: bigCRS + } +}); +assert.includes(err.getWriteError().errmsg, + "Only one simple loop is allowed in a big polygon, instead provided 2"); +err = t.insert({ + loc: {type: "Polygon", coordinates: [[[10.0, 10.0], [-10.0, 10.0], [10.0, 10.0]]], crs: bigCRS} +}); +assert.includes(err.getWriteError().errmsg, + "Loop must have at least 3 different vertices, 2 unique vertices were provided:"); + +// parseGeoJSONCRS +const bigPoly20 = [[[10.0, 10.0], [-10.0, 10.0], [10.0, 10.0]]]; + +err = t.insert({loc: {type: "Polygon", coordinates: bigPoly20, crs: {type: "name"}}}); + +assert.includes(err.getWriteError().errmsg, + "CRS must have field \"properties\" which is an object, instead got type missing"); + +err = t.insert({ + loc: { + type: "Polygon", + coordinates: bigPoly20, + crs: {type: "name", properties: {nam: "urn:x-mongodb:crs:strictwinding:EPSG:4326"}} + } +}); +assert.includes(err.getWriteError().errmsg, + "In CRS, \"properties.name\" must be a string, instead got type missing"); + +// parseMultiPolygon +err = t.insert({loc: {type: "MultiPolygon", coordinates: ""}}); + +assert.includes(err.getWriteError().errmsg, + "MultiPolygon coordinates must be an array, instead got type string"); + +// Geometry collection +err = t.insert({ + loc: { + type: "GeometryCollection", + geometries: [ + { + type: "MultiPoint", + coordinates: [ + [-73.9580, 40.8003], + [-73.9498, 40.7968], + [-73.9737, 40.7648], + [-73.9814, 40.7681] + ] + }, + 5 + ] + } +}); +assert.includes(err.getWriteError().errmsg, + "Element 1 of \"geometries\" must be an object, instead got type double:"); +})();
\ No newline at end of file diff --git a/src/mongo/db/geo/geoparser.cpp b/src/mongo/db/geo/geoparser.cpp index 57e2fbee611..893d7832b18 100644 --- a/src/mongo/db/geo/geoparser.cpp +++ b/src/mongo/db/geo/geoparser.cpp @@ -52,16 +52,21 @@ namespace mongo { namespace dps = ::mongo::dotted_path_support; static Status parseFlatPoint(const BSONElement& elem, Point* out, bool allowAddlFields = false) { - if (!elem.isABSONObj()) - return BAD_VALUE("Point must be an array or object"); + if (!elem.isABSONObj()) { + return BAD_VALUE("Point must be an array or object, instead got type " + << typeName(elem.type())); + } + BSONObjIterator it(elem.Obj()); BSONElement x = it.next(); if (!x.isNumber()) { - return BAD_VALUE("Point must only contain numeric elements"); + return BAD_VALUE("Point must only contain numeric elements, instead got type " + << typeName(x.type())); } BSONElement y = it.next(); if (!y.isNumber()) { - return BAD_VALUE("Point must only contain numeric elements"); + return BAD_VALUE("Point must only contain numeric elements, instead got type " + << typeName(y.type())); } if (!allowAddlFields && it.more()) { return BAD_VALUE("Point must only contain two numeric elements"); @@ -86,7 +91,7 @@ static Status coordToPoint(double lng, double lat, S2Point* out) { // We don't rely on drem to clean up non-sane points. We just don't let them become // spherical. if (!isValidLngLat(lng, lat)) - return BAD_VALUE("longitude/latitude is out of bounds, lng: " << lng << " lat: " << lat); + return BAD_VALUE("Longitude/latitude is out of bounds, lng: " << lng << " lat: " << lat); // Note that it's (lat, lng) for S2 but (lng, lat) for MongoDB. S2LatLng ll = S2LatLng::FromDegrees(lat, lng).Normalized(); // This shouldn't happen since we should only have valid lng/lats. @@ -101,7 +106,8 @@ static Status coordToPoint(double lng, double lat, S2Point* out) { static Status parseGeoJSONCoordinate(const BSONElement& elem, S2Point* out) { if (Array != elem.type()) { - return BAD_VALUE("GeoJSON coordinates must be an array"); + return BAD_VALUE("GeoJSON coordinates must be an array, instead got type " + << typeName(elem.type())); } Point p; // GeoJSON allows extra elements, e.g. altitude. @@ -116,7 +122,8 @@ static Status parseGeoJSONCoordinate(const BSONElement& elem, S2Point* out) { // "coordinates": [ [100.0, 0.0], [101.0, 1.0] ] static Status parseArrayOfCoordinates(const BSONElement& elem, vector<S2Point>* out) { if (Array != elem.type()) { - return BAD_VALUE("GeoJSON coordinates must be an array of coordinates"); + return BAD_VALUE("GeoJSON coordinates must be an array of coordinates, instead got type " + << typeName(elem.type())); } BSONObjIterator it(elem.Obj()); // Iterate all coordinates in array @@ -146,7 +153,8 @@ static Status isLoopClosed(const vector<S2Point>& loop, const BSONElement loopEl } if (loop[0] != loop[loop.size() - 1]) { - return BAD_VALUE("Loop is not closed: " << loopElt.toString(false)); + return BAD_VALUE("Loop is not closed, first vertex does not equal last vertex: " + << loopElt.toString(false)); } return Status::OK(); @@ -156,7 +164,8 @@ static Status parseGeoJSONPolygonCoordinates(const BSONElement& elem, bool skipValidation, S2Polygon* out) { if (Array != elem.type()) { - return BAD_VALUE("Polygon coordinates must be an array"); + return BAD_VALUE("Polygon coordinates must be an array, instead got type " + << typeName(elem.type())); } std::vector<std::unique_ptr<S2Loop>> loops; @@ -184,8 +193,9 @@ static Status parseGeoJSONPolygonCoordinates(const BSONElement& elem, // At least 3 vertices. if (points.size() < 3) { - return BAD_VALUE( - "Loop must have at least 3 different vertices: " << coordinateElt.toString(false)); + return BAD_VALUE("Loop must have at least 3 different vertices, " + << points.size() << " unique vertices were provided: " + << coordinateElt.toString(false)); } loops.push_back(std::make_unique<S2Loop>(points)); @@ -266,15 +276,17 @@ static Status parseGeoJSONPolygonCoordinates(const BSONElement& elem, } static Status parseBigSimplePolygonCoordinates(const BSONElement& elem, BigSimplePolygon* out) { - if (Array != elem.type()) - return BAD_VALUE("Coordinates of polygon must be an array"); + if (Array != elem.type()) { + return BAD_VALUE("Coordinates of polygon must be an array, instead got type " + << typeName(elem.type())); + } const vector<BSONElement>& coordinates = elem.Array(); // Only one loop is allowed in a BigSimplePolygon if (coordinates.size() != 1) { - return BAD_VALUE( - "Only one simple loop is allowed in a big polygon: " << elem.toString(false)); + return BAD_VALUE("Only one simple loop is allowed in a big polygon, instead provided " + << coordinates.size() << " loops: " << elem.toString(false)); } vector<S2Point> exteriorVertices; @@ -297,7 +309,9 @@ static Status parseBigSimplePolygonCoordinates(const BSONElement& elem, BigSimpl // At least 3 vertices. if (exteriorVertices.size() < 3) { - return BAD_VALUE("Loop must have at least 3 different vertices: " << elem.toString(false)); + return BAD_VALUE("Loop must have at least 3 different vertices, " + << exteriorVertices.size() + << " unique vertices were provided: " << elem.toString(false)); } std::unique_ptr<S2Loop> loop(new S2Loop(exteriorVertices)); @@ -326,8 +340,10 @@ static Status parseGeoJSONCRS(const BSONObj& obj, CRS* crs, bool allowStrictSphe return Status::OK(); } - if (!crsElt.isABSONObj()) - return BAD_VALUE("GeoJSON CRS must be an object"); + if (!crsElt.isABSONObj()) { + return BAD_VALUE("GeoJSON CRS must be an object, instead got type " + << typeName(crsElt.type())); + } BSONObj crsObj = crsElt.embeddedObject(); // "type": "name" @@ -336,17 +352,22 @@ static Status parseGeoJSONCRS(const BSONObj& obj, CRS* crs, bool allowStrictSphe // "properties" BSONElement propertiesElt = crsObj["properties"]; - if (!propertiesElt.isABSONObj()) - return BAD_VALUE("CRS must have field \"properties\" which is an object"); + if (!propertiesElt.isABSONObj()) { + return BAD_VALUE("CRS must have field \"properties\" which is an object, instead got type " + << typeName(propertiesElt.type())); + } BSONObj propertiesObj = propertiesElt.embeddedObject(); - if (String != propertiesObj["name"].type()) - return BAD_VALUE("In CRS, \"properties.name\" must be a string"); + if (String != propertiesObj["name"].type()) { + return BAD_VALUE("In CRS, \"properties.name\" must be a string, instead got type " + << typeName(propertiesObj["name"].type())); + } + const string& name = propertiesObj["name"].String(); if (CRS_CRS84 == name || CRS_EPSG_4326 == name) { *crs = SPHERE; } else if (CRS_STRICT_WINDING == name) { if (!allowStrictSphere) { - return BAD_VALUE("Strict winding order is only supported by polygon"); + return BAD_VALUE("Strict winding order CRS is only supported by polygon"); } *crs = STRICT_SPHERE; } else { @@ -369,8 +390,8 @@ static Status parseGeoJSONLineCoordinates(const BSONElement& elem, eraseDuplicatePoints(&vertices); if (!skipValidation) { if (vertices.size() < 2) - return BAD_VALUE( - "GeoJSON LineString must have at least 2 vertices: " << elem.toString(false)); + return BAD_VALUE("GeoJSON LineString must have at least 2 vertices, instead got " + << vertices.size() << " vertices: " << elem.toString(false)); string err; if (!S2Polyline::IsValid(vertices, &err)) @@ -384,9 +405,10 @@ static Status parseGeoJSONLineCoordinates(const BSONElement& elem, // Parse legacy point or GeoJSON point, used by geo near. // Only stored legacy points allow additional fields. Status parsePoint(const BSONElement& elem, PointWithCRS* out, bool allowAddlFields) { - if (!elem.isABSONObj()) - return BAD_VALUE("Point must be an array or object"); - + if (!elem.isABSONObj()) { + return BAD_VALUE("Point must be an array or object, instead got type " + << typeName(elem.type())); + } BSONObj obj = elem.Obj(); // location: [1, 2] or location: {x: 1, y:2} if (Array == elem.type() || obj.firstElement().isNumber()) { @@ -439,7 +461,8 @@ Status GeoParser::parseLegacyPolygon(const BSONObj& obj, PolygonWithCRS* out) { points.push_back(p); } if (points.size() < 3) - return BAD_VALUE("Polygon must have at least 3 points"); + return BAD_VALUE("Polygon must have at least 3 points, instead got " << points.size() + << " vertices"); out->oldPolygon.init(points); out->crs = FLAT; return Status::OK(); @@ -461,7 +484,7 @@ Status GeoParser::parseGeoJSONPoint(const BSONObj& obj, PointWithCRS* out) { // Projection out->crs = FLAT; if (!ShapeProjection::supportsProject(*out, SPHERE)) - return BAD_VALUE("longitude/latitude is out of bounds, lng: " << out->oldPoint.x << " lat: " + return BAD_VALUE("Longitude/latitude is out of bounds, lng: " << out->oldPoint.x << " lat: " << out->oldPoint.y); ShapeProjection::projectInto(out, SPHERE); return Status::OK(); @@ -534,8 +557,11 @@ Status GeoParser::parseMultiLine(const BSONObj& obj, bool skipValidation, MultiL return status; BSONElement coordElt = dps::extractElementAtPath(obj, GEOJSON_COORDINATES); - if (Array != coordElt.type()) - return BAD_VALUE("MultiLineString coordinates must be an array"); + if (Array != coordElt.type()) { + return BAD_VALUE("MultiLineString coordinates must be an array, instead got type " + << typeName(coordElt.type())); + } + out->lines.clear(); auto& lines = out->lines; @@ -564,9 +590,10 @@ Status GeoParser::parseMultiPolygon(const BSONObj& obj, return status; BSONElement coordElt = dps::extractElementAtPath(obj, GEOJSON_COORDINATES); - if (Array != coordElt.type()) - return BAD_VALUE("MultiPolygon coordinates must be an array"); - + if (Array != coordElt.type()) { + return BAD_VALUE("MultiPolygon coordinates must be an array, instead got type " + << typeName(coordElt.type())); + } out->polygons.clear(); auto& polygons = out->polygons; @@ -597,11 +624,11 @@ Status GeoParser::parseLegacyCenter(const BSONObj& obj, CapWithCRS* out) { BSONElement radius = objIt.next(); // radius >= 0 and is not NaN if (!radius.isNumber() || !(radius.number() >= 0)) - return BAD_VALUE("radius must be a non-negative number"); + return BAD_VALUE("Radius must be a non-negative number: " << radius.toString(false)); // No more if (objIt.more()) - return BAD_VALUE("Only 2 fields allowed for circular region"); + return BAD_VALUE("Only 2 fields allowed for circular region, but more were provided"); out->circle.radius = radius.number(); out->crs = FLAT; @@ -627,13 +654,15 @@ Status GeoParser::parseCenterSphere(const BSONObj& obj, CapWithCRS* out) { // Radius BSONElement radiusElt = objIt.next(); // radius >= 0 and is not NaN - if (!radiusElt.isNumber() || !(radiusElt.number() >= 0)) - return BAD_VALUE("radius must be a non-negative number"); + if (!radiusElt.isNumber() || !(radiusElt.number() >= 0)) { + return BAD_VALUE("Radius must be a non-negative number: " << radiusElt.toString(false)); + } + double radius = radiusElt.number(); // No more elements if (objIt.more()) - return BAD_VALUE("Only 2 fields allowed for circular region"); + return BAD_VALUE("Only 2 fields allowed for circular region, but more were provided"); out->cap = S2Cap::FromAxisAngle(centerPoint, S1Angle::Radians(radius)); out->circle.radius = radius; @@ -656,16 +685,20 @@ Status GeoParser::parseGeometryCollection(const BSONObj& obj, bool skipValidation, GeometryCollection* out) { BSONElement coordElt = dps::extractElementAtPath(obj, GEOJSON_GEOMETRIES); - if (Array != coordElt.type()) - return BAD_VALUE("GeometryCollection geometries must be an array"); - + if (Array != coordElt.type()) { + return BAD_VALUE("GeometryCollection geometries must be an array, instead got type " + << typeName(coordElt.type())); + } const vector<BSONElement>& geometries = coordElt.Array(); if (0 == geometries.size()) return BAD_VALUE("GeometryCollection geometries must have at least 1 element"); for (size_t i = 0; i < geometries.size(); ++i) { if (Object != geometries[i].type()) - return BAD_VALUE("Element " << i << " of \"geometries\" is not an object"); + return BAD_VALUE("Element " << i + << " of \"geometries\" must be an object, instead got type " + << typeName(geometries[i].type()) << ": " + << geometries[i].toString(false)); const BSONObj& geoObj = geometries[i].Obj(); GeoJSONType type = parseGeoJSONType(geoObj); |