Store the coerced base value in the BaseMutation #1928
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/basevalues
branch
from
f49174e to
630d1eb
Compare
| @@ -605,6 +606,31 @@ export class ObjectValue extends FieldValue { | |||
| return field; | |||
| } | |||
|
|
|||
| /** Recursively extracts the FieldPaths that are set in this ObjectValue. */ | |||
There was a problem hiding this comment.
I feel like the comments here can be elaborated, something like:
Returns a FieldMask built from all FieldPaths starting from this ObjectValue, including paths from nested objects.
There was a problem hiding this comment.
Done (went with your version).
| * applied. Mutations that replace the entire document return 'null'. | ||
| * If applicable, returns the base value to persist with this mutation. If a | ||
| * base value is provided, the mutation is always applied to this base value, | ||
| * even if document has already beenupdated. |
| * If applicable, returns the field mask for this mutation. Fields that are | ||
| * not included in this field mask are not modified when this mutation is | ||
| * applied. Mutations that replace the entire document return 'null'. | ||
| * If applicable, returns the base value to persist with this mutation. If a |
There was a problem hiding this comment.
Replaced with "If this mutation is not idempotent"
There was a problem hiding this comment.
Mostly requesting for comments changes and renames.
I do have a question that does not seem obvious from reading the code around non-idempotent mutations (not particularly related to this change): if two users apply to increment to the same field, one of them has to apply it to the other's result and therefore ignoring the base value for its own mutation, is that a good guess on what the expected behavior is?
ghost
assigned
|
Adding @wilhuff for Owners approval. |
| // The following update includes an update of the nested map "deep", which | ||
| // updates it to contain a single ServerTimestamp. As such, the update is | ||
| // split into two mutations: One that sets "deep" to an empty map and | ||
| // overwrites the previous ServerTimestamp value and a second ransform that |
| @@ -209,6 +209,14 @@ apiDescribe('Server Timestamps', (persistence: boolean) => { | |||
| }); | |||
|
|
|||
| it('can return previous value', () => { | |||
| // The following update includes an update of the nested map "deep", which | |||
There was a problem hiding this comment.
"The following update includes an update" sounds wrong. Maybe you meant "The following test includes an update"?
Also "As such" doesn't work here. I think what you're trying to say is that the implementation of the update call is to split it into two mutations ...
There was a problem hiding this comment.
Removed "as such" and replaced with "This update".
Port of firebase/firebase-android-sdk#564
The PR changes what we store in a BaseMutation. With the changes here, every transform operation computes what it needs to store in the BaseMutation. For Numeric increment, this is either an existing numeric value or the IntegerValue.of(0), replacing all invalid base values at time of creation.