Integration tests and FSTUserDataConverter fixes for array transforms. #1108
Conversation
| @@ -276,6 +276,13 @@ - (void)deleteDocumentRef:(FIRDocumentReference *)ref { | |||
| [self awaitExpectations]; | |||
| } | |||
|
|
|||
| - (void)mergeDocumentRef:(FIRDocumentReference *)ref data:(NSDictionary<NSString *, id> *)data { | |||
| [ref setData:data | |||
| merge:TRUE | |||
There was a problem hiding this comment.
Gah, woops! Good catch.
| } else if ([input isKindOfClass:[FIRFieldValue class]]) { | ||
| // parseSentinelFieldValue may add an FSTFieldTransform, but nothing should be included in | ||
| // the actual parsed data, so we don't add this path to our fieldMask and we return nil. | ||
| [self parseSentinelFieldValue:(FIRFieldValue *)input context:context]; |
There was a problem hiding this comment.
This code is no longer calling [context appendToFieldMaskWithFieldPath:*context.path] which doesn't seem right.
If I update(["a.b": FieldValue.serverTimestamp()]) shouldn't there be a field mask for "a.b"?
There was a problem hiding this comment.
Actually, no... and that's the bug I was fixing. With serverTimestamp() it doesn't actually matter (so the existing code was "wrong" but non-harmful), but if you do:
update(["a": 42, "b": FieldValue.arrayUnion("added")])
We parse that into two mutations:
- PatchMutation with fieldMask a and data {a: 42}
- TransformMutation with [b => arrayUnion("added")]
It's critical that mutation #1 does not include "b" in the fieldMask, else it'll end up clearing the array prior to the TransformMutation adding "added" to it.
And so the code here needs to not appendToFieldMask when a value parses into a transform. (note that FieldValue.delete() is not a transform, so I had to add code below to still add to the FieldMask in that case)
I expanded on the comment to hopefully make it a bit clearer.
There was a problem hiding this comment.
That makes sense. Thanks for explaining.
| @@ -276,6 +276,13 @@ - (void)deleteDocumentRef:(FIRDocumentReference *)ref { | |||
| [self awaitExpectations]; | |||
| } | |||
|
|
|||
| - (void)mergeDocumentRef:(FIRDocumentReference *)ref data:(NSDictionary<NSString *, id> *)data { | |||
| [ref setData:data | |||
| merge:TRUE | |||
There was a problem hiding this comment.
Gah, woops! Good catch.
| } else if ([input isKindOfClass:[FIRFieldValue class]]) { | ||
| // parseSentinelFieldValue may add an FSTFieldTransform, but nothing should be included in | ||
| // the actual parsed data, so we don't add this path to our fieldMask and we return nil. | ||
| [self parseSentinelFieldValue:(FIRFieldValue *)input context:context]; |
There was a problem hiding this comment.
Actually, no... and that's the bug I was fixing. With serverTimestamp() it doesn't actually matter (so the existing code was "wrong" but non-harmful), but if you do:
update(["a": 42, "b": FieldValue.arrayUnion("added")])
We parse that into two mutations:
- PatchMutation with fieldMask a and data {a: 42}
- TransformMutation with [b => arrayUnion("added")]
It's critical that mutation #1 does not include "b" in the fieldMask, else it'll end up clearing the array prior to the TransformMutation adding "added" to it.
And so the code here needs to not appendToFieldMask when a value parses into a transform. (note that FieldValue.delete() is not a transform, so I had to add code below to still add to the FieldMask in that case)
I expanded on the comment to hopefully make it a bit clearer.
| } else if ([input isKindOfClass:[FIRFieldValue class]]) { | ||
| // parseSentinelFieldValue may add an FSTFieldTransform, but nothing should be included in | ||
| // the actual parsed data, so we don't add this path to our fieldMask and we return nil. | ||
| [self parseSentinelFieldValue:(FIRFieldValue *)input context:context]; |
There was a problem hiding this comment.
That makes sense. Thanks for explaining.
|
I'll re-target to master and merge once the prereq PRs land... |
mikelehen
force-pushed
the
mikelehen/array-transforms-tests-and-fixes
branch
from
5dc00b4 to
6e08e8c
Compare
mikelehen
changed the base branch from
mikelehen/array-transforms-serialization
to
master
No description provided.