From 3dd8ddfa3a710ec4e2af6ad7033bfca14ec03d48 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Tue, 17 Dec 2024 11:12:09 -0500 Subject: [PATCH] Fix: sort document reference by long type id (#6594) --- .../firebase/firestore/FirestoreTest.java | 160 ++++++++++++++++++ .../firebase/firestore/model/BasePath.java | 31 +++- 2 files changed, 190 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java index 958e5b58c15..f975b637789 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java @@ -15,6 +15,7 @@ package com.google.firebase.firestore; import static com.google.firebase.firestore.AccessHelper.getAsyncQueue; +import static com.google.firebase.firestore.testutil.IntegrationTestUtil.checkOnlineAndOfflineResultsMatch; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.isRunningAgainstEmulator; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.newTestSettings; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.provider; @@ -63,6 +64,7 @@ import java.util.Map; import java.util.concurrent.CountDownLatch; import java.util.concurrent.Semaphore; +import java.util.stream.Collectors; import org.junit.After; import org.junit.Test; import org.junit.runner.RunWith; @@ -1493,4 +1495,162 @@ public void testCanGetSameOrDifferentPersistentCacheIndexManager() { assertNotSame(indexManager3, indexManager6); assertNotSame(indexManager5, indexManager6); } + + @Test + public void snapshotListenerSortsQueryByDocumentIdsSameAsGetQuery() { + Map> testDocs = + map( + "A", map("a", 1), + "a", map("a", 1), + "Aa", map("a", 1), + "7", map("a", 1), + "12", map("a", 1), + "__id7__", map("a", 1), + "__id12__", map("a", 1), + "__id-2__", map("a", 1), + "__id1_", map("a", 1), + "_id1__", map("a", 1), + "__id", map("a", 1), + "__id9223372036854775807__", map("a", 1), + "__id-9223372036854775808__", map("a", 1)); + + CollectionReference colRef = testCollectionWithDocs(testDocs); + + // Run get query + Query orderedQuery = colRef.orderBy(FieldPath.documentId()); + List expectedDocIds = + Arrays.asList( + "__id-9223372036854775808__", + "__id-2__", + "__id7__", + "__id12__", + "__id9223372036854775807__", + "12", + "7", + "A", + "Aa", + "__id", + "__id1_", + "_id1__", + "a"); + + QuerySnapshot getSnapshot = waitFor(orderedQuery.get()); + List getSnapshotDocIds = + getSnapshot.getDocuments().stream().map(ds -> ds.getId()).collect(Collectors.toList()); + + // Run query with snapshot listener + EventAccumulator eventAccumulator = new EventAccumulator(); + ListenerRegistration registration = + orderedQuery.addSnapshotListener(eventAccumulator.listener()); + + List watchSnapshotDocIds = new ArrayList<>(); + try { + QuerySnapshot watchSnapshot = eventAccumulator.await(); + watchSnapshotDocIds = + watchSnapshot.getDocuments().stream() + .map(documentSnapshot -> documentSnapshot.getId()) + .collect(Collectors.toList()); + } finally { + registration.remove(); + } + + // Assert that get and snapshot listener requests sort docs in the same, expected order + assertTrue(getSnapshotDocIds.equals(expectedDocIds)); + assertTrue(watchSnapshotDocIds.equals(expectedDocIds)); + } + + @Test + public void snapshotListenerSortsFilteredQueryByDocumentIdsSameAsGetQuery() { + Map> testDocs = + map( + "A", map("a", 1), + "a", map("a", 1), + "Aa", map("a", 1), + "7", map("a", 1), + "12", map("a", 1), + "__id7__", map("a", 1), + "__id12__", map("a", 1), + "__id-2__", map("a", 1), + "__id1_", map("a", 1), + "_id1__", map("a", 1), + "__id", map("a", 1), + "__id9223372036854775807__", map("a", 1), + "__id-9223372036854775808__", map("a", 1)); + + CollectionReference colRef = testCollectionWithDocs(testDocs); + + // Run get query + Query filteredQuery = + colRef + .whereGreaterThan(FieldPath.documentId(), "__id7__") + .whereLessThanOrEqualTo(FieldPath.documentId(), "A") + .orderBy(FieldPath.documentId()); + List expectedDocIds = + Arrays.asList("__id12__", "__id9223372036854775807__", "12", "7", "A"); + + QuerySnapshot getSnapshot = waitFor(filteredQuery.get()); + List getSnapshotDocIds = + getSnapshot.getDocuments().stream().map(ds -> ds.getId()).collect(Collectors.toList()); + + // Run query with snapshot listener + EventAccumulator eventAccumulator = new EventAccumulator(); + ListenerRegistration registration = + filteredQuery.addSnapshotListener(eventAccumulator.listener()); + + List watchSnapshotDocIds = new ArrayList<>(); + try { + QuerySnapshot watchSnapshot = eventAccumulator.await(); + watchSnapshotDocIds = + watchSnapshot.getDocuments().stream() + .map(documentSnapshot -> documentSnapshot.getId()) + .collect(Collectors.toList()); + } finally { + registration.remove(); + } + + // Assert that get and snapshot listener requests sort docs in the same, expected order + assertTrue(getSnapshotDocIds.equals(expectedDocIds)); + assertTrue(watchSnapshotDocIds.equals(expectedDocIds)); + } + + @Test + public void sdkOrdersQueryByDocumentIdTheSameWayOnlineAndOffline() { + Map> testDocs = + map( + "A", map("a", 1), + "a", map("a", 1), + "Aa", map("a", 1), + "7", map("a", 1), + "12", map("a", 1), + "__id7__", map("a", 1), + "__id12__", map("a", 1), + "__id-2__", map("a", 1), + "__id1_", map("a", 1), + "_id1__", map("a", 1), + "__id", map("a", 1), + "__id9223372036854775807__", map("a", 1), + "__id-9223372036854775808__", map("a", 1)); + + CollectionReference colRef = testCollectionWithDocs(testDocs); + // Test query + Query orderedQuery = colRef.orderBy(FieldPath.documentId()); + List expectedDocIds = + Arrays.asList( + "__id-9223372036854775808__", + "__id-2__", + "__id7__", + "__id12__", + "__id9223372036854775807__", + "12", + "7", + "A", + "Aa", + "__id", + "__id1_", + "_id1__", + "a"); + + // Run query with snapshot listener + checkOnlineAndOfflineResultsMatch(orderedQuery, expectedDocIds.toArray(new String[0])); + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java index 67ad87f093e..58fedecd7ad 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java @@ -83,13 +83,18 @@ public B keepFirst(int count) { return createPathWithSegments(segments.subList(0, count)); } + /** + * Compare 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. + */ @Override public int compareTo(@NonNull B o) { int i = 0; int myLength = length(); int theirLength = o.length(); while (i < myLength && i < theirLength) { - int localCompare = getSegment(i).compareTo(o.getSegment(i)); + int localCompare = compareSegments(getSegment(i), o.getSegment(i)); if (localCompare != 0) { return localCompare; } @@ -98,6 +103,30 @@ public int compareTo(@NonNull B o) { return Util.compareIntegers(myLength, theirLength); } + private static int compareSegments(String lhs, String rhs) { + boolean isLhsNumeric = isNumericId(lhs); + boolean isRhsNumeric = isNumericId(rhs); + + if (isLhsNumeric && !isRhsNumeric) { // Only lhs is numeric + return -1; + } else if (!isLhsNumeric && isRhsNumeric) { // Only rhs is numeric + return 1; + } else if (isLhsNumeric && isRhsNumeric) { // both numeric + return Long.compare(extractNumericId(lhs), extractNumericId(rhs)); + } else { // both string + return lhs.compareTo(rhs); + } + } + + /** Checks if a segment is a numeric ID (starts with "__id" and ends with "__"). */ + private static boolean isNumericId(String segment) { + return segment.startsWith("__id") && segment.endsWith("__"); + } + + private static long extractNumericId(String segment) { + return Long.parseLong(segment.substring(4, segment.length() - 2)); + } + /** @return Returns the last segment of the path */ public String getLastSegment() { return segments.get(length() - 1);