Geospatial feedback (#6645)

* verify local results match a server query * disallow geowithin on top level tables * fix geo queries with ANY/ALL/NONE * geospatial validation of points * rename GeoCenterSphere -> GeoCircle * review feedback * better testing and fix any/all/none geospatial * format
realm · May 19, 2023 · 128f3c0 · 128f3c0
1 parent 6cc16a6
commit 128f3c0
Show file tree

Hide file tree

Showing 14 changed files with 663 additions and 381 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@
 * `platform` and `cpu_arch` fields in the `device_info` structure in App::Config can no longer be provided by the SDK's, they are inferred by the library ([PR #6612](https://github.com/realm/realm-core/pull/6612))
 * `bundle_id` is now a required field in the `device_info` structure in App::Config ([PR #6612](https://github.com/realm/realm-core/pull/6612))
 * The API for sectioned results change notifications has changed. Changes are now reported in a vector rather than a sparse map.
+* Renamed `GeoCenterSphere` to `GeoCircle` and in RQL `geoSphere` to `geoCircle`. The GeoPoints of query shapes are now validated before use and an exception will be thrown if invalid. Geospatial queries are no longer allowed on top-level tables. Fixed query results using ANY/ALL/NONE and matching on lists ([PR #6645](https://github.com/realm/realm-core/issues/6645))
 
 ### Compatibility
 * Fileformat: Generates files with format v23. Reads and automatically upgrade from fileformat v5.

diff --git a/src/realm/geospatial.cpp b/src/realm/geospatial.cpp
@@ -72,8 +72,8 @@ std::string Geospatial::get_type_string() const noexcept
             return "Box";
         case Type::Polygon:
             return "Polygon";
-        case Type::CenterSphere:
-            return "CenterSphere";
+        case Type::Circle:
+            return "Circle";
         case Type::Invalid:
             return "Invalid";
     }
@@ -254,8 +254,8 @@ std::string Geospatial::to_string() const
                            }
                            return util::format("GeoPolygon(%1)", points);
                        },
-                       [](const GeoCenterSphere& sphere) {
-                           return util::format("GeoSphere(%1, %2)", point_str(sphere.center), sphere.radius_radians);
+                       [](const GeoCircle& circle) {
+                           return util::format("GeoCircle(%1, %2)", point_str(circle.center), circle.radius_radians);
                        },
                        [](const mpark::monostate&) {
                            return std::string("NULL");
@@ -271,9 +271,30 @@ std::ostream& operator<<(std::ostream& ostr, const Geospatial& geo)
 
 // The following validation follows the server:
 // https://github.com/mongodb/mongo/blob/053ff9f355555cddddf3a476ffa9ddf899b1657d/src/mongo/db/geo/geoparser.cpp#L140
-static void erase_duplicate_points(std::vector<S2Point>* vertices)
+
+
+// Technically lat/long bounds, not really tied to earth radius.
+static bool is_valid_lat_lng(double lng, double lat)
+{
+    return abs(lng) <= 180 && abs(lat) <= 90;
+}
+
+static Status coord_to_point(double lng, double lat, S2Point& out)
+{
+    if (!is_valid_lat_lng(lng, lat))
+        return Status(ErrorCodes::InvalidQueryArg,
+                      util::format("Longitude/latitude is out of bounds, lng: %1 lat: %2", lng, 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.
+    REALM_ASSERT_EX(ll.is_valid(), util::format("coords invalid after normalization, lng = %1, lat = %2", lng, lat));
+    out = ll.ToPoint();
+    return Status::OK();
+}
+
+static void erase_duplicate_adjacent_points(std::vector<S2Point>& vertices)
 {
-    vertices->erase(std::unique(vertices->begin(), vertices->end()), vertices->end());
+    vertices.erase(std::unique(vertices.begin(), vertices.end()), vertices.end());
 }
 
 static Status is_ring_closed(const std::vector<S2Point>& ring, const std::vector<GeoPoint>& points)
@@ -293,6 +314,7 @@ static Status is_ring_closed(const std::vector<S2Point>& ring, const std::vector
 
 static Status parse_polygon_coordinates(const GeoPolygon& polygon, S2Polygon* out)
 {
+    REALM_ASSERT(out);
     std::vector<S2Loop*> rings;
     rings.reserve(polygon.points.size());
     Status status = Status::OK();
@@ -311,15 +333,19 @@ static Status parse_polygon_coordinates(const GeoPolygon& polygon, S2Polygon* ou
         std::vector<S2Point> points;
         points.reserve(polygon.points[i].size());
         for (auto&& p : polygon.points[i]) {
-            // FIXME rewrite without copying
-            points.push_back(S2LatLng::FromDegrees(p.latitude, p.longitude).ToPoint());
+            S2Point s2p;
+            status = coord_to_point(p.longitude, p.latitude, s2p);
+            if (!status.is_ok()) {
+                return status;
+            }
+            points.push_back(s2p);
         }
 
         status = is_ring_closed(points, polygon.points[i]);
         if (!status.is_ok())
             return status;
 
-        erase_duplicate_points(&points);
+        erase_duplicate_adjacent_points(points);
         // Drop the duplicated last point.
         points.resize(points.size() - 1);
 
@@ -414,8 +440,19 @@ GeoRegion::GeoRegion(const Geospatial& geo)
         Status& m_status_out;
         std::unique_ptr<S2Region> operator()(const GeoBox& box) const
         {
-            return std::make_unique<S2LatLngRect>(S2LatLng::FromDegrees(box.lo.latitude, box.lo.longitude),
-                                                  S2LatLng::FromDegrees(box.hi.latitude, box.hi.longitude));
+            S2Point s2_lo, s2_hi;
+            m_status_out = coord_to_point(box.lo.longitude, box.lo.latitude, s2_lo);
+            if (!m_status_out.is_ok())
+                return nullptr;
+            m_status_out = coord_to_point(box.hi.longitude, box.hi.latitude, s2_hi);
+            if (!m_status_out.is_ok())
+                return nullptr;
+            auto ret = std::make_unique<S2LatLngRect>(S2LatLng(s2_lo), S2LatLng(s2_hi));
+            if (!ret->is_valid()) {
+                m_status_out = Status(ErrorCodes::InvalidQueryArg, "Invalid rectangle");
+                return nullptr;
+            }
+            return ret;
         }
 
         std::unique_ptr<S2Region> operator()(const GeoPolygon& polygon) const
@@ -425,10 +462,18 @@ GeoRegion::GeoRegion(const Geospatial& geo)
             return poly;
         }
 
-        std::unique_ptr<S2Region> operator()(const GeoCenterSphere& sphere) const
+        std::unique_ptr<S2Region> operator()(const GeoCircle& circle) const
         {
-            auto center = S2LatLng::FromDegrees(sphere.center.latitude, sphere.center.longitude).ToPoint();
-            auto radius = S1Angle::Radians(sphere.radius_radians);
+            S2Point center;
+            m_status_out = coord_to_point(circle.center.longitude, circle.center.latitude, center);
+            if (!m_status_out.is_ok())
+                return nullptr;
+            if (circle.radius_radians < 0 || std::isnan(circle.radius_radians)) {
+                m_status_out =
+                    Status(ErrorCodes::InvalidQueryArg, "The radius of a circle must be a non-negative number");
+                return nullptr;
+            }
+            auto radius = S1Angle::Radians(circle.radius_radians);
             return std::make_unique<S2Cap>(S2Cap::FromAxisAngle(center, radius));
         }
 

diff --git a/src/realm/geospatial.hpp b/src/realm/geospatial.hpp
@@ -149,11 +149,11 @@ struct GeoPolygon {
     std::vector<std::vector<GeoPoint>> points;
 };
 
-struct GeoCenterSphere {
+struct GeoCircle {
     double radius_radians = 0.0;
     GeoPoint center;
 
-    bool operator==(const GeoCenterSphere& other) const
+    bool operator==(const GeoCircle& other) const
     {
         return radius_radians == other.radius_radians && center == other.center;
     }
@@ -162,15 +162,15 @@ struct GeoCenterSphere {
     // src/mongo/db/geo/geoconstants.h
     constexpr static double c_radius_meters = 6378100.0;
 
-    static GeoCenterSphere from_kms(double km, GeoPoint&& p)
+    static GeoCircle from_kms(double km, GeoPoint&& p)
     {
-        return GeoCenterSphere{km * 1000 / c_radius_meters, p};
+        return GeoCircle{km * 1000 / c_radius_meters, p};
     }
 };
 
 class Geospatial {
 public:
-    enum class Type : uint8_t { Invalid, Point, Box, Polygon, CenterSphere };
+    enum class Type : uint8_t { Invalid, Point, Box, Polygon, Circle };
 
     Geospatial()
         : m_value(mpark::monostate{})
@@ -188,8 +188,8 @@ class Geospatial {
         : m_value(polygon)
     {
     }
-    Geospatial(GeoCenterSphere centerSphere)
-        : m_value(centerSphere)
+    Geospatial(GeoCircle circle)
+        : m_value(circle)
     {
     }
 
@@ -233,7 +233,7 @@ class Geospatial {
 
 private:
     // Must be in the same order as the Type enum
-    mpark::variant<mpark::monostate, GeoPoint, GeoBox, GeoPolygon, GeoCenterSphere> m_value;
+    mpark::variant<mpark::monostate, GeoPoint, GeoBox, GeoPolygon, GeoCircle> m_value;
 
     friend class GeoRegion;
 };
@@ -252,9 +252,9 @@ class GeoRegion {
 };
 
 template <>
-inline const GeoCenterSphere& Geospatial::get<GeoCenterSphere>() const noexcept
+inline const GeoCircle& Geospatial::get<GeoCircle>() const noexcept
 {
-    return mpark::get<GeoCenterSphere>(m_value);
+    return mpark::get<GeoCircle>(m_value);
 }
 
 template <>

diff --git a/src/realm/parser/driver.cpp b/src/realm/parser/driver.cpp
@@ -1361,8 +1361,8 @@ GeospatialNode::GeospatialNode(GeospatialNode::Box, GeoPoint& p1, GeoPoint& p2)
 {
 }
 
-GeospatialNode::GeospatialNode(Sphere, GeoPoint& p, double radius)
-    : m_geo{Geospatial{GeoCenterSphere{radius, p}}}
+GeospatialNode::GeospatialNode(Circle, GeoPoint& p, double radius)
+    : m_geo{Geospatial{GeoCircle{radius, p}}}
 {
 }
 

diff --git a/src/realm/parser/driver.hpp b/src/realm/parser/driver.hpp
@@ -201,10 +201,10 @@ class GeospatialNode : public ValueNode {
     struct Box {};
     struct Polygon {};
     struct Loop {};
-    struct Sphere {};
+    struct Circle {};
 #if REALM_ENABLE_GEOSPATIAL
     GeospatialNode(Box, GeoPoint& p1, GeoPoint& p2);
-    GeospatialNode(Sphere, GeoPoint& p, double radius);
+    GeospatialNode(Circle, GeoPoint& p, double radius);
     GeospatialNode(Polygon, GeoPoint& p);
     GeospatialNode(Loop, GeoPoint& p);
     void add_point_to_loop(GeoPoint& p);

diff --git a/src/realm/parser/generated/query_bison.cpp b/src/realm/parser/generated/query_bison.cpp
@@ -883,7 +883,7 @@ namespace yy {
                  { yyo << "<>"; }
         break;
 
-      case symbol_kind::SYM_GEOSPHERE: // "geosphere"
+      case symbol_kind::SYM_GEOCIRCLE: // "geocircle"
                  { yyo << "<>"; }
         break;
 
@@ -1760,8 +1760,8 @@ namespace yy {
                                             { yylhs.value.as < GeospatialNode* > () = drv.m_parse_nodes.create<GeospatialNode>(GeospatialNode::Box{}, *yystack_[3].value.as < std::optional<GeoPoint> > (), *yystack_[1].value.as < std::optional<GeoPoint> > ()); }
     break;
 
-  case 45: // geospatial: "geosphere" '(' geopoint ',' coordinate ')'
-                                                { yylhs.value.as < GeospatialNode* > () = drv.m_parse_nodes.create<GeospatialNode>(GeospatialNode::Sphere{}, *yystack_[3].value.as < std::optional<GeoPoint> > (), yystack_[1].value.as < double > ()); }
+  case 45: // geospatial: "geocircle" '(' geopoint ',' coordinate ')'
+                                                { yylhs.value.as < GeospatialNode* > () = drv.m_parse_nodes.create<GeospatialNode>(GeospatialNode::Circle{}, *yystack_[3].value.as < std::optional<GeoPoint> > (), yystack_[1].value.as < double > ()); }
     break;
 
   case 46: // geospatial: "geopolygon" '(' geopoly_content ')'
@@ -2717,7 +2717,7 @@ namespace yy {
   "\"null\"", "\"==\"", "\"!=\"", "\"<\"", "\">\"", "\">=\"", "\"<=\"",
   "\"[c]\"", "\"any\"", "\"all\"", "\"none\"", "\"@links\"", "\"@max\"",
   "\"@min\"", "\"@sun\"", "\"@average\"", "\"&&\"", "\"||\"", "\"!\"",
-  "\"geobox\"", "\"geopolygon\"", "\"geosphere\"", "\"identifier\"",
+  "\"geobox\"", "\"geopolygon\"", "\"geocircle\"", "\"identifier\"",
   "\"string\"", "\"base64\"", "\"infinity\"", "\"NaN\"", "\"natural0\"",
   "\"number\"", "\"float\"", "\"date\"", "\"UUID\"", "\"ObjectId\"",
   "\"link\"", "\"typed link\"", "\"argument\"", "\"beginswith\"",

diff --git a/src/realm/parser/generated/query_bison.hpp b/src/realm/parser/generated/query_bison.hpp
@@ -621,7 +621,7 @@ namespace yy {
     TOK_NOT = 281,                 // "!"
     TOK_GEOBOX = 282,              // "geobox"
     TOK_GEOPOLYGON = 283,          // "geopolygon"
-    TOK_GEOSPHERE = 284,           // "geosphere"
+    TOK_GEOCIRCLE = 284,           // "geocircle"
     TOK_ID = 285,                  // "identifier"
     TOK_STRING = 286,              // "string"
     TOK_BASE64 = 287,              // "base64"
@@ -700,7 +700,7 @@ namespace yy {
         SYM_NOT = 26,                            // "!"
         SYM_GEOBOX = 27,                         // "geobox"
         SYM_GEOPOLYGON = 28,                     // "geopolygon"
-        SYM_GEOSPHERE = 29,                      // "geosphere"
+        SYM_GEOCIRCLE = 29,                      // "geocircle"
         SYM_ID = 30,                             // "identifier"
         SYM_STRING = 31,                         // "string"
         SYM_BASE64 = 32,                         // "base64"
@@ -1467,7 +1467,7 @@ switch (yykind)
       {
 #if !defined _MSC_VER || defined __clang__
         YY_ASSERT (tok == token::TOK_END
-                   || (token::TOK_YYerror <= tok && tok <= token::TOK_GEOSPHERE)
+                   || (token::TOK_YYerror <= tok && tok <= token::TOK_GEOCIRCLE)
                    || tok == 43
                    || tok == 45
                    || tok == 42
@@ -1978,16 +1978,16 @@ switch (yykind)
 #if 201103L <= YY_CPLUSPLUS
       static
       symbol_type
-      make_GEOSPHERE ()
+      make_GEOCIRCLE ()
       {
-        return symbol_type (token::TOK_GEOSPHERE);
+        return symbol_type (token::TOK_GEOCIRCLE);
       }
 #else
       static
       symbol_type
-      make_GEOSPHERE ()
+      make_GEOCIRCLE ()
       {
-        return symbol_type (token::TOK_GEOSPHERE);
+        return symbol_type (token::TOK_GEOCIRCLE);
       }
 #endif
 #if 201103L <= YY_CPLUSPLUS