From 79e0ab65738509cde499254b5212279acd943e7c Mon Sep 17 00:00:00 2001 From: Brian Chen Date: Fri, 4 Dec 2020 10:20:06 -0800 Subject: [PATCH] resolve comments rd.2 --- .changeset/rich-birds-wink.md | 2 +- .../firestore/src/api/user_data_reader.ts | 1 + packages/firestore/src/model/mutation.ts | 20 +++++++++---------- packages/firestore/src/remote/serializer.ts | 4 ++-- .../test/unit/local/local_serializer.test.ts | 10 +++++----- .../test/unit/model/mutation.test.ts | 8 +++----- 6 files changed, 21 insertions(+), 24 deletions(-) diff --git a/.changeset/rich-birds-wink.md b/.changeset/rich-birds-wink.md index 4490097ef36..68a54590637 100644 --- a/.changeset/rich-birds-wink.md +++ b/.changeset/rich-birds-wink.md @@ -1,4 +1,4 @@ --- '@firebase/firestore': minor --- -A write to a document that contains field transforms is no longer split into separate operations. This reduces the number of writes that the backend performs. \ No newline at end of file +A write to a document that contains FieldValue transforms is no longer split up into two separate operations. This reduces the number of writes the backend performs and allows each WriteBatch to hold 500 writes regardless of how many FieldValue transformations are attached. \ No newline at end of file diff --git a/packages/firestore/src/api/user_data_reader.ts b/packages/firestore/src/api/user_data_reader.ts index 389aa8c9b77..c117d8ad334 100644 --- a/packages/firestore/src/api/user_data_reader.ts +++ b/packages/firestore/src/api/user_data_reader.ts @@ -102,6 +102,7 @@ export class ParsedSetData { export class ParsedUpdateData { constructor( readonly data: ObjectValue, + // The fieldMask does not include document transforms. readonly fieldMask: FieldMask, readonly fieldTransforms: FieldTransform[] ) {} diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts index b40963edc4c..f99b556e72d 100644 --- a/packages/firestore/src/model/mutation.ts +++ b/packages/firestore/src/model/mutation.ts @@ -428,11 +428,12 @@ export function mutationEquals(left: Mutation, right: Mutation): boolean { return false; } + if (!fieldTransformsAreEqual(left.fieldTransforms, right.fieldTransforms)) { + return false; + } + if (left.type === MutationType.Set) { - return ( - (left as SetMutation).value.isEqual((right as SetMutation).value) && - fieldTransformsAreEqual(left.fieldTransforms, right.fieldTransforms) - ); + return (left as SetMutation).value.isEqual((right as SetMutation).value); } if (left.type === MutationType.Patch) { @@ -440,8 +441,7 @@ export function mutationEquals(left: Mutation, right: Mutation): boolean { (left as PatchMutation).data.isEqual((right as PatchMutation).data) && (left as PatchMutation).fieldMask.isEqual( (right as PatchMutation).fieldMask - ) && - fieldTransformsAreEqual(left.fieldTransforms, right.fieldTransforms) + ) ); } @@ -502,7 +502,7 @@ function applySetMutationToRemoteDocument( // remote document the server has accepted the mutation so the precondition // must have held. let newData = mutation.value; - if (mutation.fieldTransforms && mutationResult.transformResults) { + if (mutationResult.transformResults) { const transformResults = serverTransformResults( mutation.fieldTransforms, maybeDoc, @@ -635,7 +635,7 @@ function applyPatchMutationToLocalView( function patchDocument( mutation: PatchMutation, maybeDoc: MaybeDocument | null, - transformResults?: ProtoValue[] + transformResults: ProtoValue[] ): ObjectValue { let data: ObjectValue; if (maybeDoc instanceof Document) { @@ -644,9 +644,7 @@ function patchDocument( data = ObjectValue.empty(); } data = patchObject(mutation, data); - if (transformResults) { - data = transformObject(mutation.fieldTransforms, data, transformResults); - } + data = transformObject(mutation.fieldTransforms, data, transformResults); return data; } diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 6ad672e4c2a..78ad9d5393a 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -634,7 +634,7 @@ export function toMutation( return fail('Unknown mutation type ' + mutation.type); } - if (mutation.fieldTransforms && mutation.fieldTransforms.length > 0) { + if (mutation.fieldTransforms.length > 0) { result.updateTransforms = mutation.fieldTransforms.map(transform => toFieldTransform(serializer, transform) ); @@ -659,7 +659,7 @@ export function fromMutation( ? proto.updateTransforms.map(transform => fromFieldTransform(serializer, transform) ) - : undefined; + : []; if (proto.update) { assertPresent(proto.update.name, 'name'); diff --git a/packages/firestore/test/unit/local/local_serializer.test.ts b/packages/firestore/test/unit/local/local_serializer.test.ts index 8e84077ad67..841a26cf63d 100644 --- a/packages/firestore/test/unit/local/local_serializer.test.ts +++ b/packages/firestore/test/unit/local/local_serializer.test.ts @@ -69,7 +69,7 @@ describe('Local Serializer', () => { currentDocument: { exists: true } }; // Updated transform proto representation. - const updateTransformWrite = { + const updateTransforms = { updateTransforms: [ { fieldPath: 'integer', increment: { integerValue: '42' } }, { fieldPath: 'double', increment: { doubleValue: 13.37 } } @@ -90,7 +90,7 @@ describe('Local Serializer', () => { const serialized = toMutation(s, mutationBatch.mutations[0]); expect(serialized).to.deep.equal({ ...setMutationWrite, - ...updateTransformWrite + ...updateTransforms }); }); @@ -108,7 +108,7 @@ describe('Local Serializer', () => { const serialized = toMutation(s, mutationBatch.mutations[0]); expect(serialized).to.deep.equal({ ...patchMutationWrite, - ...updateTransformWrite + ...updateTransforms }); }); @@ -180,9 +180,9 @@ describe('Local Serializer', () => { ); const expected = [ setMutationWrite, - { ...setMutationWrite, ...updateTransformWrite }, + { ...setMutationWrite, ...updateTransforms }, deleteMutationWrite, - { ...patchMutationWrite, ...updateTransformWrite }, + { ...patchMutationWrite, ...updateTransforms }, patchMutationWrite ]; const mutationBatch = fromDbMutationBatch(localSerializer, dbMutationBatch); diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts index 90605dc56c4..bf861ea665f 100644 --- a/packages/firestore/test/unit/model/mutation.test.ts +++ b/packages/firestore/test/unit/model/mutation.test.ts @@ -247,16 +247,15 @@ describe('Mutation', () => { foo: FieldValue.arrayUnion('tag'), 'bar.baz': FieldValue.arrayUnion(true, { nested: { a: [1, 2] } }) }); - expect(transform.fieldTransforms).to.not.be.undefined; expect(transform.fieldTransforms).to.have.lengthOf(2); - const first = transform.fieldTransforms![0]; + const first = transform.fieldTransforms[0]; expect(first.field).to.deep.equal(field('foo')); expect(first.transform).to.deep.equal( new ArrayUnionTransformOperation([wrap('tag')]) ); - const second = transform.fieldTransforms![1]; + const second = transform.fieldTransforms[1]; expect(second.field).to.deep.equal(field('bar.baz')); expect(second.transform).to.deep.equal( new ArrayUnionTransformOperation([ @@ -273,10 +272,9 @@ describe('Mutation', () => { const transform = patchMutation('collection/key', { foo: FieldValue.arrayRemove('tag') }); - expect(transform.fieldTransforms).to.not.be.undefined; expect(transform.fieldTransforms).to.have.lengthOf(1); - const first = transform.fieldTransforms![0]; + const first = transform.fieldTransforms[0]; expect(first.field).to.deep.equal(field('foo')); expect(first.transform).to.deep.equal( new ArrayRemoveTransformOperation([wrap('tag')])