Fix regression for merge with increment #3048
Conversation
schmidt-sebastian
force-pushed
the
mrschmidt/fixmergeincrement
branch
from
e917183
to
48f583c
Compare
Binary Size ReportAffected SDKs
Test Logs
|
| @@ -217,12 +220,12 @@ export class ParseContext { | |||
| childContextForArray(index: number): ParseContext { | |||
| // TODO(b/34871131): We don't support array paths right now; so make path | |||
| // null. | |||
There was a problem hiding this comment.
Should "null" be changed to "undefined" in the comment as well?
There was a problem hiding this comment.
Yes, good catch.
| @@ -138,11 +138,14 @@ interface ContextSettings { | |||
| * A path within the object being parsed. This could be an empty path (in | |||
| * which case the context represents the root of the data being parsed), or a | |||
| * nonempty path (indicating the context represents a nested location within | |||
| * the data). | |||
| * the data). Defaults to null. | |||
There was a problem hiding this comment.
Doesn't it default to "undefined"? Consider just omitting this statement because it is seems like it is just explaining how the "?" property modifier works.
There was a problem hiding this comment.
That's true. I dropped it.
| readonly path?: FieldPath; | ||
| /** | ||
| * Whether or not this context corresponds to an element of an array. | ||
| * Defaults to false. |
There was a problem hiding this comment.
Consider re-wording this as something like "if undefined, it will be treated as false" because the way that it is worded suggests to me that it gets assigned a default value of false somehow, which I do not think is accurate.
There was a problem hiding this comment.
Changed to If not set, elements are treated as if they were outside of arrays.
| readonly path?: FieldPath; | ||
| /** | ||
| * Whether or not this context corresponds to an element of an array. | ||
| * Defaults to false. |
This reverts a behavior change in #3001
Before this PR, the parse contexts we used to parse ArrayTransform and Increment values was detached from the original parse context that was used to parse the update. Thus, setting the field path for the value that was being incremented did not end up modifying the context that was used to generate the Proto. The result of this was
{ updateMask: [] }. By removing the copy, we ended up with{ updateMask: ['sum'] }, which ended up rewriting the value before applying the increment.Fixes #3045