2d not suitable for within: geometry queries, more tests

12 files changed, 225 insertions, 43 deletions

diff --git a/jstests/geo_allowedcomparisons.js b/jstests/geo_allowedcomparisons.js
new file mode 100644
index 00000000000..eabd0a99d72
--- /dev/null
+++ b/jstests/geo_allowedcomparisons.js
@@ -0,0 +1,107 @@
+// A test for what geometries can interact with what other geometries.
+t = db.geo_allowedcomparisons;
+
+// Any GeoJSON object can intersect with any geojson object.
+geojsonPoint = { "type" : "Point", "coordinates": [ 0, 0 ] };
+oldPoint = [0,0];
+
+// GeoJSON polygons can contain any geojson object and OLD points.
+geojsonPoly = { "type" : "Polygon",
+                "coordinates" : [ [ [-5,-5], [-5,5], [5,5], [5,-5], [-5,-5]]]};
+
+// This can be contained by GJ polygons, intersected by anything GJ and old points.
+geojsonLine = { "type" : "LineString", "coordinates": [ [ 0, 0], [1, 1]]}
+
+// $centerSphere can contain old or new points.
+oldCenterSphere = [[0, 0], Math.PI / 180];
+// $box can contain old points.
+oldBox = [[-5,-5], [5,5]];
+// $polygon can contain old points.
+oldPolygon = [[-5,-5], [-5,5], [5,5], [5,-5], [-5,-5]]
+// $center can contain old points.
+oldCenter = [[0, 0], 1];
+
+t.drop();
+t.ensureIndex({geo: "2d"});
+// 2d doesn't know what to do w/this
+t.insert({geo: geojsonPoint});
+assert(db.getLastError());
+// Old points are OK.
+t.insert({geo: oldPoint})
+assert(!db.getLastError());
+// Lines not OK in 2d
+t.insert({geo: geojsonLine})
+assert(db.getLastError())
+// Shapes are not OK to insert in 2d
+t.insert({geo: geojsonPoly})
+assert(db.getLastError());
+t.insert({geo: oldCenterSphere})
+assert(db.getLastError());
+t.insert({geo: oldCenter})
+assert(db.getLastError());
+// If we try to insert a polygon, it thinks it's an array of points.  Let's not
+// do that.  Ditto for the box.
+
+// Verify that even if we can't index them, we can use them in a matcher.
+t.insert({gj: geojsonLine})
+t.insert({gj: geojsonPoly})
+geojsonPoint2 = { "type" : "Point", "coordinates": [ 0, 0.001 ] };
+t.insert({gjp: geojsonPoint2})
+
+// We convert between old and new style points.
+assert.eq(1, t.find({gjp: {$within: {$box: oldBox}}}).itcount());
+assert.eq(1, t.find({gjp: {$within: {$polygon: oldPolygon}}}).itcount());
+assert.eq(1, t.find({gjp: {$within: {$center: oldCenter}}}).itcount());
+assert.eq(1, t.find({gjp: {$within: {$centerSphere: oldCenterSphere}}}).itcount())
+
+function runTests() {
+    // Each find the box, the polygon, and the old point.
+    assert.eq(1, t.find({geo: {$within: {$box: oldBox}}}).itcount())
+    assert.eq(1, t.find({geo: {$within: {$polygon: oldPolygon}}}).itcount())
+    // Each find the old point.
+    assert.eq(1, t.find({geo: {$within: {$center: oldCenter}}}).itcount())
+    assert.eq(1, t.find({geo: {$within: {$centerSphere: oldCenterSphere}}}).itcount())
+    // Using geojson with 2d-style within syntax should choke.
+    assert.throws(function() { return t.find({geo: {$within: {$polygon: geojsonPoly}}})
+                                       .itcount();})
+    // Using old polygon w/new syntax should choke too.
+    assert.throws(function() { return t.find({geo: {$within: {$geometry: oldPolygon}}})
+                                       .itcount();})
+    assert.throws(function() { return t.find({geo: {$within: {$geometry: oldBox}}})
+                                       .itcount();})
+    assert.throws(function() { return t.find({geo: {$within: {$geometry: oldCenter}}})
+                                       .itcount();})
+    assert.throws(function() { return t.find({geo: {$within: {$geometry: oldCenterSphere}}})
+                                       .itcount();})
+    // Even if we only have a 2d index, the 2d suitability function should
+    // allow the matcher to deal with this.  If we have a 2dsphere index we use it.
+    assert.eq(1, t.find({geo: {$within: {$geometry: geojsonPoly}}}).itcount())
+    assert.eq(1, t.find({geo: {$geoIntersects: {$geometry: geojsonPoly}}}).itcount())
+    assert.eq(1, t.find({geo: {$geoIntersects: {$geometry: oldPoint}}}).itcount())
+    assert.eq(1, t.find({geo: {$geoIntersects: {$geometry: geojsonPoint}}}).itcount())
+}
+
+// We have a 2d index right now.  Let's see what it does.
+runTests();
+
+// No index now.
+t.dropIndex({geo: "2d"})
+runTests();
+
+// 2dsphere index now.
+t.ensureIndex({geo: "2dsphere"})
+assert(!db.getLastError())
+// 2dsphere does not support arrays of points.
+t.insert({geo: [geojsonPoint2, geojsonPoint]})
+assert(db.getLastError())
+runTests();
+
+// Old stuff is not GeoJSON (or old-style point).  All should fail.
+t.insert({geo: oldBox})
+assert(db.getLastError())
+t.insert({geo: oldPolygon})
+assert(db.getLastError())
+t.insert({geo: oldCenter})
+assert(db.getLastError())
+t.insert({geo: oldCenterSphere})
+assert(db.getLastError())
diff --git a/src/mongo/db/geo/2d.cpp b/src/mongo/db/geo/2d.cpp
index d51f207816f..84e50c0be1b 100644
--- a/src/mongo/db/geo/2d.cpp
+++ b/src/mongo/db/geo/2d.cpp
@@ -250,8 +250,23 @@ namespace mongo {
                 BSONObj sub = e.embeddedObject();
                 switch (sub.firstElement().getGtLtOp()) {
                 case BSONObj::opNEAR:
-                case BSONObj::opWITHIN:
                     return OPTIMAL;
+                case BSONObj::opWITHIN: {
+                    // Don't return optimal if it's $within: {$geometry: ... }
+                    // because we will error out in that case, but the matcher
+                    // or 2dsphere index may handle it.
+                    BSONElement elt = sub.firstElement();
+                    if (Object == elt.type()) {
+                        BSONObjIterator it(elt.embeddedObject());
+                        while (it.more()) {
+                            BSONElement elt = it.next();
+                            if (mongoutils::str::equals("$geometry", elt.fieldName())) {
+                                return USELESS;
+                            }
+                        }
+                    }
+                    return OPTIMAL;
+                }
                 default:
                     // We can try to match if there's no other indexing defined,
                     // this is assumed a point
diff --git a/src/mongo/db/geo/geoparser.cpp b/src/mongo/db/geo/geoparser.cpp
index 659d67a0b92..3b53b30cdd2 100644
--- a/src/mongo/db/geo/geoparser.cpp
+++ b/src/mongo/db/geo/geoparser.cpp
@@ -109,6 +109,12 @@ namespace mongo {
         *out = S2Cell(point);
     }
 
+    void GeoParser::parseGeoJSONPoint(const BSONObj& obj, Point* out) {
+        const vector<BSONElement>& coords = obj.getFieldDotted(GEOJSON_COORDINATES).Array();
+        out->x = coords[0].Number();
+        out->y = coords[1].Number();
+    }
+
     void GeoParser::parseGeoJSONPoint(const BSONObj& obj, S2Point* out) {
         const vector<BSONElement>& coords = obj.getFieldDotted(GEOJSON_COORDINATES).Array();
         *out = coordsToPoint(coords);
@@ -215,6 +221,17 @@ namespace mongo {
                 polyBuilder.AssemblePolygon(out, NULL));
     }
 
+    bool GeoParser::parsePoint(const BSONObj &obj, Point *out) {
+        if (isGeoJSONPoint(obj)) {
+            parseGeoJSONPoint(obj, out);
+            return true;
+        } else if (isLegacyPoint(obj)) {
+            parseLegacyPoint(obj, out);
+            return true;
+        }
+        return false;
+    }
+
     bool GeoParser::parsePoint(const BSONObj &obj, S2Point *out) {
         if (isGeoJSONPoint(obj)) {
             parseGeoJSONPoint(obj, out);
diff --git a/src/mongo/db/geo/geoparser.h b/src/mongo/db/geo/geoparser.h
index 225b151a5b4..72d998267ff 100644
--- a/src/mongo/db/geo/geoparser.h
+++ b/src/mongo/db/geo/geoparser.h
@@ -45,12 +45,14 @@ namespace mongo {
         // You can just use these bool parsePoint(...) methods.
         static bool parsePoint(const BSONObj &obj, S2Point *out);
         static bool parsePoint(const BSONObj &obj, S2Cell *out);
+        static bool parsePoint(const BSONObj &obj, Point *out);
         // Check to see if it's GeoJSON or if it's legacy geo.
         static bool isPoint(const BSONObj &obj);
 
         static bool isGeoJSONPoint(const BSONObj &obj);
         static void parseGeoJSONPoint(const BSONObj &obj, S2Point *out);
         static void parseGeoJSONPoint(const BSONObj &obj, S2Cell *out);
+        static void parseGeoJSONPoint(const BSONObj &obj, Point *out);
 
         static bool isLegacyPoint(const BSONObj &obj);
         static void parseLegacyPoint(const BSONObj &obj, S2Point *out);
diff --git a/src/mongo/db/geo/geoquery.cpp b/src/mongo/db/geo/geoquery.cpp
index d4e9058bda9..4bd8d17b4e0 100644
--- a/src/mongo/db/geo/geoquery.cpp
+++ b/src/mongo/db/geo/geoquery.cpp
@@ -113,22 +113,19 @@ namespace mongo {
             return true;
         }
 
-        bool hasGeometry = false;
-
         BSONObjIterator it(obj);
-        while (it.more()) {
-            BSONElement e = it.next();
-            if (!e.isABSONObj()) { return false; }
-            BSONObj embeddedObj = e.embeddedObject();
-            // Legacy within #2 : t.find({ loc : { $within : { $box : ...
-            bool contains = mongoutils::str::equals(e.fieldName(), "$within");
-            if (contains && geoContainer.parseFrom(embeddedObj)) {
-                predicate = GeoQuery::WITHIN;
-                hasGeometry = true;
-            }
+        if (!it.more()) { return false; }
+        BSONElement e = it.next();
+        if (!e.isABSONObj()) { return false; }
+        BSONObj embeddedObj = e.embeddedObject();
+        // Legacy within #2 : t.find({ loc : { $within : { $box/etc : ...
+        bool contains = mongoutils::str::equals(e.fieldName(), "$within");
+        if (contains && geoContainer.parseFrom(embeddedObj)) {
+            predicate = GeoQuery::WITHIN;
+            return true;
         }
 
-        return hasGeometry;
+        return false;
     }
 
     bool GeoQuery::parseNewQuery(const BSONObj &obj) {
@@ -191,11 +188,9 @@ namespace mongo {
         return geoContainer.getRegion();
     }
 
-    bool GeoQuery::satisfiesPredicate(const BSONObj &obj) const {
+    bool GeoQuery::satisfiesPredicate(const GeometryContainer &otherContainer) const {
         verify(predicate == WITHIN || predicate == INTERSECT);
 
-        GeometryContainer otherContainer;
-        if (!otherContainer.parseFrom(obj)) { return false; }
         if (WITHIN == predicate) {
             return geoContainer.contains(otherContainer);
         } else {
@@ -306,6 +301,8 @@ namespace mongo {
     }
 
     bool GeometryContainer::parseFrom(const BSONObj& obj) {
+        // Free up any pointers we might have left over from previous parses.
+        *this = GeometryContainer();
         if (GeoParser::isGeoJSONPolygon(obj)) {
             // We can't really pass these things around willy-nilly except by ptr.
             _polygon.reset(new S2Polygon());
@@ -313,11 +310,8 @@ namespace mongo {
         } else if (GeoParser::isPoint(obj)) {
             _cell.reset(new S2Cell());
             GeoParser::parsePoint(obj, _cell.get());
-            // Old style points get upgraded to S2 points.
-            if (GeoParser::isLegacyPoint(obj)) {
-                _oldPoint.reset(new Point());
-                GeoParser::parseLegacyPoint(obj, _oldPoint.get());
-            }
+            _oldPoint.reset(new Point());
+            GeoParser::parsePoint(obj, _oldPoint.get());
         } else if (GeoParser::isLineString(obj)) {
             _line.reset(new S2Polyline());
             GeoParser::parseLineString(obj, _line.get());
diff --git a/src/mongo/db/geo/geoquery.h b/src/mongo/db/geo/geoquery.h
index d35221dde57..2933cb18bba 100644
--- a/src/mongo/db/geo/geoquery.h
+++ b/src/mongo/db/geo/geoquery.h
@@ -101,7 +101,7 @@ namespace mongo {
         };
 
         bool parseFrom(const BSONObj &obj);
-        bool satisfiesPredicate(const BSONObj &obj) const;
+        bool satisfiesPredicate(const GeometryContainer &otherContainer) const;
 
         bool hasS2Region() const;
         const S2Region& getRegion() const;
diff --git a/src/mongo/db/geo/s2common.cpp b/src/mongo/db/geo/s2common.cpp
index 801b5f652e7..e2da239cd63 100644
--- a/src/mongo/db/geo/s2common.cpp
+++ b/src/mongo/db/geo/s2common.cpp
@@ -26,7 +26,8 @@ namespace mongo {
         return ss.str();
     }
 
-    void S2SearchUtil::setCoverLimitsBasedOnArea(double area, S2RegionCoverer *coverer, int coarsestIndexedLevel) {
+    void S2SearchUtil::setCoverLimitsBasedOnArea(double area, S2RegionCoverer *coverer,
+                                                 int coarsestIndexedLevel) {
         area = sqrt(area);
         coverer->set_min_level(min(coarsestIndexedLevel, 2 + S2::kAvgEdge.GetClosestLevel(area)));
         coverer->set_max_level(4 + coverer->min_level());
diff --git a/src/mongo/db/geo/s2cursor.cpp b/src/mongo/db/geo/s2cursor.cpp
index e40f671abde..5f2e4a2d027 100644
--- a/src/mongo/db/geo/s2cursor.cpp
+++ b/src/mongo/db/geo/s2cursor.cpp
@@ -128,7 +128,10 @@ namespace mongo {
                      !match && (oi != geoFieldElements.end()); ++oi) {
                     if (!oi->isABSONObj()) { continue; }
                     const BSONObj &geoObj = oi->Obj();
-                    match = _fields[i].satisfiesPredicate(geoObj);
+                    GeometryContainer geoContainer;
+                    uassert(16698, "malformed geometry: " + geoObj.toString(),
+                            geoContainer.parseFrom(geoObj));
+                    match = _fields[i].satisfiesPredicate(geoContainer);
                 }
 
                 if (match) { ++geoFieldsMatched; }
diff --git a/src/mongo/db/geo/s2index.cpp b/src/mongo/db/geo/s2index.cpp
index 8468bf8b328..7e3f7e3dfd5 100644
--- a/src/mongo/db/geo/s2index.cpp
+++ b/src/mongo/db/geo/s2index.cpp
@@ -260,7 +260,8 @@ namespace mongo {
 
             // See here for GeoJSON format: geojson.org/geojson-spec.html
             for (BSONElementSet::iterator i = elements.begin(); i != elements.end(); ++i) {
-                if (!i->isABSONObj()) { continue; }  // error?
+                uassert(16700, "Can't parse geometry from element: " + i->toString(),
+                        i->isABSONObj());
                 const BSONObj &obj = i->Obj();
 
                 vector<string> cells;
diff --git a/src/mongo/db/geo/s2nearcursor.cpp b/src/mongo/db/geo/s2nearcursor.cpp
index 022808d5e54..5ce7127577b 100644
--- a/src/mongo/db/geo/s2nearcursor.cpp
+++ b/src/mongo/db/geo/s2nearcursor.cpp
@@ -77,14 +77,26 @@ namespace mongo {
     double S2NearCursor::currentDistance() const { return _results.top().distance; }
 
     // This is called when we're about to yield.
-    void S2NearCursor::noteLocation() { _results = priority_queue<Result>(); }
+    void S2NearCursor::noteLocation() {
+        LOG(1) << "yielding, tossing " << _results.size() << " results" << endl;
+        _results = priority_queue<Result>();
+    }
+
     // Called when we're un-yielding.
     // Note that this is (apparently) a valid call sequence:
     // 1. noteLocation()
     // 2. ok()
     // 3. checkLocation()
     // As such we might have results and only want to fill the result queue if it's empty.
-    void S2NearCursor::checkLocation() { if (_results.empty()) { fillResults(); } }
+    void S2NearCursor::checkLocation() {
+        LOG(1) << "unyielding, have " << _results.size() << " results in queue";
+        if (_results.empty()) {
+            LOG(1) << ", filling..." << endl;
+            fillResults();
+            LOG(1) << "now have " << _results.size() << " results in queue";
+        }
+        LOG(1) << endl;
+    }
 
     void S2NearCursor::explainDetails(BSONObjBuilder& b) {
         b << "nscanned" << _nscanned;
@@ -94,24 +106,42 @@ namespace mongo {
     }
 
     bool S2NearCursor::ok() {
-        if (_numToReturn <= 0) { return false; }
-        if (_innerRadius > _maxDistance) { return false; }
-        if (_results.empty()) { fillResults(); }
+        if (_numToReturn <= 0) {
+            LOG(2) << "not OK, no more to return" << endl;
+            return false;
+        }
+        if (_innerRadius > _maxDistance) {
+            LOG(2) << "not OK, exhausted search bounds" << endl;
+            return false;
+        }
+        if (_results.empty()) {
+            LOG(2) << "results empty in OK, filling" << endl;
+            fillResults();
+        }
         // If fillResults can't find anything, we're outta results.
         return !_results.empty();
     }
 
     void S2NearCursor::nextAnnulus() {
+        LOG(1) << "growing annulus from (" << _innerRadius << ", " << _outerRadius;
         _innerRadius = _outerRadius;
         _outerRadius += _radiusIncrement;
         _outerRadius = min(_outerRadius, _maxDistance);
         verify(_innerRadius <= _outerRadius);
+        LOG(1) << ") to (" << _innerRadius << ", " << _outerRadius << ")" << endl;
         ++_numShells;
     }
 
     bool S2NearCursor::advance() {
-        if (_numToReturn <= 0) { return false; }
-        if (_innerRadius > _maxDistance) { return false; }
+        if (_numToReturn <= 0) {
+            LOG(2) << "advancing but no more to return" << endl;
+            return false;
+        }
+
+        if (_innerRadius > _maxDistance) {
+            LOG(2) << "advancing but exhausted search distance" << endl;
+            return false;
+        }
 
         if (!_results.empty()) {
             _returned.insert(_results.top().loc);
@@ -126,7 +156,7 @@ namespace mongo {
 
         if (_results.empty()) { fillResults(); }
 
-        // The only reason this should be empty is if there are no more results to be had.
+        // The only reason _results should be empty now is if there are no more possible results.
         return !_results.empty();
     }
 
@@ -153,6 +183,9 @@ namespace mongo {
         double area = outerCap.area() - innerCap.area();
         S2SearchUtil::setCoverLimitsBasedOnArea(area, &coverer, _params.coarsestIndexedLevel);
         coverer.GetCovering(shell, &cover);
+        LOG(2) << "annulus cover size is " << cover.size()
+               << ", params (" << coverer.min_level() << ", " << coverer.max_level() << ")"
+               << endl;
         inExpr = S2SearchUtil::coverAsBSON(cover, _nearQuery.field,
                                            _params.coarsestIndexedLevel);
         // Shell takes ownership of the regions we push in, but they're local variables and
@@ -207,6 +240,7 @@ namespace mongo {
             // issues if possible.
             set<DiskLoc> seen;
 
+            LOG(1) << "looking at annulus from " << _innerRadius << " to " << _outerRadius << endl;
             // Do the actual search through this annulus.
             for (; cursor->ok(); cursor->advance()) {
                 ++_nscanned;
@@ -238,7 +272,10 @@ namespace mongo {
                             !match && (oi != geoFieldElements.end()); ++oi) {
                         if (!oi->isABSONObj()) { continue; }
                         const BSONObj &geoObj = oi->Obj();
-                        match = _indexedGeoFields[i].satisfiesPredicate(geoObj);
+                        GeometryContainer geoContainer;
+                        uassert(16699, "ill-formed geometry: " + geoObj.toString(),
+                                geoContainer.parseFrom(geoObj));
+                        match = _indexedGeoFields[i].satisfiesPredicate(geoContainer);
                     }
 
                     if (match) { ++geoFieldsMatched; }
@@ -276,6 +313,7 @@ namespace mongo {
                     }
                 }
             }
+
             if (_results.empty()) {
                 _radiusIncrement *= 2;
                 nextAnnulus();
@@ -301,6 +339,7 @@ namespace mongo {
             them = point.GetCenter();
         } else {
             warning() << "unknown geometry: " << obj.toString();
+            return numeric_limits<double>::max();
         }
         S1Angle angle(us, them);
         return angle.radians() * _params.radius;
diff --git a/src/mongo/db/matcher.cpp b/src/mongo/db/matcher.cpp
index 2f2cb8b35e0..13336251c40 100644
--- a/src/mongo/db/matcher.cpp
+++ b/src/mongo/db/matcher.cpp
@@ -950,15 +950,18 @@ namespace mongo {
             jsobj.getFieldsDotted(it->getField().c_str(), s, false);
             int matches = 0;
             for (BSONElementSet::const_iterator i = s.begin(); i != s.end(); ++i) {
-                if (!i->isABSONObj()) { continue; }
-                if (it->matches(i->Obj())) { ++matches; break; }
-                // Maybe it's an array of geometries
+                if (!i->isABSONObj()) { return false; }
+                GeometryContainer container;
+                if (container.parseFrom(i->Obj()) && it->matches(container)) {
+                    ++matches; break;
+                }
+                // Maybe it's an array of geometries.
                 BSONObjIterator geoIt(i->Obj());
                 while (geoIt.more()) {
                     BSONElement e = geoIt.next();
-                    if (e.isABSONObj() && it->matches(e.embeddedObject())) {
-                        ++matches; break;
-                    }
+                    if (!e.isABSONObj()) { return false; }
+                    if (!container.parseFrom(e.embeddedObject())) { return false; }
+                    if (it->matches(container)) { ++matches; break; }
                 }
             }
             if (0 == matches) { return false; }
diff --git a/src/mongo/db/matcher.h b/src/mongo/db/matcher.h
index 952cdedcfa7..67f663dd1c6 100644
--- a/src/mongo/db/matcher.h
+++ b/src/mongo/db/matcher.h
@@ -56,8 +56,8 @@ namespace mongo {
 
         string getField() const { return geoQuery.getField(); }
 
-        bool matches(BSONObj obj) const {
-            bool satisfied = geoQuery.satisfiesPredicate(obj);
+        bool matches(const GeometryContainer &container) const {
+            bool satisfied = geoQuery.satisfiesPredicate(container);
             if (isNot) { return !satisfied; }
             else { return satisfied; }
         }