Skip to content

Commit

Permalink
fix: Allow an explicit MustExist precondition for update (#1985)
Browse files Browse the repository at this point in the history
  • Loading branch information
dconeybe authored Jan 24, 2024
1 parent cd6bc86 commit 99d60a6
Show file tree
Hide file tree
Showing 6 changed files with 126 additions and 18 deletions.
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
{
"tests": [
{
"description": "update: Exists precondition is invalid",
"comment": "The Update method does not support an explicit exists 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": {
"exists": true
"exists": false
},
"jsonData": "{\"a\": 1}",
"isError": true
Expand Down
38 changes: 38 additions & 0 deletions dev/conformance/conformance-tests/update-exists-true-precond.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
{
"tests": [
{
"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": {
"exists": true
},
"jsonData": "{\"a\": 1}",
"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
}
}
]
}
}
}
]
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
{
"tests": [
{
"description": "update-paths: Exists precondition is invalid",
"comment": "The Update method does not support an explicit exists 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": {
"exists": true
"exists": false
},
"fieldPaths": [
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"tests": [
{
"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": {
"exists": true
},
"fieldPaths": [
{
"field": [
"a"
]
}
],
"jsonValues": [
"1"
],
"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
}
}
]
}
}
}
]
}
24 changes: 13 additions & 11 deletions dev/src/write-batch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -660,12 +660,12 @@ 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.
* @param options Options describing other things for this function to validate.
*/
function validatePrecondition(
arg: string | number,
value: unknown,
allowExists: boolean
options?: {allowedExistsValues?: boolean[]}
): void {
if (typeof value !== 'object' || value === null) {
throw new Error('Input is not an object.');
Expand All @@ -677,20 +677,22 @@ function validatePrecondition(

if (precondition.exists !== undefined) {
++conditions;
if (!allowExists) {
if (typeof precondition.exists !== 'boolean') {
throw new Error(
`${invalidArgumentMessage(
arg,
'precondition'
)} "exists" is not an allowed precondition.`
)} "exists" is not a boolean.'`
);
}
if (typeof precondition.exists !== 'boolean') {
if (
options?.allowedExistsValues &&
options.allowedExistsValues.indexOf(precondition.exists) < 0
) {
throw new Error(
`${invalidArgumentMessage(
arg,
'precondition'
)} "exists" is not a boolean.'`
`${invalidArgumentMessage(arg, 'precondition')} ` +
`"exists" is not allowed to have the value ${precondition.exists} ` +
`(allowed values: ${options.allowedExistsValues.join(', ')})`
);
}
}
Expand Down Expand Up @@ -733,7 +735,7 @@ function validateUpdatePrecondition(
options?: RequiredArgumentOptions
): asserts value is {lastUpdateTime?: Timestamp} {
if (!validateOptional(value, options)) {
validatePrecondition(arg, value, /* allowExists= */ false);
validatePrecondition(arg, value, {allowedExistsValues: [true]});
}
}

Expand All @@ -753,7 +755,7 @@ function validateDeletePrecondition(
options?: RequiredArgumentOptions
): void {
if (!validateOptional(value, options)) {
validatePrecondition(arg, value, /* allowExists= */ true);
validatePrecondition(arg, value);
}
}

Expand Down
23 changes: 22 additions & 1 deletion dev/test/document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down Expand Up @@ -2069,7 +2090,7 @@ describe('update document', () => {
it("doesn't accept argument after precondition", () => {
expect(() => {
firestore.doc('collectionId/documentId').update('foo', 'bar', {
exists: true,
exists: false,
});
}).to.throw(INVALID_ARGUMENTS_TO_UPDATE);

Expand Down

0 comments on commit 99d60a6

Please sign in to comment.