From 8f6c8ccacae067282b07e677755d8be389953be1 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 23 Oct 2018 10:59:42 -0700 Subject: [PATCH 1/3] Port structural changes from Multi-Tab --- .../firestore/core/FirestoreClient.java | 1 - .../firestore/core/QueryListener.java | 17 +--- .../firebase/firestore/core/SyncEngine.java | 55 ++++++++----- .../firestore/core/TargetIdGenerator.java | 81 ++++++++++--------- .../firebase/firestore/core/ViewSnapshot.java | 22 +++++ .../firebase/firestore/local/LocalStore.java | 2 +- .../firestore/local/MemoryMutationQueue.java | 5 -- .../firestore/local/MutationQueue.java | 9 --- .../firestore/local/SQLiteMutationQueue.java | 5 -- .../firestore/core/TargetIdGeneratorTest.java | 36 ++------- .../firebase/firestore/spec/SpecTestCase.java | 1 - 11 files changed, 111 insertions(+), 123 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java index 0a4cf8fdc8e..0f68cebde54 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java @@ -248,7 +248,6 @@ public void handleRejectedWrite(int batchId, Status error) { @Override public void handleOnlineStateChange(OnlineState onlineState) { syncEngine.handleOnlineStateChange(onlineState); - eventManager.handleOnlineStateChange(onlineState); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java index a0074874414..f495f5c072a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java @@ -19,8 +19,6 @@ import com.google.firebase.firestore.EventListener; import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.core.DocumentViewChange.Type; -import com.google.firebase.firestore.model.Document; -import com.google.firebase.firestore.model.DocumentSet; import java.util.ArrayList; import java.util.List; import javax.annotation.Nullable; @@ -154,23 +152,12 @@ private boolean shouldRaiseEvent(ViewSnapshot snapshot) { private void raiseInitialEvent(ViewSnapshot snapshot) { hardAssert(!raisedInitialEvent, "Trying to raise initial event for second time"); snapshot = - new ViewSnapshot( + ViewSnapshot.fromInitialDocuments( snapshot.getQuery(), snapshot.getDocuments(), - DocumentSet.emptySet(snapshot.getQuery().comparator()), - QueryListener.getInitialViewChanges(snapshot), - snapshot.isFromCache(), snapshot.getMutatedKeys(), - /*didSyncStateChange=*/ true); + snapshot.isFromCache()); raisedInitialEvent = true; listener.onEvent(snapshot, null); } - - private static List getInitialViewChanges(ViewSnapshot snapshot) { - List res = new ArrayList<>(); - for (Document doc : snapshot.getDocuments()) { - res.add(DocumentViewChange.create(Type.ADDED, doc)); - } - return res; - } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java index f512e891183..026ca5a2d9e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java @@ -91,11 +91,16 @@ private static class LimboResolution { private static final String TAG = SyncEngine.class.getSimpleName(); - /** A callback used to handle events from the SyncEngine */ + /** Interface implemented by EventManager to handle notifications from SyncEngine. */ interface SyncEngineCallback { + /** Handles new view snapshots. */ void onViewSnapshots(List snapshotList); + /** Handles the failure of a query. */ void onError(Query query, Status error); + + /** Handles a change in online state. */ + void handleOnlineStateChange(OnlineState onlineState); } /** The local store, used to persist mutations and cached documents. */ @@ -133,7 +138,7 @@ interface SyncEngineCallback { private User currentUser; - private SyncEngineCallback callback; + private SyncEngineCallback syncEngineListener; public SyncEngine(LocalStore localStore, RemoteStore remoteStore, User initialUser) { this.localStore = localStore; @@ -147,16 +152,16 @@ public SyncEngine(LocalStore localStore, RemoteStore remoteStore, User initialUs limboDocumentRefs = new ReferenceSet(); mutationUserCallbacks = new HashMap<>(); - targetIdGenerator = TargetIdGenerator.getSyncEngineGenerator(0); + targetIdGenerator = TargetIdGenerator.forSyncEngine(); currentUser = initialUser; } public void setCallback(SyncEngineCallback callback) { - this.callback = callback; + this.syncEngineListener = callback; } private void assertCallback(String method) { - hardAssert(callback != null, "Trying to call %s before setting callback", method); + hardAssert(syncEngineListener != null, "Trying to call %s before setting callback", method); } /** @@ -171,6 +176,16 @@ public int listen(Query query) { hardAssert(!queryViewsByQuery.containsKey(query), "We already listen to query: %s", query); QueryData queryData = localStore.allocateQuery(query); + ViewSnapshot viewSnapshot = initializeViewAndComputeSnapshot(queryData); + syncEngineListener.onViewSnapshots(Collections.singletonList(viewSnapshot)); + + remoteStore.listen(queryData); + return queryData.getTargetId(); + } + + private ViewSnapshot initializeViewAndComputeSnapshot(QueryData queryData) { + Query query = queryData.getQuery(); + ImmutableSortedMap docs = localStore.executeQuery(query); ImmutableSortedSet remoteKeys = localStore.getRemoteDocumentKeys(queryData.getTargetId()); @@ -185,10 +200,7 @@ public int listen(Query query) { QueryView queryView = new QueryView(query, queryData.getTargetId(), view); queryViewsByQuery.put(query, queryView); queryViewsByTarget.put(queryData.getTargetId(), queryView); - callback.onViewSnapshots(Collections.singletonList(viewChange.getSnapshot())); - - remoteStore.listen(queryData); - return queryData.getTargetId(); + return viewChange.getSnapshot(); } /** Stops listening to a query previously listened to via listen. */ @@ -200,7 +212,7 @@ void stopListening(Query query) { localStore.releaseQuery(query); remoteStore.stopListening(queryView.getTargetId()); - removeAndCleanup(queryView); + removeAndCleanupQuery(queryView); } /** @@ -215,7 +227,7 @@ public void writeMutations(List mutations, TaskCompletionSource LocalWriteResult result = localStore.writeLocally(mutations); addUserCallback(result.getBatchId(), userTask); - emitNewSnapshot(result.getChanges(), /*remoteEvent=*/ null); + emitNewSnapsAndNotifyLocalStore(result.getChanges(), /*remoteEvent=*/ null); remoteStore.fillWritePipeline(); } @@ -313,7 +325,7 @@ public void handleRemoteEvent(RemoteEvent event) { } ImmutableSortedMap changes = localStore.applyRemoteEvent(event); - emitNewSnapshot(changes, event); + emitNewSnapsAndNotifyLocalStore(changes, event); } /** Applies an OnlineState change to the sync engine and notifies any views of the change. */ @@ -329,7 +341,8 @@ public void handleOnlineStateChange(OnlineState onlineState) { newViewSnapshots.add(viewChange.getSnapshot()); } } - callback.onViewSnapshots(newViewSnapshots); + syncEngineListener.onViewSnapshots(newViewSnapshots); + syncEngineListener.handleOnlineStateChange(onlineState); } @Override @@ -381,9 +394,9 @@ public void handleRejectedListen(int targetId, Status error) { hardAssert(queryView != null, "Unknown target: %s", targetId); Query query = queryView.getQuery(); localStore.releaseQuery(query); - removeAndCleanup(queryView); + removeAndCleanupQuery(queryView); logErrorIfInteresting(error, "Listen for %s failed", query); - callback.onError(query, error); + syncEngineListener.onError(query, error); } } @@ -399,7 +412,7 @@ public void handleSuccessfulWrite(MutationBatchResult mutationBatchResult) { ImmutableSortedMap changes = localStore.acknowledgeBatch(mutationBatchResult); - emitNewSnapshot(changes, /*remoteEvent=*/ null); + emitNewSnapsAndNotifyLocalStore(changes, /*remoteEvent=*/ null); } @Override @@ -416,7 +429,7 @@ public void handleRejectedWrite(int batchId, Status status) { // they consistently happen before listen events. notifyUser(batchId, status); - emitNewSnapshot(changes, /*remoteEvent=*/ null); + emitNewSnapsAndNotifyLocalStore(changes, /*remoteEvent=*/ null); } /** Resolves the task corresponding to this write result. */ @@ -439,7 +452,7 @@ private void notifyUser(int batchId, @Nullable Status status) { } } - private void removeAndCleanup(QueryView view) { + private void removeAndCleanupQuery(QueryView view) { queryViewsByQuery.remove(view.getQuery()); queryViewsByTarget.remove(view.getTargetId()); @@ -469,7 +482,7 @@ private void removeLimboTarget(DocumentKey key) { * Computes a new snapshot from the changes and calls the registered callback with the new * snapshot. */ - private void emitNewSnapshot( + private void emitNewSnapsAndNotifyLocalStore( ImmutableSortedMap changes, @Nullable RemoteEvent remoteEvent) { List newSnapshots = new ArrayList<>(); List documentChangesInAllViews = new ArrayList<>(); @@ -498,7 +511,7 @@ private void emitNewSnapshot( documentChangesInAllViews.add(docChanges); } } - callback.onViewSnapshots(newSnapshots); + syncEngineListener.onViewSnapshots(newSnapshots); localStore.notifyLocalViewChanges(documentChangesInAllViews); } @@ -553,7 +566,7 @@ public void handleCredentialChange(User user) { if (userChanged) { // Notify local store and emit any resulting events from swapping out the mutation queue. ImmutableSortedMap changes = localStore.handleUserChange(user); - emitNewSnapshot(changes, /*remoteEvent=*/ null); + emitNewSnapsAndNotifyLocalStore(changes, /*remoteEvent=*/ null); } // Notify remote store so it can restart its streams. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TargetIdGenerator.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TargetIdGenerator.java index 3e32a6e06d3..8dc35e60c9e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TargetIdGenerator.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TargetIdGenerator.java @@ -14,68 +14,77 @@ package com.google.firebase.firestore.core; +import static com.google.firebase.firestore.util.Assert.hardAssert; + /** - * Generates monotonically increasing integer IDs. There are separate generators for different - * scopes. While these generators will operate independently of each other, they are scoped, such - * that no two generators will ever produce the same ID. This is useful, because sometimes the - * backend may group IDs from separate parts of the client into the same ID space. + * Generates monotonically increasing target IDs for sending targets to the watch stream. + * + *

The client constructs two generators, one for the query cache (via forQueryCache()), and one + * for limbo documents (via forSyncEngine()). These two generators produce non-overlapping IDs (by + * using even and odd IDs respectively). + * + *

By separating the target ID space, the query cache can generate target IDs that persist across + * client restarts, while sync engine can independently generate in-memory target IDs that are + * transient and can be reused after a restart. */ +// TODO(mrschmidt): Explore removing this class in favor of generating these IDs directly in +// SyncEngine and LocalStore. public class TargetIdGenerator { /** * Creates and returns the TargetIdGenerator for the local store. * - * @param after An ID to start at. Every call to nextID will return an id > after. * @return A shared instance of TargetIdGenerator. */ - public static TargetIdGenerator getLocalStoreIdGenerator(int after) { - return new TargetIdGenerator(LOCAL_STATE_ID, after); + public static TargetIdGenerator forQueryCache(int after) { + TargetIdGenerator generator = new TargetIdGenerator(QUERY_CACHE_ID, after); + // Make sure that the next call to `nextId()` returns the first value after 'after'. + generator.nextId(); + return generator; } /** * Creates and returns the TargetIdGenerator for the sync engine. * - * @param after An ID to start at. Every call to nextID will return an id > after. * @return A shared instance of TargetIdGenerator. */ - public static TargetIdGenerator getSyncEngineGenerator(int after) { - return new TargetIdGenerator(SYNC_ENGINE_ID, after); + public static TargetIdGenerator forSyncEngine() { + // Sync engine assigns target IDs for limbo document detection. + return new TargetIdGenerator(SYNC_ENGINE_ID, 1); } - private static final int LOCAL_STATE_ID = 0; + private static final int QUERY_CACHE_ID = 0; private static final int SYNC_ENGINE_ID = 1; private static final int RESERVED_BITS = 1; - private int previousId; + private int nextId; + private int generatorId; + + /** + * Instantiates a new TargetIdGenerator, using the seed as the first target ID to return. + */ + TargetIdGenerator(int generatorId, int seed) { + hardAssert( + (generatorId & RESERVED_BITS) == generatorId, + "Generator ID %d contains more than %d reserved bits", + generatorId, + RESERVED_BITS); + this.generatorId = generatorId; + seek(seed); + } - TargetIdGenerator(int generatorId, int after) { - int afterWithoutGenerator = (after >>> RESERVED_BITS) << RESERVED_BITS; - int afterGenerator = after - afterWithoutGenerator; - if (afterGenerator >= generatorId) { - // For example, if: - // self.generatorID = 0b0000 - // after = 0b1011 - // afterGenerator = 0b0001 - // Then: - // previous = 0b1010 - // next = 0b1100 - previousId = afterWithoutGenerator | generatorId; - } else { - // For example, if: - // self.generatorID = 0b0001 - // after = 0b1010 - // afterGenerator = 0b0000 - // Then: - // previous = 0b1001 - // next = 0b1011 - previousId = (afterWithoutGenerator | generatorId) - (1 << RESERVED_BITS); - } + private void seek(int targetId) { + hardAssert( + (targetId & RESERVED_BITS) == this.generatorId, + "Cannot supply target ID from different generator ID"); + this.nextId = targetId; } /** @return the next id in the sequence */ public int nextId() { - previousId += 1 << RESERVED_BITS; - return previousId; + int nextId = this.nextId; + this.nextId += 1 << RESERVED_BITS; + return nextId; } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java index ad54a67a1eb..79cb376cf7f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java @@ -15,8 +15,10 @@ package com.google.firebase.firestore.core; import com.google.firebase.database.collection.ImmutableSortedSet; +import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.DocumentSet; +import java.util.ArrayList; import java.util.List; /** A view snapshot is an immutable capture of the results of a query and the changes to them. */ @@ -53,6 +55,26 @@ public ViewSnapshot( this.didSyncStateChange = didSyncStateChange; } + /** Returns a view snapshot as if all documents in the snapshot were added. */ + public static ViewSnapshot fromInitialDocuments( + Query query, + DocumentSet documents, + ImmutableSortedSet mutatedKeys, + boolean fromCache) { + List viewChanges = new ArrayList<>(); + for (Document doc : documents) { + viewChanges.add(DocumentViewChange.create(DocumentViewChange.Type.ADDED, doc)); + } + return new ViewSnapshot( + query, + documents, + DocumentSet.emptySet(query.comparator()), + viewChanges, + fromCache, + mutatedKeys, + true); + } + public Query getQuery() { return query; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index 64ccbb066b2..fdeacf22ae3 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -124,7 +124,7 @@ public LocalStore(Persistence persistence, User initialUser) { persistence.isStarted(), "LocalStore was passed an unstarted persistence implementation"); this.persistence = persistence; queryCache = persistence.getQueryCache(); - targetIdGenerator = TargetIdGenerator.getLocalStoreIdGenerator(queryCache.getHighestTargetId()); + targetIdGenerator = TargetIdGenerator.forQueryCache(queryCache.getHighestTargetId()); mutationQueue = persistence.getMutationQueue(initialUser); remoteDocuments = persistence.getRemoteDocumentCache(); localDocuments = new LocalDocumentsView(remoteDocuments, mutationQueue); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java index 5ff240de16e..9af1b1faaba 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryMutationQueue.java @@ -106,11 +106,6 @@ public boolean isEmpty() { return queue.isEmpty(); } - @Override - public int getNextBatchId() { - return nextBatchId; - } - @Override public void acknowledgeBatch(MutationBatch batch, ByteString streamToken) { int batchId = batch.getBatchId(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java index bb8061e1d02..da67269cbea 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MutationQueue.java @@ -38,15 +38,6 @@ interface MutationQueue { /** Returns true if this queue contains no mutation batches. */ boolean isEmpty(); - /** - * Returns the next batch ID that will be assigned to a new mutation batch. - * - *

Callers generally don't care about this value except to test that the mutation queue is - * properly maintaining the invariant that getHighestAcknowledgedBatchId is less than - * getNextBatchId. - */ - int getNextBatchId(); - /** Acknowledges the given batch. */ void acknowledgeBatch(MutationBatch batch, ByteString streamToken); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java index b3bf28ce55d..ccf5bb70490 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteMutationQueue.java @@ -159,11 +159,6 @@ public boolean isEmpty() { return db.query("SELECT batch_id FROM mutations WHERE uid = ? LIMIT 1").binding(uid).isEmpty(); } - @Override - public int getNextBatchId() { - return nextBatchId; - } - @Override public void acknowledgeBatch(MutationBatch batch, ByteString streamToken) { int batchId = batch.getBatchId(); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/TargetIdGeneratorTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/TargetIdGeneratorTest.java index 0ff6a4f351e..14dd8d1cbbb 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/TargetIdGeneratorTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/TargetIdGeneratorTest.java @@ -28,54 +28,32 @@ public class TargetIdGeneratorTest { @Test public void testConstructor() { - assertEquals(2, TargetIdGenerator.getLocalStoreIdGenerator(0).nextId()); - assertEquals(1, TargetIdGenerator.getSyncEngineGenerator(0).nextId()); - } - - @Test - public void testSkipPast() { - TargetIdGenerator gen = new TargetIdGenerator(1, -1); - assertEquals(1, gen.nextId()); - - gen = new TargetIdGenerator(1, 2); - assertEquals(3, gen.nextId()); - - gen = new TargetIdGenerator(1, 4); - assertEquals(5, gen.nextId()); - - for (int i = 4; i < 12; i++) { - TargetIdGenerator gen0 = new TargetIdGenerator(0, i); - TargetIdGenerator gen1 = new TargetIdGenerator(1, i); - assertEquals(i + 2 & ~1, gen0.nextId()); - assertEquals(i + 1 | 1, gen1.nextId()); - } - - gen = new TargetIdGenerator(1, 12); - assertEquals(13, gen.nextId()); - - gen = new TargetIdGenerator(0, 22); - assertEquals(24, gen.nextId()); + assertEquals(2, TargetIdGenerator.forQueryCache(0).nextId()); + assertEquals(1, TargetIdGenerator.forSyncEngine().nextId()); } @Test public void testIncrement() { TargetIdGenerator gen = new TargetIdGenerator(0, 0); + assertEquals(0, gen.nextId()); assertEquals(2, gen.nextId()); assertEquals(4, gen.nextId()); assertEquals(6, gen.nextId()); gen = new TargetIdGenerator(0, 46); + assertEquals(46, gen.nextId()); assertEquals(48, gen.nextId()); assertEquals(50, gen.nextId()); assertEquals(52, gen.nextId()); assertEquals(54, gen.nextId()); - gen = new TargetIdGenerator(1, 0); + gen = new TargetIdGenerator(1, 1); assertEquals(1, gen.nextId()); assertEquals(3, gen.nextId()); assertEquals(5, gen.nextId()); - gen = new TargetIdGenerator(1, 46); + gen = new TargetIdGenerator(1, 45); + assertEquals(45, gen.nextId()); assertEquals(47, gen.nextId()); assertEquals(49, gen.nextId()); assertEquals(51, gen.nextId()); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java index 034b3016b2d..7751f009a37 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java @@ -275,7 +275,6 @@ private void initClient() { @Override public void handleOnlineStateChange(OnlineState onlineState) { syncEngine.handleOnlineStateChange(onlineState); - eventManager.handleOnlineStateChange(onlineState); } private List>> getCurrentOutstandingWrites() { From 56ea93af3ed0b8d5a4a12ead3261f28af85e66df Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 25 Oct 2018 11:08:29 -0700 Subject: [PATCH 2/3] Add excludesMetaddataChanges --- .../firebase/firestore/QuerySnapshot.java | 5 ++++ .../firestore/core/QueryListener.java | 3 ++- .../firestore/core/TargetIdGenerator.java | 4 +--- .../google/firebase/firestore/core/View.java | 3 ++- .../firebase/firestore/core/ViewSnapshot.java | 17 +++++++++++-- .../firebase/firestore/QuerySnapshotTest.java | 3 ++- .../google/firebase/firestore/TestUtil.java | 3 ++- .../firestore/core/QueryListenerTest.java | 24 ++++++++++++------- .../firestore/core/ViewSnapshotTest.java | 12 +++++++++- 9 files changed, 56 insertions(+), 18 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/QuerySnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/QuerySnapshot.java index 929cd46f36e..2b65f613d3f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/QuerySnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/QuerySnapshot.java @@ -117,6 +117,11 @@ public List getDocumentChanges() { @NonNull @PublicApi public List getDocumentChanges(MetadataChanges metadataChanges) { + if (MetadataChanges.INCLUDE.equals(metadataChanges) && snapshot.excludesMetadataChanges()) { + throw new IllegalArgumentException( + "To include metadata changes with your document changes, you must also pass MetadataChanges.INCLUDE to addSnapshotListener()."); + } + if (cachedChanges == null || cachedChangesMetadataState != metadataChanges) { cachedChanges = Collections.unmodifiableList( diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java index f495f5c072a..e49bd117a3c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java @@ -80,7 +80,8 @@ public void onViewSnapshot(ViewSnapshot newSnapshot) { documentChanges, newSnapshot.isFromCache(), newSnapshot.getMutatedKeys(), - newSnapshot.didSyncStateChange()); + newSnapshot.didSyncStateChange(), + /* excludesMetadataChanges= */ true); } if (!raisedInitialEvent) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TargetIdGenerator.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TargetIdGenerator.java index 8dc35e60c9e..8d77e09dcba 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TargetIdGenerator.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TargetIdGenerator.java @@ -61,9 +61,7 @@ public static TargetIdGenerator forSyncEngine() { private int nextId; private int generatorId; - /** - * Instantiates a new TargetIdGenerator, using the seed as the first target ID to return. - */ + /** Instantiates a new TargetIdGenerator, using the seed as the first target ID to return. */ TargetIdGenerator(int generatorId, int seed) { hardAssert( (generatorId & RESERVED_BITS) == generatorId, diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java index 1d4bb456709..af51b3f0f6a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java @@ -310,7 +310,8 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh viewChanges, fromCache, docChanges.mutatedKeys, - syncStatedChanged); + syncStatedChanged, + /* excludesMetadataChanges= */ false); } return new ViewChange(snapshot, limboDocumentChanges); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java index 79cb376cf7f..01e7112b1b3 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java @@ -23,6 +23,7 @@ /** A view snapshot is an immutable capture of the results of a query and the changes to them. */ public class ViewSnapshot { + /** The possibly states a document can be in w.r.t syncing from local storage to the backend. */ public enum SyncState { NONE, @@ -37,6 +38,7 @@ public enum SyncState { private final boolean isFromCache; private final ImmutableSortedSet mutatedKeys; private final boolean didSyncStateChange; + private boolean excludesMetadataChanges; public ViewSnapshot( Query query, @@ -45,7 +47,8 @@ public ViewSnapshot( List changes, boolean isFromCache, ImmutableSortedSet mutatedKeys, - boolean didSyncStateChange) { + boolean didSyncStateChange, + boolean excludesMetadataChanges) { this.query = query; this.documents = documents; this.oldDocuments = oldDocuments; @@ -53,6 +56,7 @@ public ViewSnapshot( this.isFromCache = isFromCache; this.mutatedKeys = mutatedKeys; this.didSyncStateChange = didSyncStateChange; + this.excludesMetadataChanges = excludesMetadataChanges; } /** Returns a view snapshot as if all documents in the snapshot were added. */ @@ -72,7 +76,8 @@ public static ViewSnapshot fromInitialDocuments( viewChanges, fromCache, mutatedKeys, - true); + /* didSyncStateChange= */ true, + /* excludesMetadataChanges= */ false); } public Query getQuery() { @@ -107,6 +112,10 @@ public boolean didSyncStateChange() { return didSyncStateChange; } + public boolean excludesMetadataChanges() { + return excludesMetadataChanges; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -124,6 +133,9 @@ public boolean equals(Object o) { if (didSyncStateChange != that.didSyncStateChange) { return false; } + if (excludesMetadataChanges != that.excludesMetadataChanges) { + return false; + } if (!query.equals(that.query)) { return false; } @@ -148,6 +160,7 @@ public int hashCode() { result = 31 * result + mutatedKeys.hashCode(); result = 31 * result + (isFromCache ? 1 : 0); result = 31 * result + (didSyncStateChange ? 1 : 0); + result = 31 * result + (excludesMetadataChanges ? 1 : 0); return result; } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java index 73621e834f6..6889566083a 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java @@ -124,7 +124,8 @@ public void testIncludeMetadataChanges() { documentChanges, /*isFromCache=*/ false, /*mutatedKeys=*/ keySet(), - /*didSyncStateChange=*/ true); + /*didSyncStateChange=*/ true, + /* excludesMetadataChanges= */ false); QuerySnapshot snapshot = new QuerySnapshot(new Query(fooQuery, firestore), viewSnapshot, firestore); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/TestUtil.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/TestUtil.java index 5272456732b..9309064147e 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/TestUtil.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/TestUtil.java @@ -133,7 +133,8 @@ public static QuerySnapshot querySnapshot( documentChanges, isFromCache, mutatedKeys, - true); + true, + /* excludesMetadataChanges= */ false); return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java index 982711b94c4..e7877ae7860 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java @@ -107,7 +107,8 @@ public void testRaisesCollectionEvents() { asList(change1, change4), snap2.isFromCache(), snap2.getMutatedKeys(), - /* didSyncStateChange= */ true); + /* didSyncStateChange= */ true, + /* excludesMetadataChanges= */ false); assertEquals(asList(snap2Prime), otherAccum); } @@ -254,7 +255,8 @@ public void testRaisesQueryMetadataEventsOnlyWhenHasPendingWritesOnTheQueryChang asList(), snap4.isFromCache(), snap4.getMutatedKeys(), - snap4.didSyncStateChange()); + snap4.didSyncStateChange(), + snap4.excludesMetadataChanges()); assertEquals(asList(snap1, snap3, expectedSnapshot4), fullAccum); } @@ -288,7 +290,8 @@ public void testMetadataOnlyDocumentChangesAreFilteredOut() { asList(change3), snap2.isFromCache(), snap2.getMutatedKeys(), - snap2.didSyncStateChange()); + snap2.didSyncStateChange(), + snap2.excludesMetadataChanges()); assertEquals(asList(snap1, expectedSnapshot2), filteredAccum); } @@ -327,7 +330,8 @@ public void testWillWaitForSyncIfOnline() { asList(change1, change2), /* isFromCache= */ false, snap3.getMutatedKeys(), - /* didSyncStateChange= */ true); + /* didSyncStateChange= */ true, + /* excludesMetadataChanges= */ false); assertEquals(asList(expectedSnapshot), events); } @@ -364,7 +368,8 @@ public void testWillRaiseInitialEventWhenGoingOffline() { asList(change1), /* isFromCache= */ true, snap1.getMutatedKeys(), - /* didSyncStateChange= */ true); + /* didSyncStateChange= */ true, + /* excludesMetadataChanges= */ false); ViewSnapshot expectedSnapshot2 = new ViewSnapshot( snap2.getQuery(), @@ -373,7 +378,8 @@ public void testWillRaiseInitialEventWhenGoingOffline() { asList(change2), /* isFromCache= */ true, snap2.getMutatedKeys(), - /* didSyncStateChange= */ false); + /* didSyncStateChange= */ false, + /* excludesMetadataChanges= */ false); assertEquals(asList(expectedSnapshot1, expectedSnapshot2), events); } @@ -399,7 +405,8 @@ public void testWillRaiseInitialEventWhenGoingOfflineAndThereAreNoDocs() { asList(), /* isFromCache= */ true, snap1.getMutatedKeys(), - /* didSyncStateChange= */ true); + /* didSyncStateChange= */ true, + /* excludesMetadataChanges= */ false); assertEquals(asList(expectedSnapshot), events); } @@ -424,7 +431,8 @@ public void testWillRaiseInitialEventWhenStartingOfflineAndThereAreNoDocs() { asList(), /* isFromCache= */ true, snap1.getMutatedKeys(), - /* didSyncStateChange= */ true); + /* didSyncStateChange= */ true, + /* excludesMetadataChanges= */ false); assertEquals(asList(expectedSnapshot), events); } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java index 859a0dbf908..554f81e5b2c 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java @@ -48,9 +48,18 @@ public void testConstructor() { boolean fromCache = true; boolean hasPendingWrites = true; boolean syncStateChanges = true; + boolean excludesMetadataChanges = true; ViewSnapshot snapshot = - new ViewSnapshot(query, docs, oldDocs, changes, fromCache, mutatedKeys, syncStateChanges); + new ViewSnapshot( + query, + docs, + oldDocs, + changes, + fromCache, + mutatedKeys, + syncStateChanges, + excludesMetadataChanges); assertEquals(query, snapshot.getQuery()); assertEquals(docs, snapshot.getDocuments()); @@ -60,5 +69,6 @@ public void testConstructor() { assertEquals(mutatedKeys, snapshot.getMutatedKeys()); assertEquals(hasPendingWrites, snapshot.hasPendingWrites()); assertEquals(syncStateChanges, snapshot.didSyncStateChange()); + assertEquals(excludesMetadataChanges, snapshot.excludesMetadataChanges()); } } From e139cec7ecaaa097e35e3b77ad69f6e8440859bd Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 25 Oct 2018 12:44:44 -0700 Subject: [PATCH 3/3] Fix Unit tests --- .../firestore/core/QueryListener.java | 3 +- .../firebase/firestore/core/ViewSnapshot.java | 7 ++- .../firestore/core/QueryListenerTest.java | 52 +++++++++++++++---- 3 files changed, 48 insertions(+), 14 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java index e49bd117a3c..4c594429396 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java @@ -157,7 +157,8 @@ private void raiseInitialEvent(ViewSnapshot snapshot) { snapshot.getQuery(), snapshot.getDocuments(), snapshot.getMutatedKeys(), - snapshot.isFromCache()); + snapshot.isFromCache(), + snapshot.excludesMetadataChanges()); raisedInitialEvent = true; listener.onEvent(snapshot, null); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java index 01e7112b1b3..a05cffea965 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java @@ -64,7 +64,8 @@ public static ViewSnapshot fromInitialDocuments( Query query, DocumentSet documents, ImmutableSortedSet mutatedKeys, - boolean fromCache) { + boolean fromCache, + boolean excludesMetadataChanges) { List viewChanges = new ArrayList<>(); for (Document doc : documents) { viewChanges.add(DocumentViewChange.create(DocumentViewChange.Type.ADDED, doc)); @@ -77,7 +78,7 @@ public static ViewSnapshot fromInitialDocuments( fromCache, mutatedKeys, /* didSyncStateChange= */ true, - /* excludesMetadataChanges= */ false); + excludesMetadataChanges); } public Query getQuery() { @@ -180,6 +181,8 @@ public String toString() { + mutatedKeys.size() + ", didSyncStateChange=" + didSyncStateChange + + ", excludesMetadataChanges=" + + excludesMetadataChanges + ")"; } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java index e7877ae7860..d3e0a4b9d6c 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java @@ -26,6 +26,7 @@ import static org.junit.Assert.assertTrue; import com.google.firebase.firestore.FirebaseFirestoreException; +import com.google.firebase.firestore.MetadataChanges; import com.google.firebase.firestore.core.DocumentViewChange.Type; import com.google.firebase.firestore.core.EventManager.ListenOptions; import com.google.firebase.firestore.core.View.DocumentChanges; @@ -162,6 +163,7 @@ public void testDoesNotRaiseEventsForMetadataChangesUnlessSpecified() { ListenOptions options1 = new ListenOptions(); ListenOptions options2 = new ListenOptions(); options2.includeQueryMetadataChanges = true; + options2.includeDocumentMetadataChanges = true; QueryListener filteredListener = queryListener(query, options1, filteredAccum); QueryListener fullListener = queryListener(query, options2, fullAccum); @@ -181,7 +183,11 @@ public void testDoesNotRaiseEventsForMetadataChangesUnlessSpecified() { fullListener.onViewSnapshot(snap2); // no event fullListener.onViewSnapshot(snap3); // doc2 update - assertEquals(asList(snap1, snap3), filteredAccum); + assertEquals( + asList( + applyExpectedMetadata(snap1, MetadataChanges.EXCLUDE), + applyExpectedMetadata(snap3, MetadataChanges.EXCLUDE)), + filteredAccum); assertEquals(asList(snap1, snap2, snap3), fullAccum); } @@ -215,7 +221,11 @@ public void testRaisesDocumentMetadataEventsOnlyWhenSpecified() { fullListener.onViewSnapshot(snap2); fullListener.onViewSnapshot(snap3); - assertEquals(asList(snap1, snap3), filteredAccum); + assertEquals( + asList( + applyExpectedMetadata(snap1, MetadataChanges.EXCLUDE), + applyExpectedMetadata(snap3, MetadataChanges.EXCLUDE)), + filteredAccum); // Second listener should receive doc1prime as added document not modified assertEquals(asList(snap1, snap2, snap3), fullAccum); } @@ -256,8 +266,14 @@ public void testRaisesQueryMetadataEventsOnlyWhenHasPendingWritesOnTheQueryChang snap4.isFromCache(), snap4.getMutatedKeys(), snap4.didSyncStateChange(), - snap4.excludesMetadataChanges()); - assertEquals(asList(snap1, snap3, expectedSnapshot4), fullAccum); + /* excludeMetadataChanges= */ true); // This test excludes document metadata changes + + assertEquals( + asList( + applyExpectedMetadata(snap1, MetadataChanges.EXCLUDE), + applyExpectedMetadata(snap3, MetadataChanges.EXCLUDE), + expectedSnapshot4), + fullAccum); } @Test @@ -291,8 +307,10 @@ public void testMetadataOnlyDocumentChangesAreFilteredOut() { snap2.isFromCache(), snap2.getMutatedKeys(), snap2.didSyncStateChange(), - snap2.excludesMetadataChanges()); - assertEquals(asList(snap1, expectedSnapshot2), filteredAccum); + /* excludesMetadataChanges= */ true); + assertEquals( + asList(applyExpectedMetadata(snap1, MetadataChanges.EXCLUDE), expectedSnapshot2), + filteredAccum); } @Test @@ -331,7 +349,7 @@ public void testWillWaitForSyncIfOnline() { /* isFromCache= */ false, snap3.getMutatedKeys(), /* didSyncStateChange= */ true, - /* excludesMetadataChanges= */ false); + /* excludesMetadataChanges= */ true); assertEquals(asList(expectedSnapshot), events); } @@ -369,7 +387,7 @@ public void testWillRaiseInitialEventWhenGoingOffline() { /* isFromCache= */ true, snap1.getMutatedKeys(), /* didSyncStateChange= */ true, - /* excludesMetadataChanges= */ false); + /* excludesMetadataChanges= */ true); ViewSnapshot expectedSnapshot2 = new ViewSnapshot( snap2.getQuery(), @@ -379,7 +397,7 @@ public void testWillRaiseInitialEventWhenGoingOffline() { /* isFromCache= */ true, snap2.getMutatedKeys(), /* didSyncStateChange= */ false, - /* excludesMetadataChanges= */ false); + /* excludesMetadataChanges= */ true); assertEquals(asList(expectedSnapshot1, expectedSnapshot2), events); } @@ -406,7 +424,7 @@ public void testWillRaiseInitialEventWhenGoingOfflineAndThereAreNoDocs() { /* isFromCache= */ true, snap1.getMutatedKeys(), /* didSyncStateChange= */ true, - /* excludesMetadataChanges= */ false); + /* excludesMetadataChanges= */ true); assertEquals(asList(expectedSnapshot), events); } @@ -432,7 +450,19 @@ public void testWillRaiseInitialEventWhenStartingOfflineAndThereAreNoDocs() { /* isFromCache= */ true, snap1.getMutatedKeys(), /* didSyncStateChange= */ true, - /* excludesMetadataChanges= */ false); + /* excludesMetadataChanges= */ true); assertEquals(asList(expectedSnapshot), events); } + + private ViewSnapshot applyExpectedMetadata(ViewSnapshot snap, MetadataChanges metadata) { + return new ViewSnapshot( + snap.getQuery(), + snap.getDocuments(), + snap.getOldDocuments(), + snap.getChanges(), + snap.isFromCache(), + snap.getMutatedKeys(), + snap.didSyncStateChange(), + MetadataChanges.EXCLUDE.equals(metadata)); + } }