Skip to content

Commit a3a03b1

Browse files
schmidt-sebastianschmidt-sebastian
committed
Review
1 parent 4719d11 commit a3a03b1

File tree

7 files changed

+77
-95
lines changed

7 files changed

+77
-95
lines changed

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java

Lines changed: 2 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
import static com.google.firebase.firestore.model.DocumentCollections.emptyMutableDocumentMap;
1818
import static com.google.firebase.firestore.util.Assert.hardAssert;
19-
import static com.google.firebase.firestore.util.Util.trimMap;
2019

2120
import androidx.annotation.NonNull;
2221
import com.google.firebase.database.collection.ImmutableSortedMap;
@@ -28,10 +27,7 @@
2827
import com.google.firebase.firestore.model.SnapshotVersion;
2928
import java.util.HashMap;
3029
import java.util.Iterator;
31-
import java.util.List;
3230
import java.util.Map;
33-
import java.util.Set;
34-
import java.util.TreeSet;
3531

3632
/** In-memory cache of remote documents. */
3733
final class MemoryRemoteDocumentCache implements RemoteDocumentCache {
@@ -88,30 +84,8 @@ public Map<DocumentKey, MutableDocument> getAll(Iterable<DocumentKey> keys) {
8884
@Override
8985
public Map<DocumentKey, MutableDocument> getAll(
9086
String collectionGroup, IndexOffset offset, int limit) {
91-
// Note: This method is pretty inefficient, but t is not called since the method is only used
92-
// during index backfill, which is not supported by memory persistence.
93-
94-
List<ResourcePath> collectionParents = indexManager.getCollectionParents(collectionGroup);
95-
Map<DocumentKey, MutableDocument> matchingDocuments = new HashMap<>();
96-
97-
for (ResourcePath collectionParent : collectionParents) {
98-
ResourcePath documentParent = collectionParent.append(collectionGroup);
99-
Iterator<Map.Entry<DocumentKey, MutableDocument>> iterator =
100-
docs.iteratorFrom(DocumentKey.fromPath(documentParent.append("")));
101-
while (iterator.hasNext()) {
102-
Map.Entry<DocumentKey, MutableDocument> entry = iterator.next();
103-
DocumentKey key = entry.getKey();
104-
MutableDocument document = entry.getValue();
105-
if (!documentParent.isPrefixOf(key.getPath())) {
106-
break;
107-
}
108-
if (IndexOffset.fromDocument(document).compareTo(offset) > 0) {
109-
matchingDocuments.put(key, document);
110-
}
111-
}
112-
}
113-
114-
return trimMap(matchingDocuments, limit, IndexOffset.DOCUMENT_COMPARATOR);
87+
// This method should only be called from the IndexBackfiller if SQLite is enabled.
88+
throw new UnsupportedOperationException("getAll(String, IndexOffset, int) is not supported.");
11589
}
11690

11791
@Override

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@
1616

1717
import static com.google.firebase.firestore.util.Assert.fail;
1818
import static com.google.firebase.firestore.util.Assert.hardAssert;
19+
import static com.google.firebase.firestore.util.Util.firstNEntries;
1920
import static com.google.firebase.firestore.util.Util.repeatSequence;
20-
import static com.google.firebase.firestore.util.Util.trimMap;
2121

2222
import androidx.annotation.VisibleForTesting;
2323
import com.google.firebase.Timestamp;
@@ -157,7 +157,7 @@ public Map<DocumentKey, MutableDocument> getAll(
157157
getAll(
158158
collections.subList(i, Math.min(collections.size(), i + pageSize)), offset, limit));
159159
}
160-
return trimMap(results, limit, IndexOffset.DOCUMENT_COMPARATOR);
160+
return firstNEntries(results, limit, IndexOffset.DOCUMENT_COMPARATOR);
161161
}
162162
}
163163

‎firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,8 @@ public static IndexState create(
116116
/** Stores the latest read time and document that were processed for an index. */
117117
@AutoValue
118118
public abstract static class IndexOffset implements Comparable<IndexOffset> {
119-
public static final Comparator<MutableDocument> DOCUMENT_COMPARATOR = (l,r)->IndexOffset.fromDocument(l).compareTo(IndexOffset.fromDocument(r));
119+
public static final Comparator<MutableDocument> DOCUMENT_COMPARATOR =
120+
(l, r) -> IndexOffset.fromDocument(l).compareTo(IndexOffset.fromDocument(r));
120121

121122
public static final IndexOffset NONE = create(SnapshotVersion.NONE, DocumentKey.empty());
122123

‎firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -369,15 +369,15 @@ private static <T> T advanceIterator(Iterator<T> it) {
369369
return it.hasNext() ? it.next() : null;
370370
}
371371

372-
/** Returns a map with the first {#code count} elements of {#code data} when sorted by comp. */
373-
public static <K, V> Map<K, V> trimMap(Map<K, V> data, int count, Comparator<V> comp) {
374-
if (data.size() <= count) {
372+
/** Returns a map with the first {#code n} elements of {#code data} when sorted by comp. */
373+
public static <K, V> Map<K, V> firstNEntries(Map<K, V> data, int n, Comparator<V> comp) {
374+
if (data.size() <= n) {
375375
return data;
376376
} else {
377377
List<Map.Entry<K, V>> sortedVlaues = new ArrayList<>(data.entrySet());
378378
Collections.sort(sortedVlaues, (l, r) -> comp.compare(l.getValue(), r.getValue()));
379379
Map<K, V> result = new HashMap<>();
380-
for (int i = 0; i < count; ++i) {
380+
for (int i = 0; i < n; ++i) {
381381
result.put(sortedVlaues.get(i).getKey(), sortedVlaues.get(i).getValue());
382382
}
383383
return result;

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java

Lines changed: 3 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ abstract class RemoteDocumentCacheTestCase {
5757
private final Map<String, Object> DOC_DATA = map("data", 2);
5858

5959
private Persistence persistence;
60-
private RemoteDocumentCache remoteDocumentCache;
60+
protected RemoteDocumentCache remoteDocumentCache;
6161

6262
@Before
6363
public void setUp() {
@@ -246,57 +246,6 @@ public void testDocumentsMatchingUsesReadTimeNotUpdateTime() {
246246
assertThat(values(results)).containsExactly(doc("b/old", 1, DOC_DATA));
247247
}
248248

249-
@Test
250-
public void testNextDocumentsFromCollectionGroup() {
251-
addTestDocumentAtPath("a/1");
252-
addTestDocumentAtPath("a/2");
253-
addTestDocumentAtPath("b/3");
254-
255-
Map<DocumentKey, MutableDocument> results =
256-
remoteDocumentCache.getAll("a", IndexOffset.NONE, Integer.MAX_VALUE);
257-
assertThat(results.keySet()).containsExactly(key("a/1"), key("a/2"));
258-
}
259-
260-
@Test
261-
public void testNextDocumentsFromCollectionGroupWithLimit() {
262-
addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11);
263-
addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 1, /* readTime= */ 12);
264-
addTestDocumentAtPath("a/3", /* updateTime= */ 1, /* readTime= */ 13);
265-
266-
Map<DocumentKey, MutableDocument> results =
267-
remoteDocumentCache.getAll("a", IndexOffset.NONE, 2);
268-
assertThat(results.keySet()).containsExactly(key("a/1"), key("b/2/a/2"));
269-
}
270-
271-
@Test
272-
public void testNextDocumentsFromCollectionGroupWithOffset() {
273-
addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11);
274-
addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 2, /* readTime= = */ 12);
275-
addTestDocumentAtPath("a/3", /* updateTime= */ 3, /* readTime= = */ 13);
276-
277-
Map<DocumentKey, MutableDocument> results =
278-
remoteDocumentCache.getAll("a", IndexOffset.create(version(11)), 2);
279-
assertThat(results.keySet()).containsExactly(key("b/2/a/2"), key("a/3"));
280-
}
281-
282-
@Test
283-
public void testNextDocumentsForNonExistingCollectionGroup() {
284-
Map<DocumentKey, MutableDocument> results =
285-
remoteDocumentCache.getAll("a", IndexOffset.create(version(11)), 2);
286-
assertThat(results).isEmpty();
287-
}
288-
289-
@Test
290-
public void testNextDocumentsForLargeCollectionGroup() {
291-
int size = 999 / SQLiteRemoteDocumentCache.BINDS_PER_STATEMENT + 1;
292-
for (int i = 0; i < size; ++i) {
293-
addTestDocumentAtPath("a/" + i + "/b/doc");
294-
}
295-
Map<DocumentKey, MutableDocument> results =
296-
remoteDocumentCache.getAll("b", IndexOffset.NONE, size);
297-
assertThat(results).hasSize(size);
298-
}
299-
300249
@Test
301250
public void testLatestReadTime() {
302251
SnapshotVersion latestReadTime = remoteDocumentCache.getLatestReadTime();
@@ -315,11 +264,11 @@ public void testLatestReadTime() {
315264
assertEquals(version(3), latestReadTime);
316265
}
317266

318-
private MutableDocument addTestDocumentAtPath(String path) {
267+
protected MutableDocument addTestDocumentAtPath(String path) {
319268
return addTestDocumentAtPath(path, 42, 42);
320269
}
321270

322-
private MutableDocument addTestDocumentAtPath(String path, int updateTime, int readTime) {
271+
protected MutableDocument addTestDocumentAtPath(String path, int updateTime, int readTime) {
323272
MutableDocument doc = doc(path, updateTime, map("data", 2));
324273
add(doc, version(readTime));
325274
return doc;

‎firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCacheTest.java

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,15 @@
1414

1515
package com.google.firebase.firestore.local;
1616

17+
import static com.google.common.truth.Truth.assertThat;
18+
import static com.google.firebase.firestore.testutil.TestUtil.key;
19+
import static com.google.firebase.firestore.testutil.TestUtil.version;
20+
21+
import com.google.firebase.firestore.model.DocumentKey;
22+
import com.google.firebase.firestore.model.FieldIndex;
23+
import com.google.firebase.firestore.model.MutableDocument;
24+
import java.util.Map;
25+
import org.junit.Test;
1726
import org.junit.runner.RunWith;
1827
import org.robolectric.RobolectricTestRunner;
1928
import org.robolectric.annotation.Config;
@@ -26,4 +35,55 @@ public final class SQLiteRemoteDocumentCacheTest extends RemoteDocumentCacheTest
2635
Persistence getPersistence() {
2736
return PersistenceTestHelpers.createSQLitePersistence();
2837
}
38+
39+
@Test
40+
public void testNextDocumentsFromCollectionGroup() {
41+
addTestDocumentAtPath("a/1");
42+
addTestDocumentAtPath("a/2");
43+
addTestDocumentAtPath("b/3");
44+
45+
Map<DocumentKey, MutableDocument> results =
46+
remoteDocumentCache.getAll("a", FieldIndex.IndexOffset.NONE, Integer.MAX_VALUE);
47+
assertThat(results.keySet()).containsExactly(key("a/1"), key("a/2"));
48+
}
49+
50+
@Test
51+
public void testNextDocumentsFromCollectionGroupWithLimit() {
52+
addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11);
53+
addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 1, /* readTime= */ 12);
54+
addTestDocumentAtPath("a/3", /* updateTime= */ 1, /* readTime= */ 13);
55+
56+
Map<DocumentKey, MutableDocument> results =
57+
remoteDocumentCache.getAll("a", FieldIndex.IndexOffset.NONE, 2);
58+
assertThat(results.keySet()).containsExactly(key("a/1"), key("b/2/a/2"));
59+
}
60+
61+
@Test
62+
public void testNextDocumentsFromCollectionGroupWithOffset() {
63+
addTestDocumentAtPath("a/1", /* updateTime= */ 1, /* readTime= */ 11);
64+
addTestDocumentAtPath("b/2/a/2", /* updateTime= */ 2, /* readTime= = */ 12);
65+
addTestDocumentAtPath("a/3", /* updateTime= */ 3, /* readTime= = */ 13);
66+
67+
Map<DocumentKey, MutableDocument> results =
68+
remoteDocumentCache.getAll("a", FieldIndex.IndexOffset.create(version(11)), 2);
69+
assertThat(results.keySet()).containsExactly(key("b/2/a/2"), key("a/3"));
70+
}
71+
72+
@Test
73+
public void testNextDocumentsFromNonExistingCollectionGroup() {
74+
Map<DocumentKey, MutableDocument> results =
75+
remoteDocumentCache.getAll("a", FieldIndex.IndexOffset.create(version(11)), 2);
76+
assertThat(results).isEmpty();
77+
}
78+
79+
@Test
80+
public void testNextDocumentsForLargeCollectionGroup() {
81+
int size = 999 / SQLiteRemoteDocumentCache.BINDS_PER_STATEMENT + 1;
82+
for (int i = 0; i < size; ++i) {
83+
addTestDocumentAtPath("a/" + i + "/b/doc");
84+
}
85+
Map<DocumentKey, MutableDocument> results =
86+
remoteDocumentCache.getAll("b", FieldIndex.IndexOffset.NONE, size);
87+
assertThat(results).hasSize(size);
88+
}
2989
}

‎firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
package com.google.firebase.firestore.util;
1616

1717
import static com.google.common.truth.Truth.assertThat;
18-
import static com.google.firebase.firestore.util.Util.trimMap;
18+
import static com.google.firebase.firestore.util.Util.firstNEntries;
1919
import static org.junit.Assert.assertEquals;
2020

2121
import com.google.firebase.firestore.testutil.TestUtil;
@@ -26,7 +26,6 @@
2626
import java.util.HashMap;
2727
import java.util.List;
2828
import java.util.Map;
29-
3029
import org.junit.Test;
3130
import org.junit.runner.RunWith;
3231
import org.junit.runners.JUnit4;
@@ -73,15 +72,14 @@ public void testDiffCollectionsWithEmptyLists() {
7372
validateDiffCollection(Collections.emptyList(), Collections.emptyList());
7473
}
7574

76-
7775
@Test
78-
public void testTrimMap() {
76+
public void testFirstNEntries() {
7977
Map<Integer, Integer> data = new HashMap<>();
8078
data.put(1, 1);
8179
data.put(3, 3);
8280
data.put(2, 2);
83-
data = trimMap(data, 2, Integer::compare);
84-
assertThat(data).containsExactly(1, 1, 2,2);
81+
data = firstNEntries(data, 2, Integer::compare);
82+
assertThat(data).containsExactly(1, 1, 2, 2);
8583
}
8684

8785
private void validateDiffCollection(List<String> before, List<String> after) {

0 commit comments

Comments
 (0)