Skip to content

Backport Android updateTransform changes #4225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 7, 2021
Merged

Backport Android updateTransform changes #4225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9ce95c7
Backport Android updateTransform changes
Dec 18, 2020
ac590f7
Update packages/firestore/src/model/mutation.ts
Dec 21, 2020
95974e2
remove outdated test
Dec 21, 2020
8321655
lint: remove unused function
Dec 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 27 additions & 49 deletions packages/firestore/src/model/mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,11 @@ export class MutationResult {
*/
readonly version: SnapshotVersion,
/**
* The resulting fields returned from the backend after a
* TransformMutation has been committed. Contains one FieldValue for each
* FieldTransform that was in the mutation.
* The resulting fields returned from the backend after a mutation
* containing field transforms has been committed. Contains one FieldValue
* for each FieldTransform that was in the mutation.
*
* Will be null if the mutation was not a TransformMutation.
* Will be null if the mutation did not contain any field transforms.
*/
readonly transformResults: Array<ProtoValue | null> | null
) {}
Expand Down Expand Up @@ -178,8 +178,8 @@ export function preconditionIsValidForDocument(
* Mutations not only act on the value of the document but also its version.
*
* For local mutations (mutations that haven't been committed yet), we preserve
* the existing version for Set, Patch, and Transform mutations. For Delete
* mutations, we reset the version to 0.
* the existing version for Set and Patch mutations. For Delete mutations, we
* reset the version to 0.
*
* Here's the expected transition table.
*
Expand All @@ -191,28 +191,22 @@ export function preconditionIsValidForDocument(
* PatchMutation Document(v3) Document(v3)
* PatchMutation NoDocument(v3) NoDocument(v3)
* PatchMutation null null
* TransformMutation Document(v3) Document(v3)
* TransformMutation NoDocument(v3) NoDocument(v3)
* TransformMutation null null
* DeleteMutation Document(v3) NoDocument(v0)
* DeleteMutation NoDocument(v3) NoDocument(v0)
* DeleteMutation null NoDocument(v0)
*
* For acknowledged mutations, we use the updateTime of the WriteResponse as
* the resulting version for Set, Patch, and Transform mutations. As deletes
* have no explicit update time, we use the commitTime of the WriteResponse for
* the resulting version for Set and Patch mutations. As deletes have no
* explicit update time, we use the commitTime of the WriteResponse for
* Delete mutations.
*
* If a mutation is acknowledged by the backend but fails the precondition check
* locally, we return an `UnknownDocument` and rely on Watch to send us the
* updated version.
*
* Note that TransformMutations don't create Documents (in the case of being
* applied to a NoDocument), even though they would on the backend. This is
* because the client always combines the TransformMutation with a SetMutation
* or PatchMutation and we only want to apply the transform if the prior
* mutation resulted in a Document (always true for a SetMutation, but not
* necessarily for a PatchMutation).
* Field transforms are used only with Patch and Set Mutations. We use the
* `updateTransforms` message to store transforms, rather than the `transforms`s
* messages.
*
* ## Subclassing Notes
*
Expand Down Expand Up @@ -335,13 +329,7 @@ export function extractMutationBaseValue(
mutation: Mutation,
maybeDoc: MaybeDocument | null
): ObjectValue | null {
if (mutation.fieldTransforms !== undefined) {
return extractTransformMutationBaseValue(
mutation.fieldTransforms,
maybeDoc
);
}
return null;
return extractTransformMutationBaseValue(mutation.fieldTransforms, maybeDoc);
}

function extractTransformMutationBaseValue(
Expand Down Expand Up @@ -489,19 +477,17 @@ function applySetMutationToLocalView(
}

let newData = mutation.value;
if (mutation.fieldTransforms) {
const transformResults = localTransformResults(
mutation.fieldTransforms,
localWriteTime,
maybeDoc,
baseDoc
);
newData = transformObject(
mutation.fieldTransforms,
newData,
transformResults
);
}
const transformResults = localTransformResults(
mutation.fieldTransforms,
localWriteTime,
maybeDoc,
baseDoc
);
newData = transformObject(
mutation.fieldTransforms,
newData,
transformResults
);

const version = getPostMutationVersion(maybeDoc);
return new Document(mutation.key, version, newData, {
Expand Down Expand Up @@ -623,8 +609,8 @@ function patchObject(mutation: PatchMutation, data: ObjectValue): ObjectValue {

/**
* Creates a list of "transform results" (a transform result is a field value
* representing the result of applying a transform) for use after a
* TransformMutation has been acknowledged by the server.
* representing the result of applying a transform) for use after a mutation
* containing transforms has been acknowledged by the server.
*
* @param fieldTransforms - The field transforms to apply the result to.
* @param baseDoc - The document prior to applying this mutation batch.
Expand Down Expand Up @@ -664,10 +650,10 @@ function serverTransformResults(
/**
* Creates a list of "transform results" (a transform result is a field value
* representing the result of applying a transform) for use when applying a
* TransformMutation locally.
* transform locally.
*
* @param fieldTransforms - The field transforms to apply the result to.
* @param localWriteTime - The local time of the transform mutation (used to
* @param localWriteTime - The local time of the mutation (used to
* generate ServerTimestampValues).
* @param maybeDoc - The current state of the document after applying all
* previous mutations.
Expand All @@ -689,14 +675,6 @@ function localTransformResults(
previousValue = maybeDoc.field(fieldTransform.field);
}

if (previousValue === null && baseDoc instanceof Document) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this logic no longer needed? This seems pretty critical. Or is it handled elsewhere now?

Copy link
Author

@thebrianchen thebrianchen Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this if-statement checks for the edge case where a PatchMutation clears the values of a nested map before the TransformMutation is applied. With the addition of updateTransforms, field transformation are encoded directly on the Patch or Set Mutation, rather than in a separate TransformMutation. This means that we won't bump into this edge case anymore since the update and fieldTransform are on the same mutation and thus applied at the same time.

I've also removed the now obsolete server timestamp test that tested this edge case.

// If the current document does not contain a value for the mutated
// field, use the value that existed before applying this mutation
// batch. This solves an edge case where a PatchMutation clears the
// values in a nested map before the TransformMutation is applied.
previousValue = baseDoc.field(fieldTransform.field);
}

transformResults.push(
applyTransformOperationToLocalView(
transform,
Expand Down
5 changes: 2 additions & 3 deletions packages/firestore/src/model/server_timestamps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,8 @@ import { normalizeTimestamp } from './normalize';
*
* Notes:
* - ServerTimestampValue instances are created as the result of applying a
* TransformMutation (see TransformMutation.applyTo()). They can only exist in
* the local view of a document. Therefore they do not need to be parsed or
* serialized.
* transform. They can only exist in the local view of a document. Therefore
* they do not need to be parsed or serialized.
* - When evaluated locally (e.g. for snapshot.data()), they by default
* evaluate to `null`. This behavior can be configured by passing custom
* FieldValueOptions to value().
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/src/model/transform_operation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { normalizeNumber } from './normalize';
import { serverTimestamp } from './server_timestamps';
import { isArray, isInteger, isNumber, valueEquals } from './values';

/** Represents a transform within a TransformMutation. */
/** Used to represent a field transform on a mutation. */
export class TransformOperation {
// Make sure that the structural type of `TransformOperation` is unique.
// See https://github.com/microsoft/TypeScript/issues/5451
Expand Down
47 changes: 0 additions & 47 deletions packages/firestore/test/integration/api/server_timestamp.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,25 +104,6 @@ apiDescribe('Server Timestamps', (persistence: boolean) => {
);
}

/**
* Verifies a snapshot containing setData but using the previous field value
* for the timestamps.
*/
function verifyTimestampsUsePreviousValue(
current: firestore.DocumentSnapshot,
prev: firestore.DocumentSnapshot | null
): void {
if (!prev) {
verifyTimestampsAreNull(current);
} else {
expect(current.exists).to.equal(true);
const when = prev.get('when');
expect(current.data({ serverTimestamps: 'previous' })).to.deep.equal(
expectedDataWithTimestamp(when)
);
}
}

/**
* Wraps a test, getting a docRef and event accumulator, and cleaning them
* up when done.
Expand Down Expand Up @@ -207,34 +188,6 @@ apiDescribe('Server Timestamps', (persistence: boolean) => {
});
});

it('can return previous value', () => {
// The following test includes an update of the nested map "deep", which
// updates it to contain a single ServerTimestamp. This update is split
// into two mutations: One that sets "deep" to an empty map and overwrites
// the previous ServerTimestamp value and a second transform that writes
// the new ServerTimestamp. This step in the test verifies that we can
// still access the old ServerTimestamp value (from `previousSnapshot`) even
// though it was removed in an intermediate step.

let previousSnapshot: firestore.DocumentSnapshot;

return withTestSetup(() => {
return writeInitialData()
.then(() => docRef.update(updateData))
.then(() => accumulator.awaitLocalEvent())
.then(snapshot => verifyTimestampsUsePreviousValue(snapshot, null))
.then(() => accumulator.awaitRemoteEvent())
.then(snapshot => {
previousSnapshot = snapshot;
})
.then(() => docRef.update(updateData))
.then(() => accumulator.awaitLocalEvent())
.then(snapshot =>
verifyTimestampsUsePreviousValue(snapshot, previousSnapshot)
);
});
});

it('can return previous value of different type', () => {
return withTestSetup(() => {
return writeInitialData()
Expand Down