From ca64d8319f412fc865eaef2deec7e5e2a8c5333c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 19 Jan 2024 12:46:37 -0500 Subject: [PATCH 01/10] write-batch.ts: allow specifying 'exists' precondition to update --- dev/src/write-batch.ts | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/dev/src/write-batch.ts b/dev/src/write-batch.ts index 33536a275..5a967eddb 100644 --- a/dev/src/write-batch.ts +++ b/dev/src/write-batch.ts @@ -660,13 +660,8 @@ export class WriteBatch implements firestore.WriteBatch { * @internal * @param arg The argument name or argument index (for varargs methods). * @param value The object to validate - * @param allowExists Whether to allow the 'exists' preconditions. */ -function validatePrecondition( - arg: string | number, - value: unknown, - allowExists: boolean -): void { +function validatePrecondition(arg: string | number, value: unknown): void { if (typeof value !== 'object' || value === null) { throw new Error('Input is not an object.'); } @@ -677,14 +672,6 @@ function validatePrecondition( if (precondition.exists !== undefined) { ++conditions; - if (!allowExists) { - throw new Error( - `${invalidArgumentMessage( - arg, - 'precondition' - )} "exists" is not an allowed precondition.` - ); - } if (typeof precondition.exists !== 'boolean') { throw new Error( `${invalidArgumentMessage( @@ -733,7 +720,7 @@ function validateUpdatePrecondition( options?: RequiredArgumentOptions ): asserts value is {lastUpdateTime?: Timestamp} { if (!validateOptional(value, options)) { - validatePrecondition(arg, value, /* allowExists= */ false); + validatePrecondition(arg, value); } } @@ -753,7 +740,7 @@ function validateDeletePrecondition( options?: RequiredArgumentOptions ): void { if (!validateOptional(value, options)) { - validatePrecondition(arg, value, /* allowExists= */ true); + validatePrecondition(arg, value); } } From 358d1e2501f7a1324d9aa09658fdc69562d5b868 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 19 Jan 2024 12:53:46 -0500 Subject: [PATCH 02/10] empty commit to make conventional-commits bot happy From f8481d9dc9ef821ff4325ee00220440d82c66ff4 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 19 Jan 2024 13:17:26 -0500 Subject: [PATCH 03/10] fix unit tests --- dev/test/document.ts | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/dev/test/document.ts b/dev/test/document.ts index f37bf0050..a8c9ffde6 100644 --- a/dev/test/document.ts +++ b/dev/test/document.ts @@ -1629,6 +1629,27 @@ describe('update document', () => { }); }); + it('allows explicitly specifying {exists:true} precondition', () => { + const overrides: ApiOverride = { + commit: request => { + requestEquals( + request, + update({ + document: document('documentId', 'foo', 'bar'), + mask: updateMask('foo'), + }) + ); + return response(writeResult(1)); + }, + }; + + return createInstance(overrides).then(firestore => { + return firestore + .doc('collectionId/documentId') + .update('foo', 'bar', {exists: true}); + }); + }); + it('returns update time', () => { const overrides: ApiOverride = { commit: request => { @@ -2067,12 +2088,6 @@ describe('update document', () => { }); it("doesn't accept argument after precondition", () => { - expect(() => { - firestore.doc('collectionId/documentId').update('foo', 'bar', { - exists: true, - }); - }).to.throw(INVALID_ARGUMENTS_TO_UPDATE); - expect(() => { firestore .doc('collectionId/documentId') From c497258a0e72e01e61680533e15b739c73acc6dd Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 19 Jan 2024 14:33:09 -0500 Subject: [PATCH 04/10] test: update conformance tests to https://github.com/googleapis/conformance-tests/commit/3264aa6a984960f2815788cab535152cb22d72d2 --- .../query-cursor-startafter-docsnap.json | 59 +++++++++++++++++++ .../query-invalid-operator.json | 4 +- .../set-arrayunion-merge.json | 48 +++++++++++++++ 3 files changed, 109 insertions(+), 2 deletions(-) create mode 100644 dev/conformance/conformance-tests/query-cursor-startafter-docsnap.json create mode 100644 dev/conformance/conformance-tests/set-arrayunion-merge.json diff --git a/dev/conformance/conformance-tests/query-cursor-startafter-docsnap.json b/dev/conformance/conformance-tests/query-cursor-startafter-docsnap.json new file mode 100644 index 000000000..9cc49e3a2 --- /dev/null +++ b/dev/conformance/conformance-tests/query-cursor-startafter-docsnap.json @@ -0,0 +1,59 @@ +{ + "tests": [ + { + "description": "query: StartAfter with document snapshot", + "comment": "Cursor methods are allowed to use document snapshots with start_after. It should result in the document's data in the query.", + "query": { + "collPath": "projects/projectID/databases/(default)/documents/C", + "clauses": [ + { + "orderBy": { + "path": { + "field": [ + "a" + ] + }, + "direction": "asc" + } + }, + { + "startAt": { + "jsonValues": [ + "{\"a\": \"b\"}" + ] + } + } + ], + "query": { + "from": [ + { + "collectionId": "C" + } + ], + "orderBy": [ + { + "field": { + "fieldPath": "a" + }, + "direction": "ASCENDING" + } + ], + "startAt": { + "values": [ + { + "mapValue": { + "fields": { + "a": { + "stringValue": "b" + } + } + } + } + ], + "before": true + } + } + } + } + ] +} diff --git a/dev/conformance/conformance-tests/query-invalid-operator.json b/dev/conformance/conformance-tests/query-invalid-operator.json index 70f28424d..0acfeae67 100644 --- a/dev/conformance/conformance-tests/query-invalid-operator.json +++ b/dev/conformance/conformance-tests/query-invalid-operator.json @@ -2,7 +2,7 @@ "tests": [ { "description": "query: invalid operator in Where clause", - "comment": "The ! operator is not supported.", + "comment": "The |~| operator is not supported.", "query": { "collPath": "projects/projectID/databases/(default)/documents/C", "clauses": [ @@ -13,7 +13,7 @@ "a" ] }, - "op": "!", + "op": "|~|", "jsonValue": "4" } } diff --git a/dev/conformance/conformance-tests/set-arrayunion-merge.json b/dev/conformance/conformance-tests/set-arrayunion-merge.json new file mode 100644 index 000000000..46c2fbfb3 --- /dev/null +++ b/dev/conformance/conformance-tests/set-arrayunion-merge.json @@ -0,0 +1,48 @@ +{ + "tests": [ + { + "description": "set: merge ArrayUnion field", + "comment": "An ArrayUnion value can occur at any depth. In this case,\nthe transform applies to the field path \"b.c\". \"a\" is left alone and remains in the object.", + "set": { + "option": { + "all": true + }, + "docRefPath": "projects/projectID/databases/(default)/documents/C/d", + "jsonData": "{\"a\": 1, \"b\": {\"c\": [\"ArrayUnion\", \"foo\", \"bar\"]}}", + "request": { + "database": "projects/projectID/databases/(default)", + "writes": [ + { + "update": { + "name": "projects/projectID/databases/(default)/documents/C/d", + "fields": { + "a": { + "integerValue": "1" + } + } + }, + "updateMask": { + "fieldPaths": ["a"] + }, + "updateTransforms": [ + { + "fieldPath": "b.c", + "appendMissingElements": { + "values": [ + { + "stringValue": "foo" + }, + { + "stringValue": "bar" + } + ] + } + } + ] + } + ] + } + } + } + ] +} From 1f73d2be21d76bfc52fc11b02334fd68d55abdb3 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 19 Jan 2024 14:36:39 -0500 Subject: [PATCH 05/10] update conformance tests from https://github.com/googleapis/conformance-tests/pull/82 --- .../update-exists-precond.json | 28 +++++++++++++++++-- .../update-paths-exists-precond.json | 28 +++++++++++++++++-- 2 files changed, 50 insertions(+), 6 deletions(-) diff --git a/dev/conformance/conformance-tests/update-exists-precond.json b/dev/conformance/conformance-tests/update-exists-precond.json index bdbe274b4..66e5d1124 100644 --- a/dev/conformance/conformance-tests/update-exists-precond.json +++ b/dev/conformance/conformance-tests/update-exists-precond.json @@ -1,15 +1,37 @@ { "tests": [ { - "description": "update: Exists precondition is invalid", - "comment": "The Update method does not support an explicit exists precondition.", + "description": "update: Exists precondition is valid", + "comment": "The Update method supports an explicit exists precondition.", "update": { "docRefPath": "projects/projectID/databases/(default)/documents/C/d", "precondition": { "exists": true }, "jsonData": "{\"a\": 1}", - "isError": true + "request": { + "database": "projects/projectID/databases/(default)", + "writes": [ + { + "update": { + "name": "projects/projectID/databases/(default)/documents/C/d", + "fields": { + "a": { + "integerValue": "1" + } + } + }, + "updateMask": { + "fieldPaths": [ + "a" + ] + }, + "currentDocument": { + "exists": true + } + } + ] + } } } ] diff --git a/dev/conformance/conformance-tests/update-paths-exists-precond.json b/dev/conformance/conformance-tests/update-paths-exists-precond.json index d495db033..6325c7546 100644 --- a/dev/conformance/conformance-tests/update-paths-exists-precond.json +++ b/dev/conformance/conformance-tests/update-paths-exists-precond.json @@ -1,8 +1,8 @@ { "tests": [ { - "description": "update-paths: Exists precondition is invalid", - "comment": "The Update method does not support an explicit exists precondition.", + "description": "update-paths: Exists precondition is valid", + "comment": "The Update method supports an explicit exists precondition.", "updatePaths": { "docRefPath": "projects/projectID/databases/(default)/documents/C/d", "precondition": { @@ -18,7 +18,29 @@ "jsonValues": [ "1" ], - "isError": true + "request": { + "database": "projects/projectID/databases/(default)", + "writes": [ + { + "update": { + "name": "projects/projectID/databases/(default)/documents/C/d", + "fields": { + "a": { + "integerValue": "1" + } + } + }, + "updateMask": { + "fieldPaths": [ + "a" + ] + }, + "currentDocument": { + "exists": true + } + } + ] + } } } ] From 395574ccd2d4f3297898de277d66765dbec4244f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 19 Jan 2024 14:55:56 -0500 Subject: [PATCH 06/10] ensure that update() rejects a precondition with {exists:false} --- dev/src/write-batch.ts | 19 +++++++++++++++++-- dev/test/document.ts | 6 ++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/dev/src/write-batch.ts b/dev/src/write-batch.ts index 5a967eddb..4281dcde7 100644 --- a/dev/src/write-batch.ts +++ b/dev/src/write-batch.ts @@ -660,8 +660,13 @@ export class WriteBatch implements firestore.WriteBatch { * @internal * @param arg The argument name or argument index (for varargs methods). * @param value The object to validate + * @param options Options describing other things for this function to validate. */ -function validatePrecondition(arg: string | number, value: unknown): void { +function validatePrecondition( + arg: string | number, + value: unknown, + options?: {allowedExistsValues?: boolean[]} +): void { if (typeof value !== 'object' || value === null) { throw new Error('Input is not an object.'); } @@ -680,6 +685,16 @@ function validatePrecondition(arg: string | number, value: unknown): void { )} "exists" is not a boolean.'` ); } + if ( + options?.allowedExistsValues && + options.allowedExistsValues.indexOf(precondition.exists) < 0 + ) { + throw new Error( + `${invalidArgumentMessage(arg, 'precondition')}` + + `"exists" is not allowed to have the value ${precondition.exists} ` + + `(allowed values: ${options.allowedExistsValues.join(', ')})` + ); + } } if (precondition.lastUpdateTime !== undefined) { @@ -720,7 +735,7 @@ function validateUpdatePrecondition( options?: RequiredArgumentOptions ): asserts value is {lastUpdateTime?: Timestamp} { if (!validateOptional(value, options)) { - validatePrecondition(arg, value); + validatePrecondition(arg, value, {allowedExistsValues: [true]}); } } diff --git a/dev/test/document.ts b/dev/test/document.ts index a8c9ffde6..bc140aeef 100644 --- a/dev/test/document.ts +++ b/dev/test/document.ts @@ -2088,6 +2088,12 @@ describe('update document', () => { }); it("doesn't accept argument after precondition", () => { + expect(() => { + firestore.doc('collectionId/documentId').update('foo', 'bar', { + exists: false, + }); + }).to.throw(INVALID_ARGUMENTS_TO_UPDATE); + expect(() => { firestore .doc('collectionId/documentId') From e748d5e489ec31388e82de3f093ca3fcf694e638 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 19 Jan 2024 21:26:51 -0500 Subject: [PATCH 07/10] Add tests for update with exists=false precondition --- .../update-exists-false-precond.json | 39 +++++++++++++++ ...d.json => update-exists-true-precond.json} | 4 +- .../update-paths-exists-false-precond.json | 48 +++++++++++++++++++ ... => update-paths-exists-true-precond.json} | 4 +- 4 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 dev/conformance/conformance-tests/update-exists-false-precond.json rename dev/conformance/conformance-tests/{update-exists-precond.json => update-exists-true-precond.json} (84%) create mode 100644 dev/conformance/conformance-tests/update-paths-exists-false-precond.json rename dev/conformance/conformance-tests/{update-paths-exists-precond.json => update-paths-exists-true-precond.json} (86%) diff --git a/dev/conformance/conformance-tests/update-exists-false-precond.json b/dev/conformance/conformance-tests/update-exists-false-precond.json new file mode 100644 index 000000000..03c94ad39 --- /dev/null +++ b/dev/conformance/conformance-tests/update-exists-false-precond.json @@ -0,0 +1,39 @@ +{ + "tests": [ + { + "description": "update: Exists=false precondition is valid", + "comment": "The Update method supports an explicit exists=false precondition.", + "update": { + "docRefPath": "projects/projectID/databases/(default)/documents/C/d", + "precondition": { + "exists": false + }, + "jsonData": "{\"a\": 1}", + "isError": true, + "request": { + "database": "projects/projectID/databases/(default)", + "writes": [ + { + "update": { + "name": "projects/projectID/databases/(default)/documents/C/d", + "fields": { + "a": { + "integerValue": "1" + } + } + }, + "updateMask": { + "fieldPaths": [ + "a" + ] + }, + "currentDocument": { + "exists": false + } + } + ] + } + } + } + ] +} diff --git a/dev/conformance/conformance-tests/update-exists-precond.json b/dev/conformance/conformance-tests/update-exists-true-precond.json similarity index 84% rename from dev/conformance/conformance-tests/update-exists-precond.json rename to dev/conformance/conformance-tests/update-exists-true-precond.json index 66e5d1124..f1aa800e4 100644 --- a/dev/conformance/conformance-tests/update-exists-precond.json +++ b/dev/conformance/conformance-tests/update-exists-true-precond.json @@ -1,8 +1,8 @@ { "tests": [ { - "description": "update: Exists precondition is valid", - "comment": "The Update method supports an explicit exists precondition.", + "description": "update: Exists=true precondition is valid", + "comment": "The Update method supports an explicit exists=true precondition.", "update": { "docRefPath": "projects/projectID/databases/(default)/documents/C/d", "precondition": { diff --git a/dev/conformance/conformance-tests/update-paths-exists-false-precond.json b/dev/conformance/conformance-tests/update-paths-exists-false-precond.json new file mode 100644 index 000000000..4a9431501 --- /dev/null +++ b/dev/conformance/conformance-tests/update-paths-exists-false-precond.json @@ -0,0 +1,48 @@ +{ + "tests": [ + { + "description": "update-paths: Exists=false precondition is valid", + "comment": "The Update method supports an explicit exists=false precondition.", + "updatePaths": { + "docRefPath": "projects/projectID/databases/(default)/documents/C/d", + "precondition": { + "exists": false + }, + "fieldPaths": [ + { + "field": [ + "a" + ] + } + ], + "jsonValues": [ + "1" + ], + "isError": true, + "request": { + "database": "projects/projectID/databases/(default)", + "writes": [ + { + "update": { + "name": "projects/projectID/databases/(default)/documents/C/d", + "fields": { + "a": { + "integerValue": "1" + } + } + }, + "updateMask": { + "fieldPaths": [ + "a" + ] + }, + "currentDocument": { + "exists": false + } + } + ] + } + } + } + ] +} diff --git a/dev/conformance/conformance-tests/update-paths-exists-precond.json b/dev/conformance/conformance-tests/update-paths-exists-true-precond.json similarity index 86% rename from dev/conformance/conformance-tests/update-paths-exists-precond.json rename to dev/conformance/conformance-tests/update-paths-exists-true-precond.json index 6325c7546..39d61c99a 100644 --- a/dev/conformance/conformance-tests/update-paths-exists-precond.json +++ b/dev/conformance/conformance-tests/update-paths-exists-true-precond.json @@ -1,8 +1,8 @@ { "tests": [ { - "description": "update-paths: Exists precondition is valid", - "comment": "The Update method supports an explicit exists precondition.", + "description": "update-paths: Exists=true precondition is valid", + "comment": "The Update method supports an explicit exists=true precondition.", "updatePaths": { "docRefPath": "projects/projectID/databases/(default)/documents/C/d", "precondition": { From c793fda697a3908b62bbe6435607a79aa3b55e6f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 19 Jan 2024 22:25:07 -0500 Subject: [PATCH 08/10] fix name and description of exists=false conformance tests --- .../conformance-tests/update-exists-false-precond.json | 4 ++-- .../conformance-tests/update-paths-exists-false-precond.json | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dev/conformance/conformance-tests/update-exists-false-precond.json b/dev/conformance/conformance-tests/update-exists-false-precond.json index 03c94ad39..49aea0b8c 100644 --- a/dev/conformance/conformance-tests/update-exists-false-precond.json +++ b/dev/conformance/conformance-tests/update-exists-false-precond.json @@ -1,8 +1,8 @@ { "tests": [ { - "description": "update: Exists=false precondition is valid", - "comment": "The Update method supports an explicit exists=false precondition.", + "description": "update: Exists=false precondition is invalid", + "comment": "The Update method does not support an explicit exists=false precondition.", "update": { "docRefPath": "projects/projectID/databases/(default)/documents/C/d", "precondition": { diff --git a/dev/conformance/conformance-tests/update-paths-exists-false-precond.json b/dev/conformance/conformance-tests/update-paths-exists-false-precond.json index 4a9431501..6d4cd4c3e 100644 --- a/dev/conformance/conformance-tests/update-paths-exists-false-precond.json +++ b/dev/conformance/conformance-tests/update-paths-exists-false-precond.json @@ -1,8 +1,8 @@ { "tests": [ { - "description": "update-paths: Exists=false precondition is valid", - "comment": "The Update method supports an explicit exists=false precondition.", + "description": "update-paths: Exists=false precondition is invalid", + "comment": "The Update method does not support an explicit exists=false precondition.", "updatePaths": { "docRefPath": "projects/projectID/databases/(default)/documents/C/d", "precondition": { From d10bfd76c318b47f6a1d61765060f7143c759766 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 19 Jan 2024 22:29:14 -0500 Subject: [PATCH 09/10] write-batch.ts: add missing space in error message --- dev/src/write-batch.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev/src/write-batch.ts b/dev/src/write-batch.ts index 4281dcde7..7cce55e04 100644 --- a/dev/src/write-batch.ts +++ b/dev/src/write-batch.ts @@ -690,7 +690,7 @@ function validatePrecondition( options.allowedExistsValues.indexOf(precondition.exists) < 0 ) { throw new Error( - `${invalidArgumentMessage(arg, 'precondition')}` + + `${invalidArgumentMessage(arg, 'precondition')} ` + `"exists" is not allowed to have the value ${precondition.exists} ` + `(allowed values: ${options.allowedExistsValues.join(', ')})` ); From 15304c193b5c2779a14726989b68da986a7452e4 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 22 Jan 2024 10:52:48 -0500 Subject: [PATCH 10/10] update conformance tests --- .../update-exists-false-precond.json | 25 +------------------ .../update-paths-exists-false-precond.json | 25 +------------------ 2 files changed, 2 insertions(+), 48 deletions(-) diff --git a/dev/conformance/conformance-tests/update-exists-false-precond.json b/dev/conformance/conformance-tests/update-exists-false-precond.json index 49aea0b8c..031fd1af3 100644 --- a/dev/conformance/conformance-tests/update-exists-false-precond.json +++ b/dev/conformance/conformance-tests/update-exists-false-precond.json @@ -9,30 +9,7 @@ "exists": false }, "jsonData": "{\"a\": 1}", - "isError": true, - "request": { - "database": "projects/projectID/databases/(default)", - "writes": [ - { - "update": { - "name": "projects/projectID/databases/(default)/documents/C/d", - "fields": { - "a": { - "integerValue": "1" - } - } - }, - "updateMask": { - "fieldPaths": [ - "a" - ] - }, - "currentDocument": { - "exists": false - } - } - ] - } + "isError": true } } ] diff --git a/dev/conformance/conformance-tests/update-paths-exists-false-precond.json b/dev/conformance/conformance-tests/update-paths-exists-false-precond.json index 6d4cd4c3e..f122b5307 100644 --- a/dev/conformance/conformance-tests/update-paths-exists-false-precond.json +++ b/dev/conformance/conformance-tests/update-paths-exists-false-precond.json @@ -18,30 +18,7 @@ "jsonValues": [ "1" ], - "isError": true, - "request": { - "database": "projects/projectID/databases/(default)", - "writes": [ - { - "update": { - "name": "projects/projectID/databases/(default)/documents/C/d", - "fields": { - "a": { - "integerValue": "1" - } - } - }, - "updateMask": { - "fieldPaths": [ - "a" - ] - }, - "currentDocument": { - "exists": false - } - } - ] - } + "isError": true } } ]