Skip to content

Commit

Permalink
Fix: sort document reference by long type id (#6594)
Browse files Browse the repository at this point in the history
  • Loading branch information
@milaGGL
milaGGL authored Dec 17, 2024
1 parent 50eacb7 commit 3dd8ddf
Show file tree
Hide file tree
Showing 2 changed files with 190 additions and 1 deletion.
160 changes: 160 additions & 0 deletions firebase-firestore/src/androidTest/java/com/google/firebase/firestore/FirestoreTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1493,4 +1495,162 @@ public void testCanGetSameOrDifferentPersistentCacheIndexManager() {
assertNotSame(indexManager3, indexManager6);
assertNotSame(indexManager5, indexManager6);
}

@Test
public void snapshotListenerSortsQueryByDocumentIdsSameAsGetQuery() {
Map<String, Map<String, Object>> 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<String> expectedDocIds =
Arrays.asList(
"__id-9223372036854775808__",
"__id-2__",
"__id7__",
"__id12__",
"__id9223372036854775807__",
"12",
"7",
"A",
"Aa",
"__id",
"__id1_",
"_id1__",
"a");

QuerySnapshot getSnapshot = waitFor(orderedQuery.get());
List<String> getSnapshotDocIds =
getSnapshot.getDocuments().stream().map(ds -> ds.getId()).collect(Collectors.toList());

// Run query with snapshot listener
EventAccumulator<QuerySnapshot> eventAccumulator = new EventAccumulator<QuerySnapshot>();
ListenerRegistration registration =
orderedQuery.addSnapshotListener(eventAccumulator.listener());

List<String> 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<String, Map<String, Object>> 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<String> expectedDocIds =
Arrays.asList("__id12__", "__id9223372036854775807__", "12", "7", "A");

QuerySnapshot getSnapshot = waitFor(filteredQuery.get());
List<String> getSnapshotDocIds =
getSnapshot.getDocuments().stream().map(ds -> ds.getId()).collect(Collectors.toList());

// Run query with snapshot listener
EventAccumulator<QuerySnapshot> eventAccumulator = new EventAccumulator<QuerySnapshot>();
ListenerRegistration registration =
filteredQuery.addSnapshotListener(eventAccumulator.listener());

List<String> 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<String, Map<String, Object>> 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<String> 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]));
}
}
31 changes: 30 additions & 1 deletion firebase-firestore/src/main/java/com/google/firebase/firestore/model/BasePath.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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);
Expand Down

0 comments on commit 3dd8ddf

Please sign in to comment.