Skip to content

Commit fb8544c

Browse files
tom-andersentom-andersen
authored
Refactor IndexBackfiller constructor (#3797)
1 parent a283019 commit fb8544c

File tree

10 files changed

+81
-73
lines changed

10 files changed

+81
-73
lines changed

‎firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import com.google.firebase.database.collection.ImmutableSortedSet;
2222
import com.google.firebase.firestore.auth.User;
2323
import com.google.firebase.firestore.core.OnlineState;
24-
import com.google.firebase.firestore.local.IndexBackfiller;
2524
import com.google.firebase.firestore.local.LocalStore;
2625
import com.google.firebase.firestore.local.MemoryPersistence;
2726
import com.google.firebase.firestore.local.Persistence;
@@ -79,9 +78,7 @@ public ImmutableSortedSet<DocumentKey> getRemoteKeysForTarget(int targetId) {
7978
QueryEngine queryEngine = new QueryEngine();
8079
Persistence persistence = MemoryPersistence.createEagerGcMemoryPersistence();
8180
persistence.start();
82-
IndexBackfiller indexBackfiller = new IndexBackfiller(persistence, new AsyncQueue());
83-
LocalStore localStore =
84-
new LocalStore(persistence, indexBackfiller, queryEngine, User.UNAUTHENTICATED);
81+
LocalStore localStore = new LocalStore(persistence, queryEngine, User.UNAUTHENTICATED);
8582
RemoteStore remoteStore =
8683
new RemoteStore(callback, localStore, datastore, testQueue, connectivityMonitor);
8784

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

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

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

17+
import static com.google.firebase.firestore.util.Assert.hardAssertNonNull;
18+
1719
import android.content.Context;
1820
import androidx.annotation.Nullable;
1921
import com.google.firebase.firestore.FirebaseFirestoreSettings;
@@ -101,7 +103,7 @@ Context getContext() {
101103
}
102104

103105
public Persistence getPersistence() {
104-
return persistence;
106+
return hardAssertNonNull(persistence, "persistence not initialized yet");
105107
}
106108

107109
@Nullable
@@ -115,29 +117,37 @@ public IndexBackfiller getIndexBackfiller() {
115117
}
116118

117119
public LocalStore getLocalStore() {
118-
return localStore;
120+
return hardAssertNonNull(localStore, "localStore not initialized yet");
119121
}
120122

121123
public SyncEngine getSyncEngine() {
122-
return syncEngine;
124+
return hardAssertNonNull(syncEngine, "syncEngine not initialized yet");
123125
}
124126

125127
public RemoteStore getRemoteStore() {
126-
return remoteStore;
128+
return hardAssertNonNull(remoteStore, "remoteStore not initialized yet");
127129
}
128130

129131
public EventManager getEventManager() {
130-
return eventManager;
132+
return hardAssertNonNull(eventManager, "eventManager not initialized yet");
131133
}
132134

133135
protected ConnectivityMonitor getConnectivityMonitor() {
134-
return connectivityMonitor;
136+
return hardAssertNonNull(connectivityMonitor, "connectivityMonitor not initialized yet");
135137
}
136138

137139
public void initialize(Configuration configuration) {
140+
/**
141+
* The order in which components are created is important.
142+
*
143+
* <p>The implementation of abstract createX methods (e.g. createRemoteStore) will call the getX
144+
* methods (e.g. getLocalStore). Consequently, creating components out of order will cause
145+
* createX method to fail because a dependency is null.
146+
*
147+
* <p>To catch incorrect order, all getX methods have runtime check for null.
148+
*/
138149
persistence = createPersistence(configuration);
139150
persistence.start();
140-
indexBackfiller = createIndexBackfiller(configuration);
141151
localStore = createLocalStore(configuration);
142152
connectivityMonitor = createConnectivityMonitor(configuration);
143153
remoteStore = createRemoteStore(configuration);
@@ -146,6 +156,7 @@ public void initialize(Configuration configuration) {
146156
localStore.start();
147157
remoteStore.start();
148158
garbageCollectionScheduler = createGarbageCollectionScheduler(configuration);
159+
indexBackfiller = createIndexBackfiller(configuration);
149160
}
150161

151162
protected abstract Scheduler createGarbageCollectionScheduler(Configuration configuration);

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,7 @@ protected EventManager createEventManager(Configuration configuration) {
5454

5555
@Override
5656
protected LocalStore createLocalStore(Configuration configuration) {
57-
return new LocalStore(
58-
getPersistence(), getIndexBackfiller(), new QueryEngine(), configuration.getInitialUser());
57+
return new LocalStore(getPersistence(), new QueryEngine(), configuration.getInitialUser());
5958
}
6059

6160
@Override

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ protected Scheduler createGarbageCollectionScheduler(Configuration configuration
3535

3636
@Override
3737
protected IndexBackfiller createIndexBackfiller(Configuration configuration) {
38-
return new IndexBackfiller(getPersistence(), configuration.getAsyncQueue());
38+
return new IndexBackfiller(getPersistence(), configuration.getAsyncQueue(), getLocalStore());
3939
}
4040

4141
@Override

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

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,9 @@
1414

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

17-
import static com.google.firebase.firestore.util.Assert.hardAssert;
18-
1917
import androidx.annotation.Nullable;
2018
import androidx.annotation.VisibleForTesting;
19+
import com.google.common.base.Supplier;
2120
import com.google.firebase.firestore.model.Document;
2221
import com.google.firebase.firestore.model.DocumentKey;
2322
import com.google.firebase.firestore.model.FieldIndex.IndexOffset;
@@ -41,21 +40,27 @@ public class IndexBackfiller {
4140

4241
private final Scheduler scheduler;
4342
private final Persistence persistence;
44-
private LocalDocumentsView localDocumentsView;
45-
private IndexManager indexManager;
43+
private final Supplier<IndexManager> indexManagerOfCurrentUser;
44+
private final Supplier<LocalDocumentsView> localDocumentsViewOfCurrentUser;
4645
private int maxDocumentsToProcess = MAX_DOCUMENTS_TO_PROCESS;
4746

48-
public IndexBackfiller(Persistence persistence, AsyncQueue asyncQueue) {
49-
this.persistence = persistence;
50-
this.scheduler = new Scheduler(asyncQueue);
47+
public IndexBackfiller(Persistence persistence, AsyncQueue asyncQueue, LocalStore localStore) {
48+
this(
49+
persistence,
50+
asyncQueue,
51+
localStore::getIndexManagerForCurrentUser,
52+
localStore::getLocalDocumentsForCurrentUser);
5153
}
5254

53-
public void setLocalDocumentsView(LocalDocumentsView localDocumentsView) {
54-
this.localDocumentsView = localDocumentsView;
55-
}
56-
57-
public void setIndexManager(IndexManager indexManager) {
58-
this.indexManager = indexManager;
55+
public IndexBackfiller(
56+
Persistence persistence,
57+
AsyncQueue asyncQueue,
58+
Supplier<IndexManager> indexManagerOfCurrentUser,
59+
Supplier<LocalDocumentsView> localDocumentsViewOfCurrentUser) {
60+
this.persistence = persistence;
61+
this.scheduler = new Scheduler(asyncQueue);
62+
this.indexManagerOfCurrentUser = indexManagerOfCurrentUser;
63+
this.localDocumentsViewOfCurrentUser = localDocumentsViewOfCurrentUser;
5964
}
6065

6166
public class Scheduler implements com.google.firebase.firestore.local.Scheduler {
@@ -97,13 +102,12 @@ public Scheduler getScheduler() {
97102

98103
/** Runs a single backfill operation and returns the number of documents processed. */
99104
public int backfill() {
100-
hardAssert(localDocumentsView != null, "setLocalDocumentsView() not called");
101-
hardAssert(indexManager != null, "setIndexManager() not called");
102105
return persistence.runTransaction("Backfill Indexes", () -> this.writeIndexEntries());
103106
}
104107

105108
/** Writes index entries until the cap is reached. Returns the number of documents processed. */
106109
private int writeIndexEntries() {
110+
IndexManager indexManager = indexManagerOfCurrentUser.get();
107111
Set<String> processedCollectionGroups = new HashSet<>();
108112
int documentsRemaining = maxDocumentsToProcess;
109113
while (documentsRemaining > 0) {
@@ -123,6 +127,8 @@ private int writeIndexEntries() {
123127
*/
124128
private int writeEntriesForCollectionGroup(
125129
String collectionGroup, int documentsRemainingUnderCap) {
130+
IndexManager indexManager = indexManagerOfCurrentUser.get();
131+
LocalDocumentsView localDocumentsView = localDocumentsViewOfCurrentUser.get();
126132
// Use the earliest offset of all field indexes to query the local cache.
127133
IndexOffset existingOffset = indexManager.getMinOffset(collectionGroup);
128134

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

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -114,9 +114,6 @@ public final class LocalStore implements BundleCallback {
114114
/** Manages the list of active field and collection indices. */
115115
private IndexManager indexManager;
116116

117-
/** Manages field index backfill. */
118-
private final @Nullable IndexBackfiller indexBackfiller;
119-
120117
/** The set of all mutations that have been sent but not yet been applied to the backend. */
121118
private MutationQueue mutationQueue;
122119

@@ -150,16 +147,11 @@ public final class LocalStore implements BundleCallback {
150147
/** Used to generate targetIds for queries tracked locally. */
151148
private final TargetIdGenerator targetIdGenerator;
152149

153-
public LocalStore(
154-
Persistence persistence,
155-
@Nullable IndexBackfiller indexBackfiller,
156-
QueryEngine queryEngine,
157-
User initialUser) {
150+
public LocalStore(Persistence persistence, QueryEngine queryEngine, User initialUser) {
158151
hardAssert(
159152
persistence.isStarted(), "LocalStore was passed an unstarted persistence implementation");
160153
this.persistence = persistence;
161154
this.queryEngine = queryEngine;
162-
this.indexBackfiller = indexBackfiller;
163155

164156
targetCache = persistence.getTargetCache();
165157
bundleCache = persistence.getBundleCache();
@@ -184,10 +176,6 @@ private void initializeUserComponents(User user) {
184176

185177
remoteDocuments.setIndexManager(indexManager);
186178
queryEngine.initialize(localDocuments, indexManager);
187-
if (indexBackfiller != null) {
188-
indexBackfiller.setIndexManager(indexManager);
189-
indexBackfiller.setLocalDocumentsView(localDocuments);
190-
}
191179
}
192180

193181
public void start() {
@@ -204,6 +192,14 @@ private void startMutationQueue() {
204192
persistence.runTransaction("Start MutationQueue", () -> mutationQueue.start());
205193
}
206194

195+
public IndexManager getIndexManagerForCurrentUser() {
196+
return indexManager;
197+
}
198+
199+
public LocalDocumentsView getLocalDocumentsForCurrentUser() {
200+
return localDocuments;
201+
}
202+
207203
// PORTING NOTE: no shutdown for LocalStore or persistence components on Android.
208204

209205
public ImmutableSortedMap<DocumentKey, Document> handleUserChange(User user) {

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,23 @@ public static void hardAssert(boolean condition, String messageFormat, Object...
3232
}
3333
}
3434

35+
/**
36+
* Triggers a hard assertion. The null check is guaranteed to be checked at runtime. If the object
37+
* is null an AssertionError will be thrown. The string messageFormat will be formatted with the
38+
* provided args using {@link String#format(String, Object...)}.
39+
*
40+
* @param obj The object to be compared with null
41+
* @param messageFormat The message to throw if the condition is false, formatted using {@link
42+
* String#format(String, Object...)}.
43+
* @param args The args to pass to String.format
44+
*/
45+
public static <T> T hardAssertNonNull(T obj, String messageFormat, Object... args) {
46+
if (obj == null) {
47+
throw fail(messageFormat, args);
48+
}
49+
return obj;
50+
}
51+
3552
/**
3653
* Throws an AssertionError with the provided message. The string messageFormat will be formatted
3754
* with the provided args using {@link String#format(String, Object...)}. The method returns an

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,9 @@ public void setUp() {
8383
persistence.getMutationQueue(User.UNAUTHENTICATED, indexManager),
8484
documentOverlayCache,
8585
indexManager);
86-
backfiller = new IndexBackfiller(persistence, new AsyncQueue());
87-
backfiller.setIndexManager(indexManager);
88-
backfiller.setLocalDocumentsView(localDocumentsView);
86+
backfiller =
87+
new IndexBackfiller(
88+
persistence, new AsyncQueue(), () -> indexManager, () -> localDocumentsView);
8989
}
9090

9191
@After

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,10 +125,9 @@ public void setUp() {
125125

126126
localStorePersistence = getPersistence();
127127
queryEngine = new CountingQueryEngine(new QueryEngine());
128-
indexBackfiller = new IndexBackfiller(localStorePersistence, new AsyncQueue());
129-
localStore =
130-
new LocalStore(localStorePersistence, indexBackfiller, queryEngine, User.UNAUTHENTICATED);
128+
localStore = new LocalStore(localStorePersistence, queryEngine, User.UNAUTHENTICATED);
131129
localStore.start();
130+
indexBackfiller = new IndexBackfiller(localStorePersistence, new AsyncQueue(), localStore);
132131
}
133132

134133
@After

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

Lines changed: 8 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import com.google.firebase.firestore.model.Document;
3333
import com.google.firebase.firestore.model.MutableDocument;
3434
import com.google.firebase.firestore.model.mutation.Mutation;
35-
import com.google.firebase.firestore.util.AsyncQueue;
3635
import java.util.Collections;
3736
import java.util.List;
3837
import org.junit.After;
@@ -53,9 +52,7 @@ public void setUp() {
5352
// Setup persistence to version 12, which is before Overlay.
5453
persistence =
5554
PersistenceTestHelpers.createSQLitePersistenceForVersion("test-data-migration", 12);
56-
IndexBackfiller indexBackfiller = new IndexBackfiller(persistence, new AsyncQueue());
57-
localStore =
58-
new LocalStore(persistence, indexBackfiller, new QueryEngine(), User.UNAUTHENTICATED);
55+
localStore = new LocalStore(persistence, new QueryEngine(), User.UNAUTHENTICATED);
5956
localStore.start();
6057
}
6158

@@ -92,9 +89,7 @@ public void testCreateOverlayFromSet() {
9289
// Switch to new persistence and run migrations
9390
this.persistence.shutdown();
9491
persistence = PersistenceTestHelpers.createSQLitePersistence("test-data-migration");
95-
IndexBackfiller indexBackfiller = new IndexBackfiller(persistence, new AsyncQueue());
96-
localStore =
97-
new LocalStore(persistence, indexBackfiller, new QueryEngine(), User.UNAUTHENTICATED);
92+
localStore = new LocalStore(persistence, new QueryEngine(), User.UNAUTHENTICATED);
9893
localStore.start();
9994

10095
assertEquals(
@@ -119,9 +114,7 @@ public void testSkipsIfAlreadyMigrated() {
119114
// Switch to new persistence and run migrations
120115
this.persistence.shutdown();
121116
persistence = PersistenceTestHelpers.createSQLitePersistence("test-data-migration");
122-
IndexBackfiller indexBackfiller = new IndexBackfiller(persistence, new AsyncQueue());
123-
localStore =
124-
new LocalStore(persistence, indexBackfiller, new QueryEngine(), User.UNAUTHENTICATED);
117+
localStore = new LocalStore(persistence, new QueryEngine(), User.UNAUTHENTICATED);
125118
localStore.start();
126119

127120
assertEquals(
@@ -137,9 +130,7 @@ public void testSkipsIfAlreadyMigrated() {
137130
// Switch to new persistence and run migrations which should be a no-op.
138131
this.persistence.shutdown();
139132
persistence = PersistenceTestHelpers.createSQLitePersistence("test-data-migration");
140-
indexBackfiller = new IndexBackfiller(persistence, new AsyncQueue());
141-
localStore =
142-
new LocalStore(persistence, indexBackfiller, new QueryEngine(), User.UNAUTHENTICATED);
133+
localStore = new LocalStore(persistence, new QueryEngine(), User.UNAUTHENTICATED);
143134
localStore.start();
144135

145136
SQLiteOverlayMigrationManager migrationManager =
@@ -159,9 +150,7 @@ public void testCreateOverlayFromDelete() {
159150
// Switch to new persistence and run migrations
160151
this.persistence.shutdown();
161152
persistence = PersistenceTestHelpers.createSQLitePersistence("test-data-migration");
162-
IndexBackfiller indexBackfiller = new IndexBackfiller(persistence, new AsyncQueue());
163-
localStore =
164-
new LocalStore(persistence, indexBackfiller, new QueryEngine(), User.UNAUTHENTICATED);
153+
localStore = new LocalStore(persistence, new QueryEngine(), User.UNAUTHENTICATED);
165154
localStore.start();
166155

167156
assertEquals(
@@ -191,9 +180,7 @@ public void testCreateOverlayFromPatches() {
191180
// Switch to new persistence and run migrations
192181
this.persistence.shutdown();
193182
persistence = PersistenceTestHelpers.createSQLitePersistence("test-data-migration");
194-
IndexBackfiller indexBackfiller = new IndexBackfiller(persistence, new AsyncQueue());
195-
localStore =
196-
new LocalStore(persistence, indexBackfiller, new QueryEngine(), User.UNAUTHENTICATED);
183+
localStore = new LocalStore(persistence, new QueryEngine(), User.UNAUTHENTICATED);
197184
localStore.start();
198185

199186
DocumentOverlayCache overlay = persistence.getDocumentOverlayCache(User.UNAUTHENTICATED);
@@ -218,18 +205,14 @@ public void testCreateOverlayForDifferentUsers() {
218205
writeMutation(setMutation("foo/bar", map("foo", "set-by-unauthenticated")));
219206

220207
// Switch to a different user
221-
IndexBackfiller indexBackfiller = new IndexBackfiller(persistence, new AsyncQueue());
222-
localStore =
223-
new LocalStore(persistence, indexBackfiller, new QueryEngine(), new User("another_user"));
208+
localStore = new LocalStore(persistence, new QueryEngine(), new User("another_user"));
224209
localStore.start();
225210
writeMutation(setMutation("foo/bar", map("foo", "set-by-another_user")));
226211

227212
// Switch to new persistence and run migrations
228213
this.persistence.shutdown();
229214
persistence = PersistenceTestHelpers.createSQLitePersistence("test-data-migration");
230-
indexBackfiller = new IndexBackfiller(persistence, new AsyncQueue());
231-
localStore =
232-
new LocalStore(persistence, indexBackfiller, new QueryEngine(), User.UNAUTHENTICATED);
215+
localStore = new LocalStore(persistence, new QueryEngine(), User.UNAUTHENTICATED);
233216
localStore.start();
234217

235218
assertEquals(

0 commit comments

Comments
 (0)