From 830d631659ae2657adc91c73120d5b2b64d4e943 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 8 May 2024 10:35:58 -0400 Subject: [PATCH 01/37] dbToken Persistence --- .../firestore/local/GlobalsCache.java | 19 ++++++++ .../firebase/firestore/local/LocalStore.java | 4 ++ .../firestore/local/MemoryGlobalsCache.java | 19 ++++++++ .../firestore/local/MemoryPersistence.java | 12 +++++ .../firebase/firestore/local/Persistence.java | 3 ++ .../firestore/local/SQLiteGlobalsCache.java | 45 +++++++++++++++++++ .../firestore/local/SQLitePersistence.java | 5 +++ .../firestore/local/SQLiteSchema.java | 20 ++++++++- .../firestore/local/GlobalsCacheTest.java | 38 ++++++++++++++++ .../local/MemoryGlobalsCacheTest.java | 15 +++++++ .../local/SQLiteGlobalCacheTest.java | 15 +++++++ 11 files changed, 194 insertions(+), 1 deletion(-) create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java create mode 100644 firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java create mode 100644 firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryGlobalsCacheTest.java create mode 100644 firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteGlobalCacheTest.java diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java new file mode 100644 index 00000000000..edafbae1d30 --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java @@ -0,0 +1,19 @@ +package com.google.firebase.firestore.local; + +import com.google.protobuf.ByteString; + +/** + * General purpose cache for global values for user. + * + *

Global state that cuts across components should be saved here. Following are contained herein: + * + *

`db_token` tracks server interaction across Listen and Write streams. This facilitates cache + * synchronization and invalidation. + */ +interface GlobalsCache { + + ByteString getDbToken(); + + void setDbToken(ByteString value); + +} 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 443803386e4..a94115a0228 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 @@ -111,6 +111,9 @@ public final class LocalStore implements BundleCallback { /** Manages our in-memory or durable persistence. */ private final Persistence persistence; + /** General purpose global state. */ + private GlobalsCache globalsCache; + /** Manages the list of active field and collection indices. */ private IndexManager indexManager; @@ -168,6 +171,7 @@ public LocalStore(Persistence persistence, QueryEngine queryEngine, User initial private void initializeUserComponents(User user) { // TODO(indexing): Add spec tests that test these components change after a user change + globalsCache = persistence.getGlobalsCache(user); indexManager = persistence.getIndexManager(user); mutationQueue = persistence.getMutationQueue(user, indexManager); documentOverlayCache = persistence.getDocumentOverlayCache(user); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java new file mode 100644 index 00000000000..5021944bbd3 --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java @@ -0,0 +1,19 @@ +package com.google.firebase.firestore.local; + +import com.google.protobuf.ByteString; + +/** In-memory cache of global values */ +final class MemoryGlobalsCache implements GlobalsCache { + + private ByteString dbToken; + + @Override + public ByteString getDbToken() { + return dbToken; + } + + @Override + public void setDbToken(ByteString value) { + dbToken = value; + } +} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryPersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryPersistence.java index 2f3fce2c638..3f801d724b5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryPersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryPersistence.java @@ -31,6 +31,7 @@ public final class MemoryPersistence extends Persistence { // tests affecting both the in-memory and SQLite-backed persistence layers. Tests can create a new // LocalStore wrapping this Persistence instance and this will make the in-memory persistence // layer behave as if it were actually persisting values. + private final Map userGlobals; private final Map mutationQueues; private final Map overlays; private final MemoryIndexManager indexManager; @@ -57,6 +58,7 @@ public static MemoryPersistence createLruGcMemoryPersistence( /** Use static helpers to instantiate */ private MemoryPersistence() { + userGlobals = new HashMap<>(); mutationQueues = new HashMap<>(); indexManager = new MemoryIndexManager(); targetCache = new MemoryTargetCache(this); @@ -117,6 +119,16 @@ MemoryRemoteDocumentCache getRemoteDocumentCache() { return remoteDocumentCache; } + @Override + GlobalsCache getGlobalsCache(User user) { + MemoryGlobalsCache globals = userGlobals.get(user); + if (globals == null) { + globals = new MemoryGlobalsCache(); + userGlobals.put(user, globals); + } + return globals; + } + @Override MemoryIndexManager getIndexManager(User user) { // We do not currently support indices for memory persistence, so we can return the same shared diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/Persistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/Persistence.java index 3c266493aa0..837e77daef0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/Persistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/Persistence.java @@ -86,6 +86,9 @@ public abstract class Persistence { /** Creates a RemoteDocumentCache representing the persisted cache of remote documents. */ abstract RemoteDocumentCache getRemoteDocumentCache(); + /** Creates GlobalCache that provides access to global values for user. */ + abstract GlobalsCache getGlobalsCache(User user); + /** Creates an IndexManager that manages our persisted query indexes. */ abstract IndexManager getIndexManager(User user); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java new file mode 100644 index 00000000000..b58a3bed086 --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java @@ -0,0 +1,45 @@ +package com.google.firebase.firestore.local; + +import com.google.firebase.firestore.auth.User; +import com.google.protobuf.ByteString; + +public class SQLiteGlobalsCache implements GlobalsCache{ + + private static final String DB_TOKEN = "dbToken"; + private final SQLitePersistence db; + + /** The normalized uid (for example, null => "") used in the uid column. */ + private final String uid; + + public SQLiteGlobalsCache(SQLitePersistence persistence, User user) { + this.db = persistence; + this.uid = user.isAuthenticated() ? user.getUid() : ""; + } + + @Override + public ByteString getDbToken() { + byte[] bytes = get(DB_TOKEN); + return bytes == null ? null : ByteString.copyFrom(bytes); + } + + @Override + public void setDbToken(ByteString value) { + set(DB_TOKEN, value.toByteArray()); + } + + private byte[] get(String global) { + return db.query("SELECT value FROM globals WHERE uid = ? AND global = ?") + .binding(uid, global) + .firstValue(row -> row.getBlob(0)); + } + + private void set(String global, byte[] value) { + db.execute( + "INSERT OR REPLACE INTO globals " + + "(uid, global, value) " + + "VALUES (?, ?, ?)", + uid, + global, + value); + } +} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java index b9fe1f19aa6..9fa52300a01 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java @@ -205,6 +205,11 @@ RemoteDocumentCache getRemoteDocumentCache() { return remoteDocumentCache; } + @Override + GlobalsCache getGlobalsCache(User user) { + return new SQLiteGlobalsCache(this, user); + } + @Override void runTransaction(String action, Runnable operation) { Logger.debug(TAG, "Starting transaction: %s", action); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index b0c8ac2dabd..ff753c835b8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -48,7 +48,7 @@ class SQLiteSchema { * The version of the schema. Increase this by one for each migration added to runMigrations * below. */ - static final int VERSION = 16; + static final int VERSION = 17; /** * The batch size for data migrations. @@ -179,6 +179,10 @@ void runSchemaUpgrades(int fromVersion, int toVersion) { createFieldIndex(); } + if (fromVersion < 17 && toVersion >= 17) { + createGlobalsTable(); + } + /* * Adding a new schema upgrade? READ THIS FIRST! * @@ -713,6 +717,20 @@ private void addPendingDataMigration(String migration) { new String[] {migration}); } + private void createGlobalsTable() { + ifTablesDontExist( + new String[] {"globals"}, + () -> { + // A table of key value pairs by user. + db.execSQL( + "CREATE TABLE globals (" + + "uid TEXT, " + + "global TEXT, " + + "value BLOB, " + + "PRIMARY KEY (uid, global))"); + }); + } + private boolean tableExists(String table) { return !new SQLitePersistence.Query(db, "SELECT 1=1 FROM sqlite_master WHERE tbl_name = ?") .binding(table) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java new file mode 100644 index 00000000000..3050c51519f --- /dev/null +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java @@ -0,0 +1,38 @@ +package com.google.firebase.firestore.local; + +import static org.junit.Assert.assertEquals; + +import com.google.firebase.firestore.auth.User; +import com.google.protobuf.ByteString; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.nio.charset.StandardCharsets; + +public abstract class GlobalsCacheTest { + + private Persistence persistence; + private GlobalsCache globalsCache; + + @Before + public void setUp() { + persistence = getPersistence(); + globalsCache = persistence.getGlobalsCache(User.UNAUTHENTICATED); + } + + @After + public void tearDown() { + persistence.shutdown(); + } + + abstract Persistence getPersistence(); + + @Test + public void setAndGetDbToken() { + ByteString value = ByteString.copyFrom("TestData", StandardCharsets.UTF_8); + globalsCache.setDbToken(value); + assertEquals(value, globalsCache.getDbToken()); + } +} diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryGlobalsCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryGlobalsCacheTest.java new file mode 100644 index 00000000000..795f9e81b0e --- /dev/null +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryGlobalsCacheTest.java @@ -0,0 +1,15 @@ +package com.google.firebase.firestore.local; + +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +@RunWith(RobolectricTestRunner.class) +@Config(manifest = Config.NONE) +public class MemoryGlobalsCacheTest extends GlobalsCacheTest { + + @Override + Persistence getPersistence() { + return PersistenceTestHelpers.createEagerGCMemoryPersistence(); + } +} diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteGlobalCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteGlobalCacheTest.java new file mode 100644 index 00000000000..7ba28e431cd --- /dev/null +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteGlobalCacheTest.java @@ -0,0 +1,15 @@ +package com.google.firebase.firestore.local; + +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +@RunWith(RobolectricTestRunner.class) +@Config(manifest = Config.NONE) +public class SQLiteGlobalCacheTest extends GlobalsCacheTest { + + @Override + Persistence getPersistence() { + return PersistenceTestHelpers.createSQLitePersistence(); + } +} From 37b97be67ccb80c195e73a09e8464899b41fefee Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 8 May 2024 10:45:54 -0400 Subject: [PATCH 02/37] Copyright --- .../firebase/firestore/local/GlobalsCache.java | 14 ++++++++++++++ .../firestore/local/MemoryGlobalsCache.java | 14 ++++++++++++++ .../firestore/local/SQLiteGlobalsCache.java | 14 ++++++++++++++ .../firebase/firestore/local/GlobalsCacheTest.java | 14 ++++++++++++++ .../firestore/local/MemoryGlobalsCacheTest.java | 14 ++++++++++++++ .../firestore/local/SQLiteGlobalCacheTest.java | 14 ++++++++++++++ 6 files changed, 84 insertions(+) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java index edafbae1d30..33efa1ef00c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java @@ -1,3 +1,17 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package com.google.firebase.firestore.local; import com.google.protobuf.ByteString; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java index 5021944bbd3..f21f0e0ba45 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java @@ -1,3 +1,17 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package com.google.firebase.firestore.local; import com.google.protobuf.ByteString; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java index b58a3bed086..55b96a64008 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java @@ -1,3 +1,17 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package com.google.firebase.firestore.local; import com.google.firebase.firestore.auth.User; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java index 3050c51519f..6aca309743d 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java @@ -1,3 +1,17 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package com.google.firebase.firestore.local; import static org.junit.Assert.assertEquals; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryGlobalsCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryGlobalsCacheTest.java index 795f9e81b0e..7a9108f7110 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryGlobalsCacheTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryGlobalsCacheTest.java @@ -1,3 +1,17 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package com.google.firebase.firestore.local; import org.junit.runner.RunWith; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteGlobalCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteGlobalCacheTest.java index 7ba28e431cd..f91a1bb5b18 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteGlobalCacheTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteGlobalCacheTest.java @@ -1,3 +1,17 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package com.google.firebase.firestore.local; import org.junit.runner.RunWith; From d9cf32e1dc70381bde20fc022417227e4167abaf Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 14 May 2024 11:14:12 -0400 Subject: [PATCH 03/37] Remove user from globals cache. --- .../firebase/firestore/local/LocalStore.java | 2 +- .../firestore/local/MemoryPersistence.java | 13 ++++--------- .../firebase/firestore/local/Persistence.java | 4 ++-- .../firestore/local/SQLiteGlobalsCache.java | 15 +++++---------- .../firestore/local/SQLitePersistence.java | 4 ++-- .../firebase/firestore/local/SQLiteSchema.java | 18 ++++++++---------- .../firestore/local/GlobalsCacheTest.java | 2 +- 7 files changed, 23 insertions(+), 35 deletions(-) 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 a94115a0228..89282977822 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 @@ -156,6 +156,7 @@ public LocalStore(Persistence persistence, QueryEngine queryEngine, User initial this.persistence = persistence; this.queryEngine = queryEngine; + globalsCache = persistence.getGlobalsCache(); targetCache = persistence.getTargetCache(); bundleCache = persistence.getBundleCache(); targetIdGenerator = TargetIdGenerator.forTargetCache(targetCache.getHighestTargetId()); @@ -171,7 +172,6 @@ public LocalStore(Persistence persistence, QueryEngine queryEngine, User initial private void initializeUserComponents(User user) { // TODO(indexing): Add spec tests that test these components change after a user change - globalsCache = persistence.getGlobalsCache(user); indexManager = persistence.getIndexManager(user); mutationQueue = persistence.getMutationQueue(user, indexManager); documentOverlayCache = persistence.getDocumentOverlayCache(user); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryPersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryPersistence.java index 3f801d724b5..b96ee596cb2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryPersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryPersistence.java @@ -31,7 +31,7 @@ public final class MemoryPersistence extends Persistence { // tests affecting both the in-memory and SQLite-backed persistence layers. Tests can create a new // LocalStore wrapping this Persistence instance and this will make the in-memory persistence // layer behave as if it were actually persisting values. - private final Map userGlobals; + private final MemoryGlobalsCache globalsCache; private final Map mutationQueues; private final Map overlays; private final MemoryIndexManager indexManager; @@ -58,7 +58,7 @@ public static MemoryPersistence createLruGcMemoryPersistence( /** Use static helpers to instantiate */ private MemoryPersistence() { - userGlobals = new HashMap<>(); + globalsCache = new MemoryGlobalsCache(); mutationQueues = new HashMap<>(); indexManager = new MemoryIndexManager(); targetCache = new MemoryTargetCache(this); @@ -120,13 +120,8 @@ MemoryRemoteDocumentCache getRemoteDocumentCache() { } @Override - GlobalsCache getGlobalsCache(User user) { - MemoryGlobalsCache globals = userGlobals.get(user); - if (globals == null) { - globals = new MemoryGlobalsCache(); - userGlobals.put(user, globals); - } - return globals; + GlobalsCache getGlobalsCache() { + return globalsCache; } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/Persistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/Persistence.java index 837e77daef0..39611130d02 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/Persistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/Persistence.java @@ -86,8 +86,8 @@ public abstract class Persistence { /** Creates a RemoteDocumentCache representing the persisted cache of remote documents. */ abstract RemoteDocumentCache getRemoteDocumentCache(); - /** Creates GlobalCache that provides access to global values for user. */ - abstract GlobalsCache getGlobalsCache(User user); + /** Creates GlobalCache that provides access to global values. */ + abstract GlobalsCache getGlobalsCache(); /** Creates an IndexManager that manages our persisted query indexes. */ abstract IndexManager getIndexManager(User user); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java index 55b96a64008..51e8467313c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java @@ -22,12 +22,8 @@ public class SQLiteGlobalsCache implements GlobalsCache{ private static final String DB_TOKEN = "dbToken"; private final SQLitePersistence db; - /** The normalized uid (for example, null => "") used in the uid column. */ - private final String uid; - - public SQLiteGlobalsCache(SQLitePersistence persistence, User user) { + public SQLiteGlobalsCache(SQLitePersistence persistence) { this.db = persistence; - this.uid = user.isAuthenticated() ? user.getUid() : ""; } @Override @@ -42,17 +38,16 @@ public void setDbToken(ByteString value) { } private byte[] get(String global) { - return db.query("SELECT value FROM globals WHERE uid = ? AND global = ?") - .binding(uid, global) + return db.query("SELECT value FROM globals WHERE global = ?") + .binding(global) .firstValue(row -> row.getBlob(0)); } private void set(String global, byte[] value) { db.execute( "INSERT OR REPLACE INTO globals " - + "(uid, global, value) " - + "VALUES (?, ?, ?)", - uid, + + "(global, value) " + + "VALUES (?, ?)", global, value); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java index 9fa52300a01..5bc77f2b5ba 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java @@ -206,8 +206,8 @@ RemoteDocumentCache getRemoteDocumentCache() { } @Override - GlobalsCache getGlobalsCache(User user) { - return new SQLiteGlobalsCache(this, user); + GlobalsCache getGlobalsCache() { + return new SQLiteGlobalsCache(this); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index ff753c835b8..e5e882eab56 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -719,16 +719,14 @@ private void addPendingDataMigration(String migration) { private void createGlobalsTable() { ifTablesDontExist( - new String[] {"globals"}, - () -> { - // A table of key value pairs by user. - db.execSQL( - "CREATE TABLE globals (" - + "uid TEXT, " - + "global TEXT, " - + "value BLOB, " - + "PRIMARY KEY (uid, global))"); - }); + new String[] {"globals"}, + () -> { + // A table of key value pairs by user. + db.execSQL( + "CREATE TABLE globals (" + + "global TEXT PRIMARY KEY, " + + "value BLOB)"); + }); } private boolean tableExists(String table) { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java index 6aca309743d..d5274211e83 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java @@ -33,7 +33,7 @@ public abstract class GlobalsCacheTest { @Before public void setUp() { persistence = getPersistence(); - globalsCache = persistence.getGlobalsCache(User.UNAUTHENTICATED); + globalsCache = persistence.getGlobalsCache(); } @After From 297d2c455ff986b248989f1be1d95b7a4bc866d7 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 16 May 2024 22:22:20 -0400 Subject: [PATCH 04/37] Fix --- .../com/google/firebase/firestore/local/SQLiteGlobalsCache.java | 1 - .../com/google/firebase/firestore/local/GlobalsCacheTest.java | 1 - 2 files changed, 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java index 51e8467313c..a9c7233b608 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java @@ -14,7 +14,6 @@ package com.google.firebase.firestore.local; -import com.google.firebase.firestore.auth.User; import com.google.protobuf.ByteString; public class SQLiteGlobalsCache implements GlobalsCache{ diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java index d5274211e83..2693277ed95 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java @@ -16,7 +16,6 @@ import static org.junit.Assert.assertEquals; -import com.google.firebase.firestore.auth.User; import com.google.protobuf.ByteString; import org.junit.After; From cb0835674a82aea686d1d09e2d261c8e979fc668 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 27 May 2024 12:08:35 -0400 Subject: [PATCH 05/37] Fix from review comments --- .../firebase/firestore/local/GlobalsCache.java | 2 +- .../firestore/local/SQLiteGlobalsCache.java | 14 +++++++------- .../firebase/firestore/local/SQLiteSchema.java | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java index 33efa1ef00c..984908be4c4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java @@ -17,7 +17,7 @@ import com.google.protobuf.ByteString; /** - * General purpose cache for global values for user. + * General purpose cache for global values. * *

Global state that cuts across components should be saved here. Following are contained herein: * diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java index a9c7233b608..b4f7a2ba91a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java @@ -16,7 +16,7 @@ import com.google.protobuf.ByteString; -public class SQLiteGlobalsCache implements GlobalsCache{ +public class SQLiteGlobalsCache implements GlobalsCache { private static final String DB_TOKEN = "dbToken"; private final SQLitePersistence db; @@ -36,18 +36,18 @@ public void setDbToken(ByteString value) { set(DB_TOKEN, value.toByteArray()); } - private byte[] get(String global) { - return db.query("SELECT value FROM globals WHERE global = ?") - .binding(global) + private byte[] get(String name) { + return db.query("SELECT value FROM globals WHERE name = ?") + .binding(name) .firstValue(row -> row.getBlob(0)); } - private void set(String global, byte[] value) { + private void set(String name, byte[] value) { db.execute( "INSERT OR REPLACE INTO globals " - + "(global, value) " + + "(name, value) " + "VALUES (?, ?)", - global, + name, value); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index e5e882eab56..e5b548d8983 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -724,7 +724,7 @@ private void createGlobalsTable() { // A table of key value pairs by user. db.execSQL( "CREATE TABLE globals (" - + "global TEXT PRIMARY KEY, " + + "name TEXT PRIMARY KEY, " + "value BLOB)"); }); } From 686e19c2d8f537a95b9974cdf2939e99ef248d03 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 5 Jun 2024 10:43:16 -0400 Subject: [PATCH 06/37] Rename --- .../google/firebase/firestore/local/GlobalsCache.java | 4 ++-- .../google/firebase/firestore/local/LocalStore.java | 8 ++++++++ .../firebase/firestore/local/MemoryGlobalsCache.java | 10 +++++----- .../firebase/firestore/local/SQLiteGlobalsCache.java | 10 +++++----- .../firebase/firestore/local/GlobalsCacheTest.java | 4 ++-- 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java index 984908be4c4..e5c04552f67 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java @@ -26,8 +26,8 @@ */ interface GlobalsCache { - ByteString getDbToken(); + ByteString getSessionsToken(); - void setDbToken(ByteString value); + void setSessionToken(ByteString value); } 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 89282977822..9d268a503ba 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 @@ -396,6 +396,14 @@ public SnapshotVersion getLastRemoteSnapshotVersion() { return targetCache.getLastRemoteSnapshotVersion(); } + public ByteString getSessionToken() { + return globalsCache.getSessionsToken(); + } + + public void setSessionsToken(ByteString sessionToken) { + globalsCache.setSessionToken(sessionToken); + } + /** * Updates the "ground-state" (remote) documents. We assume that the remote event reflects any * write batches that have been acknowledged or rejected (specifically, we do not re-apply local diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java index f21f0e0ba45..e02dcbb2f7b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java @@ -19,15 +19,15 @@ /** In-memory cache of global values */ final class MemoryGlobalsCache implements GlobalsCache { - private ByteString dbToken; + private ByteString sessionToken; @Override - public ByteString getDbToken() { - return dbToken; + public ByteString getSessionsToken() { + return sessionToken; } @Override - public void setDbToken(ByteString value) { - dbToken = value; + public void setSessionToken(ByteString value) { + sessionToken = value; } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java index b4f7a2ba91a..a26c99a1490 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java @@ -18,7 +18,7 @@ public class SQLiteGlobalsCache implements GlobalsCache { - private static final String DB_TOKEN = "dbToken"; + private static final String SESSION_TOKEN = "sessionToken"; private final SQLitePersistence db; public SQLiteGlobalsCache(SQLitePersistence persistence) { @@ -26,14 +26,14 @@ public SQLiteGlobalsCache(SQLitePersistence persistence) { } @Override - public ByteString getDbToken() { - byte[] bytes = get(DB_TOKEN); + public ByteString getSessionsToken() { + byte[] bytes = get(SESSION_TOKEN); return bytes == null ? null : ByteString.copyFrom(bytes); } @Override - public void setDbToken(ByteString value) { - set(DB_TOKEN, value.toByteArray()); + public void setSessionToken(ByteString value) { + set(SESSION_TOKEN, value.toByteArray()); } private byte[] get(String name) { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java index 2693277ed95..fde94288282 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java @@ -45,7 +45,7 @@ public void tearDown() { @Test public void setAndGetDbToken() { ByteString value = ByteString.copyFrom("TestData", StandardCharsets.UTF_8); - globalsCache.setDbToken(value); - assertEquals(value, globalsCache.getDbToken()); + globalsCache.setSessionToken(value); + assertEquals(value, globalsCache.getSessionsToken()); } } From be8097fd6429fdd34229d401527a02e473cead1d Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 5 Jun 2024 15:53:26 -0400 Subject: [PATCH 07/37] AbstractStream refactor --- .../firestore/remote/AbstractStream.java | 15 +++++++-- .../firestore/remote/WatchStream.java | 7 ++++- .../firestore/remote/WriteStream.java | 31 ++++++------------- 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/AbstractStream.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/AbstractStream.java index 30123f6bdc2..c1cd01dfbe9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/AbstractStream.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/AbstractStream.java @@ -77,6 +77,8 @@ void run(Runnable task) { class StreamObserver implements IncomingStreamObserver { private final CloseGuardedRunner dispatcher; + private int responseCount = 0; + StreamObserver(CloseGuardedRunner dispatcher) { this.dispatcher = dispatcher; } @@ -107,17 +109,24 @@ public void onHeaders(Metadata headers) { @Override public void onNext(RespT response) { + final int currentResponseCount = responseCount + 1; dispatcher.run( () -> { if (Logger.isDebugEnabled()) { Logger.debug( AbstractStream.this.getClass().getSimpleName(), - "(%x) Stream received: %s", + "(%x) Stream received (%s): %s", System.identityHashCode(AbstractStream.this), + currentResponseCount, response); } - AbstractStream.this.onNext(response); + if (currentResponseCount == 1) { + AbstractStream.this.onFirst(response); + } else { + AbstractStream.this.onNext(response); + } }); + responseCount = currentResponseCount; } @Override @@ -429,6 +438,8 @@ private void onOpen() { } } + public abstract void onFirst(RespT change); + public abstract void onNext(RespT change); private void performBackoff() { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchStream.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchStream.java index e9d6830c7fd..515fefa1099 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchStream.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchStream.java @@ -100,7 +100,12 @@ public void unwatchTarget(int targetId) { } @Override - public void onNext(com.google.firestore.v1.ListenResponse listenResponse) { + public void onFirst(ListenResponse listenResponse) { + onNext(listenResponse); + } + + @Override + public void onNext(ListenResponse listenResponse) { // A successful response means the stream is healthy backoff.reset(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java index f35b15a5151..7f951d521bc 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java @@ -159,30 +159,19 @@ void writeMutations(List mutations) { writeRequest(request.build()); } + @Override + public void onFirst(WriteResponse response) { + lastStreamToken = response.getStreamToken(); + + // The first response is the handshake response + handshakeComplete = true; + + + } + @Override public void onNext(WriteResponse response) { lastStreamToken = response.getStreamToken(); - if (!handshakeComplete) { - // The first response is the handshake response - handshakeComplete = true; - - listener.onHandshakeComplete(); - } else { - // A successful first write response means the stream is healthy, - // Note, that we could consider a successful handshake healthy, however, - // the write itself might be causing an error we want to back off from. - backoff.reset(); - - SnapshotVersion commitVersion = serializer.decodeVersion(response.getCommitTime()); - - int count = response.getWriteResultsCount(); - List results = new ArrayList<>(count); - for (int i = 0; i < count; i++) { - com.google.firestore.v1.WriteResult result = response.getWriteResults(i); - results.add(serializer.decodeMutationResult(result, commitVersion)); - } - listener.onWriteResponse(commitVersion, results); - } } } From b61a395cddead3a4a57662bba999df9274a6e485 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 5 Jun 2024 15:56:30 -0400 Subject: [PATCH 08/37] AbstractStream refactor --- .../firebase/firestore/remote/WriteStream.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java index 7f951d521bc..0727e23665d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java @@ -173,5 +173,19 @@ public void onFirst(WriteResponse response) { public void onNext(WriteResponse response) { lastStreamToken = response.getStreamToken(); + // A successful first write response means the stream is healthy, + // Note, that we could consider a successful handshake healthy, however, + // the write itself might be causing an error we want to back off from. + backoff.reset(); + + SnapshotVersion commitVersion = serializer.decodeVersion(response.getCommitTime()); + + int count = response.getWriteResultsCount(); + List results = new ArrayList<>(count); + for (int i = 0; i < count; i++) { + com.google.firestore.v1.WriteResult result = response.getWriteResults(i); + results.add(serializer.decodeMutationResult(result, commitVersion)); + } + listener.onWriteResponse(commitVersion, results); } } From 0d25982b1dad0f5717aeeec81e8058bb080a965b Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 5 Jun 2024 15:57:08 -0400 Subject: [PATCH 09/37] Whitespace --- .../java/com/google/firebase/firestore/remote/WriteStream.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java index 0727e23665d..8cf61c738f1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java @@ -165,8 +165,6 @@ public void onFirst(WriteResponse response) { // The first response is the handshake response handshakeComplete = true; - - } @Override From 3113171f7e1d1fae9861a306786ed2598d3ee727 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 14 Jun 2024 14:48:37 -0400 Subject: [PATCH 10/37] Refactor ComponentProvider --- .../firebase/firestore/AccessHelper.java | 4 + .../firestore/remote/RemoteStoreTest.java | 16 +--- .../firebase/firestore/remote/StreamTest.java | 70 +++++--------- .../testutil/IntegrationTestUtil.java | 33 ++++++- .../firebase/firestore/FirebaseFirestore.java | 40 ++++---- .../firestore/core/ComponentProvider.java | 91 ++++++++++--------- .../firestore/core/FirestoreClient.java | 32 +++---- .../core/MemoryComponentProvider.java | 26 ++---- .../core/SQLiteComponentProvider.java | 15 ++- .../remote/AndroidConnectivityMonitor.java | 4 +- .../firebase/firestore/remote/Datastore.java | 41 ++------- .../firestore/remote/FirestoreChannel.java | 26 ++++-- .../firestore/remote/GrpcCallProvider.java | 4 +- .../remote/RemoteComponenetProvider.java | 86 ++++++++++++++++++ .../firestore/remote/RemoteStore.java | 11 +-- .../remote/WatchChangeAggregator.java | 26 +++--- .../firestore/remote/MockDatastore.java | 27 +----- .../firestore/remote/RemoteEventTest.java | 12 ++- .../firestore/spec/MemorySpecTest.java | 11 ++- .../firestore/spec/SQLiteSpecTest.java | 7 +- .../firebase/firestore/spec/SpecTestCase.java | 27 ++++-- .../testutil/TestTargetMetadataProvider.java | 12 --- .../firebase/firestore/testutil/TestUtil.java | 16 +--- 23 files changed, 346 insertions(+), 291 deletions(-) create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java index b9d0bce0c55..9fec3d209f2 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java @@ -18,8 +18,10 @@ import com.google.firebase.FirebaseApp; import com.google.firebase.firestore.auth.CredentialsProvider; import com.google.firebase.firestore.auth.User; +import com.google.firebase.firestore.core.ComponentProvider; import com.google.firebase.firestore.model.DatabaseId; import com.google.firebase.firestore.util.AsyncQueue; +import com.google.firebase.firestore.util.Function; /** Gives access to package private methods in integration tests. */ public final class AccessHelper { @@ -32,6 +34,7 @@ public static FirebaseFirestore newFirebaseFirestore( CredentialsProvider authProvider, CredentialsProvider appCheckProvider, AsyncQueue asyncQueue, + Function componentProviderFactory, FirebaseApp firebaseApp, FirebaseFirestore.InstanceRegistry instanceRegistry) { return new FirebaseFirestore( @@ -41,6 +44,7 @@ public static FirebaseFirestore newFirebaseFirestore( authProvider, appCheckProvider, asyncQueue, + componentProviderFactory, firebaseApp, instanceRegistry, null); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java index bf2b97219f6..851b503c980 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java @@ -20,6 +20,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.auth.User; +import com.google.firebase.firestore.core.DatabaseInfo; import com.google.firebase.firestore.core.OnlineState; import com.google.firebase.firestore.local.LocalStore; import com.google.firebase.firestore.local.MemoryPersistence; @@ -40,14 +41,8 @@ public class RemoteStoreTest { @Test public void testRemoteStoreStreamStopsWhenNetworkUnreachable() { AsyncQueue testQueue = new AsyncQueue(); - Datastore datastore = - new Datastore( - IntegrationTestUtil.testEnvDatabaseInfo(), - testQueue, - null, - null, - ApplicationProvider.getApplicationContext(), - null); + RemoteSerializer serializer = new RemoteSerializer(IntegrationTestUtil.testEnvDatabaseId()); + Datastore datastore = new Datastore(testQueue, serializer, null); Semaphore networkChangeSemaphore = new Semaphore(0); RemoteStore.RemoteStoreCallback callback = new RemoteStore.RemoteStoreCallback() { @@ -75,12 +70,11 @@ public ImmutableSortedSet getRemoteKeysForTarget(int targetId) { }; FakeConnectivityMonitor connectivityMonitor = new FakeConnectivityMonitor(); - QueryEngine queryEngine = new QueryEngine(); Persistence persistence = MemoryPersistence.createEagerGcMemoryPersistence(); persistence.start(); - LocalStore localStore = new LocalStore(persistence, queryEngine, User.UNAUTHENTICATED); + LocalStore localStore = new LocalStore(persistence, new QueryEngine(), User.UNAUTHENTICATED); RemoteStore remoteStore = - new RemoteStore(callback, localStore, datastore, testQueue, connectivityMonitor); + new RemoteStore(IntegrationTestUtil.testEnvDatabaseId(), callback, localStore, datastore, testQueue, connectivityMonitor); waitFor(testQueue.enqueue(remoteStore::forceEnableNetwork)); drain(testQueue); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/StreamTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/StreamTest.java index c1fd0728933..4cb1529ca4c 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/StreamTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/StreamTest.java @@ -26,9 +26,11 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import androidx.annotation.NonNull; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; +import com.google.firebase.firestore.core.DatabaseInfo; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.MutationResult; @@ -109,14 +111,7 @@ public void onWriteResponse( /** Creates a WriteStream and gets it in a state that accepts mutations. */ private WriteStream createAndOpenWriteStream( AsyncQueue testQueue, StreamStatusCallback callback) { - Datastore datastore = - new Datastore( - IntegrationTestUtil.testEnvDatabaseInfo(), - testQueue, - new EmptyCredentialsProvider(), - new EmptyAppCheckTokenProvider(), - ApplicationProvider.getApplicationContext(), - null); + Datastore datastore = createTestDatastore(testQueue, null); final WriteStream writeStream = datastore.createWriteStream(callback); waitForWriteStreamOpen(testQueue, writeStream, callback); return writeStream; @@ -131,18 +126,25 @@ private void waitForWriteStreamOpen( waitFor(callback.handshakeSemaphore); } + @NonNull + private static Datastore createTestDatastore(AsyncQueue testQueue, GrpcMetadataProvider metadataProvider) { + DatabaseInfo databaseInfo = IntegrationTestUtil.testEnvDatabaseInfo(); + RemoteSerializer remoteSerializer = new RemoteSerializer(databaseInfo.getDatabaseId()); + FirestoreChannel firestoreChannel = new FirestoreChannel( + testQueue, + ApplicationProvider.getApplicationContext(), + new EmptyCredentialsProvider(), + new EmptyAppCheckTokenProvider(), + databaseInfo, + metadataProvider); + return new Datastore(testQueue, remoteSerializer, firestoreChannel); + } + @Test public void testWatchStreamStopBeforeHandshake() throws Exception { AsyncQueue testQueue = new AsyncQueue(); GrpcMetadataProvider mockGrpcProvider = mock(GrpcMetadataProvider.class); - Datastore datastore = - new Datastore( - IntegrationTestUtil.testEnvDatabaseInfo(), - testQueue, - new EmptyCredentialsProvider(), - new EmptyAppCheckTokenProvider(), - ApplicationProvider.getApplicationContext(), - mockGrpcProvider); + Datastore datastore = createTestDatastore(testQueue, mockGrpcProvider); StreamStatusCallback streamCallback = new StreamStatusCallback() {}; final WatchStream watchStream = datastore.createWatchStream(streamCallback); @@ -158,14 +160,7 @@ public void testWatchStreamStopBeforeHandshake() throws Exception { @Test public void testWriteStreamStopAfterHandshake() throws Exception { AsyncQueue testQueue = new AsyncQueue(); - Datastore datastore = - new Datastore( - IntegrationTestUtil.testEnvDatabaseInfo(), - testQueue, - new EmptyCredentialsProvider(), - new EmptyAppCheckTokenProvider(), - ApplicationProvider.getApplicationContext(), - null); + Datastore datastore = createTestDatastore(testQueue, null); final WriteStream[] writeStreamWrapper = new WriteStream[1]; StreamStatusCallback streamCallback = new StreamStatusCallback() { @@ -206,14 +201,7 @@ public void onWriteResponse( @Test public void testWriteStreamStopPartial() throws Exception { AsyncQueue testQueue = new AsyncQueue(); - Datastore datastore = - new Datastore( - IntegrationTestUtil.testEnvDatabaseInfo(), - testQueue, - new EmptyCredentialsProvider(), - new EmptyAppCheckTokenProvider(), - ApplicationProvider.getApplicationContext(), - null); + Datastore datastore = createTestDatastore(testQueue, null); StreamStatusCallback streamCallback = new StreamStatusCallback() {}; final WriteStream writeStream = datastore.createWriteStream(streamCallback); @@ -287,14 +275,7 @@ public void testStreamStaysIdle() throws Exception { public void testStreamRefreshesTokenUponExpiration() throws Exception { AsyncQueue testQueue = new AsyncQueue(); MockCredentialsProvider mockCredentialsProvider = new MockCredentialsProvider(); - Datastore datastore = - new Datastore( - IntegrationTestUtil.testEnvDatabaseInfo(), - testQueue, - mockCredentialsProvider, - new EmptyAppCheckTokenProvider(), - ApplicationProvider.getApplicationContext(), - null); + Datastore datastore = createTestDatastore(testQueue, null); StreamStatusCallback callback = new StreamStatusCallback(); WriteStream writeStream = datastore.createWriteStream(callback); waitForWriteStreamOpen(testQueue, writeStream, callback); @@ -317,14 +298,7 @@ public void testStreamRefreshesTokenUponExpiration() throws Exception { public void testTokenIsNotInvalidatedOnceStreamIsHealthy() throws Exception { AsyncQueue testQueue = new AsyncQueue(); MockCredentialsProvider mockCredentialsProvider = new MockCredentialsProvider(); - Datastore datastore = - new Datastore( - IntegrationTestUtil.testEnvDatabaseInfo(), - testQueue, - mockCredentialsProvider, - new EmptyAppCheckTokenProvider(), - ApplicationProvider.getApplicationContext(), - null); + Datastore datastore = createTestDatastore(testQueue, null); StreamStatusCallback callback = new StreamStatusCallback(); WriteStream writeStream = datastore.createWriteStream(callback); waitForWriteStreamOpen(testQueue, writeStream, callback); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index 0cde1711910..0fe49c20478 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -22,6 +22,8 @@ import android.content.Context; import android.os.StrictMode; + +import androidx.annotation.NonNull; import androidx.test.core.app.ApplicationProvider; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; @@ -36,12 +38,15 @@ import com.google.firebase.firestore.FirebaseFirestore; import com.google.firebase.firestore.FirebaseFirestoreSettings; import com.google.firebase.firestore.ListenerRegistration; +import com.google.firebase.firestore.MemoryCacheSettings; +import com.google.firebase.firestore.MemoryEagerGcSettings; import com.google.firebase.firestore.MetadataChanges; import com.google.firebase.firestore.Query; import com.google.firebase.firestore.QuerySnapshot; import com.google.firebase.firestore.Source; import com.google.firebase.firestore.WriteBatch; import com.google.firebase.firestore.auth.User; +import com.google.firebase.firestore.core.ComponentProvider; import com.google.firebase.firestore.core.DatabaseInfo; import com.google.firebase.firestore.model.DatabaseId; import com.google.firebase.firestore.testutil.provider.FirestoreProvider; @@ -87,6 +92,25 @@ public void changeUserTo(User user) { /** A set of helper methods for tests */ public class IntegrationTestUtil { + @NonNull + public static ComponentProvider.Configuration testConfiguration(AsyncQueue testQueue) { + return new ComponentProvider.Configuration( + ApplicationProvider.getApplicationContext(), + testQueue, + testEnvDatabaseInfo(), + User.UNAUTHENTICATED, + 100, + new FirebaseFirestoreSettings.Builder() + .setLocalCacheSettings(MemoryCacheSettings.newBuilder() + .setGcSettings(MemoryEagerGcSettings.newBuilder().build()) + .build()) + .build(), + new EmptyCredentialsProvider(), + new EmptyAppCheckTokenProvider(), + null + ); + } + public enum TargetBackend { EMULATOR, QA, @@ -170,14 +194,20 @@ public static TargetBackend getTargetBackend() { } } + @NonNull public static DatabaseInfo testEnvDatabaseInfo() { return new DatabaseInfo( - DatabaseId.forProject(provider.projectId()), + testEnvDatabaseId(), "test-persistenceKey", getFirestoreHost(), getSslEnabled()); } + @NonNull + public static DatabaseId testEnvDatabaseId() { + return DatabaseId.forProject(provider.projectId()); + } + public static FirebaseFirestoreSettings newTestSettings() { Logger.debug("IntegrationTestUtil", "target backend is: %s", backend.name()); FirebaseFirestoreSettings.Builder settings = new FirebaseFirestoreSettings.Builder(); @@ -315,6 +345,7 @@ public static FirebaseFirestore testFirestore( MockCredentialsProvider.instance(), new EmptyAppCheckTokenProvider(), asyncQueue, + ComponentProvider::defaultFactory, /*firebaseApp=*/ null, /*instanceRegistry=*/ (dbId) -> {}); waitFor(firestore.clearPersistence()); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index 3af05a167ab..f304bfff974 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -39,6 +39,7 @@ import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.ActivityScope; import com.google.firebase.firestore.core.AsyncEventListener; +import com.google.firebase.firestore.core.ComponentProvider; import com.google.firebase.firestore.core.DatabaseInfo; import com.google.firebase.firestore.core.FirestoreClient; import com.google.firebase.firestore.local.SQLitePersistence; @@ -75,6 +76,8 @@ */ public class FirebaseFirestore { + private final Function componentProviderFactory; + /** * Provides a registry management interface for {@code FirebaseFirestore} instances. * @@ -205,18 +208,17 @@ static FirebaseFirestore newInstance( // so there is no need to include it in the persistence key. String persistenceKey = app.getName(); - FirebaseFirestore firestore = - new FirebaseFirestore( - context, - databaseId, - persistenceKey, - authProvider, - appCheckProvider, - queue, - app, - instanceRegistry, - metadataProvider); - return firestore; + return new FirebaseFirestore( + context, + databaseId, + persistenceKey, + authProvider, + appCheckProvider, + queue, + ComponentProvider::defaultFactory, + app, + instanceRegistry, + metadataProvider); } @VisibleForTesting @@ -227,6 +229,7 @@ static FirebaseFirestore newInstance( CredentialsProvider authProvider, CredentialsProvider appCheckProvider, AsyncQueue asyncQueue, + @NonNull Function componentProviderFactory, @Nullable FirebaseApp firebaseApp, InstanceRegistry instanceRegistry, @Nullable GrpcMetadataProvider metadataProvider) { @@ -237,6 +240,7 @@ static FirebaseFirestore newInstance( this.authProvider = checkNotNull(authProvider); this.appCheckProvider = checkNotNull(appCheckProvider); this.asyncQueue = checkNotNull(asyncQueue); + this.componentProviderFactory = checkNotNull(componentProviderFactory); // NOTE: We allow firebaseApp to be null in tests only. this.firebaseApp = firebaseApp; this.instanceRegistry = instanceRegistry; @@ -256,10 +260,9 @@ public FirebaseFirestoreSettings getFirestoreSettings() { * can only be called before calling any other methods on this object. */ public void setFirestoreSettings(@NonNull FirebaseFirestoreSettings settings) { - settings = mergeEmulatorSettings(settings, this.emulatorSettings); - + checkNotNull(settings, "Provided settings must not be null."); synchronized (databaseId) { - checkNotNull(settings, "Provided settings must not be null."); + settings = mergeEmulatorSettings(settings, emulatorSettings); // As a special exception, don't throw if the same settings are passed repeatedly. This // should make it simpler to get a Firestore instance in an activity. @@ -288,8 +291,8 @@ public void useEmulator(@NonNull String host, int port) { "Cannot call useEmulator() after instance has already been initialized."); } - this.emulatorSettings = new EmulatedServiceSettings(host, port); - this.settings = mergeEmulatorSettings(this.settings, this.emulatorSettings); + emulatorSettings = new EmulatedServiceSettings(host, port); + settings = mergeEmulatorSettings(settings, emulatorSettings); } private void ensureClientConfigured() { @@ -312,7 +315,8 @@ private void ensureClientConfigured() { authProvider, appCheckProvider, asyncQueue, - metadataProvider); + metadataProvider, + componentProviderFactory.apply(settings)); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ComponentProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ComponentProvider.java index e63af145978..9b5794a1a0b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ComponentProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ComponentProvider.java @@ -14,11 +14,17 @@ package com.google.firebase.firestore.core; +import static com.google.firebase.firestore.util.Assert.hardAssert; import static com.google.firebase.firestore.util.Assert.hardAssertNonNull; import android.content.Context; + +import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; + import com.google.firebase.firestore.FirebaseFirestoreSettings; +import com.google.firebase.firestore.auth.CredentialsProvider; import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.local.IndexBackfiller; import com.google.firebase.firestore.local.LocalStore; @@ -26,6 +32,9 @@ import com.google.firebase.firestore.local.Scheduler; import com.google.firebase.firestore.remote.ConnectivityMonitor; import com.google.firebase.firestore.remote.Datastore; +import com.google.firebase.firestore.remote.GrpcMetadataProvider; +import com.google.firebase.firestore.remote.RemoteComponenetProvider; +import com.google.firebase.firestore.remote.RemoteSerializer; import com.google.firebase.firestore.remote.RemoteStore; import com.google.firebase.firestore.util.AsyncQueue; @@ -36,70 +45,72 @@ */ public abstract class ComponentProvider { + private RemoteComponenetProvider remoteProvider = new RemoteComponenetProvider(); private Persistence persistence; private LocalStore localStore; private SyncEngine syncEngine; private RemoteStore remoteStore; private EventManager eventManager; - private ConnectivityMonitor connectivityMonitor; @Nullable private IndexBackfiller indexBackfiller; @Nullable private Scheduler garbageCollectionScheduler; + @NonNull + public static ComponentProvider defaultFactory(@NonNull FirebaseFirestoreSettings settings) { + return settings.isPersistenceEnabled() + ? new SQLiteComponentProvider() + : new MemoryComponentProvider(); + } + /** Configuration options for the component provider. */ - public static class Configuration { + public static final class Configuration { + + public final Context context; + public final AsyncQueue asyncQueue; + public final DatabaseInfo databaseInfo; + public final User initialUser; + public final int maxConcurrentLimboResolutions; + public final FirebaseFirestoreSettings settings; + public final CredentialsProvider authProvider; + public final CredentialsProvider appCheckProvider; - private final Context context; - private final AsyncQueue asyncQueue; - private final DatabaseInfo databaseInfo; - private final Datastore datastore; - private final User initialUser; - private final int maxConcurrentLimboResolutions; - private final FirebaseFirestoreSettings settings; + @Nullable + public final GrpcMetadataProvider metadataProvider; public Configuration( Context context, AsyncQueue asyncQueue, DatabaseInfo databaseInfo, - Datastore datastore, User initialUser, int maxConcurrentLimboResolutions, - FirebaseFirestoreSettings settings) { + FirebaseFirestoreSettings settings, + CredentialsProvider authProvider, + CredentialsProvider appCheckProvider, + @Nullable GrpcMetadataProvider metadataProvider + ) { this.context = context; this.asyncQueue = asyncQueue; this.databaseInfo = databaseInfo; - this.datastore = datastore; this.initialUser = initialUser; this.maxConcurrentLimboResolutions = maxConcurrentLimboResolutions; this.settings = settings; + this.authProvider = authProvider; + this.appCheckProvider = appCheckProvider; + this.metadataProvider = metadataProvider; } + } - FirebaseFirestoreSettings getSettings() { - return settings; - } - - AsyncQueue getAsyncQueue() { - return asyncQueue; - } - - DatabaseInfo getDatabaseInfo() { - return databaseInfo; - } - - Datastore getDatastore() { - return datastore; - } - - User getInitialUser() { - return initialUser; - } + @VisibleForTesting + public void setRemoteProvider(RemoteComponenetProvider remoteProvider) { + hardAssert(remoteStore == null, "cannot set remoteProvider after initialize"); + this.remoteProvider = remoteProvider; + } - int getMaxConcurrentLimboResolutions() { - return maxConcurrentLimboResolutions; - } + public RemoteSerializer getRemoteSerializer() { + return remoteProvider.getRemoteSerializer(); + } - Context getContext() { - return context; - } + public Datastore getDatastore() { + return remoteProvider.getDatastore(); } public Persistence getPersistence() { @@ -133,7 +144,7 @@ public EventManager getEventManager() { } protected ConnectivityMonitor getConnectivityMonitor() { - return hardAssertNonNull(connectivityMonitor, "connectivityMonitor not initialized yet"); + return remoteProvider.getConnectivityMonitor(); } public void initialize(Configuration configuration) { @@ -146,10 +157,10 @@ public void initialize(Configuration configuration) { * *

To catch incorrect order, all getX methods have runtime check for null. */ + remoteProvider.initialize(configuration); persistence = createPersistence(configuration); persistence.start(); localStore = createLocalStore(configuration); - connectivityMonitor = createConnectivityMonitor(configuration); remoteStore = createRemoteStore(configuration); syncEngine = createSyncEngine(configuration); eventManager = createEventManager(configuration); @@ -167,8 +178,6 @@ public void initialize(Configuration configuration) { protected abstract LocalStore createLocalStore(Configuration configuration); - protected abstract ConnectivityMonitor createConnectivityMonitor(Configuration configuration); - protected abstract Persistence createPersistence(Configuration configuration); protected abstract RemoteStore createRemoteStore(Configuration configuration); 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 38b0adef065..6d411982207 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 @@ -44,7 +44,6 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldIndex; import com.google.firebase.firestore.model.mutation.Mutation; -import com.google.firebase.firestore.remote.Datastore; import com.google.firebase.firestore.remote.GrpcMetadataProvider; import com.google.firebase.firestore.remote.RemoteSerializer; import com.google.firebase.firestore.remote.RemoteStore; @@ -72,8 +71,6 @@ public final class FirestoreClient { private final CredentialsProvider appCheckProvider; private final AsyncQueue asyncQueue; private final BundleSerializer bundleSerializer; - private final GrpcMetadataProvider metadataProvider; - private Persistence persistence; private LocalStore localStore; private RemoteStore remoteStore; @@ -90,13 +87,13 @@ public FirestoreClient( FirebaseFirestoreSettings settings, CredentialsProvider authProvider, CredentialsProvider appCheckProvider, - final AsyncQueue asyncQueue, - @Nullable GrpcMetadataProvider metadataProvider) { + AsyncQueue asyncQueue, + @Nullable GrpcMetadataProvider metadataProvider, + ComponentProvider componentProvider) { this.databaseInfo = databaseInfo; this.authProvider = authProvider; this.appCheckProvider = appCheckProvider; this.asyncQueue = asyncQueue; - this.metadataProvider = metadataProvider; this.bundleSerializer = new BundleSerializer(new RemoteSerializer(databaseInfo.getDatabaseId())); @@ -111,7 +108,7 @@ public FirestoreClient( try { // Block on initial user being available User initialUser = Tasks.await(firstUser.getTask()); - initialize(context, initialUser, settings); + initialize(context, initialUser, settings, componentProvider, metadataProvider); } catch (InterruptedException | ExecutionException e) { throw new RuntimeException(e); } @@ -270,29 +267,28 @@ public Task waitForPendingWrites() { return source.getTask(); } - private void initialize(Context context, User user, FirebaseFirestoreSettings settings) { + private void initialize( + Context context, + User user, + FirebaseFirestoreSettings settings, + ComponentProvider provider, + GrpcMetadataProvider metadataProvider) { // Note: The initialization work must all be synchronous (we can't dispatch more work) since // external write/listen operations could get queued to run before that subsequent work // completes. Logger.debug(LOG_TAG, "Initializing. user=%s", user.getUid()); - Datastore datastore = - new Datastore( - databaseInfo, asyncQueue, authProvider, appCheckProvider, context, metadataProvider); ComponentProvider.Configuration configuration = new ComponentProvider.Configuration( context, asyncQueue, databaseInfo, - datastore, user, MAX_CONCURRENT_LIMBO_RESOLUTIONS, - settings); - - ComponentProvider provider = - settings.isPersistenceEnabled() - ? new SQLiteComponentProvider() - : new MemoryComponentProvider(); + settings, + authProvider, + appCheckProvider, + metadataProvider); provider.initialize(configuration); persistence = provider.getPersistence(); gcScheduler = provider.getGarbageCollectionScheduler(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/MemoryComponentProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/MemoryComponentProvider.java index fc8180b6937..6781fad65de 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/MemoryComponentProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/MemoryComponentProvider.java @@ -29,9 +29,7 @@ import com.google.firebase.firestore.local.Scheduler; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.mutation.MutationBatchResult; -import com.google.firebase.firestore.remote.AndroidConnectivityMonitor; import com.google.firebase.firestore.remote.RemoteEvent; -import com.google.firebase.firestore.remote.RemoteSerializer; import com.google.firebase.firestore.remote.RemoteStore; import io.grpc.Status; @@ -60,12 +58,7 @@ protected EventManager createEventManager(Configuration configuration) { @Override protected LocalStore createLocalStore(Configuration configuration) { - return new LocalStore(getPersistence(), new QueryEngine(), configuration.getInitialUser()); - } - - @Override - protected AndroidConnectivityMonitor createConnectivityMonitor(Configuration configuration) { - return new AndroidConnectivityMonitor(configuration.getContext()); + return new LocalStore(getPersistence(), new QueryEngine(), configuration.initialUser); } private boolean isMemoryLruGcEnabled(FirebaseFirestoreSettings settings) { @@ -80,13 +73,11 @@ private boolean isMemoryLruGcEnabled(FirebaseFirestoreSettings settings) { @Override protected Persistence createPersistence(Configuration configuration) { - if (isMemoryLruGcEnabled(configuration.getSettings())) { - LocalSerializer serializer = - new LocalSerializer( - new RemoteSerializer(configuration.getDatabaseInfo().getDatabaseId())); + if (isMemoryLruGcEnabled(configuration.settings)) { + LocalSerializer serializer = new LocalSerializer(getRemoteSerializer()); LruGarbageCollector.Params params = LruGarbageCollector.Params.WithCacheSizeBytes( - configuration.getSettings().getCacheSizeBytes()); + configuration.settings.getCacheSizeBytes()); return MemoryPersistence.createLruGcMemoryPersistence(params, serializer); } @@ -96,10 +87,11 @@ protected Persistence createPersistence(Configuration configuration) { @Override protected RemoteStore createRemoteStore(Configuration configuration) { return new RemoteStore( + configuration.databaseInfo.getDatabaseId(), new RemoteStoreCallback(), getLocalStore(), - configuration.getDatastore(), - configuration.getAsyncQueue(), + getDatastore(), + configuration.asyncQueue, getConnectivityMonitor()); } @@ -108,8 +100,8 @@ protected SyncEngine createSyncEngine(Configuration configuration) { return new SyncEngine( getLocalStore(), getRemoteStore(), - configuration.getInitialUser(), - configuration.getMaxConcurrentLimboResolutions()); + configuration.initialUser, + configuration.maxConcurrentLimboResolutions); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java index e77d3a64a29..99586a8784a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java @@ -21,7 +21,6 @@ import com.google.firebase.firestore.local.Persistence; import com.google.firebase.firestore.local.SQLitePersistence; import com.google.firebase.firestore.local.Scheduler; -import com.google.firebase.firestore.remote.RemoteSerializer; /** Provides all components needed for Firestore with SQLite persistence. */ public class SQLiteComponentProvider extends MemoryComponentProvider { @@ -30,25 +29,25 @@ public class SQLiteComponentProvider extends MemoryComponentProvider { protected Scheduler createGarbageCollectionScheduler(Configuration configuration) { LruDelegate lruDelegate = ((SQLitePersistence) getPersistence()).getReferenceDelegate(); LruGarbageCollector gc = lruDelegate.getGarbageCollector(); - return gc.newScheduler(configuration.getAsyncQueue(), getLocalStore()); + return gc.newScheduler(configuration.asyncQueue, getLocalStore()); } @Override protected IndexBackfiller createIndexBackfiller(Configuration configuration) { - return new IndexBackfiller(getPersistence(), configuration.getAsyncQueue(), getLocalStore()); + return new IndexBackfiller(getPersistence(), configuration.asyncQueue, getLocalStore()); } @Override protected Persistence createPersistence(Configuration configuration) { LocalSerializer serializer = - new LocalSerializer(new RemoteSerializer(configuration.getDatabaseInfo().getDatabaseId())); + new LocalSerializer(getRemoteSerializer()); LruGarbageCollector.Params params = LruGarbageCollector.Params.WithCacheSizeBytes( - configuration.getSettings().getCacheSizeBytes()); + configuration.settings.getCacheSizeBytes()); return new SQLitePersistence( - configuration.getContext(), - configuration.getDatabaseInfo().getPersistenceKey(), - configuration.getDatabaseInfo().getDatabaseId(), + configuration.context, + configuration.databaseInfo.getPersistenceKey(), + configuration.databaseInfo.getDatabaseId(), serializer, params); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/AndroidConnectivityMonitor.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/AndroidConnectivityMonitor.java index e8077e46089..4a4384651b0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/AndroidConnectivityMonitor.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/AndroidConnectivityMonitor.java @@ -43,7 +43,7 @@ *

Implementation note: Most of the code here was shamelessly stolen from * https://github.com/grpc/grpc-java/blob/master/android/src/main/java/io/grpc/android/AndroidChannelBuilder.java */ -public final class AndroidConnectivityMonitor implements ConnectivityMonitor { +final class AndroidConnectivityMonitor implements ConnectivityMonitor { private static final String LOG_TAG = "AndroidConnectivityMonitor"; @@ -52,7 +52,7 @@ public final class AndroidConnectivityMonitor implements ConnectivityMonitor { @Nullable private Runnable unregisterRunnable; private final List> callbacks = new ArrayList<>(); - public AndroidConnectivityMonitor(Context context) { + AndroidConnectivityMonitor(Context context) { // This notnull restriction could be eliminated... the pre-N method doesn't // require a Context, and we could use that even on N+ if necessary. hardAssert(context != null, "Context must be non-null"); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java index fe8ddfa06e0..b9f22ed74cb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java @@ -17,16 +17,13 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import static com.google.firebase.firestore.util.Util.exceptionFromStatus; -import android.content.Context; import android.os.Build; -import androidx.annotation.Nullable; + +import androidx.annotation.VisibleForTesting; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.firebase.firestore.AggregateField; import com.google.firebase.firestore.FirebaseFirestoreException; -import com.google.firebase.firestore.auth.CredentialsProvider; -import com.google.firebase.firestore.auth.User; -import com.google.firebase.firestore.core.DatabaseInfo; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MutableDocument; @@ -89,50 +86,26 @@ public class Datastore { "x-google-service", "x-google-gfe-request-trace")); - private final DatabaseInfo databaseInfo; - private final RemoteSerializer serializer; + protected final RemoteSerializer serializer; private final AsyncQueue workerQueue; private final FirestoreChannel channel; - public Datastore( - DatabaseInfo databaseInfo, - AsyncQueue workerQueue, - CredentialsProvider authProvider, - CredentialsProvider appCheckProvider, - Context context, - @Nullable GrpcMetadataProvider metadataProvider) { - this.databaseInfo = databaseInfo; + Datastore(AsyncQueue workerQueue, RemoteSerializer serializer, FirestoreChannel channel) { this.workerQueue = workerQueue; - this.serializer = new RemoteSerializer(databaseInfo.getDatabaseId()); - this.channel = - initializeChannel( - databaseInfo, workerQueue, authProvider, appCheckProvider, context, metadataProvider); - } - - FirestoreChannel initializeChannel( - DatabaseInfo databaseInfo, - AsyncQueue workerQueue, - CredentialsProvider authProvider, - CredentialsProvider appCheckProvider, - Context context, - @Nullable GrpcMetadataProvider metadataProvider) { - return new FirestoreChannel( - workerQueue, context, authProvider, appCheckProvider, databaseInfo, metadataProvider); + this.serializer = serializer; + this.channel = channel; } void shutdown() { channel.shutdown(); } + @VisibleForTesting(otherwise = VisibleForTesting.NONE) AsyncQueue getWorkerQueue() { return workerQueue; } - DatabaseInfo getDatabaseInfo() { - return databaseInfo; - } - /** Creates a new WatchStream that is still unstarted but uses a common shared channel */ WatchStream createWatchStream(WatchStream.Callback listener) { return new WatchStream(channel, workerQueue, serializer, listener); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/FirestoreChannel.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/FirestoreChannel.java index ec138e72976..51b2ea26a66 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/FirestoreChannel.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/FirestoreChannel.java @@ -86,21 +86,35 @@ public void onClose(Status status) {} CredentialsProvider appCheckProvider, DatabaseInfo databaseInfo, GrpcMetadataProvider metadataProvider) { + this(asyncQueue, authProvider, appCheckProvider, databaseInfo.getDatabaseId(), metadataProvider, + getGrpcCallProvider(asyncQueue, context, authProvider, appCheckProvider, databaseInfo)); + } + + FirestoreChannel( + AsyncQueue asyncQueue, + CredentialsProvider authProvider, + CredentialsProvider appCheckProvider, + DatabaseId databaseId, + GrpcMetadataProvider metadataProvider, + GrpcCallProvider grpcCallProvider) { this.asyncQueue = asyncQueue; this.metadataProvider = metadataProvider; this.authProvider = authProvider; this.appCheckProvider = appCheckProvider; - - FirestoreCallCredentials firestoreHeaders = - new FirestoreCallCredentials(authProvider, appCheckProvider); - this.callProvider = new GrpcCallProvider(asyncQueue, context, databaseInfo, firestoreHeaders); - - DatabaseId databaseId = databaseInfo.getDatabaseId(); + this.callProvider = grpcCallProvider; this.resourcePrefixValue = String.format( "projects/%s/databases/%s", databaseId.getProjectId(), databaseId.getDatabaseId()); } + private static GrpcCallProvider getGrpcCallProvider( + AsyncQueue asyncQueue, Context context, CredentialsProvider authProvider, + CredentialsProvider appCheckProvider, DatabaseInfo databaseInfo) { + FirestoreCallCredentials firestoreHeaders = + new FirestoreCallCredentials(authProvider, appCheckProvider); + return new GrpcCallProvider(asyncQueue, context, databaseInfo, firestoreHeaders); + } + /** * Shuts down the grpc channel. This is not reversible and renders the FirestoreChannel unusable. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java index 4109f30ddee..4cb9f9347c3 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/GrpcCallProvider.java @@ -117,7 +117,7 @@ private ManagedChannel initChannel(Context context, DatabaseInfo databaseInfo) { } /** Creates a new ClientCall. */ - Task> createClientCall( + public Task> createClientCall( MethodDescriptor methodDescriptor) { return channelTask.continueWithTask( asyncQueue.getExecutor(), @@ -125,7 +125,7 @@ Task> createClientCall( } /** Shuts down the gRPC channel and the internal worker queue. */ - void shutdown() { + public void shutdown() { // Handling shutdown synchronously to avoid re-enqueuing on the AsyncQueue after shutdown has // started. ManagedChannel channel; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java new file mode 100644 index 00000000000..3048532a169 --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java @@ -0,0 +1,86 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore.remote; + +import static com.google.firebase.firestore.util.Assert.hardAssertNonNull; + +import com.google.firebase.firestore.core.ComponentProvider; +import com.google.firebase.firestore.util.AsyncQueue; + +public class RemoteComponenetProvider { + + + private GrpcCallProvider grpcCallProvider; + private RemoteSerializer remoteSerializer; + private FirestoreChannel firestoreChannel; + private Datastore datastore; + private ConnectivityMonitor connectivityMonitor; + + public void initialize(ComponentProvider.Configuration configuration) { + remoteSerializer = createRemoteSerializer(configuration); + grpcCallProvider = createGrpcCallProvider(configuration); + firestoreChannel = createFirestoreChannel(configuration); + datastore = createDatastore(configuration); + connectivityMonitor = createConnectivityMonitor(configuration); + } + + public GrpcCallProvider getGrpcCallProvider() { + return hardAssertNonNull(grpcCallProvider, "grpcCallProvider not initialized yet"); + } + + public RemoteSerializer getRemoteSerializer() { + return hardAssertNonNull(remoteSerializer, "remoteSerializer not initialized yet"); + } + + public FirestoreChannel getFirestoreChannel() { + return hardAssertNonNull(firestoreChannel, "firestoreChannel not initialized yet"); + } + + public Datastore getDatastore() { + return hardAssertNonNull(datastore, "datastore not initialized yet"); + } + + public ConnectivityMonitor getConnectivityMonitor() { + return hardAssertNonNull(connectivityMonitor, "connectivityMonitor not initialized yet"); + } + + protected GrpcCallProvider createGrpcCallProvider(ComponentProvider.Configuration configuration) { + FirestoreCallCredentials firestoreHeaders = + new FirestoreCallCredentials(configuration.authProvider, configuration.appCheckProvider); + return new GrpcCallProvider(configuration.asyncQueue, configuration.context, configuration.databaseInfo, firestoreHeaders); + } + + protected RemoteSerializer createRemoteSerializer(ComponentProvider.Configuration configuration) { + return new RemoteSerializer(configuration.databaseInfo.getDatabaseId()); + } + + protected FirestoreChannel createFirestoreChannel(ComponentProvider.Configuration configuration) { + return new FirestoreChannel( + configuration.asyncQueue, + configuration.authProvider, + configuration.appCheckProvider, + configuration.databaseInfo.getDatabaseId(), + configuration.metadataProvider, + getGrpcCallProvider()); + } + + protected Datastore createDatastore(ComponentProvider.Configuration configuration) { + return new Datastore(configuration.asyncQueue, getRemoteSerializer(), getFirestoreChannel()); + } + + protected ConnectivityMonitor createConnectivityMonitor(ComponentProvider.Configuration configuration) { + return new AndroidConnectivityMonitor(configuration.context); + } +} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java index eda25fa2792..ab1cf942127 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java @@ -65,6 +65,8 @@ public final class RemoteStore implements WatchChangeAggregator.TargetMetadataPr /** The log tag to use for this class. */ private static final String LOG_TAG = "RemoteStore"; + private final DatabaseId databaseId; + /** A callback interface for events from RemoteStore. */ public interface RemoteStoreCallback { /** @@ -153,11 +155,13 @@ public interface RemoteStoreCallback { private final Deque writePipeline; public RemoteStore( + DatabaseId databaseId, RemoteStoreCallback remoteStoreCallback, LocalStore localStore, Datastore datastore, AsyncQueue workerQueue, ConnectivityMonitor connectivityMonitor) { + this.databaseId = databaseId; this.remoteStoreCallback = remoteStoreCallback; this.localStore = localStore; this.datastore = datastore; @@ -443,7 +447,7 @@ private void startWatchStream() { hardAssert( shouldStartWatchStream(), "startWatchStream() called when shouldStartWatchStream() is false."); - watchChangeAggregator = new WatchChangeAggregator(this); + watchChangeAggregator = new WatchChangeAggregator(databaseId, this); watchStream.start(); onlineStateTracker.handleWatchStreamStart(); @@ -762,11 +766,6 @@ public TargetData getTargetDataForTarget(int targetId) { return this.listenTargets.get(targetId); } - @Override - public DatabaseId getDatabaseId() { - return this.datastore.getDatabaseInfo().getDatabaseId(); - } - public Task> runAggregateQuery( Query query, List aggregateFields) { if (canUseNetwork()) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java index d5064fa3d8e..eb28d9f827d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java @@ -17,6 +17,8 @@ import static com.google.firebase.firestore.util.Assert.fail; import static com.google.firebase.firestore.util.Assert.hardAssert; +import android.provider.ContactsContract; + import androidx.annotation.Nullable; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.DocumentViewChange; @@ -59,9 +61,6 @@ public interface TargetMetadataProvider { */ @Nullable TargetData getTargetDataForTarget(int targetId); - - /** Returns the database ID of the Firestore instance. */ - DatabaseId getDatabaseId(); } private final TargetMetadataProvider targetMetadataProvider; @@ -81,6 +80,8 @@ public interface TargetMetadataProvider { */ private Map pendingTargetResets = new HashMap<>(); + private final DatabaseId databaseId; + /** The log tag to use for this class. */ private static final String LOG_TAG = "WatchChangeAggregator"; @@ -91,7 +92,8 @@ enum BloomFilterApplicationStatus { FALSE_POSITIVE } - public WatchChangeAggregator(TargetMetadataProvider targetMetadataProvider) { + public WatchChangeAggregator(DatabaseId databaseId, TargetMetadataProvider targetMetadataProvider) { + this.databaseId = databaseId; this.targetMetadataProvider = targetMetadataProvider; } @@ -240,7 +242,7 @@ public void handleExistenceFilter(ExistenceFilterWatchChange watchChange) { TestingHooks.ExistenceFilterMismatchInfo.from( currentSize, watchChange.getExistenceFilter(), - targetMetadataProvider.getDatabaseId(), + databaseId, bloomFilter, status)); } @@ -299,15 +301,13 @@ private int filterRemovedDocuments(BloomFilter bloomFilter, int targetId) { ImmutableSortedSet existingKeys = targetMetadataProvider.getRemoteKeysForTarget(targetId); int removalCount = 0; + String rootDocumentsPath = "projects/" + + databaseId.getProjectId() + + "/databases/" + + databaseId.getDatabaseId() + + "/documents/"; for (DocumentKey key : existingKeys) { - DatabaseId databaseId = targetMetadataProvider.getDatabaseId(); - String documentPath = - "projects/" - + databaseId.getProjectId() - + "/databases/" - + databaseId.getDatabaseId() - + "/documents/" - + key.getPath().canonicalString(); + String documentPath = rootDocumentsPath + key.getPath().canonicalString(); if (!bloomFilter.mightContain(documentPath)) { this.removeDocumentFromTarget(targetId, key, /*updatedDocument=*/ null); removalCount++; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/MockDatastore.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/MockDatastore.java index 91077548585..53dc0b91dfa 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/MockDatastore.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/MockDatastore.java @@ -16,10 +16,6 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; -import android.content.Context; -import androidx.annotation.Nullable; -import com.google.firebase.firestore.auth.CredentialsProvider; -import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.DatabaseInfo; import com.google.firebase.firestore.local.TargetData; import com.google.firebase.firestore.model.SnapshotVersion; @@ -27,8 +23,6 @@ import com.google.firebase.firestore.model.mutation.MutationResult; import com.google.firebase.firestore.remote.WatchChange.WatchTargetChange; import com.google.firebase.firestore.spec.SpecTestCase; -import com.google.firebase.firestore.testutil.EmptyAppCheckTokenProvider; -import com.google.firebase.firestore.testutil.EmptyCredentialsProvider; import com.google.firebase.firestore.util.AsyncQueue; import com.google.firebase.firestore.util.Util; import io.grpc.Status; @@ -217,31 +211,14 @@ int getWritesSent() { private MockWatchStream watchStream; private MockWriteStream writeStream; - private final RemoteSerializer serializer; - private int writeStreamRequestCount; private int watchStreamRequestCount; - public MockDatastore(DatabaseInfo databaseInfo, AsyncQueue workerQueue, Context context) { + public MockDatastore(DatabaseInfo databaseInfo, AsyncQueue workerQueue) { super( - databaseInfo, workerQueue, - new EmptyCredentialsProvider(), - new EmptyAppCheckTokenProvider(), - context, + new RemoteSerializer(databaseInfo.getDatabaseId()), null); - this.serializer = new RemoteSerializer(getDatabaseInfo().getDatabaseId()); - } - - @Override - FirestoreChannel initializeChannel( - DatabaseInfo databaseInfo, - AsyncQueue workerQueue, - CredentialsProvider authCredentialsProvider, - CredentialsProvider appCheckTokenProvider, - Context context, - @Nullable GrpcMetadataProvider metadataProvider) { - return null; } @Override diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteEventTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteEventTest.java index 57d1d6c0d83..27c01d6699d 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteEventTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteEventTest.java @@ -33,6 +33,7 @@ import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.local.TargetData; +import com.google.firebase.firestore.model.DatabaseId; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MutableDocument; import com.google.firebase.firestore.remote.WatchChange.DocumentChange; @@ -87,7 +88,12 @@ private WatchChangeAggregator createAggregator( Map outstandingResponses, ImmutableSortedSet existingKeys, WatchChange... watchChanges) { - WatchChangeAggregator aggregator = new WatchChangeAggregator(targetMetadataProvider); + + // The BloomFilter proto is created based on the document paths that are constructed using the + // pattern: "projects/test-project/databases/test-database/documents/"+document_key. + DatabaseId databaseId = DatabaseId.forDatabase("test-project", "test-database"); + + WatchChangeAggregator aggregator = new WatchChangeAggregator(databaseId, targetMetadataProvider); List targetIds = new ArrayList<>(); @@ -469,10 +475,6 @@ public void existenceFilterMismatchWithSuccessfulBloomFilterApplication() { WatchChange change2 = new DocumentChange(asList(1), emptyList(), doc2.getKey(), doc2); WatchChange change3 = new WatchTargetChange(WatchTargetChangeType.Current, asList(1)); - // The BloomFilter proto value below is created based on the document paths that are constructed - // using the pattern: "projects/test-project/databases/test-database/documents/"+document_key. - // Override the default database ID to ensure that the document path matches the pattern above. - targetMetadataProvider.setDatabaseId("test-project", "test-database"); WatchChangeAggregator aggregator = createAggregator( targetMap, diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java index 35383d7a6a6..187da0592a3 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java @@ -20,8 +20,9 @@ import com.google.firebase.firestore.local.LruGarbageCollector; import com.google.firebase.firestore.local.MemoryPersistence; import com.google.firebase.firestore.local.Persistence; -import com.google.firebase.firestore.model.DatabaseId; -import com.google.firebase.firestore.remote.RemoteSerializer; +import com.google.firebase.firestore.remote.Datastore; +import com.google.firebase.firestore.remote.MockDatastore; +import com.google.firebase.firestore.remote.RemoteComponenetProvider; import java.util.Set; import org.junit.runner.RunWith; import org.robolectric.RobolectricTestRunner; @@ -40,7 +41,7 @@ protected boolean isExcluded(Set tags) { @Override protected MemoryComponentProvider initializeComponentProvider( - ComponentProvider.Configuration configuration, boolean useEagerGc) { + RemoteComponenetProvider remoteProvider, ComponentProvider.Configuration configuration, boolean useEagerGc) { MemoryComponentProvider provider = new MemoryComponentProvider() { @Override @@ -48,13 +49,13 @@ protected Persistence createPersistence(Configuration configuration) { if (useEagerGc) { return MemoryPersistence.createEagerGcMemoryPersistence(); } else { - DatabaseId databaseId = DatabaseId.forProject("projectId"); - LocalSerializer serializer = new LocalSerializer(new RemoteSerializer(databaseId)); + LocalSerializer serializer = new LocalSerializer(getRemoteSerializer()); return MemoryPersistence.createLruGcMemoryPersistence( LruGarbageCollector.Params.Default(), serializer); } } }; + provider.setRemoteProvider(remoteProvider); provider.initialize(configuration); return provider; } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java index 9cc38490ea0..969774cb33c 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java @@ -16,6 +16,8 @@ import com.google.firebase.firestore.core.ComponentProvider; import com.google.firebase.firestore.core.SQLiteComponentProvider; +import com.google.firebase.firestore.remote.RemoteComponenetProvider; + import java.util.Set; import org.json.JSONObject; import org.junit.runner.RunWith; @@ -35,8 +37,11 @@ protected void specSetUp(JSONObject config) { @Override protected SQLiteComponentProvider initializeComponentProvider( - ComponentProvider.Configuration configuration, boolean garbageCollectionEnabled) { + RemoteComponenetProvider remoteProvider, + ComponentProvider.Configuration configuration, + boolean garbageCollectionEnabled) { SQLiteComponentProvider provider = new SQLiteComponentProvider(); + provider.setRemoteProvider(remoteProvider); provider.initialize(configuration); return provider; } 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 a2a57e9cc48..8a9927abca6 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 @@ -71,8 +71,10 @@ import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.MutationBatchResult; import com.google.firebase.firestore.model.mutation.MutationResult; +import com.google.firebase.firestore.remote.Datastore; import com.google.firebase.firestore.remote.ExistenceFilter; import com.google.firebase.firestore.remote.MockDatastore; +import com.google.firebase.firestore.remote.RemoteComponenetProvider; import com.google.firebase.firestore.remote.RemoteEvent; import com.google.firebase.firestore.remote.RemoteSerializer; import com.google.firebase.firestore.remote.RemoteStore; @@ -83,6 +85,8 @@ import com.google.firebase.firestore.remote.WatchChange.WatchTargetChange; import com.google.firebase.firestore.remote.WatchChange.WatchTargetChangeType; import com.google.firebase.firestore.remote.WatchStream; +import com.google.firebase.firestore.testutil.EmptyAppCheckTokenProvider; +import com.google.firebase.firestore.testutil.EmptyCredentialsProvider; import com.google.firebase.firestore.testutil.TestUtil; import com.google.firebase.firestore.util.Assert; import com.google.firebase.firestore.util.AsyncQueue; @@ -259,7 +263,9 @@ public static void log(String line) { // protected abstract ComponentProvider initializeComponentProvider( - ComponentProvider.Configuration configuration, boolean garbageCollectionEnabled); + RemoteComponenetProvider remoteProvider, + ComponentProvider.Configuration configuration, + boolean garbageCollectionEnabled); private boolean shouldRun(Set tags) { for (String tag : tags) { @@ -315,19 +321,28 @@ protected void specTearDown() throws Exception { */ private void initClient() { queue = new AsyncQueue(); - datastore = new MockDatastore(databaseInfo, queue, ApplicationProvider.getApplicationContext()); + datastore = new MockDatastore(databaseInfo, queue); ComponentProvider.Configuration configuration = new ComponentProvider.Configuration( ApplicationProvider.getApplicationContext(), queue, databaseInfo, - datastore, currentUser, maxConcurrentLimboResolutions, - new FirebaseFirestoreSettings.Builder().build()); - - ComponentProvider provider = initializeComponentProvider(configuration, useEagerGcForMemory); + new FirebaseFirestoreSettings.Builder().build(), + new EmptyCredentialsProvider(), + new EmptyAppCheckTokenProvider(), + null + ); + + RemoteComponenetProvider remoteProvider = new RemoteComponenetProvider() { + @Override + protected Datastore createDatastore(ComponentProvider.Configuration configuration) { + return datastore; + } + }; + ComponentProvider provider = initializeComponentProvider(remoteProvider, configuration, useEagerGcForMemory); localPersistence = provider.getPersistence(); if (localPersistence.getReferenceDelegate() instanceof LruDelegate) { lruGarbageCollector = diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestTargetMetadataProvider.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestTargetMetadataProvider.java index 698628b3300..59c9e38f6cf 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestTargetMetadataProvider.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestTargetMetadataProvider.java @@ -30,8 +30,6 @@ public class TestTargetMetadataProvider implements WatchChangeAggregator.TargetMetadataProvider { final Map> syncedKeys = new HashMap<>(); final Map queryData = new HashMap<>(); - DatabaseId databaseId = DatabaseId.forProject("test-project"); - @Override public ImmutableSortedSet getRemoteKeysForTarget(int targetId) { return syncedKeys.get(targetId) != null ? syncedKeys.get(targetId) : DocumentKey.emptyKeySet(); @@ -43,16 +41,6 @@ public TargetData getTargetDataForTarget(int targetId) { return queryData.get(targetId); } - @Override - public DatabaseId getDatabaseId() { - return databaseId; - } - - /** Replaces the default project ID and database ID. */ - public void setDatabaseId(String projectId, String databaseId) { - this.databaseId = DatabaseId.forDatabase(projectId, databaseId); - } - /** Sets or replaces the local state for the provided query data. */ public void setSyncedKeys(TargetData targetData, ImmutableSortedSet keys) { this.queryData.put(targetData.getTargetId(), targetData); diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index e89a1e98b23..6a48e55f508 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -441,7 +441,7 @@ public static RemoteEvent noChangeEvent(int targetId, int version, ByteString re TestTargetMetadataProvider testTargetMetadataProvider = new TestTargetMetadataProvider(); testTargetMetadataProvider.setSyncedKeys(targetData, DocumentKey.emptyKeySet()); - WatchChangeAggregator aggregator = new WatchChangeAggregator(testTargetMetadataProvider); + WatchChangeAggregator aggregator = new WatchChangeAggregator(TEST_PROJECT, testTargetMetadataProvider); WatchChange.WatchTargetChange watchChange = new WatchChange.WatchTargetChange( @@ -462,7 +462,7 @@ public static RemoteEvent existenceFilterEvent( testTargetMetadataProvider.setSyncedKeys(targetData, syncedKeys); ExistenceFilter existenceFilter = new ExistenceFilter(remoteCount); - WatchChangeAggregator aggregator = new WatchChangeAggregator(testTargetMetadataProvider); + WatchChangeAggregator aggregator = new WatchChangeAggregator(TEST_PROJECT, testTargetMetadataProvider); WatchChange.ExistenceFilterWatchChange existenceFilterWatchChange = new WatchChange.ExistenceFilterWatchChange(targetId, existenceFilter); @@ -478,6 +478,7 @@ public static RemoteEvent addedRemoteEvent( WatchChangeAggregator aggregator = new WatchChangeAggregator( + TEST_PROJECT, new WatchChangeAggregator.TargetMetadataProvider() { @Override public ImmutableSortedSet getRemoteKeysForTarget(int targetId) { @@ -489,11 +490,6 @@ public TargetData getTargetDataForTarget(int targetId) { ResourcePath collectionPath = docs.get(0).getKey().getCollectionPath(); return targetData(targetId, QueryPurpose.LISTEN, collectionPath.toString()); } - - @Override - public DatabaseId getDatabaseId() { - return TEST_PROJECT; - } }); SnapshotVersion version = SnapshotVersion.NONE; @@ -529,6 +525,7 @@ public static RemoteEvent updateRemoteEvent( new DocumentChange(updatedInTargets, removedFromTargets, doc.getKey(), doc); WatchChangeAggregator aggregator = new WatchChangeAggregator( + TEST_PROJECT, new WatchChangeAggregator.TargetMetadataProvider() { @Override public ImmutableSortedSet getRemoteKeysForTarget(int targetId) { @@ -541,11 +538,6 @@ public TargetData getTargetDataForTarget(int targetId) { ? targetData(targetId, QueryPurpose.LISTEN, doc.getKey().toString()) : null; } - - @Override - public DatabaseId getDatabaseId() { - return TEST_PROJECT; - } }); aggregator.handleDocumentChange(change); return aggregator.createRemoteEvent(doc.getVersion()); From a3be1e65649d997a764c68b10a85c725e1efe3fa Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 17 Jun 2024 11:06:43 -0400 Subject: [PATCH 11/37] Remove dead code --- .../testutil/IntegrationTestUtil.java | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index 0fe49c20478..c6d1b975867 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -38,8 +38,6 @@ import com.google.firebase.firestore.FirebaseFirestore; import com.google.firebase.firestore.FirebaseFirestoreSettings; import com.google.firebase.firestore.ListenerRegistration; -import com.google.firebase.firestore.MemoryCacheSettings; -import com.google.firebase.firestore.MemoryEagerGcSettings; import com.google.firebase.firestore.MetadataChanges; import com.google.firebase.firestore.Query; import com.google.firebase.firestore.QuerySnapshot; @@ -92,25 +90,6 @@ public void changeUserTo(User user) { /** A set of helper methods for tests */ public class IntegrationTestUtil { - @NonNull - public static ComponentProvider.Configuration testConfiguration(AsyncQueue testQueue) { - return new ComponentProvider.Configuration( - ApplicationProvider.getApplicationContext(), - testQueue, - testEnvDatabaseInfo(), - User.UNAUTHENTICATED, - 100, - new FirebaseFirestoreSettings.Builder() - .setLocalCacheSettings(MemoryCacheSettings.newBuilder() - .setGcSettings(MemoryEagerGcSettings.newBuilder().build()) - .build()) - .build(), - new EmptyCredentialsProvider(), - new EmptyAppCheckTokenProvider(), - null - ); - } - public enum TargetBackend { EMULATOR, QA, From c61a40cbaabca31b608f40ef69cdfc6dad3744fb Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 17 Jun 2024 11:13:05 -0400 Subject: [PATCH 12/37] Fix --- .../java/com/google/firebase/firestore/remote/WriteStream.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java index 8cf61c738f1..e32545f1f5c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java @@ -165,6 +165,8 @@ public void onFirst(WriteResponse response) { // The first response is the handshake response handshakeComplete = true; + + listener.onHandshakeComplete(); } @Override From 4808d4553d4fb30315394220f2ee89bca5514bf5 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 17 Jun 2024 11:36:57 -0400 Subject: [PATCH 13/37] Comments and cleanup --- .../firestore/remote/RemoteComponenetProvider.java | 8 +++++++- .../com/google/firebase/firestore/remote/RemoteStore.java | 1 + .../firebase/firestore/remote/WatchChangeAggregator.java | 2 -- .../com/google/firebase/firestore/remote/WriteStream.java | 2 ++ .../google/firebase/firestore/spec/MemorySpecTest.java | 2 -- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java index 3048532a169..c99ad9b2dd6 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java @@ -17,8 +17,14 @@ import static com.google.firebase.firestore.util.Assert.hardAssertNonNull; import com.google.firebase.firestore.core.ComponentProvider; -import com.google.firebase.firestore.util.AsyncQueue; +/** + * Initializes and wires up remote components for Firestore. + * + *

Implementations provide custom components by overriding the `createX()` methods. + *

The RemoteComponentProvider is located in the same package as the components in order to have + * package-private access to the components. + */ public class RemoteComponenetProvider { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java index ab1cf942127..cc84ae8623f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java @@ -65,6 +65,7 @@ public final class RemoteStore implements WatchChangeAggregator.TargetMetadataPr /** The log tag to use for this class. */ private static final String LOG_TAG = "RemoteStore"; + /** The database ID of the Firestore instance. */ private final DatabaseId databaseId; /** A callback interface for events from RemoteStore. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java index eb28d9f827d..304e67e2834 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java @@ -17,8 +17,6 @@ import static com.google.firebase.firestore.util.Assert.fail; import static com.google.firebase.firestore.util.Assert.hardAssert; -import android.provider.ContactsContract; - import androidx.annotation.Nullable; import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.DocumentViewChange; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java index 8cf61c738f1..e32545f1f5c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WriteStream.java @@ -165,6 +165,8 @@ public void onFirst(WriteResponse response) { // The first response is the handshake response handshakeComplete = true; + + listener.onHandshakeComplete(); } @Override diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java index 187da0592a3..75fe3f4503f 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java @@ -20,8 +20,6 @@ import com.google.firebase.firestore.local.LruGarbageCollector; import com.google.firebase.firestore.local.MemoryPersistence; import com.google.firebase.firestore.local.Persistence; -import com.google.firebase.firestore.remote.Datastore; -import com.google.firebase.firestore.remote.MockDatastore; import com.google.firebase.firestore.remote.RemoteComponenetProvider; import java.util.Set; import org.junit.runner.RunWith; From b749f5c58d98e4c86437db8bd03ae1f5c10683cc Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 17 Jun 2024 12:32:46 -0400 Subject: [PATCH 14/37] Whitespace --- .../firebase/firestore/remote/RemoteComponenetProvider.java | 1 - 1 file changed, 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java index c99ad9b2dd6..9c1a79093c0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteComponenetProvider.java @@ -27,7 +27,6 @@ */ public class RemoteComponenetProvider { - private GrpcCallProvider grpcCallProvider; private RemoteSerializer remoteSerializer; private FirestoreChannel firestoreChannel; From 29b2ef9579f45996216bed7bee394f3969d12e65 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 17 Jun 2024 16:02:19 -0400 Subject: [PATCH 15/37] gRPC integration test of write handshake --- ...rebaseFirestoreIntegrationTestFactory.java | 164 +++++++++++++ .../integration/AsyncTaskAccumulator.java | 124 ++++++++++ .../integration/FirebaseFirestoreTest.java | 219 ++++++++++++++++++ .../firestore/integration/TestClientCall.java | 87 +++++++ 4 files changed, 594 insertions(+) create mode 100644 firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java create mode 100644 firebase-firestore/src/test/java/com/google/firebase/firestore/integration/AsyncTaskAccumulator.java create mode 100644 firebase-firestore/src/test/java/com/google/firebase/firestore/integration/FirebaseFirestoreTest.java create mode 100644 firebase-firestore/src/test/java/com/google/firebase/firestore/integration/TestClientCall.java diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java new file mode 100644 index 00000000000..91836cce99d --- /dev/null +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java @@ -0,0 +1,164 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore; + +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import androidx.test.core.app.ApplicationProvider; + +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.TaskCompletionSource; +import com.google.android.gms.tasks.Tasks; +import com.google.firebase.firestore.core.ComponentProvider; +import com.google.firebase.firestore.integration.AsyncTaskAccumulator; +import com.google.firebase.firestore.model.DatabaseId; +import com.google.firebase.firestore.remote.GrpcCallProvider; +import com.google.firebase.firestore.remote.RemoteComponenetProvider; +import com.google.firebase.firestore.testutil.EmptyAppCheckTokenProvider; +import com.google.firebase.firestore.testutil.EmptyCredentialsProvider; +import com.google.firebase.firestore.util.AsyncQueue; +import com.google.firestore.v1.FirestoreGrpc; +import com.google.firestore.v1.ListenRequest; +import com.google.firestore.v1.ListenResponse; +import com.google.firestore.v1.WriteRequest; +import com.google.firestore.v1.WriteResponse; + +import com.google.firebase.firestore.integration.TestClientCall; + +/** + * Factory for producing FirebaseFirestore instances that has mocked gRPC layer. + * + *

    + *
  1. Response protos from server can be faked. + *
  2. Request protos from SDK can be verified. + *
+ * + *

+ * The FirebaseFirestoreIntegrationTestFactory is located in this package to gain package-private + * access to FirebaseFirestore methods. + */ +public final class FirebaseFirestoreIntegrationTestFactory { + + /** + * Everytime the `componentProviderFactory` on FirebaseFirestore is run, a new instance is added. + */ + public final AsyncTaskAccumulator instances = new AsyncTaskAccumulator<>(); + + /** + * Instance of Firestore components. + */ + public static class Instance { + + /** Instance of ComponentProvider */ + public final ComponentProvider componentProvider; + + /** Every listen stream created is captured here. */ + private final AsyncTaskAccumulator> listens = new AsyncTaskAccumulator<>(); + + /** Every write stream created is captured here. */ + private final AsyncTaskAccumulator> writes = new AsyncTaskAccumulator<>(); + + private Instance(ComponentProvider componentProvider) { + this.componentProvider = componentProvider; + } + + /** + * Queues work on AsyncQueue. This is required when faking responses from server since they + * must be handled through the AsyncQueue of the FirestoreClient. + */ + public Task enqueue(Runnable runnable) { + return configuration.asyncQueue.enqueue(runnable); + } + + /** + * Configuration passed to `ComponentProvider` + * + *

+ * This is never null because `Task` completes after initialization. The + * `FirebaseFirestoreIntegrationTestFactory` will set `Instance.configuration` from within + * the ComponentProvider override. + */ + private ComponentProvider.Configuration configuration; + + /** Every listen stream created */ + public Task> getListenClient(int i) { + return listens.get(i); + } + + /** Every write stream created */ + public Task> getWriteClient(int i) { + return writes.get(i); + } + + } + + /** + * The FirebaseFirestore instance. + */ + public final FirebaseFirestore firestore; + + /** + * Mockito Mock of `FirebaseFirestore.InstanceRegistry` that was passed into FirebaseFirestore + * constructor. + */ + public final FirebaseFirestore.InstanceRegistry instanceRegistry = mock(FirebaseFirestore.InstanceRegistry.class); + + public FirebaseFirestoreIntegrationTestFactory(DatabaseId databaseId) { + firestore = new FirebaseFirestore( + ApplicationProvider.getApplicationContext(), + databaseId, + "k", + new EmptyCredentialsProvider(), + new EmptyAppCheckTokenProvider(), + new AsyncQueue(), + this::componentProvider, + null, + instanceRegistry, + null + ); + } + + public void useMemoryCache() { + FirebaseFirestoreSettings.Builder builder = new FirebaseFirestoreSettings.Builder(firestore.getFirestoreSettings()); + builder.setLocalCacheSettings(MemoryCacheSettings.newBuilder().build()); + firestore.setFirestoreSettings(builder.build()); + } + + private GrpcCallProvider mockGrpcCallProvider(Instance instance) { + GrpcCallProvider mockGrpcCallProvider = mock(GrpcCallProvider.class); + when(mockGrpcCallProvider.createClientCall(eq(FirestoreGrpc.getListenMethod()))) + .thenAnswer(invocation -> Tasks.forResult(new TestClientCall<>(instance.listens.next()))); + when(mockGrpcCallProvider.createClientCall(eq(FirestoreGrpc.getWriteMethod()))) + .thenAnswer(invocation -> Tasks.forResult(new TestClientCall<>(instance.writes.next()))); + return mockGrpcCallProvider; + } + + private ComponentProvider componentProvider(FirebaseFirestoreSettings settings) { + TaskCompletionSource next = instances.next(); + ComponentProvider componentProvider = ComponentProvider.defaultFactory(settings); + Instance instance = new Instance(componentProvider); + componentProvider.setRemoteProvider(new RemoteComponenetProvider() { + @Override + protected GrpcCallProvider createGrpcCallProvider(ComponentProvider.Configuration configuration) { + instance.configuration = configuration; + next.setResult(instance); + return mockGrpcCallProvider(instance); + } + }); + return componentProvider; + } +} diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/integration/AsyncTaskAccumulator.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/integration/AsyncTaskAccumulator.java new file mode 100644 index 00000000000..922259676de --- /dev/null +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/integration/AsyncTaskAccumulator.java @@ -0,0 +1,124 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.firebase.firestore.integration; + +import androidx.annotation.NonNull; + +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.TaskCompletionSource; + +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; +import java.util.NoSuchElementException; + + +/** + * Collects asynchronous `onResult` and `onException` callback invocations. + * + *

As part of a test, a callback can be asynchronously invoked many times. This class retains + * all callback invocations as a List of Task. The test code can await a future callback. + * + *

Just like a stream, no more results are expected after an exception. + */ +public class AsyncTaskAccumulator implements Iterable> { + + private int eventCount; + private List> events; + + public AsyncTaskAccumulator() { + eventCount = 0; + events = new ArrayList<>(); + } + + /** + * Callback for next `onResult` or `onException`. Calling this method repeatedly will + * provide callbacks further into the future. Each callback should only be exactly once. + */ + public synchronized TaskCompletionSource next() { + return computeIfAbsentIndex(eventCount++); + } + + /** + * Callback that can be invoked as part of test code. + */ + public void onResult(T result) { + next().setResult(result); + } + + /** + * Callback that can be invoked as part of test code. + */ + public void onException(Exception e) { + next().setException(e); + } + + private TaskCompletionSource computeIfAbsentIndex(int i) { + while (events.size() <= i) { + events.add(new TaskCompletionSource<>()); + } + return events.get(i); + } + + /** + * Get task that completes when result arrives. + * + * @param index 0 indexed arrival sequence of results. + * @return Task. + */ + @NonNull + public synchronized Task get(int index) { + return computeIfAbsentIndex(index).getTask(); + } + + /** + * Iterates over results. + *

+ * The Iterator is thread safe. + * Iteration will stop upon task that is failed, cancelled or incomplete. + *

+ * A loop that waits for task to complete before getting next task will continue to iterate + * indefinitely. Attempting to iterate past a task that is not yet successful will throw + * {#code NoSuchElementException} and {@code #hasNext()} will be false. In this way, iteration + * in nonblocking. Last element will be failed, cancelled or awaiting result. + * + * @return Iterator of Tasks that complete. + */ + @NonNull + @Override + public Iterator> iterator() { + return new Iterator>() { + + private int i = -1; + private Task current; + + @Override + public synchronized boolean hasNext() { + // We always return first, and continue to return tasks so long as previous + // is successful. A task that hasn't completed, will also mark the end of + // iteration. + return i < 0 || current.isSuccessful(); + } + + @Override + public synchronized Task next() { + if (!hasNext()) { + throw new NoSuchElementException(); + } + i++; + return current = get(i); + } + }; + } +} diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/integration/FirebaseFirestoreTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/integration/FirebaseFirestoreTest.java new file mode 100644 index 00000000000..e4cdd9a8deb --- /dev/null +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/integration/FirebaseFirestoreTest.java @@ -0,0 +1,219 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore.integration; + +import static com.google.common.truth.Truth.assertThat; +import static com.google.firebase.firestore.testutil.TestUtil.map; +import static com.google.firebase.firestore.util.Executors.BACKGROUND_EXECUTOR; +import static org.mockito.Mockito.verify; + +import androidx.annotation.NonNull; + +import com.google.android.gms.tasks.Task; +import com.google.firebase.firestore.CollectionReference; +import com.google.firebase.firestore.DocumentReference; +import com.google.firebase.firestore.FirebaseFirestore; +import com.google.firebase.firestore.FirebaseFirestoreIntegrationTestFactory; +import com.google.firebase.firestore.UserDataReader; +import com.google.firebase.firestore.core.UserData; +import com.google.firebase.firestore.model.DatabaseId; +import com.google.firebase.firestore.model.DocumentKey; +import com.google.firebase.firestore.model.mutation.FieldTransform; +import com.google.firebase.firestore.model.mutation.Precondition; +import com.google.firebase.firestore.model.mutation.SetMutation; +import com.google.firebase.firestore.remote.RemoteSerializer; +import com.google.firestore.v1.Write; +import com.google.firestore.v1.WriteRequest; +import com.google.firestore.v1.WriteResponse; +import com.google.firestore.v1.WriteResult; + +import org.bouncycastle.util.Arrays; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mockito; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.Iterator; +import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +import io.grpc.Metadata; +import io.grpc.Status; + +@RunWith(RobolectricTestRunner.class) +@Config(manifest = Config.NONE) +public class FirebaseFirestoreTest { + + private DatabaseId databaseId; + private FirebaseFirestore firestore; + private FirebaseFirestoreIntegrationTestFactory factory; + + private static void waitForSuccess(Task task) throws InterruptedException { + waitFor(task).getResult(); + } + + private static T waitForResult(Task task) throws InterruptedException { + return waitFor(task).getResult(); + } + + @NonNull + public static String getResourcePrefixValue(DatabaseId databaseId) { + return String.format( + "projects/%s/databases/%s", databaseId.getProjectId(), databaseId.getDatabaseId()); + } + + private static Task waitFor(Task task) throws InterruptedException { + CountDownLatch countDownLatch = new CountDownLatch(1); + task.addOnSuccessListener(BACKGROUND_EXECUTOR, t -> countDownLatch.countDown()); + task.addOnFailureListener(BACKGROUND_EXECUTOR, e -> countDownLatch.countDown()); + task.addOnCanceledListener(BACKGROUND_EXECUTOR, () -> countDownLatch.countDown()); + countDownLatch.await(15, TimeUnit.SECONDS); + return task; + } + + @Before + public void before() { + databaseId = DatabaseId.forDatabase("p", "d"); + factory = new FirebaseFirestoreIntegrationTestFactory(databaseId); + factory.useMemoryCache(); + firestore = factory.firestore; + } + + @After + public void after() throws Exception { + waitForSuccess(firestore.terminate()); + verify(factory.instanceRegistry, Mockito.atLeastOnce()).remove(databaseId.getDatabaseId()); + Mockito.verifyNoMoreInteractions(factory.instanceRegistry); + + factory = null; + firestore = null; + } + + @Test + public void preserveWritesWhenDisconnectedWithInternalError() throws Exception { + CollectionReference col = firestore.collection("col"); + DocumentReference doc1 = col.document(); + DocumentReference doc2 = col.document(); + DocumentReference doc3 = col.document(); + doc1.set(map("foo", "A")); + doc2.set(map("foo", "B")); + doc3.set(map("foo", "C")); + + // Wait for first FirestoreClient to instantiate + FirebaseFirestoreIntegrationTestFactory.Instance instance = waitForResult(factory.instances.get(0)); + RemoteSerializer serializer = instance.componentProvider.getRemoteSerializer(); + + // First Write stream connection + { + // Wait for Write CallClient to be created. + TestClientCall callback = waitForResult(instance.getWriteClient(0)); + Iterator> requests = callback.requestIterator(); + + // Wait for WriteRequest handshake. + // We expect an empty init request because the database is fresh. + assertThat(waitForResult(requests.next())).isEqualTo(writeRequestHandshake()); + + // Simulate a successful InitResponse from server. + waitForSuccess(instance.enqueue(() -> callback.listener.onMessage(writeResponse()))); + + // Expect first write request. + Write write1 = serializer.encodeMutation(setMutation(doc1, map("foo", "A"))); + assertThat(waitForResult(requests.next())).isEqualTo(writeRequest(write1)); + + // Simulate write acknowledgement. + waitForSuccess(instance.enqueue(() -> callback.listener.onMessage(writeResponse(WriteResult.getDefaultInstance())))); + + // Expect second write request. + Write write2 = serializer.encodeMutation(setMutation(doc2, map("foo", "B"))); + assertThat(waitForResult(requests.next())).isEqualTo(writeRequest(write2)); + + // Simulate INTERNAL error that is retryable. ( + waitForSuccess(instance.enqueue(() -> callback.listener.onClose(Status.INTERNAL, new Metadata()))); + } + + // Second Write Stream connection + // Previous connection was closed by server with NOT_FOUND error. + { + // Wait for Write CallClient to be created. + TestClientCall callback = waitForResult(instance.getWriteClient(1)); + Iterator> requests = callback.requestIterator(); + + // Wait for WriteRequest handshake. + // We expect FirestoreClient to send InitRequest with previous token. + assertThat(waitForResult(requests.next())).isEqualTo(writeRequestHandshake()); + + // Simulate a successful InitResponse from server. + waitForSuccess(instance.enqueue(() -> callback.listener.onMessage(writeResponse()))); + + // Expect second write to be retried. + Write write2 = serializer.encodeMutation(setMutation(doc2, map("foo", "B"))); + assertThat(waitForResult(requests.next())).isEqualTo(writeRequest(write2)); + + // Simulate write acknowledgement. + waitForSuccess(instance.enqueue(() -> callback.listener.onMessage(writeResponse(WriteResult.getDefaultInstance())))); + + // Expect second write request. + Write write3 = serializer.encodeMutation(setMutation(doc3, map("foo", "C"))); + assertThat(waitForResult(requests.next())).isEqualTo(writeRequest(write3)); + } + } + + + @NonNull + private DocumentKey key(DocumentReference doc) { + return DocumentKey.fromPathString(doc.getPath()); + } + + @NonNull + public SetMutation setMutation(DocumentReference doc, Map values) { + UserDataReader dataReader = new UserDataReader(databaseId); + UserData.ParsedSetData parsed = dataReader.parseSetData(values); + + // The order of the transforms doesn't matter, but we sort them so tests can assume a particular + // order. + ArrayList fieldTransforms = new ArrayList<>(parsed.getFieldTransforms()); + Collections.sort(fieldTransforms, Comparator.comparing(FieldTransform::getFieldPath)); + + return new SetMutation(key(doc), parsed.getData(), Precondition.NONE, fieldTransforms); + } + + @NonNull + private WriteRequest writeRequestHandshake() { + return WriteRequest.newBuilder() + .setDatabase(getResourcePrefixValue(databaseId)) + .build(); + } + + @NonNull + private WriteRequest writeRequest(Write... writes) { + return WriteRequest.newBuilder() + .addAllWrites(() -> new Arrays.Iterator<>(writes)) + .build(); + } + + @NonNull + private static WriteResponse writeResponse(WriteResult... writeResults) { + return WriteResponse.newBuilder() + .addAllWriteResults(() -> new Arrays.Iterator<>(writeResults)) + .build(); + } +} diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/integration/TestClientCall.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/integration/TestClientCall.java new file mode 100644 index 00000000000..78e0399b568 --- /dev/null +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/integration/TestClientCall.java @@ -0,0 +1,87 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore.integration; + +import android.util.Pair; + +import androidx.annotation.Nullable; + +import com.google.android.gms.tasks.Task; +import com.google.android.gms.tasks.TaskCompletionSource; + +import java.util.Iterator; + +import io.grpc.ClientCall; +import io.grpc.Metadata; + +/** + * ClientCall test harness. + */ +public class TestClientCall extends ClientCall { + + public Metadata headers; + public Listener listener; + + private final AsyncTaskAccumulator requests; + private final TaskCompletionSource> startTask; + private final TaskCompletionSource> cancelTask = new TaskCompletionSource<>(); + + /** + * Construct a ClientCall test harness. + * + * The {@code #headers} and {@code #listener} will be populated when {@code #startTask} + * completes. + * + * @param startTask Will complete when ClientCall has start callback invoked. + */ + public TestClientCall(TaskCompletionSource> startTask) { + this.startTask = startTask; + this.requests = new AsyncTaskAccumulator<>(); + } + + @Override + public void start(Listener responseListener, Metadata headers) { + this.listener = responseListener; + this.headers = headers; + startTask.setResult(this); + } + + @Override + public void request(int numMessages) { + } + + @Override + public void cancel(@Nullable String message, @Nullable Throwable cause) { + cancelTask.setResult(Pair.create(message, cause)); + } + + @Override + public void halfClose() { + requests.onException(new RuntimeException("halfClose")); + } + + @Override + public void sendMessage(ReqT message) { + requests.onResult(message); + } + + public Task getRequest(int index) { + return requests.get(index); + } + + public Iterator> requestIterator() { + return requests.iterator(); + } +} From fbc7ad93a926fe74ec497b1a1975153fb4c99709 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 17 Jun 2024 17:09:21 -0400 Subject: [PATCH 16/37] PR feedback --- .../google/firebase/firestore/local/GlobalsCache.java | 7 +++++-- .../firebase/firestore/local/MemoryGlobalsCache.java | 7 +++++-- .../firebase/firestore/local/SQLiteGlobalsCache.java | 11 +++++++---- .../firebase/firestore/local/GlobalsCacheTest.java | 6 ++++++ .../firebase/firestore/local/SQLiteSchemaTest.java | 6 ++++++ 5 files changed, 29 insertions(+), 8 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java index e5c04552f67..35a6ed9b603 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/GlobalsCache.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.local; +import androidx.annotation.NonNull; + import com.google.protobuf.ByteString; /** @@ -21,13 +23,14 @@ * *

Global state that cuts across components should be saved here. Following are contained herein: * - *

`db_token` tracks server interaction across Listen and Write streams. This facilitates cache + *

`sessionToken` tracks server interaction across Listen and Write streams. This facilitates cache * synchronization and invalidation. */ interface GlobalsCache { + @NonNull ByteString getSessionsToken(); - void setSessionToken(ByteString value); + void setSessionToken(@NonNull ByteString value); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java index e02dcbb2f7b..e4f0cba8a84 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryGlobalsCache.java @@ -14,20 +14,23 @@ package com.google.firebase.firestore.local; +import androidx.annotation.NonNull; + import com.google.protobuf.ByteString; /** In-memory cache of global values */ final class MemoryGlobalsCache implements GlobalsCache { - private ByteString sessionToken; + private ByteString sessionToken = ByteString.EMPTY; + @NonNull @Override public ByteString getSessionsToken() { return sessionToken; } @Override - public void setSessionToken(ByteString value) { + public void setSessionToken(@NonNull ByteString value) { sessionToken = value; } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java index a26c99a1490..1272b0e8cdd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteGlobalsCache.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.local; +import androidx.annotation.NonNull; + import com.google.protobuf.ByteString; public class SQLiteGlobalsCache implements GlobalsCache { @@ -25,24 +27,25 @@ public SQLiteGlobalsCache(SQLitePersistence persistence) { this.db = persistence; } + @NonNull @Override public ByteString getSessionsToken() { byte[] bytes = get(SESSION_TOKEN); - return bytes == null ? null : ByteString.copyFrom(bytes); + return bytes == null ? ByteString.EMPTY : ByteString.copyFrom(bytes); } @Override - public void setSessionToken(ByteString value) { + public void setSessionToken(@NonNull ByteString value) { set(SESSION_TOKEN, value.toByteArray()); } - private byte[] get(String name) { + private byte[] get(@NonNull String name) { return db.query("SELECT value FROM globals WHERE name = ?") .binding(name) .firstValue(row -> row.getBlob(0)); } - private void set(String name, byte[] value) { + private void set(@NonNull String name, @NonNull byte[] value) { db.execute( "INSERT OR REPLACE INTO globals " + "(name, value) " diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java index fde94288282..9fbed955b63 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/GlobalsCacheTest.java @@ -48,4 +48,10 @@ public void setAndGetDbToken() { globalsCache.setSessionToken(value); assertEquals(value, globalsCache.getSessionsToken()); } + + @Test + public void setAndGetEmptyDbToken() { + globalsCache.setSessionToken(ByteString.EMPTY); + assertEquals(ByteString.EMPTY, globalsCache.getSessionsToken()); + } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index 44fe6051898..6e250332143 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -683,6 +683,12 @@ public void createsIndexingTables() { assertTableExists("index_state"); } + @Test + public void createsGlobalsTable() { + schema.runSchemaUpgrades(0, 17); + assertTableExists("globals"); + } + @Test public void createsOverlaysAndMigrationTable() { // 14 is the version we enable Overlay From 5e3f9f516827efefa55ac98bff2fa0e29227d643 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 19 Jun 2024 12:08:36 -0400 Subject: [PATCH 17/37] FirestoreClientProvider --- .../firebase/firestore/FirestoreTest.java | 3 +- .../google/firebase/firestore/QueryTest.java | 4 +- .../firestore/ServerTimestampTest.java | 8 +- .../firebase/firestore/ValidationTest.java | 4 +- .../firebase/firestore/AggregateQuery.java | 5 +- .../firebase/firestore/DocumentReference.java | 30 ++-- .../firebase/firestore/FirebaseFirestore.java | 144 +++++++----------- .../firestore/FirestoreClientProvider.java | 95 ++++++++++++ .../PersistentCacheIndexManager.java | 10 +- .../com/google/firebase/firestore/Query.java | 12 +- .../google/firebase/firestore/WriteBatch.java | 4 +- .../firestore/core/FirestoreClient.java | 45 +++--- .../core/ListenerRegistrationImpl.java | 44 ------ ...rebaseFirestoreIntegrationTestFactory.java | 1 - 14 files changed, 202 insertions(+), 207 deletions(-) create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java delete mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/core/ListenerRegistrationImpl.java 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 631dcec4dd8..6988ce09c31 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 @@ -49,6 +49,7 @@ import com.google.firebase.firestore.FirebaseFirestoreException.Code; import com.google.firebase.firestore.Query.Direction; import com.google.firebase.firestore.auth.User; +import com.google.firebase.firestore.core.FirestoreClient; import com.google.firebase.firestore.model.DatabaseId; import com.google.firebase.firestore.testutil.EventAccumulator; import com.google.firebase.firestore.testutil.IntegrationTestUtil; @@ -1132,7 +1133,7 @@ public void testAppDeleteLeadsToFirestoreTerminate() { app.delete(); - assertTrue(instance.getClient().isTerminated()); + assertTrue(instance.callClient(FirestoreClient::isTerminated)); } @Test diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index b6ec3d61a51..29ca658515e 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -536,12 +536,12 @@ public void testQueriesFireFromCacheWhenOffline() { assertFalse(querySnapshot.getMetadata().isFromCache()); // offline event with fromCache=true - waitFor(collection.firestore.getClient().disableNetwork()); + waitFor(collection.firestore.disableNetwork()); querySnapshot = accum.await(); assertTrue(querySnapshot.getMetadata().isFromCache()); // back online event with fromCache=false - waitFor(collection.firestore.getClient().enableNetwork()); + waitFor(collection.firestore.enableNetwork()); querySnapshot = accum.await(); assertFalse(querySnapshot.getMetadata().isFromCache()); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java index 4fcc6f4f6f4..fb38935ddc2 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java @@ -214,7 +214,7 @@ public void testServerTimestampsCanReturnPreviousValueOfDifferentType() { @Test public void testServerTimestampsCanRetainPreviousValueThroughConsecutiveUpdates() { writeInitialData(); - waitFor(docRef.getFirestore().getClient().disableNetwork()); + waitFor(docRef.getFirestore().disableNetwork()); accumulator.awaitRemoteEvent(); docRef.update("a", FieldValue.serverTimestamp()); @@ -226,7 +226,7 @@ public void testServerTimestampsCanRetainPreviousValueThroughConsecutiveUpdates( localSnapshot = accumulator.awaitLocalEvent(); assertEquals(42L, localSnapshot.get("a", ServerTimestampBehavior.PREVIOUS)); - waitFor(docRef.getFirestore().getClient().enableNetwork()); + waitFor(docRef.getFirestore().enableNetwork()); DocumentSnapshot remoteSnapshot = accumulator.awaitRemoteEvent(); assertThat(remoteSnapshot.get("a")).isInstanceOf(Timestamp.class); @@ -235,7 +235,7 @@ public void testServerTimestampsCanRetainPreviousValueThroughConsecutiveUpdates( @Test public void testServerTimestampsUsesPreviousValueFromLocalMutation() { writeInitialData(); - waitFor(docRef.getFirestore().getClient().disableNetwork()); + waitFor(docRef.getFirestore().disableNetwork()); accumulator.awaitRemoteEvent(); docRef.update("a", FieldValue.serverTimestamp()); @@ -249,7 +249,7 @@ public void testServerTimestampsUsesPreviousValueFromLocalMutation() { localSnapshot = accumulator.awaitLocalEvent(); assertEquals(1337L, localSnapshot.get("a", ServerTimestampBehavior.PREVIOUS)); - waitFor(docRef.getFirestore().getClient().enableNetwork()); + waitFor(docRef.getFirestore().enableNetwork()); DocumentSnapshot remoteSnapshot = accumulator.awaitRemoteEvent(); assertThat(remoteSnapshot.get("a")).isInstanceOf(Timestamp.class); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java index f8efddd7893..986c81ade11 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java @@ -453,7 +453,7 @@ public void queriesCannotBeSortedByAnUncommittedServerTimestamp() { CollectionReference collection = testCollection(); // Ensure the server timestamp stays uncommitted for the first half of the test - waitFor(collection.firestore.getClient().disableNetwork()); + waitFor(collection.firestore.disableNetwork()); TaskCompletionSource offlineCallbackDone = new TaskCompletionSource<>(); TaskCompletionSource onlineCallbackDone = new TaskCompletionSource<>(); @@ -497,7 +497,7 @@ public void queriesCannotBeSortedByAnUncommittedServerTimestamp() { document.set(map("timestamp", FieldValue.serverTimestamp())); waitFor(offlineCallbackDone.getTask()); - waitFor(collection.firestore.getClient().enableNetwork()); + waitFor(collection.firestore.enableNetwork()); waitFor(onlineCallbackDone.getTask()); listenerRegistration.remove(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java index db4015d6386..41e48180cb9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java @@ -64,10 +64,7 @@ public List getAggregateFields() { public Task get(@NonNull AggregateSource source) { Preconditions.checkNotNull(source, "AggregateSource must not be null"); TaskCompletionSource tcs = new TaskCompletionSource<>(); - query - .firestore - .getClient() - .runAggregateQuery(query.query, aggregateFieldList) + query.firestore.callClient(client -> client.runAggregateQuery(query.query, aggregateFieldList)) .continueWith( Executors.DIRECT_EXECUTOR, (task) -> { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index 3b32b84b383..7ab549210cd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -26,11 +26,8 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.firestore.FirebaseFirestoreException.Code; -import com.google.firebase.firestore.core.ActivityScope; import com.google.firebase.firestore.core.AsyncEventListener; import com.google.firebase.firestore.core.EventManager.ListenOptions; -import com.google.firebase.firestore.core.ListenerRegistrationImpl; -import com.google.firebase.firestore.core.QueryListener; import com.google.firebase.firestore.core.UserData.ParsedSetData; import com.google.firebase.firestore.core.UserData.ParsedUpdateData; import com.google.firebase.firestore.core.ViewSnapshot; @@ -38,11 +35,13 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.model.mutation.DeleteMutation; +import com.google.firebase.firestore.model.mutation.Mutation; import com.google.firebase.firestore.model.mutation.Precondition; import com.google.firebase.firestore.util.Assert; import com.google.firebase.firestore.util.Executors; import com.google.firebase.firestore.util.Util; import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; @@ -165,9 +164,8 @@ public Task set(@NonNull Object data, @NonNull SetOptions options) { options.isMerge() ? firestore.getUserDataReader().parseMergeData(data, options.getFieldMask()) : firestore.getUserDataReader().parseSetData(data); - return firestore - .getClient() - .write(Collections.singletonList(parsed.toMutation(key, Precondition.NONE))) + List mutations = singletonList(parsed.toMutation(key, Precondition.NONE)); + return firestore.callClient(client -> client.write(mutations)) .continueWith(Executors.DIRECT_EXECUTOR, voidErrorTransformer()); } @@ -229,9 +227,8 @@ public Task update( } private Task update(@NonNull ParsedUpdateData parsedData) { - return firestore - .getClient() - .write(Collections.singletonList(parsedData.toMutation(key, Precondition.exists(true)))) + List mutations = singletonList(parsedData.toMutation(key, Precondition.exists(true))); + return firestore.callClient(client -> client.write(mutations)) .continueWith(Executors.DIRECT_EXECUTOR, voidErrorTransformer()); } @@ -242,9 +239,8 @@ private Task update(@NonNull ParsedUpdateData parsedData) { */ @NonNull public Task delete() { - return firestore - .getClient() - .write(singletonList(new DeleteMutation(key, Precondition.NONE))) + List mutations = singletonList(new DeleteMutation(key, Precondition.NONE)); + return firestore.callClient(client -> client.write(mutations)) .continueWith(Executors.DIRECT_EXECUTOR, voidErrorTransformer()); } @@ -273,9 +269,7 @@ public Task get() { @NonNull public Task get(@NonNull Source source) { if (source == Source.CACHE) { - return firestore - .getClient() - .getDocumentFromLocalCache(key) + return firestore.callClient(client -> client.getDocumentFromLocalCache(key)) .continueWith( Executors.DIRECT_EXECUTOR, (Task task) -> { @@ -531,11 +525,7 @@ private ListenerRegistration addSnapshotListenerInternal( new AsyncEventListener<>(userExecutor, viewListener); com.google.firebase.firestore.core.Query query = asQuery(); - QueryListener queryListener = firestore.getClient().listen(query, options, asyncListener); - - return ActivityScope.bind( - activity, - new ListenerRegistrationImpl(firestore.getClient(), queryListener, asyncListener)); + return firestore.callClient(client -> client.listen(query, options, activity, asyncListener)); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index f304bfff974..7db94fe5a23 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -32,12 +32,10 @@ import com.google.firebase.appcheck.interop.InteropAppCheckTokenProvider; import com.google.firebase.auth.internal.InternalAuthProvider; import com.google.firebase.emulators.EmulatedServiceSettings; -import com.google.firebase.firestore.FirebaseFirestoreException.Code; import com.google.firebase.firestore.auth.CredentialsProvider; import com.google.firebase.firestore.auth.FirebaseAppCheckTokenProvider; import com.google.firebase.firestore.auth.FirebaseAuthCredentialsProvider; import com.google.firebase.firestore.auth.User; -import com.google.firebase.firestore.core.ActivityScope; import com.google.firebase.firestore.core.AsyncEventListener; import com.google.firebase.firestore.core.ComponentProvider; import com.google.firebase.firestore.core.DatabaseInfo; @@ -96,7 +94,6 @@ public interface InstanceRegistry { private final String persistenceKey; private final CredentialsProvider authProvider; private final CredentialsProvider appCheckProvider; - private final AsyncQueue asyncQueue; private final FirebaseApp firebaseApp; private final UserDataReader userDataReader; // When user requests to terminate, use this to notify `FirestoreMultiDbComponent` to deregister @@ -104,7 +101,7 @@ public interface InstanceRegistry { private final InstanceRegistry instanceRegistry; @Nullable private EmulatedServiceSettings emulatorSettings; private FirebaseFirestoreSettings settings; - private volatile FirestoreClient client; + final FirestoreClientProvider clientProvider; private final GrpcMetadataProvider metadataProvider; @Nullable private PersistentCacheIndexManager persistentCacheIndexManager; @@ -195,8 +192,6 @@ static FirebaseFirestore newInstance( } DatabaseId databaseId = DatabaseId.forDatabase(projectId, database); - AsyncQueue queue = new AsyncQueue(); - CredentialsProvider authProvider = new FirebaseAuthCredentialsProvider(deferredAuthProvider); CredentialsProvider appCheckProvider = @@ -214,7 +209,6 @@ static FirebaseFirestore newInstance( persistenceKey, authProvider, appCheckProvider, - queue, ComponentProvider::defaultFactory, app, instanceRegistry, @@ -228,7 +222,6 @@ static FirebaseFirestore newInstance( String persistenceKey, CredentialsProvider authProvider, CredentialsProvider appCheckProvider, - AsyncQueue asyncQueue, @NonNull Function componentProviderFactory, @Nullable FirebaseApp firebaseApp, InstanceRegistry instanceRegistry, @@ -239,8 +232,8 @@ static FirebaseFirestore newInstance( this.persistenceKey = checkNotNull(persistenceKey); this.authProvider = checkNotNull(authProvider); this.appCheckProvider = checkNotNull(appCheckProvider); - this.asyncQueue = checkNotNull(asyncQueue); this.componentProviderFactory = checkNotNull(componentProviderFactory); + this.clientProvider = new FirestoreClientProvider(this::newClient); // NOTE: We allow firebaseApp to be null in tests only. this.firebaseApp = firebaseApp; this.instanceRegistry = instanceRegistry; @@ -261,12 +254,12 @@ public FirebaseFirestoreSettings getFirestoreSettings() { */ public void setFirestoreSettings(@NonNull FirebaseFirestoreSettings settings) { checkNotNull(settings, "Provided settings must not be null."); - synchronized (databaseId) { - settings = mergeEmulatorSettings(settings, emulatorSettings); + synchronized (clientProvider) { + settings = mergeEmulatorSettings(settings, this.emulatorSettings); // As a special exception, don't throw if the same settings are passed repeatedly. This // should make it simpler to get a Firestore instance in an activity. - if (client != null && !this.settings.equals(settings)) { + if (clientProvider.isConfigured() && !this.settings.equals(settings)) { throw new IllegalStateException( "FirebaseFirestore has already been started and its settings can no longer be changed. " + "You can only call setFirestoreSettings() before calling any other methods on a " @@ -286,29 +279,23 @@ public void setFirestoreSettings(@NonNull FirebaseFirestoreSettings settings) { * @param port the emulator port (for example, 8080) */ public void useEmulator(@NonNull String host, int port) { - if (this.client != null) { - throw new IllegalStateException( - "Cannot call useEmulator() after instance has already been initialized."); - } - - emulatorSettings = new EmulatedServiceSettings(host, port); - settings = mergeEmulatorSettings(settings, emulatorSettings); - } + synchronized (clientProvider) { + if (clientProvider.isConfigured()) { + throw new IllegalStateException( + "Cannot call useEmulator() after instance has already been initialized."); + } - private void ensureClientConfigured() { - if (client != null) { - return; + emulatorSettings = new EmulatedServiceSettings(host, port); + settings = mergeEmulatorSettings(settings, emulatorSettings); } + } - synchronized (databaseId) { - if (client != null) { - return; - } + private FirestoreClient newClient(AsyncQueue asyncQueue) { + synchronized (clientProvider) { DatabaseInfo databaseInfo = new DatabaseInfo(databaseId, persistenceKey, settings.getHost(), settings.isSslEnabled()); - client = - new FirestoreClient( + FirestoreClient client = new FirestoreClient( context, databaseInfo, settings, @@ -317,6 +304,8 @@ private void ensureClientConfigured() { asyncQueue, metadataProvider, componentProviderFactory.apply(settings)); + + return client; } } @@ -367,7 +356,7 @@ public FirebaseApp getApp() { @PreviewApi @NonNull public Task setIndexConfiguration(@NonNull String json) { - ensureClientConfigured(); + clientProvider.ensureConfigured(); Preconditions.checkState( settings.isPersistenceEnabled(), "Cannot enable indexes when persistence is disabled"); @@ -410,7 +399,7 @@ public Task setIndexConfiguration(@NonNull String json) { throw new IllegalArgumentException("Failed to parse index configuration", e); } - return client.configureFieldIndexes(parsedIndexes); + return clientProvider.call(client -> client.configureFieldIndexes(parsedIndexes)); } /** @@ -424,12 +413,12 @@ public Task setIndexConfiguration(@NonNull String json) { * not in use. */ @Nullable - public synchronized PersistentCacheIndexManager getPersistentCacheIndexManager() { - ensureClientConfigured(); + public PersistentCacheIndexManager getPersistentCacheIndexManager() { + clientProvider.ensureConfigured(); if (persistentCacheIndexManager == null && (settings.isPersistenceEnabled() || settings.getCacheSettings() instanceof PersistentCacheSettings)) { - persistentCacheIndexManager = new PersistentCacheIndexManager(client); + persistentCacheIndexManager = new PersistentCacheIndexManager(clientProvider); } return persistentCacheIndexManager; } @@ -444,7 +433,7 @@ public synchronized PersistentCacheIndexManager getPersistentCacheIndexManager() @NonNull public CollectionReference collection(@NonNull String collectionPath) { checkNotNull(collectionPath, "Provided collection path must not be null."); - ensureClientConfigured(); + clientProvider.ensureConfigured(); return new CollectionReference(ResourcePath.fromString(collectionPath), this); } @@ -458,7 +447,7 @@ public CollectionReference collection(@NonNull String collectionPath) { @NonNull public DocumentReference document(@NonNull String documentPath) { checkNotNull(documentPath, "Provided document path must not be null."); - ensureClientConfigured(); + clientProvider.ensureConfigured(); return DocumentReference.forPath(ResourcePath.fromString(documentPath), this); } @@ -479,7 +468,7 @@ public Query collectionGroup(@NonNull String collectionId) { "Invalid collectionId '%s'. Collection IDs must not contain '/'.", collectionId)); } - ensureClientConfigured(); + clientProvider.ensureConfigured(); return new Query( new com.google.firebase.firestore.core.Query(ResourcePath.EMPTY, collectionId), this); } @@ -501,7 +490,7 @@ public Query collectionGroup(@NonNull String collectionId) { */ private Task runTransaction( TransactionOptions options, Transaction.Function updateFunction, Executor executor) { - ensureClientConfigured(); + clientProvider.ensureConfigured(); // We wrap the function they provide in order to // 1. Use internal implementation classes for Transaction, @@ -515,7 +504,7 @@ private Task runTransaction( updateFunction.apply( new Transaction(internalTransaction, FirebaseFirestore.this))); - return client.transaction(options, wrappedUpdateFunction); + return clientProvider.call(client -> client.transaction(options, wrappedUpdateFunction)); } /** @@ -566,7 +555,7 @@ public Task runTransaction( */ @NonNull public WriteBatch batch() { - ensureClientConfigured(); + clientProvider.ensureConfigured(); return new WriteBatch(this); } @@ -608,10 +597,7 @@ public Task runBatch(@NonNull WriteBatch.Function batchFunction) { @NonNull public Task terminate() { instanceRegistry.remove(this.getDatabaseId().getDatabaseId()); - - // The client must be initialized to ensure that all subsequent API usage throws an exception. - this.ensureClientConfigured(); - return client.terminate(); + return clientProvider.terminate(); } /** @@ -630,13 +616,7 @@ public Task terminate() { */ @NonNull public Task waitForPendingWrites() { - ensureClientConfigured(); - return client.waitForPendingWrites(); - } - - @VisibleForTesting - AsyncQueue getAsyncQueue() { - return asyncQueue; + return clientProvider.call(FirestoreClient::waitForPendingWrites); } /** @@ -646,8 +626,7 @@ AsyncQueue getAsyncQueue() { */ @NonNull public Task enableNetwork() { - ensureClientConfigured(); - return client.enableNetwork(); + return clientProvider.call(FirestoreClient::enableNetwork); } /** @@ -659,8 +638,7 @@ public Task enableNetwork() { */ @NonNull public Task disableNetwork() { - ensureClientConfigured(); - return client.disableNetwork(); + return clientProvider.call(FirestoreClient::disableNetwork); } /** Globally enables / disables Cloud Firestore logging for the SDK. */ @@ -692,23 +670,22 @@ public static void setLoggingEnabled(boolean loggingEnabled) { */ @NonNull public Task clearPersistence() { + return clientProvider.executeWhileShutdown(this::clearPersistence); + } + + @NonNull + private Task clearPersistence(Executor executor) { final TaskCompletionSource source = new TaskCompletionSource<>(); - asyncQueue.enqueueAndForgetEvenAfterShutdown( - () -> { - try { - if (client != null && !client.isTerminated()) { - throw new FirebaseFirestoreException( - "Persistence cannot be cleared while the firestore instance is running.", - Code.FAILED_PRECONDITION); - } - SQLitePersistence.clearPersistence(context, databaseId, persistenceKey); - source.setResult(null); - } catch (FirebaseFirestoreException e) { - source.setException(e); - } - }); + executor.execute(() -> { + try { + SQLitePersistence.clearPersistence(context, databaseId, persistenceKey); + source.setResult(null); + } catch (FirebaseFirestoreException e) { + source.setException(e); + } + }); return source.getTask(); - } + }; /** * Attaches a listener for a snapshots-in-sync event. The snapshots-in-sync event indicates that @@ -780,9 +757,8 @@ public ListenerRegistration addSnapshotsInSyncListener( */ @NonNull public LoadBundleTask loadBundle(@NonNull InputStream bundleData) { - ensureClientConfigured(); LoadBundleTask resultTask = new LoadBundleTask(); - client.loadBundle(bundleData, resultTask); + clientProvider.procedure(client -> client.loadBundle(bundleData, resultTask)); return resultTask; } @@ -820,9 +796,7 @@ public LoadBundleTask loadBundle(@NonNull ByteBuffer bundleData) { // TODO(b/261013682): Use an explicit executor in continuations. @SuppressLint("TaskMainThread") public @NonNull Task getNamedQuery(@NonNull String name) { - ensureClientConfigured(); - return client - .getNamedQuery(name) + return clientProvider.call(client -> client.getNamedQuery(name)) .continueWith( task -> { com.google.firebase.firestore.core.Query query = task.getResult(); @@ -847,25 +821,17 @@ public LoadBundleTask loadBundle(@NonNull ByteBuffer bundleData) { */ private ListenerRegistration addSnapshotsInSyncListener( Executor userExecutor, @Nullable Activity activity, @NonNull Runnable runnable) { - ensureClientConfigured(); EventListener eventListener = (Void v, FirebaseFirestoreException error) -> { hardAssert(error == null, "snapshots-in-sync listeners should never get errors."); runnable.run(); }; - AsyncEventListener asyncListener = - new AsyncEventListener(userExecutor, eventListener); - client.addSnapshotsInSyncListener(asyncListener); - return ActivityScope.bind( - activity, - () -> { - asyncListener.mute(); - client.removeSnapshotsInSyncListener(asyncListener); - }); - } - - FirestoreClient getClient() { - return client; + AsyncEventListener asyncListener = new AsyncEventListener<>(userExecutor, eventListener); + return clientProvider.call(client -> client.addSnapshotsInSyncListener(activity, asyncListener)); + } + + T callClient(Function call) { + return clientProvider.call(call); } DatabaseId getDatabaseId() { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java new file mode 100644 index 00000000000..12e7b03059a --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java @@ -0,0 +1,95 @@ +// Copyright 2024 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// +// You may obtain a copy of the License at +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore; + +import androidx.annotation.GuardedBy; +import androidx.core.util.Consumer; + +import com.google.android.gms.tasks.Task; +import com.google.firebase.firestore.core.FirestoreClient; +import com.google.firebase.firestore.util.AsyncQueue; +import com.google.firebase.firestore.util.Function; + +import java.util.concurrent.Executor; + +/** + * The FirestoreClientProvider handles the life cycle of FirestoreClients. + */ +final class FirestoreClientProvider { + + private final Function clientFactory; + @GuardedBy("this") + private FirestoreClient client; + + @GuardedBy("this") + private AsyncQueue asyncQueue; + + FirestoreClientProvider(Function clientFactory) { + this.clientFactory = clientFactory; + this.asyncQueue = new AsyncQueue(); + } + + boolean isConfigured() { + return client != null; + } + + synchronized void ensureConfigured() { + if (!isConfigured()) { + client = clientFactory.apply(asyncQueue); + } + } + + /** + * To facilitate calls to FirestoreClient without risk of FirestoreClient being terminated + * or restarted mid call. + */ + synchronized T call(Function call) { + ensureConfigured(); + return call.apply(client); + } + + /** + * To facilitate calls to FirestoreClient without risk of FirestoreClient being terminated + * or restarted mid call. + */ + synchronized void procedure(Consumer call) { + ensureConfigured(); + call.accept(client); + } + + synchronized T executeWhileShutdown(Function call) { + if (client != null && !client.isTerminated()) { + client.terminate(); + } + Executor executor = command -> asyncQueue.enqueueAndForgetEvenAfterShutdown(command); + return call.apply(executor); + } + + /** + * Shuts down the AsyncQueue and releases resources after which no progress will ever be made + * again. + */ + synchronized Task terminate() { + // The client must be initialized to ensure that all subsequent API usage throws an exception. + ensureConfigured(); + + Task terminate = client.terminate(); + + // Will cause the executor to de-reference all threads, the best we can do + asyncQueue.shutdown(); + + return terminate; + } +} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java index a529acbc8bc..d9927c63489 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/PersistentCacheIndexManager.java @@ -28,11 +28,11 @@ *

To get an instance, call {@link FirebaseFirestore#getPersistentCacheIndexManager()}. */ public final class PersistentCacheIndexManager { - @NonNull private FirestoreClient client; + @NonNull private FirestoreClientProvider client; /** @hide */ @RestrictTo(RestrictTo.Scope.LIBRARY) - PersistentCacheIndexManager(FirestoreClient client) { + PersistentCacheIndexManager(FirestoreClientProvider client) { this.client = client; } @@ -43,7 +43,7 @@ public final class PersistentCacheIndexManager { *

This feature is disabled by default. */ public void enableIndexAutoCreation() { - client.setIndexAutoCreationEnabled(true); + client.procedure(client -> client.setIndexAutoCreationEnabled(true)); } /** @@ -51,7 +51,7 @@ public void enableIndexAutoCreation() { * which have been created by calling {@link #enableIndexAutoCreation()} still take effect. */ public void disableIndexAutoCreation() { - client.setIndexAutoCreationEnabled(false); + client.procedure(client -> client.setIndexAutoCreationEnabled(false)); } /** @@ -59,6 +59,6 @@ public void disableIndexAutoCreation() { * {@link FirebaseFirestore#setIndexConfiguration(String)}, which is deprecated. */ public void deleteAllIndexes() { - client.deleteAllFieldIndexes(); + client.procedure(FirestoreClient::deleteAllFieldIndexes); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index a0e709a54e0..03801447e68 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -26,16 +26,13 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.firestore.FirebaseFirestoreException.Code; -import com.google.firebase.firestore.core.ActivityScope; import com.google.firebase.firestore.core.AsyncEventListener; import com.google.firebase.firestore.core.Bound; import com.google.firebase.firestore.core.CompositeFilter; import com.google.firebase.firestore.core.EventManager.ListenOptions; import com.google.firebase.firestore.core.FieldFilter; import com.google.firebase.firestore.core.FieldFilter.Operator; -import com.google.firebase.firestore.core.ListenerRegistrationImpl; import com.google.firebase.firestore.core.OrderBy; -import com.google.firebase.firestore.core.QueryListener; import com.google.firebase.firestore.core.ViewSnapshot; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; @@ -964,9 +961,7 @@ public Task get() { public Task get(@NonNull Source source) { validateHasExplicitOrderByForLimitToLast(); if (source == Source.CACHE) { - return firestore - .getClient() - .getDocumentsFromLocalCache(query) + return firestore.callClient(client -> client.getDocumentsFromLocalCache(query)) .continueWith( Executors.DIRECT_EXECUTOR, (Task viewSnap) -> @@ -1182,10 +1177,7 @@ private ListenerRegistration addSnapshotListenerInternal( AsyncEventListener asyncListener = new AsyncEventListener<>(executor, viewListener); - QueryListener queryListener = firestore.getClient().listen(query, options, asyncListener); - return ActivityScope.bind( - activity, - new ListenerRegistrationImpl(firestore.getClient(), queryListener, asyncListener)); + return firestore.callClient(client -> client.listen(query, options, activity, asyncListener)); } private void validateHasExplicitOrderByForLimitToLast() { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java index b8c6e9d1651..af886e759e0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java @@ -190,8 +190,8 @@ public WriteBatch delete(@NonNull DocumentReference documentRef) { public Task commit() { verifyNotCommitted(); committed = true; - if (mutations.size() > 0) { - return firestore.getClient().write(mutations); + if (!mutations.isEmpty()) { + return firestore.callClient(client -> client.write(mutations)); } else { return Tasks.forResult(null); } 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 6d411982207..604431abc2f 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 @@ -17,16 +17,17 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import android.annotation.SuppressLint; +import android.app.Activity; import android.content.Context; import androidx.annotation.Nullable; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.firestore.AggregateField; -import com.google.firebase.firestore.EventListener; import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.FirebaseFirestoreException.Code; import com.google.firebase.firestore.FirebaseFirestoreSettings; +import com.google.firebase.firestore.ListenerRegistration; import com.google.firebase.firestore.LoadBundleTask; import com.google.firebase.firestore.TransactionOptions; import com.google.firebase.firestore.auth.CredentialsProvider; @@ -171,22 +172,20 @@ public boolean isTerminated() { } /** Starts listening to a query. */ - public QueryListener listen( - Query query, ListenOptions options, EventListener listener) { + public ListenerRegistration listen( + Query query, + ListenOptions options, + @Nullable Activity activity, + AsyncEventListener listener) { this.verifyNotTerminated(); QueryListener queryListener = new QueryListener(query, options, listener); asyncQueue.enqueueAndForget(() -> eventManager.addQueryListener(queryListener)); - return queryListener; - } - - /** Stops listening to a query previously listened to. */ - public void stopListening(QueryListener listener) { - // Checks for terminate but does not raise error, allowing it to be a no-op if client is already - // terminated. - if (this.isTerminated()) { - return; - } - asyncQueue.enqueueAndForget(() -> eventManager.removeQueryListener(listener)); + return ActivityScope.bind( + activity, + () -> { + listener.mute(); + asyncQueue.enqueueAndForget(() -> eventManager.removeQueryListener(queryListener)); + }); } // TODO(b/261013682): Use an explicit executor in continuations. @@ -308,9 +307,17 @@ private void initialize( } } - public void addSnapshotsInSyncListener(EventListener listener) { + public ListenerRegistration addSnapshotsInSyncListener( + Activity activity, + AsyncEventListener listener) { verifyNotTerminated(); asyncQueue.enqueueAndForget(() -> eventManager.addSnapshotsInSyncListener(listener)); + return ActivityScope.bind( + activity, + () -> { + listener.mute(); + asyncQueue.enqueueAndForget(() -> eventManager.removeSnapshotsInSyncListener(listener)); + }); } public void loadBundle(InputStream bundleData, LoadBundleTask resultTask) { @@ -359,14 +366,6 @@ public void deleteAllFieldIndexes() { asyncQueue.enqueueAndForget(() -> localStore.deleteAllFieldIndexes()); } - public void removeSnapshotsInSyncListener(EventListener listener) { - // Checks for shutdown but does not raise error, allowing remove after shutdown to be a no-op. - if (isTerminated()) { - return; - } - asyncQueue.enqueueAndForget(() -> eventManager.removeSnapshotsInSyncListener(listener)); - } - private void verifyNotTerminated() { if (this.isTerminated()) { throw new IllegalStateException("The client has already been terminated"); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ListenerRegistrationImpl.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ListenerRegistrationImpl.java deleted file mode 100644 index b949319962f..00000000000 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ListenerRegistrationImpl.java +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright 2018 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.firebase.firestore.core; - -import com.google.firebase.firestore.ListenerRegistration; - -/** Implements the ListenerRegistration interface by removing a query from the listener. */ -public class ListenerRegistrationImpl implements ListenerRegistration { - - private final FirestoreClient client; - - /** The internal query listener object that is used to unlisten from the query. */ - private final QueryListener queryListener; - - /** The event listener for the query that raises events asynchronously. */ - private final AsyncEventListener asyncEventListener; - - public ListenerRegistrationImpl( - FirestoreClient client, - QueryListener queryListener, - AsyncEventListener asyncEventListener) { - this.client = client; - this.queryListener = queryListener; - this.asyncEventListener = asyncEventListener; - } - - @Override - public void remove() { - asyncEventListener.mute(); - client.stopListening(queryListener); - } -} diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java index 91836cce99d..0527cafc1ad 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java @@ -124,7 +124,6 @@ public FirebaseFirestoreIntegrationTestFactory(DatabaseId databaseId) { "k", new EmptyCredentialsProvider(), new EmptyAppCheckTokenProvider(), - new AsyncQueue(), this::componentProvider, null, instanceRegistry, From 7a8b052150b99e965746b65cf6d0426c73a94452 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 20 Jun 2024 22:56:20 -0400 Subject: [PATCH 18/37] Fix --- .../firebase/firestore/remote/StreamTest.java | 75 ++++++++++++++----- 1 file changed, 58 insertions(+), 17 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/StreamTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/StreamTest.java index 4cb1529ca4c..c949a04d6cb 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/StreamTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/StreamTest.java @@ -26,10 +26,11 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import androidx.annotation.NonNull; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; +import com.google.firebase.firestore.auth.CredentialsProvider; +import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.DatabaseInfo; import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.model.mutation.Mutation; @@ -111,7 +112,16 @@ public void onWriteResponse( /** Creates a WriteStream and gets it in a state that accepts mutations. */ private WriteStream createAndOpenWriteStream( AsyncQueue testQueue, StreamStatusCallback callback) { - Datastore datastore = createTestDatastore(testQueue, null); + DatabaseInfo databaseInfo = IntegrationTestUtil.testEnvDatabaseInfo(); + RemoteSerializer remoteSerializer = new RemoteSerializer(databaseInfo.getDatabaseId()); + FirestoreChannel firestoreChannel = new FirestoreChannel( + testQueue, + ApplicationProvider.getApplicationContext(), + null == null ? new EmptyCredentialsProvider() : null, + new EmptyAppCheckTokenProvider(), + databaseInfo, + (GrpcMetadataProvider) null); + Datastore datastore = new Datastore(testQueue, remoteSerializer, firestoreChannel); final WriteStream writeStream = datastore.createWriteStream(callback); waitForWriteStreamOpen(testQueue, writeStream, callback); return writeStream; @@ -126,8 +136,10 @@ private void waitForWriteStreamOpen( waitFor(callback.handshakeSemaphore); } - @NonNull - private static Datastore createTestDatastore(AsyncQueue testQueue, GrpcMetadataProvider metadataProvider) { + @Test + public void testWatchStreamStopBeforeHandshake() throws Exception { + AsyncQueue testQueue = new AsyncQueue(); + GrpcMetadataProvider mockGrpcProvider = mock(GrpcMetadataProvider.class); DatabaseInfo databaseInfo = IntegrationTestUtil.testEnvDatabaseInfo(); RemoteSerializer remoteSerializer = new RemoteSerializer(databaseInfo.getDatabaseId()); FirestoreChannel firestoreChannel = new FirestoreChannel( @@ -136,15 +148,8 @@ private static Datastore createTestDatastore(AsyncQueue testQueue, GrpcMetadataP new EmptyCredentialsProvider(), new EmptyAppCheckTokenProvider(), databaseInfo, - metadataProvider); - return new Datastore(testQueue, remoteSerializer, firestoreChannel); - } - - @Test - public void testWatchStreamStopBeforeHandshake() throws Exception { - AsyncQueue testQueue = new AsyncQueue(); - GrpcMetadataProvider mockGrpcProvider = mock(GrpcMetadataProvider.class); - Datastore datastore = createTestDatastore(testQueue, mockGrpcProvider); + mockGrpcProvider); + Datastore datastore = new Datastore(testQueue, remoteSerializer, firestoreChannel); StreamStatusCallback streamCallback = new StreamStatusCallback() {}; final WatchStream watchStream = datastore.createWatchStream(streamCallback); @@ -160,7 +165,16 @@ public void testWatchStreamStopBeforeHandshake() throws Exception { @Test public void testWriteStreamStopAfterHandshake() throws Exception { AsyncQueue testQueue = new AsyncQueue(); - Datastore datastore = createTestDatastore(testQueue, null); + DatabaseInfo databaseInfo = IntegrationTestUtil.testEnvDatabaseInfo(); + RemoteSerializer remoteSerializer = new RemoteSerializer(databaseInfo.getDatabaseId()); + FirestoreChannel firestoreChannel = new FirestoreChannel( + testQueue, + ApplicationProvider.getApplicationContext(), + new EmptyCredentialsProvider(), + new EmptyAppCheckTokenProvider(), + databaseInfo, + (GrpcMetadataProvider) null); + Datastore datastore = new Datastore(testQueue, remoteSerializer, firestoreChannel); final WriteStream[] writeStreamWrapper = new WriteStream[1]; StreamStatusCallback streamCallback = new StreamStatusCallback() { @@ -201,7 +215,16 @@ public void onWriteResponse( @Test public void testWriteStreamStopPartial() throws Exception { AsyncQueue testQueue = new AsyncQueue(); - Datastore datastore = createTestDatastore(testQueue, null); + DatabaseInfo databaseInfo = IntegrationTestUtil.testEnvDatabaseInfo(); + RemoteSerializer remoteSerializer = new RemoteSerializer(databaseInfo.getDatabaseId()); + FirestoreChannel firestoreChannel = new FirestoreChannel( + testQueue, + ApplicationProvider.getApplicationContext(), + new EmptyCredentialsProvider(), + new EmptyAppCheckTokenProvider(), + databaseInfo, + (GrpcMetadataProvider) null); + Datastore datastore = new Datastore(testQueue, remoteSerializer, firestoreChannel); StreamStatusCallback streamCallback = new StreamStatusCallback() {}; final WriteStream writeStream = datastore.createWriteStream(streamCallback); @@ -275,7 +298,16 @@ public void testStreamStaysIdle() throws Exception { public void testStreamRefreshesTokenUponExpiration() throws Exception { AsyncQueue testQueue = new AsyncQueue(); MockCredentialsProvider mockCredentialsProvider = new MockCredentialsProvider(); - Datastore datastore = createTestDatastore(testQueue, null); + DatabaseInfo databaseInfo = IntegrationTestUtil.testEnvDatabaseInfo(); + RemoteSerializer remoteSerializer = new RemoteSerializer(databaseInfo.getDatabaseId()); + FirestoreChannel firestoreChannel = new FirestoreChannel( + testQueue, + ApplicationProvider.getApplicationContext(), + mockCredentialsProvider, + new EmptyAppCheckTokenProvider(), + databaseInfo, + (GrpcMetadataProvider) null); + Datastore datastore = new Datastore(testQueue, remoteSerializer, firestoreChannel); StreamStatusCallback callback = new StreamStatusCallback(); WriteStream writeStream = datastore.createWriteStream(callback); waitForWriteStreamOpen(testQueue, writeStream, callback); @@ -298,7 +330,16 @@ public void testStreamRefreshesTokenUponExpiration() throws Exception { public void testTokenIsNotInvalidatedOnceStreamIsHealthy() throws Exception { AsyncQueue testQueue = new AsyncQueue(); MockCredentialsProvider mockCredentialsProvider = new MockCredentialsProvider(); - Datastore datastore = createTestDatastore(testQueue, null); + DatabaseInfo databaseInfo = IntegrationTestUtil.testEnvDatabaseInfo(); + RemoteSerializer remoteSerializer = new RemoteSerializer(databaseInfo.getDatabaseId()); + FirestoreChannel firestoreChannel = new FirestoreChannel( + testQueue, + ApplicationProvider.getApplicationContext(), + mockCredentialsProvider, + new EmptyAppCheckTokenProvider(), + databaseInfo, + (GrpcMetadataProvider) null); + Datastore datastore = new Datastore(testQueue, remoteSerializer, firestoreChannel); StreamStatusCallback callback = new StreamStatusCallback(); WriteStream writeStream = datastore.createWriteStream(callback); waitForWriteStreamOpen(testQueue, writeStream, callback); From fae9bc05b7b8b6d01e58e474a444f67fd0e12c25 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 3 Jul 2024 21:07:13 -0400 Subject: [PATCH 19/37] Fix after merge --- .../com/google/firebase/firestore/AccessHelper.java | 4 +--- .../firebase/firestore/CompositeIndexQueryTest.java | 2 +- .../com/google/firebase/firestore/FirestoreTest.java | 2 +- .../google/firebase/firestore/TransactionTest.java | 8 ++++---- .../firestore/testutil/IntegrationTestUtil.java | 1 - .../google/firebase/firestore/FirebaseFirestore.java | 6 +----- .../firebase/firestore/FirestoreClientProvider.java | 4 ++++ .../firebase/firestore/core/ComponentProvider.java | 12 +++++++----- .../firebase/firestore/core/FirestoreClient.java | 5 +---- .../firestore/core/MemoryComponentProvider.java | 9 ++++++--- .../firestore/core/SQLiteComponentProvider.java | 10 +++++++--- .../google/firebase/firestore/spec/SpecTestCase.java | 1 - 12 files changed, 33 insertions(+), 31 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java index 9fec3d209f2..d0800b5ce4a 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/AccessHelper.java @@ -33,7 +33,6 @@ public static FirebaseFirestore newFirebaseFirestore( String persistenceKey, CredentialsProvider authProvider, CredentialsProvider appCheckProvider, - AsyncQueue asyncQueue, Function componentProviderFactory, FirebaseApp firebaseApp, FirebaseFirestore.InstanceRegistry instanceRegistry) { @@ -43,7 +42,6 @@ public static FirebaseFirestore newFirebaseFirestore( persistenceKey, authProvider, appCheckProvider, - asyncQueue, componentProviderFactory, firebaseApp, instanceRegistry, @@ -51,6 +49,6 @@ public static FirebaseFirestore newFirebaseFirestore( } public static AsyncQueue getAsyncQueue(FirebaseFirestore firestore) { - return firestore.getAsyncQueue(); + return firestore.clientProvider.getAsyncQueue(); } } diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/CompositeIndexQueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/CompositeIndexQueryTest.java index b73d2aefc18..bc43fc14117 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/CompositeIndexQueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/CompositeIndexQueryTest.java @@ -742,7 +742,7 @@ public void testMultipleInequalityReadFromCacheWhenOffline() { assertEquals(2L, snapshot1.size()); assertFalse(snapshot1.getMetadata().isFromCache()); - waitFor(collection.firestore.getClient().disableNetwork()); + waitFor(collection.firestore.disableNetwork()); QuerySnapshot snapshot2 = waitFor(query.get()); assertEquals(2L, snapshot2.size()); 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 7a68ce90129..7c6b0773437 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 @@ -1111,7 +1111,7 @@ public void testRestartFirestoreLeadsToNewInstance() { assertSame(instance, sameInstance); waitFor(instance.document("abc/123").set(Collections.singletonMap("field", 100L))); - instance.terminate(); + waitFor(instance.terminate()); FirebaseFirestore newInstance = FirebaseFirestore.getInstance(app); newInstance.setFirestoreSettings(newTestSettings()); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java index ddc38ac418e..63e5987e754 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java @@ -371,7 +371,7 @@ public void testIncrementTransactionally() { AtomicInteger started = new AtomicInteger(0); FirebaseFirestore firestore = testFirestore(); - firestore.getAsyncQueue().skipDelaysForTimerId(TimerId.RETRY_TRANSACTION); + AccessHelper.getAsyncQueue(firestore).skipDelaysForTimerId(TimerId.RETRY_TRANSACTION); DocumentReference doc = firestore.collection("counters").document(); waitFor(doc.set(map("count", 5.0))); @@ -437,7 +437,7 @@ public void testUpdateTransactionally() { AtomicInteger counter = new AtomicInteger(0); FirebaseFirestore firestore = testFirestore(); - firestore.getAsyncQueue().skipDelaysForTimerId(TimerId.RETRY_TRANSACTION); + AccessHelper.getAsyncQueue(firestore).skipDelaysForTimerId(TimerId.RETRY_TRANSACTION); DocumentReference doc = firestore.collection("counters").document(); waitFor(doc.set(map("count", 5.0, "other", "yes"))); @@ -532,7 +532,7 @@ public void testUpdatePOJOTransactionally() { AtomicInteger started = new AtomicInteger(0); FirebaseFirestore firestore = testFirestore(); - firestore.getAsyncQueue().skipDelaysForTimerId(TimerId.RETRY_TRANSACTION); + AccessHelper.getAsyncQueue(firestore).skipDelaysForTimerId(TimerId.RETRY_TRANSACTION); DocumentReference doc = firestore.collection("counters").document(); waitFor(doc.set(new POJO(5.0, "no", "clean"))); @@ -601,7 +601,7 @@ public void testRetriesWhenDocumentThatWasReadWithoutBeingWrittenChanges() { @Test public void testReadingADocTwiceWithDifferentVersions() { FirebaseFirestore firestore = testFirestore(); - firestore.getAsyncQueue().skipDelaysForTimerId(TimerId.RETRY_TRANSACTION); + AccessHelper.getAsyncQueue(firestore).skipDelaysForTimerId(TimerId.RETRY_TRANSACTION); DocumentReference doc = firestore.collection("counters").document(); waitFor(doc.set(map("count", 15.0))); AtomicInteger counter = new AtomicInteger(0); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index 2f2ed0a5f47..6e0d8a07a92 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -300,7 +300,6 @@ public static FirebaseFirestore testFirestore( persistenceKey, MockCredentialsProvider.instance(), new EmptyAppCheckTokenProvider(), - asyncQueue, ComponentProvider::defaultFactory, /*firebaseApp=*/ null, /*instanceRegistry=*/ (dbId) -> {}); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index f6802f515ee..00ebd057db9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -210,7 +210,6 @@ static FirebaseFirestore newInstance( authProvider, appCheckProvider, ComponentProvider::defaultFactory, - ComponentProvider::defaultFactory, app, instanceRegistry, metadataProvider); @@ -224,7 +223,6 @@ static FirebaseFirestore newInstance( CredentialsProvider authProvider, CredentialsProvider appCheckProvider, @NonNull Function componentProviderFactory, - @NonNull Function componentProviderFactory, @Nullable FirebaseApp firebaseApp, InstanceRegistry instanceRegistry, @Nullable GrpcMetadataProvider metadataProvider) { @@ -236,7 +234,6 @@ static FirebaseFirestore newInstance( this.appCheckProvider = checkNotNull(appCheckProvider); this.componentProviderFactory = checkNotNull(componentProviderFactory); this.clientProvider = new FirestoreClientProvider(this::newClient); - this.componentProviderFactory = checkNotNull(componentProviderFactory); // NOTE: We allow firebaseApp to be null in tests only. this.firebaseApp = firebaseApp; this.instanceRegistry = instanceRegistry; @@ -298,10 +295,9 @@ private FirestoreClient newClient(AsyncQueue asyncQueue) { DatabaseInfo databaseInfo = new DatabaseInfo(databaseId, persistenceKey, settings.getHost(), settings.isSslEnabled()); - FirestoreClient client = new FirestoreClient( + return new FirestoreClient( context, databaseInfo, - settings, authProvider, appCheckProvider, asyncQueue, diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java index 12e7b03059a..fbef8dd266c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java @@ -92,4 +92,8 @@ synchronized Task terminate() { return terminate; } + + AsyncQueue getAsyncQueue() { + return asyncQueue; + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ComponentProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ComponentProvider.java index 9b5794a1a0b..27977f94219 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ComponentProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ComponentProvider.java @@ -45,6 +45,7 @@ */ public abstract class ComponentProvider { + protected final FirebaseFirestoreSettings settings; private RemoteComponenetProvider remoteProvider = new RemoteComponenetProvider(); private Persistence persistence; private LocalStore localStore; @@ -54,11 +55,15 @@ public abstract class ComponentProvider { @Nullable private IndexBackfiller indexBackfiller; @Nullable private Scheduler garbageCollectionScheduler; + public ComponentProvider(FirebaseFirestoreSettings settings) { + this.settings = settings; + } + @NonNull public static ComponentProvider defaultFactory(@NonNull FirebaseFirestoreSettings settings) { return settings.isPersistenceEnabled() - ? new SQLiteComponentProvider() - : new MemoryComponentProvider(); + ? new SQLiteComponentProvider(settings) + : new MemoryComponentProvider(settings); } /** Configuration options for the component provider. */ @@ -69,7 +74,6 @@ public static final class Configuration { public final DatabaseInfo databaseInfo; public final User initialUser; public final int maxConcurrentLimboResolutions; - public final FirebaseFirestoreSettings settings; public final CredentialsProvider authProvider; public final CredentialsProvider appCheckProvider; @@ -82,7 +86,6 @@ public Configuration( DatabaseInfo databaseInfo, User initialUser, int maxConcurrentLimboResolutions, - FirebaseFirestoreSettings settings, CredentialsProvider authProvider, CredentialsProvider appCheckProvider, @Nullable GrpcMetadataProvider metadataProvider @@ -92,7 +95,6 @@ public Configuration( this.databaseInfo = databaseInfo; this.initialUser = initialUser; this.maxConcurrentLimboResolutions = maxConcurrentLimboResolutions; - this.settings = settings; this.authProvider = authProvider; this.appCheckProvider = appCheckProvider; this.metadataProvider = metadataProvider; 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 604431abc2f..45ae3ea9d29 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 @@ -85,7 +85,6 @@ public final class FirestoreClient { public FirestoreClient( final Context context, DatabaseInfo databaseInfo, - FirebaseFirestoreSettings settings, CredentialsProvider authProvider, CredentialsProvider appCheckProvider, AsyncQueue asyncQueue, @@ -109,7 +108,7 @@ public FirestoreClient( try { // Block on initial user being available User initialUser = Tasks.await(firstUser.getTask()); - initialize(context, initialUser, settings, componentProvider, metadataProvider); + initialize(context, initialUser, componentProvider, metadataProvider); } catch (InterruptedException | ExecutionException e) { throw new RuntimeException(e); } @@ -269,7 +268,6 @@ public Task waitForPendingWrites() { private void initialize( Context context, User user, - FirebaseFirestoreSettings settings, ComponentProvider provider, GrpcMetadataProvider metadataProvider) { // Note: The initialization work must all be synchronous (we can't dispatch more work) since @@ -284,7 +282,6 @@ private void initialize( databaseInfo, user, MAX_CONCURRENT_LIMBO_RESOLUTIONS, - settings, authProvider, appCheckProvider, metadataProvider); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/MemoryComponentProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/MemoryComponentProvider.java index 6781fad65de..431423eeba8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/MemoryComponentProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/MemoryComponentProvider.java @@ -39,6 +39,10 @@ */ public class MemoryComponentProvider extends ComponentProvider { + public MemoryComponentProvider(FirebaseFirestoreSettings settings) { + super(settings); + } + @Override @Nullable protected Scheduler createGarbageCollectionScheduler(Configuration configuration) { @@ -73,11 +77,10 @@ private boolean isMemoryLruGcEnabled(FirebaseFirestoreSettings settings) { @Override protected Persistence createPersistence(Configuration configuration) { - if (isMemoryLruGcEnabled(configuration.settings)) { + if (isMemoryLruGcEnabled(settings)) { LocalSerializer serializer = new LocalSerializer(getRemoteSerializer()); LruGarbageCollector.Params params = - LruGarbageCollector.Params.WithCacheSizeBytes( - configuration.settings.getCacheSizeBytes()); + LruGarbageCollector.Params.WithCacheSizeBytes(settings.getCacheSizeBytes()); return MemoryPersistence.createLruGcMemoryPersistence(params, serializer); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java index 99586a8784a..216d9388e92 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java @@ -14,6 +14,7 @@ package com.google.firebase.firestore.core; +import com.google.firebase.firestore.FirebaseFirestoreSettings; import com.google.firebase.firestore.local.IndexBackfiller; import com.google.firebase.firestore.local.LocalSerializer; import com.google.firebase.firestore.local.LruDelegate; @@ -25,7 +26,11 @@ /** Provides all components needed for Firestore with SQLite persistence. */ public class SQLiteComponentProvider extends MemoryComponentProvider { - @Override + public SQLiteComponentProvider(FirebaseFirestoreSettings settings) { + super(settings); + } + + @Override protected Scheduler createGarbageCollectionScheduler(Configuration configuration) { LruDelegate lruDelegate = ((SQLitePersistence) getPersistence()).getReferenceDelegate(); LruGarbageCollector gc = lruDelegate.getGarbageCollector(); @@ -42,8 +47,7 @@ protected Persistence createPersistence(Configuration configuration) { LocalSerializer serializer = new LocalSerializer(getRemoteSerializer()); LruGarbageCollector.Params params = - LruGarbageCollector.Params.WithCacheSizeBytes( - configuration.settings.getCacheSizeBytes()); + LruGarbageCollector.Params.WithCacheSizeBytes(settings.getCacheSizeBytes()); return new SQLitePersistence( configuration.context, configuration.databaseInfo.getPersistenceKey(), 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 8a9927abca6..dabaac137ed 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 @@ -330,7 +330,6 @@ private void initClient() { databaseInfo, currentUser, maxConcurrentLimboResolutions, - new FirebaseFirestoreSettings.Builder().build(), new EmptyCredentialsProvider(), new EmptyAppCheckTokenProvider(), null From 0783dcdfc1ded2a4ea0f7f001b1eac43368a7390 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 3 Jul 2024 21:14:19 -0400 Subject: [PATCH 20/37] Whitespace --- .../firebase/firestore/core/SQLiteComponentProvider.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java index 216d9388e92..56bb3f26911 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SQLiteComponentProvider.java @@ -26,11 +26,11 @@ /** Provides all components needed for Firestore with SQLite persistence. */ public class SQLiteComponentProvider extends MemoryComponentProvider { - public SQLiteComponentProvider(FirebaseFirestoreSettings settings) { - super(settings); - } + public SQLiteComponentProvider(FirebaseFirestoreSettings settings) { + super(settings); + } - @Override + @Override protected Scheduler createGarbageCollectionScheduler(Configuration configuration) { LruDelegate lruDelegate = ((SQLitePersistence) getPersistence()).getReferenceDelegate(); LruGarbageCollector gc = lruDelegate.getGarbageCollector(); From 63e2ec2a3bf14051dac592c0babd7f15de7610a0 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 3 Jul 2024 21:22:54 -0400 Subject: [PATCH 21/37] Fix --- .../com/google/firebase/firestore/spec/MemorySpecTest.java | 3 ++- .../com/google/firebase/firestore/spec/SQLiteSpecTest.java | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java index 75fe3f4503f..358ff7784af 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/MemorySpecTest.java @@ -14,6 +14,7 @@ package com.google.firebase.firestore.spec; +import com.google.firebase.firestore.FirebaseFirestoreSettings; import com.google.firebase.firestore.core.ComponentProvider; import com.google.firebase.firestore.core.MemoryComponentProvider; import com.google.firebase.firestore.local.LocalSerializer; @@ -41,7 +42,7 @@ protected boolean isExcluded(Set tags) { protected MemoryComponentProvider initializeComponentProvider( RemoteComponenetProvider remoteProvider, ComponentProvider.Configuration configuration, boolean useEagerGc) { MemoryComponentProvider provider = - new MemoryComponentProvider() { + new MemoryComponentProvider(new FirebaseFirestoreSettings.Builder().build()) { @Override protected Persistence createPersistence(Configuration configuration) { if (useEagerGc) { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java index 969774cb33c..c7534d9f30f 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java @@ -14,6 +14,7 @@ package com.google.firebase.firestore.spec; +import com.google.firebase.firestore.FirebaseFirestoreSettings; import com.google.firebase.firestore.core.ComponentProvider; import com.google.firebase.firestore.core.SQLiteComponentProvider; import com.google.firebase.firestore.remote.RemoteComponenetProvider; @@ -40,7 +41,7 @@ protected SQLiteComponentProvider initializeComponentProvider( RemoteComponenetProvider remoteProvider, ComponentProvider.Configuration configuration, boolean garbageCollectionEnabled) { - SQLiteComponentProvider provider = new SQLiteComponentProvider(); + SQLiteComponentProvider provider = new SQLiteComponentProvider(new FirebaseFirestoreSettings.Builder().build()); provider.setRemoteProvider(remoteProvider); provider.initialize(configuration); return provider; From 44540c48da4c48cc389afa12c32503ed0319f6eb Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 17 Jul 2024 13:57:03 -0400 Subject: [PATCH 22/37] Pretty --- .../firebase/firestore/AggregateQuery.java | 4 +- .../firebase/firestore/DocumentReference.java | 13 +- .../firebase/firestore/FirebaseFirestore.java | 40 ++++--- .../firestore/FirestoreClientProvider.java | 113 +++++++++--------- .../com/google/firebase/firestore/Query.java | 3 +- .../firestore/core/FirestoreClient.java | 4 +- ...rebaseFirestoreIntegrationTestFactory.java | 24 ++-- .../firestore/spec/SQLiteSpecTest.java | 3 +- .../firebase/firestore/spec/SpecTestCase.java | 1 - 9 files changed, 106 insertions(+), 99 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java index 41e48180cb9..0472975d6c7 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/AggregateQuery.java @@ -64,7 +64,9 @@ public List getAggregateFields() { public Task get(@NonNull AggregateSource source) { Preconditions.checkNotNull(source, "AggregateSource must not be null"); TaskCompletionSource tcs = new TaskCompletionSource<>(); - query.firestore.callClient(client -> client.runAggregateQuery(query.query, aggregateFieldList)) + query + .firestore + .callClient(client -> client.runAggregateQuery(query.query, aggregateFieldList)) .continueWith( Executors.DIRECT_EXECUTOR, (task) -> { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index e9521af5747..a7c75e79c28 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -40,7 +40,6 @@ import com.google.firebase.firestore.util.Assert; import com.google.firebase.firestore.util.Executors; import com.google.firebase.firestore.util.Util; -import java.util.Collections; import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; @@ -165,7 +164,8 @@ public Task set(@NonNull Object data, @NonNull SetOptions options) { ? firestore.getUserDataReader().parseMergeData(data, options.getFieldMask()) : firestore.getUserDataReader().parseSetData(data); List mutations = singletonList(parsed.toMutation(key, Precondition.NONE)); - return firestore.callClient(client -> client.write(mutations)) + return firestore + .callClient(client -> client.write(mutations)) .continueWith(Executors.DIRECT_EXECUTOR, voidErrorTransformer()); } @@ -228,7 +228,8 @@ public Task update( private Task update(@NonNull ParsedUpdateData parsedData) { List mutations = singletonList(parsedData.toMutation(key, Precondition.exists(true))); - return firestore.callClient(client -> client.write(mutations)) + return firestore + .callClient(client -> client.write(mutations)) .continueWith(Executors.DIRECT_EXECUTOR, voidErrorTransformer()); } @@ -240,7 +241,8 @@ private Task update(@NonNull ParsedUpdateData parsedData) { @NonNull public Task delete() { List mutations = singletonList(new DeleteMutation(key, Precondition.NONE)); - return firestore.callClient(client -> client.write(mutations)) + return firestore + .callClient(client -> client.write(mutations)) .continueWith(Executors.DIRECT_EXECUTOR, voidErrorTransformer()); } @@ -269,7 +271,8 @@ public Task get() { @NonNull public Task get(@NonNull Source source) { if (source == Source.CACHE) { - return firestore.callClient(client -> client.getDocumentFromLocalCache(key)) + return firestore + .callClient(client -> client.getDocumentFromLocalCache(key)) .continueWith( Executors.DIRECT_EXECUTOR, (Task task) -> { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index 00ebd057db9..d1b8c00f9b7 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -296,13 +296,13 @@ private FirestoreClient newClient(AsyncQueue asyncQueue) { new DatabaseInfo(databaseId, persistenceKey, settings.getHost(), settings.isSslEnabled()); return new FirestoreClient( - context, - databaseInfo, - authProvider, - appCheckProvider, - asyncQueue, - metadataProvider, - componentProviderFactory.apply(settings)); + context, + databaseInfo, + authProvider, + appCheckProvider, + asyncQueue, + metadataProvider, + componentProviderFactory.apply(settings)); } } @@ -673,16 +673,18 @@ public Task clearPersistence() { @NonNull private Task clearPersistence(Executor executor) { final TaskCompletionSource source = new TaskCompletionSource<>(); - executor.execute(() -> { - try { - SQLitePersistence.clearPersistence(context, databaseId, persistenceKey); - source.setResult(null); - } catch (FirebaseFirestoreException e) { - source.setException(e); - } - }); + executor.execute( + () -> { + try { + SQLitePersistence.clearPersistence(context, databaseId, persistenceKey); + source.setResult(null); + } catch (FirebaseFirestoreException e) { + source.setException(e); + } + }); return source.getTask(); - }; + } + ; /** * Attaches a listener for a snapshots-in-sync event. The snapshots-in-sync event indicates that @@ -793,7 +795,8 @@ public LoadBundleTask loadBundle(@NonNull ByteBuffer bundleData) { // TODO(b/261013682): Use an explicit executor in continuations. @SuppressLint("TaskMainThread") public @NonNull Task getNamedQuery(@NonNull String name) { - return clientProvider.call(client -> client.getNamedQuery(name)) + return clientProvider + .call(client -> client.getNamedQuery(name)) .continueWith( task -> { com.google.firebase.firestore.core.Query query = task.getResult(); @@ -824,7 +827,8 @@ private ListenerRegistration addSnapshotsInSyncListener( runnable.run(); }; AsyncEventListener asyncListener = new AsyncEventListener<>(userExecutor, eventListener); - return clientProvider.call(client -> client.addSnapshotsInSyncListener(activity, asyncListener)); + return clientProvider.call( + client -> client.addSnapshotsInSyncListener(activity, asyncListener)); } T callClient(Function call) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java index fbef8dd266c..c3a5b526f1e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java @@ -16,12 +16,10 @@ import androidx.annotation.GuardedBy; import androidx.core.util.Consumer; - import com.google.android.gms.tasks.Task; import com.google.firebase.firestore.core.FirestoreClient; import com.google.firebase.firestore.util.AsyncQueue; import com.google.firebase.firestore.util.Function; - import java.util.concurrent.Executor; /** @@ -29,71 +27,72 @@ */ final class FirestoreClientProvider { - private final Function clientFactory; - @GuardedBy("this") - private FirestoreClient client; + private final Function clientFactory; - @GuardedBy("this") - private AsyncQueue asyncQueue; + @GuardedBy("this") + private FirestoreClient client; - FirestoreClientProvider(Function clientFactory) { - this.clientFactory = clientFactory; - this.asyncQueue = new AsyncQueue(); - } - - boolean isConfigured() { - return client != null; - } + @GuardedBy("this") + private AsyncQueue asyncQueue; - synchronized void ensureConfigured() { - if (!isConfigured()) { - client = clientFactory.apply(asyncQueue); - } - } + FirestoreClientProvider(Function clientFactory) { + this.clientFactory = clientFactory; + this.asyncQueue = new AsyncQueue(); + } - /** - * To facilitate calls to FirestoreClient without risk of FirestoreClient being terminated - * or restarted mid call. - */ - synchronized T call(Function call) { - ensureConfigured(); - return call.apply(client); - } + boolean isConfigured() { + return client != null; + } - /** - * To facilitate calls to FirestoreClient without risk of FirestoreClient being terminated - * or restarted mid call. - */ - synchronized void procedure(Consumer call) { - ensureConfigured(); - call.accept(client); + synchronized void ensureConfigured() { + if (!isConfigured()) { + client = clientFactory.apply(asyncQueue); } - - synchronized T executeWhileShutdown(Function call) { - if (client != null && !client.isTerminated()) { - client.terminate(); - } - Executor executor = command -> asyncQueue.enqueueAndForgetEvenAfterShutdown(command); - return call.apply(executor); + } + + /** + * To facilitate calls to FirestoreClient without risk of FirestoreClient being terminated + * or restarted mid call. + */ + synchronized T call(Function call) { + ensureConfigured(); + return call.apply(client); + } + + /** + * To facilitate calls to FirestoreClient without risk of FirestoreClient being terminated + * or restarted mid call. + */ + synchronized void procedure(Consumer call) { + ensureConfigured(); + call.accept(client); + } + + synchronized T executeWhileShutdown(Function call) { + if (client != null && !client.isTerminated()) { + client.terminate(); } + Executor executor = command -> asyncQueue.enqueueAndForgetEvenAfterShutdown(command); + return call.apply(executor); + } - /** - * Shuts down the AsyncQueue and releases resources after which no progress will ever be made - * again. - */ - synchronized Task terminate() { - // The client must be initialized to ensure that all subsequent API usage throws an exception. - ensureConfigured(); + /** + * Shuts down the AsyncQueue and releases resources after which no progress will ever be made + * again. + */ + synchronized Task terminate() { + // The client must be initialized to ensure that all subsequent API usage throws an exception. + ensureConfigured(); - Task terminate = client.terminate(); + Task terminate = client.terminate(); - // Will cause the executor to de-reference all threads, the best we can do - asyncQueue.shutdown(); + // Will cause the executor to de-reference all threads, the best we can do + asyncQueue.shutdown(); - return terminate; - } + return terminate; + } - AsyncQueue getAsyncQueue() { - return asyncQueue; - } + AsyncQueue getAsyncQueue() { + return asyncQueue; + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 28645eb50ee..ea4ea11731d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -961,7 +961,8 @@ public Task get() { public Task get(@NonNull Source source) { validateHasExplicitOrderByForLimitToLast(); if (source == Source.CACHE) { - return firestore.callClient(client -> client.getDocumentsFromLocalCache(query)) + return firestore + .callClient(client -> client.getDocumentsFromLocalCache(query)) .continueWith( Executors.DIRECT_EXECUTOR, (Task viewSnap) -> 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 45ae3ea9d29..e1c2b03acc1 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 @@ -26,7 +26,6 @@ import com.google.firebase.firestore.AggregateField; import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.FirebaseFirestoreException.Code; -import com.google.firebase.firestore.FirebaseFirestoreSettings; import com.google.firebase.firestore.ListenerRegistration; import com.google.firebase.firestore.LoadBundleTask; import com.google.firebase.firestore.TransactionOptions; @@ -305,8 +304,7 @@ private void initialize( } public ListenerRegistration addSnapshotsInSyncListener( - Activity activity, - AsyncEventListener listener) { + Activity activity, AsyncEventListener listener) { verifyNotTerminated(); asyncQueue.enqueueAndForget(() -> eventManager.addSnapshotsInSyncListener(listener)); return ActivityScope.bind( diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java index 425a5974545..a8afe3d52d3 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/FirebaseFirestoreIntegrationTestFactory.java @@ -30,7 +30,6 @@ import com.google.firebase.firestore.remote.RemoteComponenetProvider; import com.google.firebase.firestore.testutil.EmptyAppCheckTokenProvider; import com.google.firebase.firestore.testutil.EmptyCredentialsProvider; -import com.google.firebase.firestore.util.AsyncQueue; import com.google.firestore.v1.FirestoreGrpc; import com.google.firestore.v1.ListenRequest; import com.google.firestore.v1.ListenResponse; @@ -117,17 +116,18 @@ public Task> getWriteClient(int i) { public final FirebaseFirestore.InstanceRegistry instanceRegistry = mock(FirebaseFirestore.InstanceRegistry.class); - public FirebaseFirestoreIntegrationTestFactory(DatabaseId databaseId) { - firestore = new FirebaseFirestore( - ApplicationProvider.getApplicationContext(), - databaseId, - "k", - new EmptyCredentialsProvider(), - new EmptyAppCheckTokenProvider(), - this::componentProvider, - null, - instanceRegistry, - null); + public FirebaseFirestoreIntegrationTestFactory(DatabaseId databaseId) { + firestore = + new FirebaseFirestore( + ApplicationProvider.getApplicationContext(), + databaseId, + "k", + new EmptyCredentialsProvider(), + new EmptyAppCheckTokenProvider(), + this::componentProvider, + null, + instanceRegistry, + null); } public void useMemoryCache() { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java index f2673f55fbd..959a106ddff 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SQLiteSpecTest.java @@ -40,7 +40,8 @@ protected SQLiteComponentProvider initializeComponentProvider( RemoteComponenetProvider remoteProvider, ComponentProvider.Configuration configuration, boolean garbageCollectionEnabled) { - SQLiteComponentProvider provider = new SQLiteComponentProvider(new FirebaseFirestoreSettings.Builder().build()); + SQLiteComponentProvider provider = + new SQLiteComponentProvider(new FirebaseFirestoreSettings.Builder().build()); provider.setRemoteProvider(remoteProvider); provider.initialize(configuration); return provider; 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 82d59792c7e..8814b42baef 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 @@ -41,7 +41,6 @@ import com.google.firebase.firestore.EventListener; import com.google.firebase.firestore.FirebaseFirestore; import com.google.firebase.firestore.FirebaseFirestoreException; -import com.google.firebase.firestore.FirebaseFirestoreSettings; import com.google.firebase.firestore.ListenSource; import com.google.firebase.firestore.LoadBundleTask; import com.google.firebase.firestore.auth.User; From 272cf53ced4d761b055e6e2dcdc90b5046bbcb62 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Wed, 17 Jul 2024 15:15:52 -0400 Subject: [PATCH 23/37] Fix --- .../google/firebase/firestore/FirebaseFirestore.java | 8 +++++++- .../firebase/firestore/FirestoreClientProvider.java | 12 +++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index d1b8c00f9b7..37637a5acab 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -667,7 +667,13 @@ public static void setLoggingEnabled(boolean loggingEnabled) { */ @NonNull public Task clearPersistence() { - return clientProvider.executeWhileShutdown(this::clearPersistence); + return clientProvider.executeIfShutdown( + this::clearPersistence, + executor -> + Tasks.forException( + new FirebaseFirestoreException( + "Persistence cannot be cleared while the firestore instance is running.", + FirebaseFirestoreException.Code.FAILED_PRECONDITION))); } @NonNull diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java index c3a5b526f1e..77cc088f8f1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java @@ -68,12 +68,14 @@ synchronized void procedure(Consumer call) { call.accept(client); } - synchronized T executeWhileShutdown(Function call) { - if (client != null && !client.isTerminated()) { - client.terminate(); - } + synchronized T executeIfShutdown( + Function callIf, Function callElse) { Executor executor = command -> asyncQueue.enqueueAndForgetEvenAfterShutdown(command); - return call.apply(executor); + if (client == null || client.isTerminated()) { + return callIf.apply(executor); + } else { + return callElse.apply(executor); + } } /** From 285e6fd759a748ef53e3638c94671421c3c8fdf6 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 22 Jul 2024 10:14:12 -0400 Subject: [PATCH 24/37] Fixes from code review. --- .../firebase/firestore/DocumentReference.java | 14 +++++++++++++- .../com/google/firebase/firestore/Query.java | 13 ++++++++++++- .../firebase/firestore/core/FirestoreClient.java | 16 ++++++++++++++++ 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index a7c75e79c28..e3097d32b00 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -26,8 +26,10 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.firestore.FirebaseFirestoreException.Code; +import com.google.firebase.firestore.core.ActivityScope; import com.google.firebase.firestore.core.AsyncEventListener; import com.google.firebase.firestore.core.EventManager.ListenOptions; +import com.google.firebase.firestore.core.QueryListener; import com.google.firebase.firestore.core.UserData.ParsedSetData; import com.google.firebase.firestore.core.UserData.ParsedUpdateData; import com.google.firebase.firestore.core.ViewSnapshot; @@ -528,7 +530,17 @@ private ListenerRegistration addSnapshotListenerInternal( new AsyncEventListener<>(userExecutor, viewListener); com.google.firebase.firestore.core.Query query = asQuery(); - return firestore.callClient(client -> client.listen(query, options, activity, asyncListener)); + + return firestore.callClient( + client -> { + QueryListener queryListener = client.listen(query, options, asyncListener); + return ActivityScope.bind( + activity, + () -> { + asyncListener.mute(); + client.stopListening(queryListener); + }); + }); } @Override diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index ea4ea11731d..d5fb8a4399b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -26,6 +26,7 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.firestore.FirebaseFirestoreException.Code; +import com.google.firebase.firestore.core.ActivityScope; import com.google.firebase.firestore.core.AsyncEventListener; import com.google.firebase.firestore.core.Bound; import com.google.firebase.firestore.core.CompositeFilter; @@ -33,6 +34,7 @@ import com.google.firebase.firestore.core.FieldFilter; import com.google.firebase.firestore.core.FieldFilter.Operator; import com.google.firebase.firestore.core.OrderBy; +import com.google.firebase.firestore.core.QueryListener; import com.google.firebase.firestore.core.ViewSnapshot; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; @@ -1178,7 +1180,16 @@ private ListenerRegistration addSnapshotListenerInternal( AsyncEventListener asyncListener = new AsyncEventListener<>(executor, viewListener); - return firestore.callClient(client -> client.listen(query, options, activity, asyncListener)); + return firestore.callClient( + client -> { + QueryListener queryListener = client.listen(query, options, asyncListener); + return ActivityScope.bind( + activity, + () -> { + asyncListener.mute(); + client.stopListening(queryListener); + }); + }); } private void validateHasExplicitOrderByForLimitToLast() { 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 e1c2b03acc1..2f878605d87 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 @@ -24,6 +24,7 @@ import com.google.android.gms.tasks.TaskCompletionSource; import com.google.android.gms.tasks.Tasks; import com.google.firebase.firestore.AggregateField; +import com.google.firebase.firestore.EventListener; import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.FirebaseFirestoreException.Code; import com.google.firebase.firestore.ListenerRegistration; @@ -186,6 +187,21 @@ public ListenerRegistration listen( }); } + /** Starts listening to a query. */ + public QueryListener listen( + Query query, ListenOptions options, EventListener listener) { + this.verifyNotTerminated(); + QueryListener queryListener = new QueryListener(query, options, listener); + asyncQueue.enqueueAndForget(() -> eventManager.addQueryListener(queryListener)); + return queryListener; + } + + /** Stops listening to a query previously listened to. */ + public void stopListening(QueryListener listener) { + // `enqueueAndForget` will no-op if client is already terminated. + asyncQueue.enqueueAndForget(() -> eventManager.removeQueryListener(listener)); + } + // TODO(b/261013682): Use an explicit executor in continuations. @SuppressLint("TaskMainThread") public Task getDocumentFromLocalCache(DocumentKey docKey) { From f989c0ed6ad3c30fc8d280e1075eb4c20bd2ac4f Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 22 Jul 2024 10:21:58 -0400 Subject: [PATCH 25/37] Remove dead code. --- .../firestore/core/FirestoreClient.java | 17 ----------------- 1 file changed, 17 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 2f878605d87..d8d2488f591 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 @@ -170,23 +170,6 @@ public boolean isTerminated() { return this.asyncQueue.isShuttingDown(); } - /** Starts listening to a query. */ - public ListenerRegistration listen( - Query query, - ListenOptions options, - @Nullable Activity activity, - AsyncEventListener listener) { - this.verifyNotTerminated(); - QueryListener queryListener = new QueryListener(query, options, listener); - asyncQueue.enqueueAndForget(() -> eventManager.addQueryListener(queryListener)); - return ActivityScope.bind( - activity, - () -> { - listener.mute(); - asyncQueue.enqueueAndForget(() -> eventManager.removeQueryListener(queryListener)); - }); - } - /** Starts listening to a query. */ public QueryListener listen( Query query, ListenOptions options, EventListener listener) { From 0d35529dc0fa03b654db8c9e966e09c978113a7c Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 22 Jul 2024 10:55:05 -0400 Subject: [PATCH 26/37] Fix according to review --- .../firebase/firestore/FirebaseFirestore.java | 11 ++++++++++- .../firebase/firestore/core/FirestoreClient.java | 16 ++++++---------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index 37637a5acab..c1218829b8a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -36,6 +36,7 @@ import com.google.firebase.firestore.auth.FirebaseAppCheckTokenProvider; import com.google.firebase.firestore.auth.FirebaseAuthCredentialsProvider; import com.google.firebase.firestore.auth.User; +import com.google.firebase.firestore.core.ActivityScope; import com.google.firebase.firestore.core.AsyncEventListener; import com.google.firebase.firestore.core.ComponentProvider; import com.google.firebase.firestore.core.DatabaseInfo; @@ -834,7 +835,15 @@ private ListenerRegistration addSnapshotsInSyncListener( }; AsyncEventListener asyncListener = new AsyncEventListener<>(userExecutor, eventListener); return clientProvider.call( - client -> client.addSnapshotsInSyncListener(activity, asyncListener)); + client -> { + client.addSnapshotsInSyncListener(asyncListener); + return ActivityScope.bind( + activity, + () -> { + asyncListener.mute(); + client.removeSnapshotsInSyncListener(asyncListener); + }); + }); } T callClient(Function call) { 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 d8d2488f591..6e2d9b87b84 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 @@ -17,7 +17,6 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import android.annotation.SuppressLint; -import android.app.Activity; import android.content.Context; import androidx.annotation.Nullable; import com.google.android.gms.tasks.Task; @@ -27,7 +26,6 @@ import com.google.firebase.firestore.EventListener; import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.FirebaseFirestoreException.Code; -import com.google.firebase.firestore.ListenerRegistration; import com.google.firebase.firestore.LoadBundleTask; import com.google.firebase.firestore.TransactionOptions; import com.google.firebase.firestore.auth.CredentialsProvider; @@ -302,16 +300,9 @@ private void initialize( } } - public ListenerRegistration addSnapshotsInSyncListener( - Activity activity, AsyncEventListener listener) { + public void addSnapshotsInSyncListener(EventListener listener) { verifyNotTerminated(); asyncQueue.enqueueAndForget(() -> eventManager.addSnapshotsInSyncListener(listener)); - return ActivityScope.bind( - activity, - () -> { - listener.mute(); - asyncQueue.enqueueAndForget(() -> eventManager.removeSnapshotsInSyncListener(listener)); - }); } public void loadBundle(InputStream bundleData, LoadBundleTask resultTask) { @@ -360,6 +351,11 @@ public void deleteAllFieldIndexes() { asyncQueue.enqueueAndForget(() -> localStore.deleteAllFieldIndexes()); } + public void removeSnapshotsInSyncListener(EventListener listener) { + // `enqueueAndForget` will no-op if client is already terminated. + asyncQueue.enqueueAndForget(() -> eventManager.removeSnapshotsInSyncListener(listener)); + } + private void verifyNotTerminated() { if (this.isTerminated()) { throw new IllegalStateException("The client has already been terminated"); From 1aa54b3d8fe660f66940548b036d41ebc4efa0df Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 26 Jul 2024 13:08:36 -0400 Subject: [PATCH 27/37] Add comments --- .../firestore/FirestoreClientProvider.java | 60 ++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java index 77cc088f8f1..bebd57e3160 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirestoreClientProvider.java @@ -15,6 +15,7 @@ package com.google.firebase.firestore; import androidx.annotation.GuardedBy; +import androidx.annotation.VisibleForTesting; import androidx.core.util.Consumer; import com.google.android.gms.tasks.Task; import com.google.firebase.firestore.core.FirestoreClient; @@ -23,7 +24,33 @@ import java.util.concurrent.Executor; /** - * The FirestoreClientProvider handles the life cycle of FirestoreClients. + * The `FirestoreClientProvider` handles the life cycle of `FirestoreClient`s within a `Firestore` + * instance. + * + * The instantiation of `FirestoreClient` is delayed until there is a need for the client. This + * delay affords changes to configuration through the `Firestore` instance prior to performing a + * query. After instantiation of the `FirestoreClient`, the `Firestore` instance is considered + * configured, and any subsequent attempt to modify configuration will throw anexception. + * + * Access to `FirestoreClient` is via synchronized indirection to ensure the `FirestoreClient` is + * configured, instantiated and current. The `FirestoreClient` should be considered ephemeral, such + * that no reference to `FirestoreClient` should be retained outside of this provider. + * + * All calls to the `FirestoreClient` should be done through access methods in the + * `FirestoreClientProvider`. Access methods take a functional block of code as a parameter. The + * most current `FirestoreClient` instance will be applied to the functional block of code. + * Execution of the functional block of code will be synchronous to ensure the `FirestoreClient` + * instance remains current during execution. + * + * Retaining a reference to `FirestoreClient` outside of `FirestoreClientProvider` risks calling a + * no longer current `FirestoreClient`. Internally, the `FirestoreClient` may self reference, but + * this is with intent to couple internal logic with a specific `FirestoreClient` instance. + * + * The life of a `FirestoreClient` is tightly coupled to the life the internal `AsyncQueue`. The + * `AsyncQueue` is associated with exactly one `FirestoreClient`, and when that `FirestoreClient` is + * terminated, the `AsyncQueue` is shutdown. Internal coupling within `FirestoreClient` relies on + * `AsyncQueue` to stop processing upon shutdown. A terminated `FirestoreClient` will also rely on + * `AsyncQueue` to safeguard against external access. */ final class FirestoreClientProvider { @@ -40,10 +67,18 @@ final class FirestoreClientProvider { this.asyncQueue = new AsyncQueue(); } + /** + * Indicates whether `FirestoreClient` has been instantiated thereby preventing change to + * configuration. + */ boolean isConfigured() { return client != null; } + /** + * Prevents further change to configuration, and instantiates the `FirestoreClient` instance + * to be ready for use. + */ synchronized void ensureConfigured() { if (!isConfigured()) { client = clientFactory.apply(asyncQueue); @@ -68,6 +103,21 @@ synchronized void procedure(Consumer call) { call.accept(client); } + /** + * Conditional execution based on whether `FirestoreClient` is up and running. + * + * Handling the conditional logic as part of `FirestoreClientProvider` prevents possible race + * condition between condition check and execution functional block of code. + * + * Example, clearing the cache can only be done while `FirestoreClient` is not running. Checking + * whether `FirestoreClient` is running and then performing clearing of cache outside of a + * synchronized code block, risks another thread instantiating `FirestoreClient` after check, but + * before running code to clear cache. + * + * @param callIf Executes if client is shutdown or client hasn't been started yet. + * @param callElse Executes if client is running. + * @return Result of execution. + */ synchronized T executeIfShutdown( Function callIf, Function callElse) { Executor executor = command -> asyncQueue.enqueueAndForgetEvenAfterShutdown(command); @@ -94,6 +144,14 @@ synchronized Task terminate() { return terminate; } + /** + * Direct access to internal AsyncQueue. + * + * The danger of using this method is retaining non-synchronized direct access to AsyncQueue. + * + * @return internal AsyncQueue + */ + @VisibleForTesting AsyncQueue getAsyncQueue() { return asyncQueue; } From abbe802e659c3c704308c33224add73a107057cb Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 10:22:23 -0400 Subject: [PATCH 28/37] Abort targets on terminate --- .../firebase/firestore/FirestoreTest.java | 20 +++++++++++ .../firestore/testutil/EventAccumulator.java | 36 ++++++++++++++++--- .../firebase/firestore/core/EventManager.java | 11 ++++++ .../firestore/core/FirestoreClient.java | 1 + 4 files changed, 64 insertions(+), 4 deletions(-) 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..234b7528509 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 @@ -1269,6 +1269,26 @@ public void testCanStopListeningAfterTerminate() { registration.remove(); } + @Test + public void testQueryListenerThrowsErrorOnTermination() { + FirebaseFirestore instance = testFirestore(); + DocumentReference reference = instance.document("abc/123"); + EventAccumulator eventAccumulator = new EventAccumulator<>(); + ListenerRegistration registration = + reference.addSnapshotListener(eventAccumulator.errorListener()); + eventAccumulator.await(); + + waitFor(instance.terminate()); + + FirebaseFirestoreException error = + assertThrows(FirebaseFirestoreException.class, eventAccumulator::awaitError); + + assertEquals(error.getCode(), Code.ABORTED); + + // This should proceed without error. + registration.remove(); + } + @Test public void testWaitForPendingWritesResolves() { DocumentReference documentReference = testCollection("abc").document("123"); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java index 347a95cea26..3f1a6c1e279 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java @@ -19,6 +19,7 @@ import com.google.firebase.firestore.DocumentSnapshot; import com.google.firebase.firestore.EventListener; +import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.QuerySnapshot; import com.google.firebase.firestore.util.Logger; import java.util.ArrayList; @@ -30,11 +31,11 @@ public class EventAccumulator { private static final int MAX_EVENTS = 10; - private final BlockingQueue events; + private final BlockingQueue events; private boolean rejectAdditionalEvents; public EventAccumulator() { - events = new ArrayBlockingQueue(MAX_EVENTS); + events = new ArrayBlockingQueue<>(MAX_EVENTS); } public EventListener listener() { @@ -43,15 +44,42 @@ public EventListener listener() { hardAssert( !rejectAdditionalEvents, "Received event after `assertNoAdditionalEvents()` was called"); Logger.debug("EventAccumulator", "Received new event: " + value); - events.offer(value); + events.add(value); }; } + public EventListener errorListener() { + return (value, error) -> { + hardAssert( + !rejectAdditionalEvents, "Received event after `assertNoAdditionalEvents()` was called"); + if (error == null) { + Logger.debug("EventAccumulator", "Received new event: " + value); + events.add(value); + } else { + Logger.debug("EventAccumulator", "Received error: " + error); + events.add(error); + } + }; + } + + public FirebaseFirestoreException awaitError() { + try { + return (FirebaseFirestoreException) events.take(); + } catch (Exception e) { + Logger.debug("EventAccumulator", e.toString()); + throw fail("Failed to receive error"); + } + } + public List await(int numEvents) { try { List result = new ArrayList<>(numEvents); for (int i = 0; i < numEvents; ++i) { - result.add(events.take()); + Object event = events.take(); + if (event instanceof FirebaseFirestoreException) { + throw new RuntimeException((FirebaseFirestoreException) event); + } + result.add((T) event); } return result; } catch (InterruptedException e) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java index afb8c66278a..f8819a86781 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/EventManager.java @@ -17,6 +17,7 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import com.google.firebase.firestore.EventListener; +import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.ListenSource; import com.google.firebase.firestore.core.SyncEngine.SyncEngineCallback; import com.google.firebase.firestore.util.Util; @@ -265,4 +266,14 @@ public void handleOnlineStateChange(OnlineState onlineState) { raiseSnapshotsInSyncEvent(); } } + + public void abortAllTargets() { + FirebaseFirestoreException error = Util.exceptionFromStatus(Status.ABORTED); + for (QueryListenersInfo info : queries.values()) { + for (QueryListener listener : info.listeners) { + listener.onError(error); + } + } + queries.clear(); + } } 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 6e2d9b87b84..a1bcd451d22 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 @@ -148,6 +148,7 @@ public Task enableNetwork() { public Task terminate() { authProvider.removeChangeListener(); appCheckProvider.removeChangeListener(); + asyncQueue.enqueueAndForget(() -> eventManager.abortAllTargets()); return asyncQueue.enqueueAndInitiateShutdown( () -> { remoteStore.shutdown(); From fee2df93c499cc28fe5a1105a4e06017e104be51 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 10:39:58 -0400 Subject: [PATCH 29/37] Fix --- .../firebase/firestore/FirestoreTest.java | 21 +++---------------- 1 file changed, 3 insertions(+), 18 deletions(-) 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 234b7528509..95acbe2cf8b 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 @@ -1258,24 +1258,7 @@ public void testCanStopListeningAfterTerminate() { FirebaseFirestore instance = testFirestore(); DocumentReference reference = instance.document("abc/123"); EventAccumulator eventAccumulator = new EventAccumulator<>(); - ListenerRegistration registration = reference.addSnapshotListener(eventAccumulator.listener()); - eventAccumulator.await(); - - waitFor(instance.terminate()); - - // This should proceed without error. - registration.remove(); - // Multiple calls should proceed as an effectively no-op. - registration.remove(); - } - - @Test - public void testQueryListenerThrowsErrorOnTermination() { - FirebaseFirestore instance = testFirestore(); - DocumentReference reference = instance.document("abc/123"); - EventAccumulator eventAccumulator = new EventAccumulator<>(); - ListenerRegistration registration = - reference.addSnapshotListener(eventAccumulator.errorListener()); + ListenerRegistration registration = reference.addSnapshotListener(eventAccumulator.errorListener()); eventAccumulator.await(); waitFor(instance.terminate()); @@ -1287,6 +1270,8 @@ public void testQueryListenerThrowsErrorOnTermination() { // This should proceed without error. registration.remove(); + // Multiple calls should proceed as an effectively no-op. + registration.remove(); } @Test From f3a919d807a5bc10cd65ff2a208b992316854a49 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 10:45:14 -0400 Subject: [PATCH 30/37] Pretty --- .../java/com/google/firebase/firestore/FirestoreTest.java | 3 ++- 1 file changed, 2 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 95acbe2cf8b..347dc27029d 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 @@ -1258,7 +1258,8 @@ public void testCanStopListeningAfterTerminate() { FirebaseFirestore instance = testFirestore(); DocumentReference reference = instance.document("abc/123"); EventAccumulator eventAccumulator = new EventAccumulator<>(); - ListenerRegistration registration = reference.addSnapshotListener(eventAccumulator.errorListener()); + ListenerRegistration registration = + reference.addSnapshotListener(eventAccumulator.errorListener()); eventAccumulator.await(); waitFor(instance.terminate()); From 68cd17b673d1cd94cd200d863ba406da8754376c Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 11:13:55 -0400 Subject: [PATCH 31/37] Text --- .../java/com/google/firebase/firestore/FirebaseFirestore.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java index c1218829b8a..88617cb2052 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java @@ -581,8 +581,8 @@ public Task runBatch(@NonNull WriteBatch.Function batchFunction) { *

To restart after termination, simply create a new instance of {@code FirebaseFirestore} with * {@link #getInstance()} or {@link #getInstance(FirebaseApp)}. * - *

{@code terminate()} does not cancel any pending writes and any tasks that are awaiting a - * response from the server will not be resolved. The next time you start this instance, it will + *

{@code terminate()} does not cancel any pending writes but any write tasks that are awaiting + * a response from the server will not be resolved. The next time you start this instance, it will * resume attempting to send these writes to the server. * *

Note: Under normal circumstances, calling {@code terminate()} is not required. This method From a82dcd17e429d0dfe53b11f9ff3d58406a9f8d08 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 11:23:22 -0400 Subject: [PATCH 32/37] Changelog --- firebase-firestore/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index cf3d3772d64..fa721877ddc 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,5 +1,5 @@ # Unreleased - +* [changed] Fail snapshot listeners when Firestore terminates. [#6136](//github.com/firebase/firebase-android-sdk/pull/6136) # 25.0.0 * [feature] Enable queries with range & inequality filters on multiple fields. [#5729](//github.com/firebase/firebase-android-sdk/pull/5729) From a9f30760297e36bb4a3de4db4209e1ec0da27548 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 15:32:22 -0400 Subject: [PATCH 33/37] Fix --- .../java/com/google/firebase/firestore/QueryTest.java | 4 +++- .../com/google/firebase/firestore/core/FirestoreClient.java | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 29ca658515e..fb1268fa896 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -507,7 +507,7 @@ public void watchSurvivesNetworkDisconnect() { Semaphore receivedDocument = new Semaphore(0); - collectionReference.addSnapshotListener( + ListenerRegistration listener = collectionReference.addSnapshotListener( MetadataChanges.INCLUDE, (snapshot, error) -> { if (!snapshot.isEmpty() && !snapshot.getMetadata().isFromCache()) { @@ -520,6 +520,8 @@ public void watchSurvivesNetworkDisconnect() { waitFor(firestore.enableNetwork()); waitFor(receivedDocument); + + listener.remove(); } @Test 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 a1bcd451d22..7409af54bd9 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 @@ -148,7 +148,7 @@ public Task enableNetwork() { public Task terminate() { authProvider.removeChangeListener(); appCheckProvider.removeChangeListener(); - asyncQueue.enqueueAndForget(() -> eventManager.abortAllTargets()); + asyncQueue.enqueueAndForgetEvenAfterShutdown(() -> eventManager.abortAllTargets()); return asyncQueue.enqueueAndInitiateShutdown( () -> { remoteStore.shutdown(); From dae1f737fb5ea75c401b0adb9afa63236a1bb643 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 15:41:56 -0400 Subject: [PATCH 34/37] Pretty --- .../com/google/firebase/firestore/QueryTest.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index fb1268fa896..684fbc98f26 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -507,13 +507,14 @@ public void watchSurvivesNetworkDisconnect() { Semaphore receivedDocument = new Semaphore(0); - ListenerRegistration listener = collectionReference.addSnapshotListener( - MetadataChanges.INCLUDE, - (snapshot, error) -> { - if (!snapshot.isEmpty() && !snapshot.getMetadata().isFromCache()) { - receivedDocument.release(); - } - }); + ListenerRegistration listener = + collectionReference.addSnapshotListener( + MetadataChanges.INCLUDE, + (snapshot, error) -> { + if (!snapshot.isEmpty() && !snapshot.getMetadata().isFromCache()) { + receivedDocument.release(); + } + }); waitFor(firestore.disableNetwork()); collectionReference.add(map("foo", FieldValue.serverTimestamp())); From c88cf9c22c38634356ec8f9eb9de849e1bf9672c Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 16:17:50 -0400 Subject: [PATCH 35/37] Fix --- .../java/com/google/firebase/firestore/FirestoreTest.java | 5 +---- .../java/com/google/firebase/firestore/QueryTest.java | 7 +++++-- 2 files changed, 6 insertions(+), 6 deletions(-) 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 347dc27029d..53746daff2c 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 @@ -1264,10 +1264,7 @@ public void testCanStopListeningAfterTerminate() { waitFor(instance.terminate()); - FirebaseFirestoreException error = - assertThrows(FirebaseFirestoreException.class, eventAccumulator::awaitError); - - assertEquals(error.getCode(), Code.ABORTED); + assertEquals(eventAccumulator.awaitError().getCode(), Code.ABORTED); // This should proceed without error. registration.remove(); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 684fbc98f26..f15c200ce5e 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -147,7 +147,7 @@ public void testListenUnlistenRelistenSequenceOfMirrorQueries() { // Unlisten then re-listen limit query. limitRegistration.remove(); - limit.addSnapshotListener(limitAccumulator.listener()); + limitRegistration = limit.addSnapshotListener(limitAccumulator.listener()); // Verify `limit` query still works. data = querySnapshotToValues(limitAccumulator.await()); @@ -165,13 +165,16 @@ public void testListenUnlistenRelistenSequenceOfMirrorQueries() { // Unlisten to limitToLast, update a doc, then relisten to limitToLast limitToLastRegistration.remove(); waitFor(collection.document("a").update(map("k", "a", "sort", -2))); - limitToLast.addSnapshotListener(limitToLastAccumulator.listener()); + limitToLastRegistration = limitToLast.addSnapshotListener(limitToLastAccumulator.listener()); // Verify both query get expected result. data = querySnapshotToValues(limitAccumulator.await()); assertEquals(asList(map("k", "a", "sort", -2L), map("k", "e", "sort", -1L)), data); data = querySnapshotToValues(limitToLastAccumulator.await()); assertEquals(asList(map("k", "e", "sort", -1L), map("k", "a", "sort", -2L)), data); + + limitRegistration.remove(); + limitToLastRegistration.remove(); } @Test From 70e285d7a8c8547777679b866c60964bd267caa8 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 16:34:08 -0400 Subject: [PATCH 36/37] Fix --- .../com/google/firebase/firestore/FirestoreTest.java | 3 +-- .../firestore/testutil/EventAccumulator.java | 12 +----------- 2 files changed, 2 insertions(+), 13 deletions(-) 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 53746daff2c..0cbb52eb60f 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 @@ -1258,8 +1258,7 @@ public void testCanStopListeningAfterTerminate() { FirebaseFirestore instance = testFirestore(); DocumentReference reference = instance.document("abc/123"); EventAccumulator eventAccumulator = new EventAccumulator<>(); - ListenerRegistration registration = - reference.addSnapshotListener(eventAccumulator.errorListener()); + ListenerRegistration registration = reference.addSnapshotListener(eventAccumulator.listener()); eventAccumulator.await(); waitFor(instance.terminate()); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java index 3f1a6c1e279..40cb6e5eed4 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/EventAccumulator.java @@ -39,16 +39,6 @@ public EventAccumulator() { } public EventListener listener() { - return (value, error) -> { - hardAssert(error == null, "Unexpected error: %s", error); - hardAssert( - !rejectAdditionalEvents, "Received event after `assertNoAdditionalEvents()` was called"); - Logger.debug("EventAccumulator", "Received new event: " + value); - events.add(value); - }; - } - - public EventListener errorListener() { return (value, error) -> { hardAssert( !rejectAdditionalEvents, "Received event after `assertNoAdditionalEvents()` was called"); @@ -77,7 +67,7 @@ public List await(int numEvents) { for (int i = 0; i < numEvents; ++i) { Object event = events.take(); if (event instanceof FirebaseFirestoreException) { - throw new RuntimeException((FirebaseFirestoreException) event); + fail("Unexpected error: %s", event); } result.add((T) event); } From 26bdaae3d3c94b9df415cdecb86793c8559ae1e3 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 30 Jul 2024 17:01:51 -0400 Subject: [PATCH 37/37] Fix --- .../firestore/ListenerRegistrationTest.java | 15 ++++---- .../google/firebase/firestore/SourceTest.java | 36 ++++++++++--------- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ListenerRegistrationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ListenerRegistrationTest.java index 7ff920888ad..2b8852a2309 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ListenerRegistrationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ListenerRegistrationTest.java @@ -165,12 +165,13 @@ private void activityScopedListenerStopsListeningWhenActivityStops(Activity acti DocumentReference documentReference = collectionReference.document(); Semaphore events = new Semaphore(0); - collectionReference.addSnapshotListener( - activity, - (value, error) -> { - assertNull(error); - events.release(); - }); + ListenerRegistration listener = + collectionReference.addSnapshotListener( + activity, + (value, error) -> { + assertNull(error); + events.release(); + }); // Initial events waitFor(events, 1); @@ -188,6 +189,8 @@ private void activityScopedListenerStopsListeningWhenActivityStops(Activity acti // No listeners, therefore, there should be no events. waitFor(documentReference.set(map("foo", "new-bar"))); assertEquals(0, events.availablePermits()); + + listener.remove(); } /** @param activity Must be a TestActivity or a TestFragmentActivity */ diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/SourceTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/SourceTest.java index fd6ed1652e1..571238885e3 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/SourceTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/SourceTest.java @@ -289,15 +289,17 @@ public void getDocumentWhileOfflineWithDifferentGetOptions() { // Create an initial listener for this query (to attempt to disrupt the gets below) and wait for // the listener to deliver its initial snapshot before continuing. TaskCompletionSource source = new TaskCompletionSource<>(); - docRef.addSnapshotListener( - (docSnap, error) -> { - if (error != null) { - source.setException(error); - } else { - source.setResult(null); - } - }); + ListenerRegistration listener = + docRef.addSnapshotListener( + (docSnap, error) -> { + if (error != null) { + source.setException(error); + } else { + source.setResult(null); + } + }); waitFor(source.getTask()); + listener.remove(); Task docTask = docRef.get(Source.CACHE); waitFor(docTask); @@ -339,15 +341,17 @@ public void getCollectionWhileOfflineWithDifferentGetOptions() { // Create an initial listener for this query (to attempt to disrupt the gets below) and wait for // the listener to deliver its initial snapshot before continuing. TaskCompletionSource source = new TaskCompletionSource<>(); - colRef.addSnapshotListener( - (qrySnap, error) -> { - if (error != null) { - source.setException(error); - } else { - source.setResult(null); - } - }); + ListenerRegistration listener = + colRef.addSnapshotListener( + (qrySnap, error) -> { + if (error != null) { + source.setException(error); + } else { + source.setResult(null); + } + }); waitFor(source.getTask()); + listener.remove(); Task qrySnapTask = colRef.get(Source.CACHE); waitFor(qrySnapTask);