SERVER-23146 More verbose error messages for geoparser

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);