Fix: sort document reference by long type id (#14248)

firebase · Dec 17, 2024 · 0d569cf · 0d569cf
1 parent 3a470b0
commit 0d569cf
Show file tree

Hide file tree

Showing 2 changed files with 135 additions and 1 deletion.
diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm
@@ -804,6 +804,98 @@ - (void)testCollectionGroupQueriesWithStartAtEndAtWithArbitraryDocumentIDs {
   XCTAssertEqualObjects(ids, (@[ @"cg-doc2" ]));
 }
 
+- (void)testSnapshotListenerSortsQueryByDocumentIdInTheSameOrderAsServer {
+  FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
+    @"A" : @{@"a" : @1},
+    @"a" : @{@"a" : @1},
+    @"Aa" : @{@"a" : @1},
+    @"7" : @{@"a" : @1},
+    @"12" : @{@"a" : @1},
+    @"__id7__" : @{@"a" : @1},
+    @"__id12__" : @{@"a" : @1},
+    @"__id-2__" : @{@"a" : @1},
+    @"__id1_" : @{@"a" : @1},
+    @"_id1__" : @{@"a" : @1},
+    @"__id" : @{@"a" : @1},
+    @"__id9223372036854775807__" : @{@"a" : @1},
+    @"__id-9223372036854775808__" : @{@"a" : @1},
+  }];
+
+  FIRQuery *query = [collRef queryOrderedByFieldPath:[FIRFieldPath documentID]];
+  NSArray<NSString *> *expectedDocs = @[
+    @"__id-9223372036854775808__", @"__id-2__", @"__id7__", @"__id12__",
+    @"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id", @"__id1_", @"_id1__", @"a"
+  ];
+  FIRQuerySnapshot *getSnapshot = [self readDocumentSetForRef:query];
+  XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(getSnapshot), expectedDocs);
+
+  id<FIRListenerRegistration> registration =
+      [query addSnapshotListener:self.eventAccumulator.valueEventHandler];
+  FIRQuerySnapshot *watchSnapshot = [self.eventAccumulator awaitEventWithName:@"Snapshot"];
+  XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(watchSnapshot), expectedDocs);
+
+  [registration remove];
+}
+
+- (void)testSnapshotListenerSortsFilteredQueryByDocumentIdInTheSameOrderAsServer {
+  FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
+    @"A" : @{@"a" : @1},
+    @"a" : @{@"a" : @1},
+    @"Aa" : @{@"a" : @1},
+    @"7" : @{@"a" : @1},
+    @"12" : @{@"a" : @1},
+    @"__id7__" : @{@"a" : @1},
+    @"__id12__" : @{@"a" : @1},
+    @"__id-2__" : @{@"a" : @1},
+    @"__id1_" : @{@"a" : @1},
+    @"_id1__" : @{@"a" : @1},
+    @"__id" : @{@"a" : @1},
+    @"__id9223372036854775807__" : @{@"a" : @1},
+    @"__id-9223372036854775808__" : @{@"a" : @1},
+  }];
+
+  FIRQuery *query = [[[collRef queryWhereFieldPath:[FIRFieldPath documentID]
+                                     isGreaterThan:@"__id7__"]
+      queryWhereFieldPath:[FIRFieldPath documentID]
+      isLessThanOrEqualTo:@"A"] queryOrderedByFieldPath:[FIRFieldPath documentID]];
+  NSArray<NSString *> *expectedDocs =
+      @[ @"__id12__", @"__id9223372036854775807__", @"12", @"7", @"A" ];
+  FIRQuerySnapshot *getSnapshot = [self readDocumentSetForRef:query];
+  XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(getSnapshot), expectedDocs);
+
+  id<FIRListenerRegistration> registration =
+      [query addSnapshotListener:self.eventAccumulator.valueEventHandler];
+  FIRQuerySnapshot *watchSnapshot = [self.eventAccumulator awaitEventWithName:@"Snapshot"];
+  XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(watchSnapshot), expectedDocs);
+
+  [registration remove];
+}
+
+- (void)testSdkOrdersQueryByDocumentIdTheSameWayOnlineAndOffline {
+  FIRCollectionReference *collRef = [self collectionRefWithDocuments:@{
+    @"A" : @{@"a" : @1},
+    @"a" : @{@"a" : @1},
+    @"Aa" : @{@"a" : @1},
+    @"7" : @{@"a" : @1},
+    @"12" : @{@"a" : @1},
+    @"__id7__" : @{@"a" : @1},
+    @"__id12__" : @{@"a" : @1},
+    @"__id-2__" : @{@"a" : @1},
+    @"__id1_" : @{@"a" : @1},
+    @"_id1__" : @{@"a" : @1},
+    @"__id" : @{@"a" : @1},
+    @"__id9223372036854775807__" : @{@"a" : @1},
+    @"__id-9223372036854775808__" : @{@"a" : @1},
+  }];
+
+  [self checkOnlineAndOfflineQuery:[collRef queryOrderedByFieldPath:[FIRFieldPath documentID]]
+                     matchesResult:@[
+                       @"__id-9223372036854775808__", @"__id-2__", @"__id7__", @"__id12__",
+                       @"__id9223372036854775807__", @"12", @"7", @"A", @"Aa", @"__id", @"__id1_",
+                       @"_id1__", @"a"
+                     ]];
+}
+
 - (void)testCollectionGroupQueriesWithWhereFiltersOnArbitraryDocumentIDs {
   // Use .document() to get a random collection group name to use but ensure it starts with 'b'
   // for predictable ordering.

diff --git a/Firestore/core/src/model/base_path.h b/Firestore/core/src/model/base_path.h
@@ -150,8 +150,18 @@ class BasePath {
            std::equal(begin(), end(), potential_child.begin());
   }
 
+  /**
+   * Compares the current path against another Path object. Paths are compared
+   * segment by segment, prioritizing numeric IDs (e.g., "__id123__") in numeric
+   * ascending order, followed by string segments in lexicographical order.
+   */
   util::ComparisonResult CompareTo(const T& rhs) const {
-    return util::CompareContainer(segments_, rhs.segments_);
+    size_t min_size = std::min(size(), rhs.size());
+    for (size_t i = 0; i < min_size; ++i) {
+      auto cmp = CompareSegments(segments_[i], rhs.segments_[i]);
+      if (!util::Same(cmp)) return cmp;
+    }
+    return util::Compare(size(), rhs.size());
   }
 
   friend bool operator==(const BasePath& lhs, const BasePath& rhs) {
@@ -174,6 +184,38 @@ class BasePath {
 
  private:
   SegmentsT segments_;
+
+  static const size_t kNumericIdPrefixLength = 4;
+  static const size_t kNumericIdSuffixLength = 2;
+  static const size_t kNumericIdTotalOverhead =
+      kNumericIdPrefixLength + kNumericIdSuffixLength;
+
+  static util::ComparisonResult CompareSegments(const std::string& lhs,
+                                                const std::string& rhs) {
+    bool isLhsNumeric = IsNumericId(lhs);
+    bool isRhsNumeric = IsNumericId(rhs);
+
+    if (isLhsNumeric && !isRhsNumeric) {
+      return util::ComparisonResult::Ascending;
+    } else if (!isLhsNumeric && isRhsNumeric) {
+      return util::ComparisonResult::Descending;
+    } else if (isLhsNumeric && isRhsNumeric) {
+      return util::Compare(ExtractNumericId(lhs), ExtractNumericId(rhs));
+    } else {
+      return util::Compare(lhs, rhs);
+    }
+  }
+
+  static bool IsNumericId(const std::string& segment) {
+    return segment.size() > kNumericIdTotalOverhead &&
+           segment.substr(0, kNumericIdPrefixLength) == "__id" &&
+           segment.substr(segment.size() - kNumericIdSuffixLength) == "__";
+  }
+
+  static int64_t ExtractNumericId(const std::string& segment) {
+    return std::stol(segment.substr(kNumericIdPrefixLength,
+                                    segment.size() - kNumericIdSuffixLength));
+  }
 };
 
 }  // namespace impl