Implement collection groups in C++ Query (#3327)

* Add a test that verifies canonical IDs don't change

* Remove Query::AtPath

This has no parallel in Typescript or Java implementations.

* Add util::Equals

... for comparing pointers by the thing to which they point.

* Use std::shared_ptr<const std::string> for collection group IDs

* Reorder/Cleanup C++ Query to match Android

* Add collection group to C++ Query

Author: wilhuff

Firestore/Example/Tests/Core/FSTQueryTests.mm

@@ -28,6 +28,7 @@
 #include "Firestore/core/src/firebase/firestore/model/resource_path.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
 #include "Firestore/core/test/firebase/firestore/testutil/testutil.h"
+#include "Firestore/core/test/firebase/firestore/testutil/xcgmock.h"
 #include "absl/strings/string_view.h"
 
 namespace testutil = firebase::firestore::testutil;
@@ -39,6 +40,8 @@
 using firebase::firestore::model::ResourcePath;
 using firebase::firestore::util::ComparisonResult;
 
+using testutil::Value;
+
 NS_ASSUME_NONNULL_BEGIN
 
 /** Convenience methods for building test queries. */
@@ -628,6 +631,54 @@ - (void)testImplicitOrderBy {
                         ]));
 }
 
+MATCHER_P(HasCanonicalId, expected, "") {
+  std::string actual = util::MakeString([arg canonicalID]);
+  *result_listener << "which has canonicalID " << actual;
+  return actual == expected;
+}
+
+- (void)testCanonicalIDs {
+  FSTQuery *query = FSTTestQuery("coll");
+  XC_ASSERT_THAT(query, HasCanonicalId("coll|f:|ob:__name__asc"));
+
+  FSTQuery *cg = [FSTQuery queryWithPath:ResourcePath::Empty()
+                         collectionGroup:std::make_shared<const std::string>("foo")];
+  XC_ASSERT_THAT(cg, HasCanonicalId("|cg:foo|f:|ob:__name__asc"));
+
+  FSTQuery *subcoll = FSTTestQuery("foo/bar/baz");
+  XC_ASSERT_THAT(subcoll, HasCanonicalId("foo/bar/baz|f:|ob:__name__asc"));
+
+  FSTQuery *filters = FSTTestQuery("coll");
+  filters = [filters queryByAddingFilter:FSTTestFilter("str", @"==", @"foo")];
+  XC_ASSERT_THAT(filters, HasCanonicalId("coll|f:str==foo|ob:__name__asc"));
+
+  // Inequality filters end up in the order by too
+  filters = [filters queryByAddingFilter:FSTTestFilter("int", @"<", @42)];
+  XC_ASSERT_THAT(filters, HasCanonicalId("coll|f:str==fooint<42|ob:intasc__name__asc"));
+
+  FSTQuery *orderBys = FSTTestQuery("coll");
+  orderBys = [orderBys queryByAddingSortBy:"up" ascending:true];
+  XC_ASSERT_THAT(orderBys, HasCanonicalId("coll|f:|ob:upasc__name__asc"));
+
+  // __name__'s order matches the trailing component
+  orderBys = [orderBys queryByAddingSortBy:"down" ascending:false];
+  XC_ASSERT_THAT(orderBys, HasCanonicalId("coll|f:|ob:upascdowndesc__name__desc"));
+
+  FSTQuery *limit = [FSTTestQuery("coll") queryBySettingLimit:25];
+  XC_ASSERT_THAT(limit, HasCanonicalId("coll|f:|ob:__name__asc|l:25"));
+
+  FSTQuery *bounds = FSTTestQuery("airports");
+  bounds = [bounds queryByAddingSortBy:"name" ascending:YES];
+  bounds = [bounds queryByAddingSortBy:"score" ascending:NO];
+  bounds = [bounds queryByAddingStartAt:[FSTBound boundWithPosition:{Value("OAK"), Value(1000)}
+                                                           isBefore:true]];
+  bounds = [bounds queryByAddingEndAt:[FSTBound boundWithPosition:{Value("SFO"), Value(2000)}
+                                                         isBefore:false]];
+  XC_ASSERT_THAT(
+      bounds,
+      HasCanonicalId("airports|f:|ob:nameascscoredesc__name__desc|lb:b:OAK1000|ub:a:SFO2000"));
+}
+
 @end
 
 NS_ASSUME_NONNULL_END

Firestore/Example/Tests/SpecTests/FSTSpecTests.mm

@@ -19,6 +19,8 @@
 #import <FirebaseFirestore/FIRFirestoreErrors.h>
 
 #include <map>
+#include <memory>
+#include <string>
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@ -176,7 +178,8 @@ - (nullable FSTQuery *)parseQuery:(id)querySpec {
     NSDictionary *queryDict = (NSDictionary *)querySpec;
     NSString *path = queryDict[@"path"];
     ResourcePath resource_path = ResourcePath::FromString(util::MakeString(path));
-    NSString *_Nullable collectionGroup = queryDict[@"collectionGroup"];
+    std::shared_ptr<const std::string> collectionGroup =
+        util::MakeStringPtr(queryDict[@"collectionGroup"]);
     __block FSTQuery *query = [FSTQuery queryWithPath:resource_path
                                       collectionGroup:collectionGroup];
     if (queryDict[@"limit"]) {

Firestore/Source/API/FIRFirestore.mm

@@ -183,7 +183,7 @@ - (FIRQuery *)collectionGroupWithID:(NSString *)collectionID {
                          collectionID);
   }
 
-  return _firestore->GetCollectionGroup(collectionID);
+  return _firestore->GetCollectionGroup(util::MakeString(collectionID));
 }
 
 - (FIRWriteBatch *)batch {

Firestore/Source/Core/FSTQuery.h

@@ -16,7 +16,10 @@
 
 #import <Foundation/Foundation.h>
 
+#include <memory>
+#include <string>
 #include <vector>
+
 #include "Firestore/core/src/firebase/firestore/core/filter.h"
 #include "Firestore/core/src/firebase/firestore/model/document_set.h"
 #include "Firestore/core/src/firebase/firestore/model/field_path.h"
@@ -166,7 +169,7 @@ NS_ASSUME_NONNULL_BEGIN
  * Initializes a query with all of its components directly.
  */
 - (instancetype)initWithPath:(model::ResourcePath)path
-             collectionGroup:(nullable NSString *)collectionGroup
+             collectionGroup:(std::shared_ptr<const std::string>)collectionGroup
                     filterBy:(NSArray<FSTFilter *> *)filters
                      orderBy:(NSArray<FSTSortOrder *> *)sortOrders
                        limit:(int32_t)limit
@@ -186,12 +189,12 @@ NS_ASSUME_NONNULL_BEGIN
  *
  * @param path The path to the location to be queried over. Must currently be
  *     empty in the case of a collection group query.
- * @param collectionGroup The collection group to be queried over. nil if this
+ * @param collectionGroup The collection group to be queried over. nullptr if this
  *     is not a collection group query.
  * @return A new instance of FSTQuery.
  */
 + (instancetype)queryWithPath:(model::ResourcePath)path
-              collectionGroup:(nullable NSString *)collectionGroup;
+              collectionGroup:(std::shared_ptr<const std::string>)collectionGroup;
 
 /**
  * Returns the list of ordering constraints that were explicitly requested on the query by the
@@ -282,7 +285,7 @@ NS_ASSUME_NONNULL_BEGIN
 - (const model::ResourcePath &)path;
 
 /** The collection group of the query. */
-@property(nonatomic, nullable, strong, readonly) NSString *collectionGroup;
+- (const std::shared_ptr<const std::string> &)collectionGroup;
 
 /** The filters on the documents returned by the query. */
 @property(nonatomic, strong, readonly) NSArray<FSTFilter *> *filters;

Firestore/Source/Core/FSTQuery.mm

@@ -35,6 +35,7 @@
 #include "Firestore/core/src/firebase/firestore/model/resource_path.h"
 #include "Firestore/core/src/firebase/firestore/objc/objc_compatibility.h"
 #include "Firestore/core/src/firebase/firestore/util/comparison.h"
+#include "Firestore/core/src/firebase/firestore/util/equality.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/hashing.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
@@ -544,8 +545,10 @@ - (instancetype)copyWithZone:(nullable NSZone *)zone {
 @interface FSTQuery () {
   // Cached value of the canonicalID property.
   NSString *_canonicalID;
-  /** The base path of the query. */
+  // The base path of the query.
   ResourcePath _path;
+  // The collection group of the query, if any.
+  std::shared_ptr<const std::string> _collectionGroup;
 }
 
 /** A list of fields given to sort by. This does not include the implicit key sort at the end. */
@@ -561,13 +564,13 @@ @implementation FSTQuery
 #pragma mark - Constructors
 
 + (instancetype)queryWithPath:(ResourcePath)path {
-  return [FSTQuery queryWithPath:std::move(path) collectionGroup:nil];
+  return [FSTQuery queryWithPath:std::move(path) collectionGroup:nullptr];
 }
 
 + (instancetype)queryWithPath:(ResourcePath)path
-              collectionGroup:(nullable NSString *)collectionGroup {
+              collectionGroup:(std::shared_ptr<const std::string>)collectionGroup {
   return [[FSTQuery alloc] initWithPath:std::move(path)
-                        collectionGroup:collectionGroup
+                        collectionGroup:std::move(collectionGroup)
                                filterBy:@[]
                                 orderBy:@[]
                                   limit:Query::kNoLimit
@@ -576,15 +579,15 @@ + (instancetype)queryWithPath:(ResourcePath)path
 }
 
 - (instancetype)initWithPath:(ResourcePath)path
-             collectionGroup:(nullable NSString *)collectionGroup
+             collectionGroup:(std::shared_ptr<const std::string>)collectionGroup
                     filterBy:(NSArray<FSTFilter *> *)filters
                      orderBy:(NSArray<FSTSortOrder *> *)sortOrders
                        limit:(int32_t)limit
                      startAt:(nullable FSTBound *)startAtBound
                        endAt:(nullable FSTBound *)endAtBound {
   if (self = [super init]) {
     _path = std::move(path);
-    _collectionGroup = collectionGroup;
+    _collectionGroup = std::move(collectionGroup);
     _filters = filters;
     _explicitSortOrders = sortOrders;
     _limit = limit;
@@ -804,6 +807,10 @@ - (nullable const FieldPath *)firstSortOrderField {
   return _path;
 }
 
+- (const std::shared_ptr<const std::string> &)collectionGroup {
+  return _collectionGroup;
+}
+
 #pragma mark - Private properties
 
 - (NSString *)canonicalID {
@@ -815,7 +822,7 @@ - (NSString *)canonicalID {
   [canonicalID appendFormat:@"%s", _path.CanonicalString().c_str()];
 
   if (self.collectionGroup) {
-    [canonicalID appendFormat:@"|cg:%@", self.collectionGroup];
+    [canonicalID appendFormat:@"|cg:%s", self.collectionGroup->c_str()];
   }
 
   // Add filters.
@@ -850,7 +857,7 @@ - (NSString *)canonicalID {
 #pragma mark - Private methods
 
 - (BOOL)isEqualToQuery:(FSTQuery *)other {
-  return self.path == other.path && objc::Equals(self.collectionGroup, other.collectionGroup) &&
+  return self.path == other.path && util::Equals(self.collectionGroup, other.collectionGroup) &&
          self.limit == other.limit && objc::Equals(self.filters, other.filters) &&
          objc::Equals(self.sortOrders, other.sortOrders) &&
          objc::Equals(self.startAt, other.startAt) && objc::Equals(self.endAt, other.endAt);
@@ -862,7 +869,7 @@ - (BOOL)pathAndCollectionGroupMatchDocument:(FSTDocument *)document {
   if (self.collectionGroup) {
     // NOTE: self.path is currently always empty since we don't expose Collection Group queries
     // rooted at a document path yet.
-    return document.key.HasCollectionId(util::MakeString(self.collectionGroup)) &&
+    return document.key.HasCollectionId(*self.collectionGroup) &&
            self.path.IsPrefixOf(documentPath);
   } else if (DocumentKey::IsDocumentKey(_path)) {
     // Exact match for document queries.

Firestore/Source/Remote/FSTSerializerBeta.mm

@@ -786,7 +786,7 @@ - (GCFSTarget_QueryTarget *)encodedQueryTarget:(FSTQuery *)query {
                 "Collection group queries should be within a document path or root.");
     queryTarget.parent = [self encodedQueryPath:path];
     GCFSStructuredQuery_CollectionSelector *from = [GCFSStructuredQuery_CollectionSelector message];
-    from.collectionId = query.collectionGroup;
+    from.collectionId = util::MakeNSString(query.collectionGroup);
     from.allDescendants = YES;
     [queryTarget.structuredQuery.fromArray addObject:from];
   } else {
@@ -827,15 +827,15 @@ - (FSTQuery *)decodedQueryFromQueryTarget:(GCFSTarget_QueryTarget *)target {
   ResourcePath path = [self decodedQueryPath:target.parent];
 
   GCFSStructuredQuery *query = target.structuredQuery;
-  NSString *collectionGroup;
+  std::shared_ptr<const std::string> collectionGroup;
   NSUInteger fromCount = query.fromArray_Count;
   if (fromCount > 0) {
     HARD_ASSERT(fromCount == 1,
                 "StructuredQuery.from with more than one collection is not supported.");
 
     GCFSStructuredQuery_CollectionSelector *from = query.fromArray[0];
     if (from.allDescendants) {
-      collectionGroup = from.collectionId;
+      collectionGroup = util::MakeStringPtr(from.collectionId);
     } else {
       path = path.Append(util::MakeString(from.collectionId));
     }

Firestore/core/src/firebase/firestore/api/firestore.h

@@ -84,7 +84,7 @@ class Firestore : public std::enable_shared_from_this<Firestore> {
   CollectionReference GetCollection(absl::string_view collection_path);
   DocumentReference GetDocument(absl::string_view document_path);
   WriteBatch GetBatch();
-  FIRQuery* GetCollectionGroup(NSString* collection_id);
+  FIRQuery* GetCollectionGroup(std::string collection_id);
 
   void RunTransaction(core::TransactionUpdateCallback update_callback,
                       core::TransactionResultCallback result_callback);

Firestore/core/src/firebase/firestore/api/firestore.mm

@@ -117,11 +117,12 @@
   return WriteBatch(shared_from_this());
 }
 
-FIRQuery* Firestore::GetCollectionGroup(NSString* collection_id) {
+FIRQuery* Firestore::GetCollectionGroup(std::string collection_id) {
   EnsureClientConfigured();
 
   FSTQuery* query = [FSTQuery queryWithPath:ResourcePath::Empty()
-                            collectionGroup:collection_id];
+                            collectionGroup:std::make_shared<const std::string>(
+                                                std::move(collection_id))];
   return [[FIRQuery alloc] initWithQuery:query firestore:shared_from_this()];
 }
 

Firestore/core/src/firebase/firestore/core/query.cc

@@ -21,6 +21,7 @@
 #include "Firestore/core/src/firebase/firestore/model/document_key.h"
 #include "Firestore/core/src/firebase/firestore/model/field_path.h"
 #include "Firestore/core/src/firebase/firestore/model/resource_path.h"
+#include "Firestore/core/src/firebase/firestore/util/equality.h"
 #include "Firestore/core/src/firebase/firestore/util/hard_assert.h"
 
 namespace firebase {
@@ -31,13 +32,46 @@ using model::Document;
 using model::DocumentKey;
 using model::ResourcePath;
 
+Query::Query(ResourcePath path, std::string collection_group)
+    : path_(std::move(path)),
+      collection_group_(
+          std::make_shared<const std::string>(std::move(collection_group))) {
+}
+
+// MARK: - Accessors
+
+bool Query::IsDocumentQuery() const {
+  return DocumentKey::IsDocumentKey(path_) && !collection_group_ &&
+         filters_.empty();
+}
+
+// MARK: - Builder methods
+
+Query Query::Filter(std::shared_ptr<core::Filter> filter) const {
+  HARD_ASSERT(!DocumentKey::IsDocumentKey(path_),
+              "No filter is allowed for document query");
+
+  // TODO(rsgowman): ensure only one inequality field
+  // TODO(rsgowman): ensure first orderby must match inequality field
+
+  auto updated_filters = filters_;
+  updated_filters.push_back(std::move(filter));
+  return Query(path_, collection_group_, std::move(updated_filters));
+}
+
+Query Query::AsCollectionQueryAtPath(ResourcePath path) const {
+  return Query(path, /*collection_group=*/nullptr, filters_);
+}
+
+// MARK: - Matching
+
 bool Query::Matches(const Document& doc) const {
   return MatchesPath(doc) && MatchesOrderBy(doc) && MatchesFilters(doc) &&
          MatchesBounds(doc);
 }
 
 bool Query::MatchesPath(const Document& doc) const {
-  ResourcePath doc_path = doc.key().path();
+  const ResourcePath& doc_path = doc.key().path();
   if (DocumentKey::IsDocumentKey(path_)) {
     return path_ == doc_path;
   } else {
@@ -62,16 +96,14 @@ bool Query::MatchesBounds(const Document&) const {
   return true;
 }
 
-Query Query::Filter(std::shared_ptr<core::Filter> filter) const {
-  HARD_ASSERT(!DocumentKey::IsDocumentKey(path_),
-              "No filter is allowed for document query");
-
-  // TODO(rsgowman): ensure only one inequality field
-  // TODO(rsgowman): ensure first orderby must match inequality field
-
-  std::vector<std::shared_ptr<core::Filter>> updated_filters = filters_;
-  updated_filters.push_back(std::move(filter));
-  return Query(path_, std::move(updated_filters));
+bool operator==(const Query& lhs, const Query& rhs) {
+  // TODO(rsgowman): check limit (once it exists)
+  // TODO(rsgowman): check orderby (once it exists)
+  // TODO(rsgowman): check startat (once it exists)
+  // TODO(rsgowman): check endat (once it exists)
+  return lhs.path() == rhs.path() &&
+         util::Equals(lhs.collection_group(), rhs.collection_group()) &&
+         lhs.filters() == rhs.filters();
 }
 
 }  // namespace core