Skip to content

Commit 664f6e3

Browse files
schmidt-sebastianschmidt-sebastian
committed
Addressed feedback
1 parent d65656b commit 664f6e3

File tree

3 files changed

+100
-123
lines changed

3 files changed

+100
-123
lines changed

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,11 @@ public boolean isCollectionGroupQuery() {
121121
* Returns true if this query does not specify any query constraints that could remove results.
122122
*/
123123
public boolean matchesAllDocuments() {
124-
return filters.isEmpty() && limit == NO_LIMIT && startAt == null && endAt == null;
124+
return filters.isEmpty()
125+
&& getExplicitOrderBy().isEmpty()
126+
&& limit == NO_LIMIT
127+
&& startAt == null
128+
&& endAt == null;
125129
}
126130

127131
/** The filters on the documents returned by the query. */

‎firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -538,6 +538,9 @@ public void testMatchesAllDocuments() {
538538
Query query = baseQuery.filter(filter("foo", "==", "bar"));
539539
assertFalse(query.matchesAllDocuments());
540540

541+
query = baseQuery.orderBy(orderBy("foo"));
542+
assertFalse(query.matchesAllDocuments());
543+
541544
query = baseQuery.limit(1);
542545
assertFalse(query.matchesAllDocuments());
543546

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

Lines changed: 92 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -15,36 +15,28 @@
1515
package com.google.firebase.firestore.local;
1616

1717
import static com.google.firebase.firestore.testutil.TestUtil.doc;
18+
import static com.google.firebase.firestore.testutil.TestUtil.docSet;
1819
import static com.google.firebase.firestore.testutil.TestUtil.filter;
1920
import static com.google.firebase.firestore.testutil.TestUtil.map;
2021
import static com.google.firebase.firestore.testutil.TestUtil.orderBy;
2122
import static com.google.firebase.firestore.testutil.TestUtil.query;
22-
import static com.google.firebase.firestore.testutil.TestUtil.values;
2323
import static com.google.firebase.firestore.testutil.TestUtil.version;
24-
import static java.util.Arrays.asList;
2524
import static org.junit.Assert.assertEquals;
26-
import static org.mockito.ArgumentMatchers.any;
27-
import static org.mockito.ArgumentMatchers.anyInt;
28-
import static org.mockito.Mockito.doAnswer;
2925

3026
import com.google.firebase.database.collection.ImmutableSortedMap;
3127
import com.google.firebase.database.collection.ImmutableSortedSet;
28+
import com.google.firebase.firestore.auth.User;
3229
import com.google.firebase.firestore.core.Query;
30+
import com.google.firebase.firestore.core.View;
3331
import com.google.firebase.firestore.model.Document;
34-
import com.google.firebase.firestore.model.DocumentCollections;
3532
import com.google.firebase.firestore.model.DocumentKey;
36-
import com.google.firebase.firestore.model.MaybeDocument;
33+
import com.google.firebase.firestore.model.DocumentSet;
3734
import com.google.firebase.firestore.model.SnapshotVersion;
3835
import com.google.protobuf.ByteString;
39-
import java.util.ArrayList;
40-
import java.util.HashMap;
41-
import java.util.Map;
42-
import org.junit.Assert;
36+
import java.util.Collections;
4337
import org.junit.Before;
4438
import org.junit.Test;
4539
import org.junit.runner.RunWith;
46-
import org.mockito.Mock;
47-
import org.mockito.MockitoAnnotations;
4840
import org.robolectric.RobolectricTestRunner;
4941
import org.robolectric.annotation.Config;
5042

@@ -60,6 +52,8 @@ public class IndexFreeQueryEngineTest {
6052
doc("coll/a", 1, map("matches", false, "order", 1), Document.DocumentState.SYNCED);
6153
private static final Document PENDING_MATCHING_DOC_A =
6254
doc("coll/a", 1, map("matches", true, "order", 1), Document.DocumentState.LOCAL_MUTATIONS);
55+
private static final Document PENDING_NON_MATCHING_DOC_A =
56+
doc("coll/a", 1, map("matches", false, "order", 1), Document.DocumentState.LOCAL_MUTATIONS);
6357
private static final Document UDPATED_DOC_A =
6458
doc("coll/a", 11, map("matches", true, "order", 1), Document.DocumentState.SYNCED);
6559
private static final Document MATCHING_DOC_B =
@@ -69,203 +63,179 @@ public class IndexFreeQueryEngineTest {
6963
private static final Document UPDATED_MATCHING_DOC_B =
7064
doc("coll/b", 11, map("matches", true, "order", 2), Document.DocumentState.SYNCED);
7165

72-
@Mock private LocalDocumentsView localDocumentsView;
73-
@Mock private QueryCache queryCache;
66+
private MemoryPersistence persistence;
67+
private MemoryRemoteDocumentCache remoteDocumentCache;
68+
private QueryCache queryCache;
7469
private QueryEngine queryEngine;
7570

76-
private final Map<DocumentKey, Document> existingQueryResults = new HashMap<>();
77-
private final Map<DocumentKey, Document> updatedQueryResults = new HashMap<>();
7871
private boolean expectIndexFreeExecution;
7972

8073
@Before
8174
public void setUp() {
82-
MockitoAnnotations.initMocks(this);
83-
8475
expectIndexFreeExecution = false;
85-
existingQueryResults.clear();
86-
updatedQueryResults.clear();
8776

77+
persistence = MemoryPersistence.createEagerGcMemoryPersistence();
78+
queryCache = new MemoryQueryCache(persistence);
8879
queryEngine = new IndexFreeQueryEngine();
89-
queryEngine.setLocalDocumentsView(localDocumentsView);
90-
91-
doAnswer(
92-
getMatchingKeysForTargetIdInvocation -> {
93-
int targetId = getMatchingKeysForTargetIdInvocation.getArgument(0);
94-
Assert.assertEquals(TEST_TARGET_ID, targetId);
95-
return existingQueryResults.keySet();
96-
})
97-
.when(queryCache)
98-
.getMatchingKeysForTargetId(anyInt());
99-
100-
doAnswer(
101-
getDocumentsInvocation -> {
102-
Iterable<DocumentKey> keys = getDocumentsInvocation.getArgument(0);
103-
104-
ImmutableSortedMap<DocumentKey, MaybeDocument> docs =
105-
DocumentCollections.emptyMaybeDocumentMap();
106-
for (DocumentKey key : keys) {
107-
docs = docs.insert(key, existingQueryResults.get(key));
108-
}
109-
110-
return docs;
111-
})
112-
.when(localDocumentsView)
113-
.getDocuments(any());
114-
115-
doAnswer(
116-
getDocumentsMatchingQueryInvocation -> {
117-
Query query = getDocumentsMatchingQueryInvocation.getArgument(0);
118-
SnapshotVersion snapshotVersion = getDocumentsMatchingQueryInvocation.getArgument(1);
119-
120-
assertEquals(
121-
"Observed query execution mode did not match expectation",
122-
expectIndexFreeExecution,
123-
!SnapshotVersion.NONE.equals(snapshotVersion));
124-
125-
ImmutableSortedMap<DocumentKey, MaybeDocument> matchingDocs =
126-
DocumentCollections.emptyMaybeDocumentMap();
127-
128-
for (Document doc : updatedQueryResults.values()) {
129-
if (query.matches(doc)) {
130-
matchingDocs = matchingDocs.insert(doc.getKey(), doc);
131-
}
132-
}
133-
134-
return matchingDocs;
135-
})
136-
.when(localDocumentsView)
137-
.getDocumentsMatchingQuery(any(), any());
80+
81+
remoteDocumentCache = persistence.getRemoteDocumentCache();
82+
83+
LocalDocumentsView localDocuments =
84+
new LocalDocumentsView(
85+
remoteDocumentCache,
86+
persistence.getMutationQueue(User.UNAUTHENTICATED),
87+
new MemoryIndexManager()) {
88+
@Override
89+
public ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingQuery(
90+
Query query, SnapshotVersion sinceReadTime) {
91+
assertEquals(
92+
"Observed query execution mode did not match expectation",
93+
expectIndexFreeExecution,
94+
!SnapshotVersion.NONE.equals(sinceReadTime));
95+
return super.getDocumentsMatchingQuery(query, sinceReadTime);
96+
}
97+
};
98+
queryEngine.setLocalDocumentsView(localDocuments);
13899
}
139100

140101
/** Add a document to local cache and the remote key mapping. */
141-
private void addExistingResult(Document doc) {
142-
existingQueryResults.put(doc.getKey(), doc);
102+
private void addDocumentToRemoteResult(Document doc) {
103+
persistence.runTransaction(
104+
"addDocumentToRemoteResult",
105+
() -> {
106+
queryCache.addMatchingKeys(
107+
DocumentKey.emptyKeySet().insert(doc.getKey()), TEST_TARGET_ID);
108+
remoteDocumentCache.add(doc, doc.getVersion());
109+
});
143110
}
144111

145112
/** Add a document to local cache but not the remote key mapping. */
146-
private void addUpdatedResult(Document doc) {
147-
updatedQueryResults.put(doc.getKey(), doc);
113+
private void addDocumentToLocalResult(Document doc) {
114+
remoteDocumentCache.add(doc, doc.getVersion());
148115
}
149116

150-
private ImmutableSortedMap<DocumentKey, Document> runQuery(
151-
Query query, QueryData queryData, boolean expectIndexFree) {
117+
private DocumentSet runQuery(Query query, QueryData queryData, boolean expectIndexFree) {
152118
expectIndexFreeExecution = expectIndexFree;
153-
ImmutableSortedSet<DocumentKey> remoteKeys =
154-
new ImmutableSortedSet<>(
155-
new ArrayList<>(existingQueryResults.keySet()), DocumentKey.comparator());
156-
return queryEngine.getDocumentsMatchingQuery(query, queryData, remoteKeys);
119+
ImmutableSortedMap<DocumentKey, Document> docs =
120+
queryEngine.getDocumentsMatchingQuery(
121+
query, queryData, queryCache.getMatchingKeysForTargetId(TEST_TARGET_ID));
122+
View view =
123+
new View(query, new ImmutableSortedSet<>(Collections.emptyList(), DocumentKey::compareTo));
124+
View.DocumentChanges viewDocChanges = view.computeDocChanges(docs);
125+
return view.applyChanges(viewDocChanges).getSnapshot().getDocuments();
157126
}
158127

159128
@Test
160129
public void usesTargetMappingForInitialView() {
161130
Query query = query("coll").filter(filter("matches", "==", true));
162131
QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true);
163132

164-
addExistingResult(MATCHING_DOC_A);
165-
addExistingResult(MATCHING_DOC_B);
133+
addDocumentToRemoteResult(MATCHING_DOC_A);
134+
addDocumentToRemoteResult(MATCHING_DOC_B);
166135

167-
ImmutableSortedMap<DocumentKey, Document> docs =
168-
runQuery(query, queryData, /* expectIndexFree= */ true);
169-
assertEquals(asList(MATCHING_DOC_A, MATCHING_DOC_B), values(docs));
136+
DocumentSet docs = runQuery(query, queryData, /* expectIndexFree= */ true);
137+
assertEquals(docSet(query.comparator(), MATCHING_DOC_A, MATCHING_DOC_B), docs);
170138
}
171139

172140
@Test
173141
public void filtersNonMatchingInitialResults() {
174142
Query query = query("coll").filter(filter("matches", "==", true));
175143
QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true);
176144

177-
addExistingResult(NON_MATCHING_DOC_A);
178-
addExistingResult(MATCHING_DOC_B);
145+
addDocumentToRemoteResult(MATCHING_DOC_A);
146+
addDocumentToRemoteResult(MATCHING_DOC_B);
179147

180-
ImmutableSortedMap<DocumentKey, Document> docs =
181-
runQuery(query, queryData, /* expectIndexFree= */ true);
182-
assertEquals(asList(MATCHING_DOC_B), values(docs));
148+
addDocumentToLocalResult(PENDING_NON_MATCHING_DOC_A);
149+
150+
DocumentSet docs = runQuery(query, queryData, /* expectIndexFree= */ true);
151+
assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs);
183152
}
184153

185154
@Test
186155
public void includesChangesSinceInitialResults() {
187156
Query query = query("coll").filter(filter("matches", "==", true));
188157
QueryData originalQueryData = queryData(query, /* hasLimboFreeSnapshot= */ true);
189158

190-
addExistingResult(MATCHING_DOC_A);
191-
addExistingResult(NON_MATCHING_DOC_B);
159+
addDocumentToRemoteResult(MATCHING_DOC_A);
160+
addDocumentToRemoteResult(NON_MATCHING_DOC_B);
192161

193-
ImmutableSortedMap<DocumentKey, Document> docs =
194-
runQuery(query, originalQueryData, /* expectIndexFree= */ true);
195-
assertEquals(asList(MATCHING_DOC_A), values(docs));
162+
DocumentSet docs = runQuery(query, originalQueryData, /* expectIndexFree= */ true);
163+
assertEquals(docSet(query.comparator(), MATCHING_DOC_A), docs);
196164

197-
addUpdatedResult(UPDATED_MATCHING_DOC_B);
165+
addDocumentToLocalResult(UPDATED_MATCHING_DOC_B);
198166

199167
docs = runQuery(query, originalQueryData, /* expectIndexFree= */ true);
200-
assertEquals(asList(MATCHING_DOC_A, UPDATED_MATCHING_DOC_B), values(docs));
168+
assertEquals(docSet(query.comparator(), MATCHING_DOC_A, UPDATED_MATCHING_DOC_B), docs);
201169
}
202170

203171
@Test
204172
public void doesNotUseInitialResultsWithoutLimboFreeSnapshotVersion() {
205173
Query query = query("coll").filter(filter("matches", "==", true));
206174
QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ false);
207175

208-
ImmutableSortedMap<DocumentKey, Document> docs =
209-
runQuery(query, queryData, /* expectIndexFree= */ false);
210-
assertEquals(asList(), values(docs));
176+
DocumentSet docs = runQuery(query, queryData, /* expectIndexFree= */ false);
177+
assertEquals(docSet(query.comparator()), docs);
211178
}
212179

213180
@Test
214181
public void doesNotUseInitialResultsForUnfilteredCollectionQuery() {
215182
Query query = query("coll");
216183
QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true);
217184

218-
ImmutableSortedMap<DocumentKey, Document> docs =
219-
runQuery(query, queryData, /* expectIndexFree= */ false);
220-
assertEquals(asList(), values(docs));
185+
DocumentSet docs = runQuery(query, queryData, /* expectIndexFree= */ false);
186+
assertEquals(docSet(query.comparator()), docs);
221187
}
222188

223189
@Test
224190
public void doesNotUseInitialResultsForLimitQueryWithDocumentRemoval() {
225-
Query query =
226-
query("coll").filter(filter("matches", "==", true)).orderBy(orderBy("order")).limit(1);
191+
Query query = query("coll").filter(filter("matches", "==", true)).limit(1);
227192

228-
addExistingResult(NON_MATCHING_DOC_A);
193+
addDocumentToRemoteResult(NON_MATCHING_DOC_A);
229194
QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true);
230-
addUpdatedResult(MATCHING_DOC_B);
195+
addDocumentToLocalResult(MATCHING_DOC_B);
231196

232-
ImmutableSortedMap<DocumentKey, Document> docs =
233-
runQuery(query, queryData, /* expectIndexFree= */ false);
234-
assertEquals(asList(MATCHING_DOC_B), values(docs));
197+
DocumentSet docs = runQuery(query, queryData, /* expectIndexFree= */ false);
198+
assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs);
235199
}
236200

237201
@Test
238202
public void doesNotUseInitialResultsForLimitQueryWithPendingWrite() {
239-
Query query = query("coll").filter(filter("matches", "==", true)).limit(1);
203+
Query query =
204+
query("coll")
205+
.filter(filter("matches", "==", true))
206+
.orderBy(orderBy("order", "desc"))
207+
.limit(1);
240208

241209
// Add a query mapping for a document that matches, but that sorts below another document due to
242210
// a pending write.
243-
addExistingResult(PENDING_MATCHING_DOC_A);
211+
addDocumentToRemoteResult(PENDING_MATCHING_DOC_A);
244212

245213
QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true);
246214

247-
addUpdatedResult(MATCHING_DOC_B);
215+
addDocumentToLocalResult(MATCHING_DOC_B);
248216

249-
ImmutableSortedMap<DocumentKey, Document> docs =
250-
runQuery(query, queryData, /* expectIndexFree= */ false);
251-
assertEquals(asList(MATCHING_DOC_B), values(docs));
217+
DocumentSet docs = runQuery(query, queryData, /* expectIndexFree= */ false);
218+
assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs);
252219
}
253220

254221
@Test
255222
public void doesNotUseInitialResultsForLimitQueryWithDocumentThatHasBeenUpdatedOutOfBand() {
256-
Query query = query("coll").filter(filter("matches", "==", true)).limit(1);
223+
Query query =
224+
query("coll")
225+
.filter(filter("matches", "==", true))
226+
.orderBy(orderBy("order", "desc"))
227+
.limit(1);
257228

258229
// Add a query mapping for a document that matches, but that sorts below another document based
259230
// due to an update that the SDK received after the query's snapshot was persisted.
260-
addExistingResult(UDPATED_DOC_A);
231+
addDocumentToRemoteResult(UDPATED_DOC_A);
261232

262233
QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true);
263234

264-
addUpdatedResult(MATCHING_DOC_B);
235+
addDocumentToLocalResult(MATCHING_DOC_B);
265236

266-
ImmutableSortedMap<DocumentKey, Document> docs =
267-
runQuery(query, queryData, /* expectIndexFree= */ false);
268-
assertEquals(asList(MATCHING_DOC_B), values(docs));
237+
DocumentSet docs = runQuery(query, queryData, /* expectIndexFree= */ false);
238+
assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs);
269239
}
270240

271241
private QueryData queryData(Query query, boolean hasLimboFreeSnapshot) {

0 commit comments

Comments
 (0)