Skip to content

Commit 7be699f

Browse files
schmidt-sebastianschmidt-sebastian
authored
Post-Merge cleanup of MutableDocuments (#2468)
1 parent 9f6ade4 commit 7be699f

File tree

21 files changed

+100
-70
lines changed

21 files changed

+100
-70
lines changed

‎firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/testutil/TestUtil.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ public static MutableDocument doc(String key, long version, ObjectValue data) {
7676
}
7777

7878
public static MutableDocument doc(DocumentKey key, long version, ObjectValue data) {
79-
return new MutableDocument(key).convertToFoundDocument(version(version), data);
79+
return MutableDocument.newFoundDocument(key, version(version), data);
8080
}
8181

8282
public static DocumentSet docSet(Comparator<Document> comparator, MutableDocument... documents) {

‎firebase-firestore/src/main/java/com/google/firebase/firestore/bundle/BundleLoader.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,8 @@ public BundleLoader(BundleCallback bundleCallback, BundleMetadata bundleMetadata
7373
documents =
7474
documents.insert(
7575
bundledDocumentMetadata.getKey(),
76-
new MutableDocument(bundledDocumentMetadata.getKey())
77-
.convertToNoDocument(bundledDocumentMetadata.getReadTime()));
76+
MutableDocument.newNoDocument(
77+
bundledDocumentMetadata.getKey(), bundledDocumentMetadata.getReadTime()));
7878
currentDocument = null;
7979
}
8080
} else if (bundleElement instanceof BundleDocument) {

‎firebase-firestore/src/main/java/com/google/firebase/firestore/bundle/BundleSerializer.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -111,9 +111,8 @@ BundleDocument decodeDocument(JSONObject document) throws JSONException {
111111
decodeMapValue(value, document.getJSONObject("fields"));
112112

113113
return new BundleDocument(
114-
new MutableDocument(key)
115-
.convertToFoundDocument(
116-
updateTime, ObjectValue.fromMap(value.getMapValue().getFieldsMap())));
114+
MutableDocument.newFoundDocument(
115+
key, updateTime, ObjectValue.fromMap(value.getMapValue().getFieldsMap())));
117116
}
118117

119118
private ResourcePath decodeName(String name) {

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -407,8 +407,7 @@ public void handleRejectedListen(int targetId, Status error) {
407407
// It's a limbo doc. Create a synthetic event saying it was deleted. This is kind of a hack.
408408
// Ideally, we would have a method in the local store to purge a document. However, it would
409409
// be tricky to keep all of the local store's invariants with another method.
410-
MutableDocument result =
411-
new MutableDocument(limboKey).convertToNoDocument(SnapshotVersion.NONE);
410+
MutableDocument result = MutableDocument.newNoDocument(limboKey, SnapshotVersion.NONE);
412411
Map<DocumentKey, MutableDocument> documentUpdates =
413412
Collections.singletonMap(limboKey, result);
414413
Set<DocumentKey> limboDocuments = Collections.singleton(limboKey);

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollection
206206
MutableDocument document = remoteDocuments.get(key);
207207
if (document == null) {
208208
// Create invalid document to apply mutations on top of
209-
document = new MutableDocument(key);
209+
document = MutableDocument.newInvalidDocument(key);
210210
remoteDocuments = remoteDocuments.insert(key, document);
211211
}
212212
mutation.applyToLocalView(document, batch.getLocalWriteTime());

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,8 @@ private MutableDocument decodeDocument(
101101
DocumentKey key = rpcSerializer.decodeKey(document.getName());
102102
SnapshotVersion version = rpcSerializer.decodeVersion(document.getUpdateTime());
103103
MutableDocument result =
104-
new MutableDocument(key)
105-
.convertToFoundDocument(version, ObjectValue.fromMap(document.getFieldsMap()));
104+
MutableDocument.newFoundDocument(
105+
key, version, ObjectValue.fromMap(document.getFieldsMap()));
106106
return hasCommittedMutations ? result.setHasCommittedMutations() : result;
107107
}
108108

@@ -121,7 +121,7 @@ private MutableDocument decodeNoDocument(
121121
com.google.firebase.firestore.proto.NoDocument proto, boolean hasCommittedMutations) {
122122
DocumentKey key = rpcSerializer.decodeKey(proto.getName());
123123
SnapshotVersion version = rpcSerializer.decodeVersion(proto.getReadTime());
124-
MutableDocument result = new MutableDocument(key).convertToNoDocument(version);
124+
MutableDocument result = MutableDocument.newNoDocument(key, version);
125125
return hasCommittedMutations ? result.setHasCommittedMutations() : result;
126126
}
127127

@@ -140,7 +140,7 @@ private MutableDocument decodeUnknownDocument(
140140
com.google.firebase.firestore.proto.UnknownDocument proto) {
141141
DocumentKey key = rpcSerializer.decodeKey(proto.getName());
142142
SnapshotVersion version = rpcSerializer.decodeVersion(proto.getVersion());
143-
return new MutableDocument(key).convertToUnknownDocument(version);
143+
return MutableDocument.newUnknownDocument(key, version);
144144
}
145145

146146
/** Encodes a MutationBatch model for local storage in the mutation queue. */

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -776,7 +776,7 @@ private void applyWriteToRemoteDocuments(MutationBatchResult batchResult) {
776776
hardAssert(ackVersion != null, "docVersions should contain every doc in the write.");
777777

778778
if (doc.getVersion().compareTo(ackVersion) < 0) {
779-
batch.applyToRemoteDocument(docKey, doc, batchResult);
779+
batch.applyToRemoteDocument(doc, batchResult);
780780
if (doc.isValidDocument()) {
781781
remoteDocuments.add(doc, batchResult.getCommitVersion());
782782
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public void remove(DocumentKey key) {
6060
@Override
6161
public MutableDocument get(DocumentKey key) {
6262
Pair<MutableDocument, SnapshotVersion> entry = docs.get(key);
63-
return entry != null ? entry.first.clone() : new MutableDocument(key);
63+
return entry != null ? entry.first.clone() : MutableDocument.newInvalidDocument(key);
6464
}
6565

6666
@Override

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ public MutableDocument get(DocumentKey documentKey) {
8282
db.query("SELECT contents FROM remote_documents WHERE path = ?")
8383
.binding(path)
8484
.firstValue(row -> decodeMaybeDocument(row.getBlob(0)));
85-
return document != null ? document : new MutableDocument(documentKey);
85+
return document != null ? document : MutableDocument.newInvalidDocument(documentKey);
8686
}
8787

8888
@Override
@@ -96,7 +96,7 @@ public Map<DocumentKey, MutableDocument> getAll(Iterable<DocumentKey> documentKe
9696
for (DocumentKey key : documentKeys) {
9797
// Make sure each key has a corresponding entry, which is null in case the document is not
9898
// found.
99-
results.put(key, new MutableDocument(key));
99+
results.put(key, MutableDocument.newInvalidDocument(key));
100100
}
101101

102102
SQLitePersistence.LongQuery longQuery =

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright 2021 Google LLC
1+
// Copyright 2018 Google LLC
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// you may not use this file except in compliance with the License.

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

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,15 @@
1717
import androidx.annotation.NonNull;
1818
import com.google.firestore.v1.Value;
1919

20+
/**
21+
* Represents a document in Firestore with a key, version, data and whether it has local mutations
22+
* applied to it.
23+
*
24+
* <p>Documents can transition between states via {@link #convertToFoundDocument}, {@link
25+
* #convertToNoDocument} and {@link #convertToUnknownDocument}. If a document does not transition to
26+
* one of these states even after all mutations have been applied, {@link #isValidDocument} returns
27+
* false and the document should be removed from all views.
28+
*/
2029
public final class MutableDocument implements Document, Cloneable {
2130

2231
private enum DocumentType {
@@ -56,8 +65,8 @@ private enum DocumentState {
5665
private ObjectValue value;
5766
private DocumentState documentState;
5867

59-
public MutableDocument(DocumentKey key) {
60-
this(key, DocumentType.INVALID, SnapshotVersion.NONE, new ObjectValue(), DocumentState.SYNCED);
68+
private MutableDocument(DocumentKey key) {
69+
this.key = key;
6170
}
6271

6372
private MutableDocument(
@@ -73,6 +82,39 @@ private MutableDocument(
7382
this.value = value;
7483
}
7584

85+
/**
86+
* Creates a document with no known version or data, but which can serve as base document for
87+
* mutations.
88+
*/
89+
public static MutableDocument newInvalidDocument(DocumentKey documentKey) {
90+
return new MutableDocument(
91+
documentKey,
92+
DocumentType.INVALID,
93+
SnapshotVersion.NONE,
94+
new ObjectValue(),
95+
DocumentState.SYNCED);
96+
}
97+
98+
/** Creates a new document that is known to exist with the given data at the given version. */
99+
public static MutableDocument newFoundDocument(
100+
DocumentKey documentKey, SnapshotVersion version, ObjectValue value) {
101+
return new MutableDocument(documentKey).convertToFoundDocument(version, value);
102+
}
103+
104+
/** Creates a new document that is known to not exisr at the given version. */
105+
public static MutableDocument newNoDocument(DocumentKey documentKey, SnapshotVersion version) {
106+
return new MutableDocument(documentKey).convertToNoDocument(version);
107+
}
108+
109+
/**
110+
* Creates a new document that is known to exist at the given version but whose data is not known
111+
* (e.g. a document that was updated without a known base document).
112+
*/
113+
public static MutableDocument newUnknownDocument(
114+
DocumentKey documentKey, SnapshotVersion version) {
115+
return new MutableDocument(documentKey).convertToUnknownDocument(version);
116+
}
117+
76118
/**
77119
* Changes the document type to indicate that it exists and that its version and data are known.
78120
*/
@@ -94,8 +136,8 @@ public MutableDocument convertToNoDocument(SnapshotVersion version) {
94136
}
95137

96138
/**
97-
* Changes the document type to indicate that it exists at a given version but that is data is not
98-
* known (e.g. a document that was updated without a known base document).
139+
* Changes the document type to indicate that it exists at a given version but that its data is
140+
* not known (e.g. a document that was updated without a known base document).
99141
*/
100142
public MutableDocument convertToUnknownDocument(SnapshotVersion version) {
101143
this.version = version;
@@ -120,29 +162,21 @@ public DocumentKey getKey() {
120162
return key;
121163
}
122164

123-
/**
124-
* Returns the version of this document if it exists or a version at which this document was
125-
* guaranteed to not exist.
126-
*/
165+
@Override
127166
public SnapshotVersion getVersion() {
128167
return version;
129168
}
130169

131-
/** Returns whether local mutations were applied via the mutation queue. */
132170
@Override
133171
public boolean hasLocalMutations() {
134172
return documentState.equals(DocumentState.HAS_LOCAL_MUTATIONS);
135173
}
136174

137-
/** Returns whether mutations were applied based on a write acknowledgment. */
138175
@Override
139176
public boolean hasCommittedMutations() {
140177
return documentState.equals(DocumentState.HAS_COMMITTED_MUTATIONS);
141178
}
142179

143-
/**
144-
* Whether this document has a local mutation applied that has not yet been acknowledged by Watch.
145-
*/
146180
@Override
147181
public boolean hasPendingWrites() {
148182
return hasLocalMutations() || hasCommittedMutations();

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

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -76,18 +76,10 @@ public MutationBatch(
7676
* Applies all the mutations in this MutationBatch to the specified document to create a new
7777
* remote document.
7878
*
79-
* @param documentKey The key of the document to apply mutations to.
8079
* @param document The document to apply mutations to.
8180
* @param batchResult The result of applying the MutationBatch to the backend.
8281
*/
83-
public void applyToRemoteDocument(
84-
DocumentKey documentKey, MutableDocument document, MutationBatchResult batchResult) {
85-
hardAssert(
86-
document.getKey().equals(documentKey),
87-
"applyToRemoteDocument: key %s doesn't match document key %s",
88-
documentKey,
89-
document.getKey());
90-
82+
public void applyToRemoteDocument(MutableDocument document, MutationBatchResult batchResult) {
9183
int size = mutations.size();
9284
List<MutationResult> mutationResults = batchResult.getMutationResults();
9385
hardAssert(
@@ -98,7 +90,7 @@ public void applyToRemoteDocument(
9890

9991
for (int i = 0; i < size; i++) {
10092
Mutation mutation = mutations.get(i);
101-
if (mutation.getKey().equals(documentKey)) {
93+
if (mutation.getKey().equals(document.getKey())) {
10294
MutationResult mutationResult = mutationResults.get(i);
10395
mutation.applyToRemoteDocument(document, mutationResult);
10496
}
@@ -129,7 +121,7 @@ public void applyToLocalView(MutableDocument document) {
129121
public ImmutableSortedMap<DocumentKey, Document> applyToLocalDocumentSet(
130122
ImmutableSortedMap<DocumentKey, Document> documentMap) {
131123
// TODO(mrschmidt): This implementation is O(n^2). If we iterate through the mutations first
132-
// (as done in `applyToLocalView(DocumentKey k, MaybeDoc d)`), we can reduce the complexity to
124+
// (as done in `applyToLocalView(MutableDocument d)`), we can reduce the complexity to
133125
// O(n).
134126
for (DocumentKey key : getKeys()) {
135127
// TODO(mutabledocuments): This method should take a map of MutableDocuments and we should

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ private MutableDocument decodeFoundDocument(BatchGetDocumentsResponse response)
246246
SnapshotVersion version = decodeVersion(response.getFound().getUpdateTime());
247247
hardAssert(
248248
!version.equals(SnapshotVersion.NONE), "Got a document response with no snapshot version");
249-
return new MutableDocument(key).convertToFoundDocument(version, value);
249+
return MutableDocument.newFoundDocument(key, version, value);
250250
}
251251

252252
private MutableDocument decodeMissingDocument(BatchGetDocumentsResponse response) {
@@ -258,7 +258,7 @@ private MutableDocument decodeMissingDocument(BatchGetDocumentsResponse response
258258
hardAssert(
259259
!version.equals(SnapshotVersion.NONE),
260260
"Got a no document response with no snapshot version");
261-
return new MutableDocument(key).convertToNoDocument(version);
261+
return MutableDocument.newNoDocument(key, version);
262262
}
263263

264264
// Mutations
@@ -874,7 +874,7 @@ public WatchChange decodeWatchChange(ListenResponse protoChange) {
874874
hardAssert(
875875
!version.equals(SnapshotVersion.NONE), "Got a document change without an update time");
876876
ObjectValue data = ObjectValue.fromMap(docChange.getDocument().getFieldsMap());
877-
MutableDocument document = new MutableDocument(key).convertToFoundDocument(version, data);
877+
MutableDocument document = MutableDocument.newFoundDocument(key, version, data);
878878
watchChange = new WatchChange.DocumentChange(added, removed, document.getKey(), document);
879879
break;
880880
case DOCUMENT_DELETE:
@@ -883,7 +883,7 @@ public WatchChange decodeWatchChange(ListenResponse protoChange) {
883883
key = decodeKey(docDelete.getDocument());
884884
// Note that version might be unset in which case we use SnapshotVersion.NONE
885885
version = decodeVersion(docDelete.getReadTime());
886-
MutableDocument doc = new MutableDocument(key).convertToNoDocument(version);
886+
MutableDocument doc = MutableDocument.newNoDocument(key, version);
887887
watchChange =
888888
new WatchChange.DocumentChange(Collections.emptyList(), removed, doc.getKey(), doc);
889889
break;

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -189,8 +189,7 @@ public void handleExistenceFilter(ExistenceFilterWatchChange watchChange) {
189189
// deleted document there might be another query that will raise this document as part of
190190
// a snapshot until it is resolved, essentially exposing inconsistency between queries.
191191
DocumentKey key = DocumentKey.fromPath(target.getPath());
192-
MutableDocument result =
193-
new MutableDocument(key).convertToNoDocument(SnapshotVersion.NONE);
192+
MutableDocument result = MutableDocument.newNoDocument(key, SnapshotVersion.NONE);
194193
removeDocumentFromTarget(targetId, key, result);
195194
} else {
196195
hardAssert(
@@ -228,7 +227,7 @@ public RemoteEvent createRemoteEvent(SnapshotVersion snapshotVersion) {
228227
// limboDocumentRefs.
229228
DocumentKey key = DocumentKey.fromPath(targetData.getTarget().getPath());
230229
if (pendingDocumentUpdates.get(key) == null && !targetContainsDocument(targetId, key)) {
231-
MutableDocument result = new MutableDocument(key).convertToNoDocument(snapshotVersion);
230+
MutableDocument result = MutableDocument.newNoDocument(key, snapshotVersion);
232231
removeDocumentFromTarget(targetId, key, result);
233232
}
234233
}

‎firebase-firestore/src/test/java/com/google/firebase/firestore/bundle/BundleSerializerTest.java

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -698,13 +698,13 @@ private void assertDecodesValue(String json, Value proto) throws JSONException {
698698
BundleDocument actualDocument = serializer.decodeDocument(new JSONObject(documentJson));
699699
BundleDocument expectedDocument =
700700
new BundleDocument(
701-
new MutableDocument(DocumentKey.fromName(TEST_DOCUMENT))
702-
.convertToFoundDocument(
703-
new SnapshotVersion(new com.google.firebase.Timestamp(1577836802, 2)),
704-
new ObjectValue(
705-
Value.newBuilder()
706-
.setMapValue(MapValue.newBuilder().putFields("foo", proto))
707-
.build())));
701+
MutableDocument.newFoundDocument(
702+
DocumentKey.fromName(TEST_DOCUMENT),
703+
new SnapshotVersion(new com.google.firebase.Timestamp(1577836802, 2)),
704+
new ObjectValue(
705+
Value.newBuilder()
706+
.setMapValue(MapValue.newBuilder().putFields("foo", proto))
707+
.build())));
708708

709709
assertEquals(expectedDocument, actualDocument);
710710
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -576,7 +576,7 @@ public void testRemoveTargetsThenGC() {
576576
() -> {
577577
SnapshotVersion newVersion = version(3);
578578
MutableDocument doc =
579-
new MutableDocument(middleDocToUpdate).convertToFoundDocument(newVersion, testValue);
579+
MutableDocument.newFoundDocument(middleDocToUpdate, newVersion, testValue);
580580
documentCache.add(doc, newVersion);
581581
updateTargetInTransaction(middleTarget);
582582
});

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,8 +107,7 @@ public void testReadSeveralDocumentsIncludingMissingDocument() {
107107

108108
List<String> keys = new ArrayList<>(Arrays.asList(paths));
109109
keys.add("foo/nonexistent");
110-
written.put(
111-
key("foo/nonexistent"), new MutableDocument(key("foo/nonexistent"))); // Add invalid doc
110+
written.put(key("foo/nonexistent"), MutableDocument.newInvalidDocument(key("foo/nonexistent")));
112111
Map<DocumentKey, MutableDocument> read = getAll(keys);
113112
assertEquals(written, read);
114113
}

‎firebase-firestore/src/test/java/com/google/firebase/firestore/model/DocumentTest.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,7 @@ public class DocumentTest {
3636
@Test
3737
public void testInstantiation() {
3838
MutableDocument document =
39-
new MutableDocument(key("messages/first"))
40-
.convertToFoundDocument(version(1), wrapObject("a", 1));
39+
MutableDocument.newFoundDocument(key("messages/first"), version(1), wrapObject("a", 1));
4140

4241
assertEquals(key("messages/first"), document.getKey());
4342
assertEquals(version(1), document.getVersion());

‎firebase-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java renamed to ‎firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutableObjectValueTest.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,15 @@
2929

3030
@RunWith(RobolectricTestRunner.class)
3131
@Config(manifest = Config.NONE)
32-
public class ObjectValueBuilderTest {
32+
public class MutableObjectValueTest {
3333
private String fooString = "foo";
3434
private Value fooValue = wrap(fooString);
3535
private String barString = "bar";
3636
private Value barValue = wrap(barString);
3737
private Value emptyObject = Value.newBuilder().setMapValue(MapValue.getDefaultInstance()).build();
3838

3939
@Test
40-
public void supportsEmptyobjectValues() {
40+
public void supportsEmptyObjectValues() {
4141
ObjectValue objectValue = new ObjectValue();
4242
assertEquals(new ObjectValue(), objectValue);
4343
}

0 commit comments

Comments
 (0)