Mutable Documents #2383
Mutable Documents #2383
Conversation
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (62be3327) is created by Prow via merging commits: 52b1dca 6a81979. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (62be3327) is created by Prow via merging commits: 52b1dca 6a81979. |
There was a problem hiding this comment.
In many ways, this is a replay of firebase/firebase-js-sdk#1886. mikelehen argued there that removing the type distinction between Document and MaybeDocument was harmful and I came to agree. Please review discussion in that PR and let's discuss.
The idea I was proposing w/r/t making mutations operate on a mutable document builder was narrow: within the mutation types, operate on the builder, but then once done applying mutations, convert to the appropriate existing immutable document type afterward. This would significantly limit the blast radius of the change.
|
I have tried to re-introduce the MaybeDocument chain by changing the current document to be a sort of builder for the old types. I am however having a bit of trouble finding a good place to draw the line between where the MutableDocument and the MaybeDocument should be used. The obvious place the LocalDocumentsView. It's input sources would be MutableDocuments (e.g. the RemoteDocumentStore and all mutation logic) and it would return MaybeDocuments. This however falls apart in two places:
The last argument also applies to drawing the boundary at LocalStore - if RemoteStore, LocalStore, LocalDocumentsView and RemoteDocumentCache all use MutableDocuments, then the remaining part of the code that uses Documents would be fairly minimal - and plus, we would have to run a conversion between MutableDocumentMap and MaybeDocumentMap for every read from LocalStore. I am probably missing something here, but these are my thoughts from today. |
|
Ping :) |
| if (doc instanceof Document) { | ||
| result = result.insert(doc.getKey(), (Document) doc); | ||
| Document doc = getDocument(DocumentKey.fromPath(path)); | ||
| if (doc.exists()) { |
There was a problem hiding this comment.
Even though Document's new implementation handles this correctly, this doesn't read as if it works: unknown documents exist, but we just don't necessarily know their contents. Michael and I were discussing calling this definitelyExists to logically exclude the unknown case.
There was a problem hiding this comment.
I renamed all these status getters to reflect the type of the document. This is now isFoundDocument(). There is still some room for improvement, so suggestions welcome.
FWIW, in my opinion unknown documents also definitely exist.
| } | ||
| mutation.applyToLocalView(document, batch.getLocalWriteTime()); | ||
| if (document.isValid()) { | ||
| results = results.insert(key, document); |
There was a problem hiding this comment.
This is now mishandling the case where the base doc was in the results and is deleted by the mutation. If the document is invalid you need to remove its key from the results map.
Query snapshots should never include non-existent documents, but this would allow them to be there.
There was a problem hiding this comment.
I rewrote this to match the old pattern. These documents already get removed by computeDocChanges (which is why the tests didn't break). I added a new test that would break though (doesNotIncludeDocumentsDeletedByMutation).
| documentKey, | ||
| maybeDoc.getKey()); | ||
| } | ||
| public void applyToLocalView(DocumentKey documentKey, Document document) { |
There was a problem hiding this comment.
Given that documents are always non-null and also have their keys, what's the value in having a separate documentKey parameter?
There was a problem hiding this comment.
We can remove it.
| Document document = documentMap.get(key); | ||
| applyToLocalView(key, document); | ||
| if (!document.isValid()) { | ||
| document.asMissingDocument(SnapshotVersion.NONE); |
There was a problem hiding this comment.
I'm having a hard time reasoning about this change here because we're mixing a number of different terms. If this is going to go anywhere, we should standardize the nomenclature in here to make this easier to reason about.
A missing document has type NO_DOCUMENT and a !valid document has type INVALID. What's the point of coercing INVALID documents to NO_DOCUMENTs?
There was a problem hiding this comment.
I don't know the answer to your question - but without this strange coercion the following assert fails:
| value.set(getPatch()); | ||
| value.set(transformResults); | ||
| document | ||
| .asFoundDocument(mutationResult.getVersion(), document.getData()) |
There was a problem hiding this comment.
This API chain reads as if the we're generating a new document from the first. For example, with methods tend to be used in place of setters specifically in immutable APIs. Chaining isn't intrinsically a problem, it's just that the names don't suggest they're mutating in place. Since we're mutating this, it seems like you could just do setCommittedMutations.
as is trickier. What do you think of changing as to setTo, resetAs, or changeTo? I'm not 100% sold on any of those being any good, but as on its own doesn't work well because it's frequently used in methods that generate a view of a backing entity without changing it.
There was a problem hiding this comment.
I mostly picked the API names because they looked nice, but it is true that they may suggest different behavior. I updated all calls to use set as the prefix.
| public void applyToLocalView(Document document, Timestamp localWriteTime) { | ||
| verifyKeyMatches(document); | ||
|
|
||
| if (getPrecondition().isValidFor(document)) { |
There was a problem hiding this comment.
There's a style preference to apply here, but since I've heard Costa argue for it before, I'll repeat it: there's value in handling guard conditions with early returns. This reduces nesting and complexity (go/tott/455).
| if (this.updateTime != null) { | ||
| return (maybeDoc instanceof Document) && maybeDoc.getVersion().equals(this.updateTime); | ||
| return maybeDoc != null && maybeDoc.getVersion().equals(this.updateTime); |
There was a problem hiding this comment.
There's nothing to prevent a non-existing document from entering this method, so it seems like this should be maybeDoc != null && maybeDoc.exists() && .... This seems doubly-so since you're handling the exists case like that below. (NO_DOCUMENT and MISSING_DOCUMENT have versions but don't exist, and couldn't have the precondition apply at the server.)
There was a problem hiding this comment.
Updated to include the isFoundDocument (previously exists()) call. I also made the argument non-nullable.
| List<Value> transformResults = | ||
| serverTransformResults(maybeDoc, mutationResult.getTransformResults()); | ||
| newData = transformObject(newData, transformResults); | ||
| Map<FieldPath, Value> transformResults = |
There was a problem hiding this comment.
Elsewhere in this PR you changed getTransformResults to be non-nullable. That actually seems unrelated to the core of this change, and introduces the risk that it wouldn't be caught in review. Consider decoupling that into a separate change.
Otherwise, the line above needs to be a !isEmpty() test.
There was a problem hiding this comment.
Separated this out as a stand alone PR: #2428
| localValue.set(transformResults); | ||
| document.asFoundDocument(mutationResult.getVersion(), localValue).withCommittedMutations(); | ||
| } else { | ||
| document.asFoundDocument(mutationResult.getVersion(), value.clone()).withCommittedMutations(); |
There was a problem hiding this comment.
You could avoid duplicating most of the arguments to asFoundDocument by following the old form of this method:
ObjectValue newData = value.clone();
if (mutationResult.getTransformResults() != null) {
transformResults = ...;
newData.set(transformResults);
}
return document.asFoundDcument(mutationResult.getVersion(), newData).withCommittedMutations();There was a problem hiding this comment.
Done (but some of this is no longer applicable)
Gil asked that this should not be part of #2383, but I think it still makes sense as an indepedent simplification
| documentKey, | ||
| maybeDoc.getKey()); | ||
| } | ||
| public void applyToLocalView(DocumentKey documentKey, Document document) { |
There was a problem hiding this comment.
We can remove it.
| public void applyToLocalView(Document document, Timestamp localWriteTime) { | ||
| verifyKeyMatches(document); | ||
|
|
||
| if (getPrecondition().isValidFor(document)) { |
| if (doc instanceof Document) { | ||
| result = result.insert(doc.getKey(), (Document) doc); | ||
| Document doc = getDocument(DocumentKey.fromPath(path)); | ||
| if (doc.exists()) { |
There was a problem hiding this comment.
I renamed all these status getters to reflect the type of the document. This is now isFoundDocument(). There is still some room for improvement, so suggestions welcome.
FWIW, in my opinion unknown documents also definitely exist.
| } | ||
| mutation.applyToLocalView(document, batch.getLocalWriteTime()); | ||
| if (document.isValid()) { | ||
| results = results.insert(key, document); |
There was a problem hiding this comment.
I rewrote this to match the old pattern. These documents already get removed by computeDocChanges (which is why the tests didn't break). I added a new test that would break though (doesNotIncludeDocumentsDeletedByMutation).
| Document document = documentMap.get(key); | ||
| applyToLocalView(key, document); | ||
| if (!document.isValid()) { | ||
| document.asMissingDocument(SnapshotVersion.NONE); |
There was a problem hiding this comment.
I don't know the answer to your question - but without this strange coercion the following assert fails:
| value.set(getPatch()); | ||
| value.set(transformResults); | ||
| document | ||
| .asFoundDocument(mutationResult.getVersion(), document.getData()) |
There was a problem hiding this comment.
I mostly picked the API names because they looked nice, but it is true that they may suggest different behavior. I updated all calls to use set as the prefix.
| if (this.updateTime != null) { | ||
| return (maybeDoc instanceof Document) && maybeDoc.getVersion().equals(this.updateTime); | ||
| return maybeDoc != null && maybeDoc.getVersion().equals(this.updateTime); |
There was a problem hiding this comment.
Updated to include the isFoundDocument (previously exists()) call. I also made the argument non-nullable.
| localValue.set(transformResults); | ||
| document.asFoundDocument(mutationResult.getVersion(), localValue).withCommittedMutations(); | ||
| } else { | ||
| document.asFoundDocument(mutationResult.getVersion(), value.clone()).withCommittedMutations(); |
There was a problem hiding this comment.
Done (but some of this is no longer applicable)
schmidt-sebastian
requested review from
dconeybe and
wilhuff
and removed request for
wilhuff
|
I pushed a couple of renamings. This PR now uses setX for all methods that modify the document state. I also renamed Document to MutableDocument. I did experiment with adding a DocumentBuilder or keeping Document separate from MutableDocunent, but it leads to some strange APIs in the RemoteDocumentCache as it would store Documents but return Builders. Let me know what you think. |
firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java
Outdated
Show resolved
Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java
Outdated
Show resolved
Hide resolved
firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return key.hashCode(); |
There was a problem hiding this comment.
I am concerned about using MutableDocument instances as keys in a table or values in a set. I'm sure everything is fine in this PR; however, future changes may accidentally change a MutableDocument that is stored in a set or used as a key and cause undefined behavior of the collection. It's kind of like storing StringBuilder objects in a Set instead of String objects. If this bug ever did happen then it would be challenging to find the root cause because the bug would be quite removed from the problematic mutation of the MutableDocument. What are your thoughts on this risk?
There was a problem hiding this comment.
Uff. The risk is absolutely there, especially as we further modify this code in the coming years. I spend the last couple of days trying to see how we could mitigate this risk without creating additional document copies (that would defeat the purpose of the PR a bit). In the end, I added a Document interface that provides getters but no setters. These Documents are now used in Views. While this does not 100% remove the risk as the documents can still be mutated in other parts of the code, the vast majority of the code base is now shielded from regressions. Let me know what you think.
There was a problem hiding this comment.
This seems like a pretty good compromise. I like it.
| boolean hasCommittedMutations; | ||
|
|
||
| public MutableDocument(DocumentKey key) { | ||
| this.key = key; |
There was a problem hiding this comment.
Optional: Consider calling the private constructor to do this work. IMO it's more readable and explicit.
| currentLevel.put(currentSegment, nextLevel); | ||
| currentLevel = nextLevel; | ||
| } | ||
| public void set(Map<FieldPath, Value> data) { |
There was a problem hiding this comment.
Can you choose a different name for the set() method? Its name implies that it clobbers all existing data but in fact it acts more like a "set each" or "set all" method. So... maybe setAll() is a better name?
There was a problem hiding this comment.
setAll() sounds good to me. It matches the Java Collections add/addAll methods.
| * Applies any overlays from `currentOverlays` that exist at `currentPath` and returns the merged | ||
| * data at `currentPath` (or null if there were no changes). | ||
| * | ||
| * @param currentPath The path at the current nesting level. Can be set toFieldValue.EMPTY_PATH to |
There was a problem hiding this comment.
Typo: toFieldValue.EMPTY_PATH
| * an `UnknownDocument` can be returned. | ||
| * Applies this mutation to the given Document for the purposes of computing a new remote document | ||
| * If the input document doesn't match the expected state (e.g. it is null or outdated), an | ||
| * `UnknownDocument` may be created |
There was a problem hiding this comment.
"may be created" is somewhat misleading, since the MutableDocument is modified in place. Please update.
| * Applies this mutation to the given MaybeDocument for the purposes of computing the new local | ||
| * view of a document. Both the input and returned documents can be null. | ||
| * Applies this mutation to the given Document for the purposes of computing the new local view of | ||
| * a document. Both the input and returned documents can be null. |
There was a problem hiding this comment.
"Both the input and returned documents can be null" is now somewhat incorrect since the method now returns null. Please update.
| @Nullable MaybeDocument maybeDoc, List<Value> serverTransformResults) { | ||
| ArrayList<Value> transformResults = new ArrayList<>(fieldTransforms.size()); | ||
| protected Map<FieldPath, Value> serverTransformResults( | ||
| MutableDocument maybeDoc, List<Value> serverTransformResults) { |
There was a problem hiding this comment.
Please rename maybeDoc. Also on line 193.
| return new Document( | ||
| key(key), version(version), wrapObject(data), Document.DocumentState.SYNCED); | ||
| public static MutableDocument doc(String key, long version, Map<String, Object> data) { | ||
| return new MutableDocument(key(key)).setFoundDocument(version(version), wrapObject(data)); |
There was a problem hiding this comment.
For readability and maintainability, could these overloaded doc() methods simply delegate to a "canonical" method? The code duplication between these methods suggest that they all do something a little different but they appear to do the same thing but some overloaded versions "massage" the input arguments.
There was a problem hiding this comment.
The canonical overload does not yet exist but I created one.
There was a problem hiding this comment.
Did you actually implement the canonical overload of doc()? I don't see it (but I could very easily be using GitHub's reviews incorrectly).
There was a problem hiding this comment.
I did - but in our other TestUtil (https://github.com/firebase/firebase-android-sdk/blob/c61f53c66ffb72197ad14fec9ad0590aad2960aa/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java). I will add it here as well.
| firestore, (Document) doc, /*fromCache=*/ false, /*hasPendingWrites=*/ false); | ||
| } else if (doc instanceof NoDocument) { | ||
| firestore, | ||
| (MutableDocument) doc, |
There was a problem hiding this comment.
This cast to MutableDocument appear to be unnecessary. Please remove it if it is not required.
| boolean hasLocalMutations; | ||
| boolean hasCommittedMutations; | ||
|
|
||
| public MutableDocument(DocumentKey key) { |
There was a problem hiding this comment.
What do you think about making the MutableDocument constructor private and instead providing "factory functions" to create new instances? This idea came to mind after reviewing the rest of this PR and seeing a lot of new MutableDocument(key).setFoundDocument(...) calls, which begs the following question: "in what state would the newly-created MutableDocument have been had setFoundDocument() not been called?".
If these call sites were instead MutableDocument.newFoundDocument(...) then it would be immediately obvious that a new object is being created and will initially be in the "found" state.
There was a problem hiding this comment.
That was part of my initial attempts, but I abandoned it because it requires me to have both the factory methods as well as the methods to transition documents (as those are used in the mutations).
There was a problem hiding this comment.
That is true; however, IMO having both "setters" and "factory methods" is a reasonable tradeoff in the name of readability at the call sites.
| if (maybeDoc instanceof Document) { | ||
| return (Document) maybeDoc; | ||
| } else if (maybeDoc instanceof NoDocument) { | ||
| MutableDocument document = result.getResult(); |
There was a problem hiding this comment.
Is the object returned from result.getResult() potentially nullable (as it was annotated to be in the original code)? If so, you will need to perform explicit null checks before invoking methods like isFoundDocument() and isNoDocument() on it.
There was a problem hiding this comment.
It's not nullable anymore. What used to be null Document is now an invalid document.
| Document newDoc = null; | ||
| MaybeDocument maybeDoc = entry.getValue(); | ||
| MutableDocument oldDoc = oldDocumentSet.getDocument(key); | ||
| MutableDocument newDoc = null; |
There was a problem hiding this comment.
I like how you've changed the logic here to improve readability. The old code initialized newDoc to null, set it to non-null if the entry was a "found" document, then set it back to null if it didn't match the query. Now, it's initialized to null then set to non-null only if the entry is a "found" document that matches the query.
I think this could be improved from a readability perspective even further by not initializing newDoc and assigning it exactly once. Namely, I'm thinking something like this:
MutableDocument newDoc;
if (! entry.getValue().isFoundDocument()) {
newDoc = null;
} else {
hardAssert(...);
if (query.matches(entry.getValue())) {
newDoc = entry.getValue();
} else {
newDoc = null;
}
}
Another idea would be to move the hardAssert to after initializing newDoc, which IMO makes the initialization of newDoc even easier to understand:
MutableDocument newDoc =
entry.getValue().isFoundDocument() && query.matches(entry.getValue())
? entry.getValue()
: null;
if (newDoc != null) {
hardAssert(...);
}
Anyways, this is an optional suggestion to consider.
There was a problem hiding this comment.
I updated this a bit in my commit that introduces the Document interface. It is simpler now, but it does bring back the old pattern of assigning newDoc back to null. Let me know what you think.
I also removed the assert since it just asserts that key is entry.getKey(), which is the assignment in the first line of this block.
| } else { | ||
| throw fail("Unknown document type %s", document.getClass().getCanonicalName()); | ||
| throw fail("Unknown Document %s", document); |
There was a problem hiding this comment.
Consider rewording this error message since the concept of an "unknown document" is already taken, and would have caused the "else if" branch above to be entered had the document been "unknown".
| if (readTime.compareTo(sinceReadTime) <= 0) { | ||
| continue; | ||
| } | ||
| MutableDocument doc = entry.getValue().first; |
There was a problem hiding this comment.
Personally, I prefer the "guard" style from the original code. The new code mixes an "if" statement and a guard, which is arguably more confusing. For readability, could you switch to exclusively use the "guard" style?
i.e.
while (iterator.hasNext()) {
Map.Entry<DocumentKey, Pair<MutableDocument, SnapshotVersion>> entry = iterator.next();
DocumentKey key = entry.getKey();
if (!queryPath.isPrefixOf(key.getPath())) {
break;
}
MutableDocument doc = entry.getValue().first;
if (! doc.isFoundDocument()) {
continue;
}
SnapshotVersion readTime = entry.getValue().second;
if (readTime.compareTo(sinceReadTime) <= 0) {
continue;
}
if (!query.matches(doc)) {
continue;
}
result = result.insert(doc.getKey(), doc.clone());
}| @@ -37,10 +35,10 @@ | |||
| * <p>The cache key is extracted from {@code maybeDocument.key}. If there is already a cache entry | |||
| * for the key, it will be replaced. | |||
| * | |||
| * @param maybeDocument A Document or NoDocument to put in the cache. | |||
| * @param document A Document or NoDocument to put in the cache. | |||
There was a problem hiding this comment.
nit: The mention of NoDocument here is obsolete. Also, Document should not begin with an uppercase letter because it is no longer the name of a class. Also on lines 50 and 58.
There was a problem hiding this comment.
Updated this entire file.
| * @return The cached Document or NoDocument entries indexed by key. If an entry is not cached, | ||
| * the corresponding key will be mapped to a null value. | ||
| * @return The cached Document or NoDocument entries indexed by key. If an entry is not cached, an | ||
| * invalid document is returned. |
There was a problem hiding this comment.
This statement that an "invalid document is returned" is incorrect because the method returns a Map, not a document. The original comment can probably be tweaked, something like "If an entry is not cached, the corresponding key will be mapped to an invalid document."
There was a problem hiding this comment.
Thanks for the suggestion. Done.
| DocumentKey documentKey, MutableDocument document, MutationBatchResult batchResult) { | ||
| hardAssert( | ||
| document.getKey().equals(documentKey), | ||
| "applyToRemoteDocument: key %s doesn't match maybeDoc key %s", |
There was a problem hiding this comment.
nit: fix this outdated mention of "maybeDoc" in this assertion message.
firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java
Outdated
Show resolved
Hide resolved
| boolean hasLocalMutations; | ||
| boolean hasCommittedMutations; | ||
|
|
||
| public MutableDocument(DocumentKey key) { |
There was a problem hiding this comment.
That is true; however, IMO having both "setters" and "factory methods" is a reasonable tradeoff in the name of readability at the call sites.
| } | ||
|
|
||
| /** Changes the document type to FOUND_DOCUMENT and sets the given version and data. */ | ||
| public MutableDocument setFoundDocument(SnapshotVersion version, ObjectValue value) { |
There was a problem hiding this comment.
What about reinitializeAsFoundDocument(), reinitializeAsNoDocument(), etc? Also, IMO "reset" is a good companion to the other "set" methods and conveys that the scope of the changes made are the entire object instead of a particular field in the object.
| return this; | ||
| } | ||
|
|
||
| public MutableDocument setCommittedMutations() { |
There was a problem hiding this comment.
IMO setCommittedMutations() sounds like you are setting the list/collection of committed mutations as opposed to a boolean flag indicating that there are committed mutations. If you rename the enum value to HAS_COMMITTED_MUTATIONS then it will be more internally consistent :) You pick what you prefer and we'll move on. Naming is hard.
| private enum DocumentType { | ||
| /** | ||
| * Represents the initial state of a MutableDocument when only the document key is known. | ||
| * Invalid documents transition to other states as mutations are applied. If a document remain |
There was a problem hiding this comment.
typo: "remain invalids" -> "remains invalid".
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return key.hashCode(); |
There was a problem hiding this comment.
This seems like a pretty good compromise. I like it.
| return new Document( | ||
| key(key), version(version), wrapObject(data), Document.DocumentState.SYNCED); | ||
| public static MutableDocument doc(String key, long version, Map<String, Object> data) { | ||
| return new MutableDocument(key(key)).setFoundDocument(version(version), wrapObject(data)); |
There was a problem hiding this comment.
Did you actually implement the canonical overload of doc()? I don't see it (but I could very easily be using GitHub's reviews incorrectly).
| @@ -74,38 +71,31 @@ IndexManager getIndexManager() { | |||
| /** | |||
| * Returns the the local view of the document identified by {@code key}. | |||
| * | |||
| * @return Local view of the document or null if we don't have any cached state for it. | |||
| * @return Local view of the document or a null if we don't have any cached state for it. | |||
There was a problem hiding this comment.
It does not, it returns an invalid document instead of null. Fixed.
| } | ||
|
|
||
| return document; | ||
| } | ||
|
|
||
| // Returns the view of the given {@code docs} as they would appear after applying all mutations in |
There was a problem hiding this comment.
This javadoc comment for applyLocalMutationsToDocuments() is incorrect since the method returns void: "Returns the view of the given". Please correct.
There was a problem hiding this comment.
Fixed. Thanks for catching this as well.
| @@ -162,13 +154,13 @@ static SnapshotVersion getPostMutationVersion(@Nullable MaybeDocument maybeDoc) | |||
| * result of applying a transform) for use after a mutation containing transforms has been | |||
| * acknowledged by the server. | |||
| * | |||
| * @param maybeDoc The current state of the document after applying all previous mutations. | |||
| * @param mutableDocument The current state of the document after applying all previous mutations. | |||
| * @param serverTransformResults The transform results received by the server. | |||
| * @return The transform results list. | |||
There was a problem hiding this comment.
ultranit: "list" is no longer accurate, since the return type has changed to a map. Please update. Also on line 192.
There was a problem hiding this comment.
Fixed (here and below)
| private enum DocumentType { | ||
| /** | ||
| * Represents the initial state of a MutableDocument when only the document key is known. | ||
| * Invalid documents transition to other states as mutations are applied. If a document remain |
| return new Document( | ||
| key(key), version(version), wrapObject(data), Document.DocumentState.SYNCED); | ||
| public static MutableDocument doc(String key, long version, Map<String, Object> data) { | ||
| return new MutableDocument(key(key)).setFoundDocument(version(version), wrapObject(data)); |
There was a problem hiding this comment.
I did - but in our other TestUtil (https://github.com/firebase/firebase-android-sdk/blob/c61f53c66ffb72197ad14fec9ad0590aad2960aa/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java). I will add it here as well.
| @@ -74,38 +71,31 @@ IndexManager getIndexManager() { | |||
| /** | |||
| * Returns the the local view of the document identified by {@code key}. | |||
| * | |||
| * @return Local view of the document or null if we don't have any cached state for it. | |||
| * @return Local view of the document or a null if we don't have any cached state for it. | |||
There was a problem hiding this comment.
It does not, it returns an invalid document instead of null. Fixed.
| } | ||
|
|
||
| return document; | ||
| } | ||
|
|
||
| // Returns the view of the given {@code docs} as they would appear after applying all mutations in |
There was a problem hiding this comment.
Fixed. Thanks for catching this as well.
| @@ -162,13 +154,13 @@ static SnapshotVersion getPostMutationVersion(@Nullable MaybeDocument maybeDoc) | |||
| * result of applying a transform) for use after a mutation containing transforms has been | |||
| * acknowledged by the server. | |||
| * | |||
| * @param maybeDoc The current state of the document after applying all previous mutations. | |||
| * @param mutableDocument The current state of the document after applying all previous mutations. | |||
| * @param serverTransformResults The transform results received by the server. | |||
| * @return The transform results list. | |||
There was a problem hiding this comment.
Fixed (here and below)
|
@schmidt-sebastian: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
This is yet another big rewrite of the document/mutation logic that aims to provide stronger ownership guarantees for the C++ client.
With this PR, Documents and ObjectValues are now mutable. All mutations directly modify the Document and documents can transition from MaybeDocument to Document, NoDocument and UnknownDocument (and back). I think this is safe because every document lookup starts in the RemoteDocumentCache: In the SQLRemoteDocumentCache we always create new document anyways, and in the MemoryDocumentCache I now clone the documents.
I also merged all existing classes into Document. A Document that is not found in the RemoteDocumentCache is now
INVALID, which allows mutations to transition the document to a valid state at a later point.