-
Notifications
You must be signed in to change notification settings - Fork 993
Add Numeric Add #1368
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.
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
Add Numeric Add #1368
Changes from 2 commits
03e0063
4f835a4
2a9ce2a
83ddee8
e62de99
06ff0d3
9c4490c
d83d07a
81332de
3e64e2f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,12 @@ | |
| import * as firestore from '@firebase/firestore-types'; | ||
|
|
||
| import { makeConstructorPrivate } from '../util/api'; | ||
| import { validateAtLeastNumberOfArgs } from '../util/input_validation'; | ||
| import { | ||
| validateArgType, | ||
| validateAtLeastNumberOfArgs, | ||
| validateExactNumberOfArgs, | ||
| validateNoArgs | ||
| } from '../util/input_validation'; | ||
| import { AnyJs } from '../util/misc'; | ||
|
|
||
| /** | ||
|
|
@@ -29,10 +34,12 @@ export abstract class FieldValueImpl implements firestore.FieldValue { | |
| protected constructor(readonly _methodName: string) {} | ||
|
|
||
| static delete(): FieldValueImpl { | ||
| validateNoArgs('FieldValue.delete', arguments); | ||
| return DeleteFieldValueImpl.instance; | ||
| } | ||
|
|
||
| static serverTimestamp(): FieldValueImpl { | ||
| validateNoArgs('FieldValue.serverTimestamp', arguments); | ||
| return ServerTimestampFieldValueImpl.instance; | ||
| } | ||
|
|
||
|
|
@@ -50,6 +57,12 @@ export abstract class FieldValueImpl implements firestore.FieldValue { | |
| return new ArrayRemoveFieldValueImpl(elements); | ||
| } | ||
|
|
||
| static numericAdd(n: number): FieldValueImpl { | ||
| validateArgType('FieldValue.numericAdd', 'number', 1, n); | ||
| validateExactNumberOfArgs('FieldValue.numericAdd', arguments, 1); | ||
| return new NumericAddFieldValueImpl(n); | ||
| } | ||
|
|
||
| isEqual(other: FieldValueImpl): boolean { | ||
| return this === other; | ||
| } | ||
|
|
@@ -83,6 +96,12 @@ export class ArrayRemoveFieldValueImpl extends FieldValueImpl { | |
| } | ||
| } | ||
|
|
||
| export class NumericAddFieldValueImpl extends FieldValueImpl { | ||
| constructor(readonly _operand: number) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would do |
||
| super('FieldValue.numericAdd'); | ||
| } | ||
| } | ||
|
|
||
| // Public instance that disallows construction at runtime. This constructor is | ||
| // used when exporting FieldValueImpl on firebase.firestore.FieldValue and will | ||
| // be called FieldValue publicly. Internally we still use FieldValueImpl which | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ import * as firestore from '@firebase/firestore-types'; | |
| import { Timestamp } from '../api/timestamp'; | ||
| import { DatabaseId } from '../core/database_info'; | ||
| import { DocumentKey } from '../model/document_key'; | ||
| import { FieldValue, ObjectValue } from '../model/field_value'; | ||
| import { FieldValue, NumberValue, ObjectValue } from '../model/field_value'; | ||
| import { | ||
| ArrayValue, | ||
| BlobValue, | ||
|
|
@@ -54,6 +54,7 @@ import * as typeUtils from '../util/types'; | |
| import { | ||
| ArrayRemoveTransformOperation, | ||
| ArrayUnionTransformOperation, | ||
| NumericAddTransformOperation, | ||
| ServerTimestampTransform | ||
| } from '../model/transform_operation'; | ||
| import { Blob } from './blob'; | ||
|
|
@@ -66,6 +67,7 @@ import { | |
| ArrayUnionFieldValueImpl, | ||
| DeleteFieldValueImpl, | ||
| FieldValueImpl, | ||
| NumericAddFieldValueImpl, | ||
| ServerTimestampFieldValueImpl | ||
| } from './field_value'; | ||
| import { GeoPoint } from './geo_point'; | ||
|
|
@@ -644,6 +646,15 @@ export class UserDataConverter { | |
| context.fieldTransforms.push( | ||
| new FieldTransform(context.path, arrayRemove) | ||
| ); | ||
| } else if (value instanceof NumericAddFieldValueImpl) { | ||
|
||
| const operand = this.parseQueryValue( | ||
| 'FieldValue.numericAdd', | ||
| value._operand | ||
| ) as NumberValue; | ||
| const numericAdd = new NumericAddTransformOperation(operand); | ||
| context.fieldTransforms.push( | ||
| new FieldTransform(context.path, numericAdd) | ||
| ); | ||
| } else { | ||
| fail('Unknown FieldValue type: ' + value); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,9 +25,9 @@ import { | |
| DocumentMap, | ||
| MaybeDocumentMap | ||
| } from '../model/collections'; | ||
| import { MaybeDocument } from '../model/document'; | ||
| import { Document, MaybeDocument } from '../model/document'; | ||
| import { DocumentKey } from '../model/document_key'; | ||
| import { Mutation } from '../model/mutation'; | ||
| import { Mutation, PatchMutation, Precondition } from '../model/mutation'; | ||
| import { | ||
| BATCHID_UNKNOWN, | ||
| MutationBatch, | ||
|
|
@@ -38,6 +38,7 @@ import { assert } from '../util/assert'; | |
| import * as log from '../util/log'; | ||
| import * as objUtils from '../util/obj'; | ||
|
|
||
| import { ObjectValue } from '../model/field_value'; | ||
| import { LocalDocumentsView } from './local_documents_view'; | ||
| import { LocalViewChanges } from './local_view_changes'; | ||
| import { MutationQueue } from './mutation_queue'; | ||
|
|
@@ -242,24 +243,65 @@ export class LocalStore { | |
| } | ||
| /* Accept locally generated Mutations and commit them to storage. */ | ||
| localWrite(mutations: Mutation[]): Promise<LocalWriteResult> { | ||
| const localWriteTime = Timestamp.now(); | ||
| const keys = mutations.reduce( | ||
| (keys, m) => keys.add(m.key), | ||
| documentKeySet() | ||
| ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/Nice/Oh... Nice/g |
||
|
|
||
| return this.persistence.runTransaction( | ||
| 'Locally write mutations', | ||
| 'readwrite', | ||
| txn => { | ||
| let batch: MutationBatch; | ||
| const localWriteTime = Timestamp.now(); | ||
| return this.mutationQueue | ||
| .addMutationBatch(txn, localWriteTime, mutations) | ||
| .next(promisedBatch => { | ||
| batch = promisedBatch; | ||
| // TODO(koss): This is doing an N^2 update by replaying ALL the | ||
| // mutations on each document (instead of just the ones added) in | ||
| // this batch. | ||
| const keys = batch.keys(); | ||
| return this.localDocuments.getDocuments(txn, keys); | ||
| }) | ||
| .next((changedDocuments: MaybeDocumentMap) => { | ||
| return { batchId: batch.batchId, changes: changedDocuments }; | ||
| // Load and apply all existing mutations. This lets us compute the | ||
| // current base state for all non-idempotent transforms before applying | ||
| // any additional user-provided writes. | ||
| return this.localDocuments | ||
| .getDocuments(txn, keys) | ||
| .next(existingDocs => { | ||
| // For non-idempotent mutations (such as `FieldValue.numericAdd()`), | ||
| // we record the base state in a separate patch mutation. This is | ||
| // later used to guarantee consistent values and prevents flicker | ||
| // even if the backend sends us an update that already includes our | ||
| // transform. | ||
| const baseMutations: Mutation[] = []; | ||
|
|
||
| for (const mutation of mutations) { | ||
| const maybeDoc = existingDocs.get(mutation.key); | ||
| if (!mutation.isIdempotent) { | ||
| // Theoretically, we should only include non-idempotent fields | ||
| // in this field mask as this mask is used to populate the base | ||
| // state for all DocumentTransforms. By including all fields, | ||
| // we incorrectly prevent rebasing of idempotent transforms | ||
| // (such as `arrayUnion()`) when any non-idempotent transforms | ||
| // are present. | ||
| const fieldMask = mutation.fieldMask; | ||
| if (fieldMask) { | ||
| const baseValues = | ||
| maybeDoc instanceof Document | ||
| ? fieldMask.applyTo(maybeDoc.data) | ||
| : ObjectValue.EMPTY; | ||
| // NOTE: The base state should only be applied if there's some | ||
| // existing document to override, so use a Precondition of | ||
| // exists=true | ||
| baseMutations.push( | ||
| new PatchMutation( | ||
| mutation.key, | ||
| baseValues, | ||
| fieldMask, | ||
| Precondition.exists(true) | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return this.mutationQueue | ||
| .addMutationBatch(txn, localWriteTime, baseMutations, mutations) | ||
| .next(batch => { | ||
| const changes = batch.applyToLocalDocumentSet(existingDocs); | ||
| return { batchId: batch.batchId, changes }; | ||
| }); | ||
| }); | ||
| } | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use validateExactNumberOfArgs() to verify we got 1 arg. delete() and serverTimestamp() should probably be verifying no args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I added a
validateNoArgsand used it indelete()andserverTimestamp().