From faac062760023f8703bce25d550ee1fb59d0214c Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 27 Oct 2017 17:46:21 -0700 Subject: [PATCH] Cleaning up WriteResults --- src/reference.js | 16 +++++++------- src/write-batch.js | 45 +++++++++++++++++++++++++++----------- system-test/firestore.js | 20 ++++++++--------- test/document.js | 27 ++++++++++++++++++++--- test/transaction.js | 18 ++++++++------- test/write-batch.js | 47 +++++++++++++++++++++++++++------------- 6 files changed, 115 insertions(+), 58 deletions(-) diff --git a/src/reference.js b/src/reference.js index 8e40ca72f..0fdca934d 100644 --- a/src/reference.js +++ b/src/reference.js @@ -352,8 +352,8 @@ class DocumentReference { return writeBatch .create(this, data) .commit() - .then(res => { - return Promise.resolve(res.writeResults[0]); + .then(writeResults => { + return Promise.resolve(writeResults[0]); }); } @@ -383,8 +383,8 @@ class DocumentReference { return writeBatch .delete(this, precondition) .commit() - .then(res => { - return Promise.resolve(res.writeResults[0]); + .then(writeResults => { + return Promise.resolve(writeResults[0]); }); } @@ -415,8 +415,8 @@ class DocumentReference { return writeBatch .set(this, data, options) .commit() - .then(res => { - return Promise.resolve(res.writeResults[0]); + .then(writeResults => { + return Promise.resolve(writeResults[0]); }); } @@ -455,8 +455,8 @@ class DocumentReference { return writeBatch.update .apply(writeBatch, [this, dataOrField].concat(preconditionOrValues)) .commit() - .then(res => { - return Promise.resolve(res.writeResults[0]); + .then(writeResults => { + return Promise.resolve(writeResults[0]); }); } diff --git a/src/write-batch.js b/src/write-batch.js index a1637228a..71a0a5931 100644 --- a/src/write-batch.js +++ b/src/write-batch.js @@ -16,6 +16,7 @@ 'use strict'; +const assert = require('assert'); const is = require('is'); /*! @@ -449,12 +450,16 @@ class WriteBatch { }); } - request.writes = this._writes; + // We create our own copy of this array here since we need to access it + // when processing the response. + let writeRequests = this._writes.slice(); + + request.writes = writeRequests; Firestore.log( 'WriteBatch.commit', 'Sending %d writes', - request.writes.length + writeRequests.length ); if (explicitTransaction) { @@ -464,22 +469,36 @@ class WriteBatch { return this._firestore .request(this._api.Firestore.commit.bind(this._api.Firestore), request) .then(resp => { - let commitTime = DocumentSnapshot.toISOTime(resp.commitTime); - let result = { - writeResults: [], - }; + const commitTime = DocumentSnapshot.toISOTime(resp.commitTime); + const writeResults = []; if (resp.writeResults) { - for (let writeResult of resp.writeResults) { - result.writeResults.push( - new WriteResult( - DocumentSnapshot.toISOTime(writeResult.updateTime) || commitTime - ) - ); + assert( + writeRequests.length === resp.writeResults.length, + `Expected one write result per operation, but got ${resp + .writeResults + .length} results for ${writeRequests.length} operations.` + ); + + for (let i = 0; i < resp.writeResults.length; ++i) { + let writeRequest = writeRequests[i]; + let writeResult = resp.writeResults[i]; + + // Don't return write results for document transforms, as the fact + // that we have to split one write operation into two distinct + // writes is an implementation detail. + if (writeRequest.update || writeRequest.delete) { + writeResults.push( + new WriteResult( + DocumentSnapshot.toISOTime(writeResult.updateTime) || + commitTime + ) + ); + } } } - return result; + return writeResults; }); } diff --git a/system-test/firestore.js b/system-test/firestore.js index 235b13141..a1d336383 100644 --- a/system-test/firestore.js +++ b/system-test/firestore.js @@ -1204,6 +1204,15 @@ describe('WriteBatch class', function() { }); }); + it('omits document transforms from write results', function() { + let batch = firestore.batch(); + batch.set(randomCol.doc(), {foo: 'a'}); + batch.set(randomCol.doc(), {foo: Firestore.FieldValue.serverTimestamp()}); + return batch.commit().then(writeResults => { + assert.equal(writeResults.length, 2); + }); + }); + it('enforces that updated document exists', function() { let ref = randomCol.doc(); let batch = firestore.batch(); @@ -1218,17 +1227,6 @@ describe('WriteBatch class', function() { }); }); - // @todo: Uncomment when server supports verify() - // Throws 'java.lang.UnsupportedOperationException: not implemented yet' - // it('has verify() method', function() { - // let ref = firestore.doc('col/doc'); - // let batch = firestore.batch(); - // batch.set(ref, {foo: 'a'}); - // return batch.commit().then(() => { - // return firestore.batch().verify_(ref, {exists:true}).commit(); - // }); - // }); - it('has delete() method', function() { let success; diff --git a/test/document.js b/test/document.js index 3bfa6058e..2ebe3da29 100644 --- a/test/document.js +++ b/test/document.js @@ -206,6 +206,27 @@ const defaultWriteResult = { ], }; +const serverTimestampWriteResult = { + commitTime: { + nanos: 3, + seconds: 4, + }, + writeResults: [ + { + updateTime: { + nanos: 3, + seconds: 4, + }, + }, + { + updateTime: { + nanos: 3, + seconds: 4, + }, + }, + ], +}; + describe('DocumentReference interface', function() { let firestore; let documentRef; @@ -368,7 +389,7 @@ describe('serialize document', function() { ) ); - callback(null, defaultWriteResult); + callback(null, serverTimestampWriteResult); }; return firestore.doc('collectionId/documentId').set({ @@ -993,7 +1014,7 @@ describe('create document', function() { ) ); - callback(null, defaultWriteResult); + callback(null, serverTimestampWriteResult); }; return firestore.doc('collectionId/documentId').create({ @@ -1056,7 +1077,7 @@ describe('update document', function() { fieldTransform('a.b', 'REQUEST_TIME', 'c.d', 'REQUEST_TIME') ) ); - callback(null, defaultWriteResult); + callback(null, serverTimestampWriteResult); }; return firestore.doc('collectionId/documentId').update( diff --git a/test/transaction.js b/test/transaction.js index 5f4b6d7d7..7aeb4632b 100644 --- a/test/transaction.js +++ b/test/transaction.js @@ -49,16 +49,18 @@ function commit(transaction, writes, err) { nanos: 0, seconds: 0, }, - writeResults: [ - { - updateTime: { - nanos: 0, - seconds: 0, - }, - }, - ], + writeResults: [], }; + for (let i = 0; i < proto.writes.length; ++i) { + response.writeResults.push({ + updateTime: { + nanos: 0, + seconds: 0, + }, + }); + } + return { type: 'commit', request: proto, diff --git a/test/write-batch.js b/test/write-batch.js index dce6b0614..0a9ca26ac 100644 --- a/test/write-batch.js +++ b/test/write-batch.js @@ -157,6 +157,18 @@ describe('batch support', function() { name: documentName, }, }, + { + transform: { + document: + 'projects/test-project/databases/(default)/documents/col/doc', + fieldTransforms: [ + { + fieldPath: 'foo', + setToServerValue: 'REQUEST_TIME', + }, + ], + }, + }, { currentDocument: { exists: true, @@ -195,6 +207,13 @@ describe('batch support', function() { seconds: 0, }, }, + // This write result conforms to the DocumentTransform and won't be returned in the response. + { + updateTime: { + nanos: 1337, + seconds: 1337, + }, + }, { updateTime: { nanos: 1, @@ -207,6 +226,12 @@ describe('batch support', function() { seconds: 2, }, }, + { + updateTime: { + nanos: 3, + seconds: 3, + }, + }, ], }); }; @@ -214,25 +239,17 @@ describe('batch support', function() { writeBatch = firestore.batch(); }); - function verifyResponse(resp) { - assert.equal( - resp.writeResults[0].writeTime, - '1970-01-01T00:00:00.000000000Z' - ); - assert.equal( - resp.writeResults[1].writeTime, - '1970-01-01T00:00:01.000000001Z' - ); - assert.equal( - resp.writeResults[2].writeTime, - '1970-01-01T00:00:02.000000002Z' - ); + function verifyResponse(writeResults) { + assert.equal(writeResults[0].writeTime, '1970-01-01T00:00:00.000000000Z'); + assert.equal(writeResults[1].writeTime, '1970-01-01T00:00:01.000000001Z'); + assert.equal(writeResults[2].writeTime, '1970-01-01T00:00:02.000000002Z'); + assert.equal(writeResults[3].writeTime, '1970-01-01T00:00:03.000000003Z'); } it('accepts multiple operations', function() { let documentName = firestore.doc('col/doc'); - writeBatch.set(documentName, {}); + writeBatch.set(documentName, {foo: Firestore.FieldValue.serverTimestamp()}); writeBatch.update(documentName, {}); writeBatch.create(documentName, {}); writeBatch.delete(documentName); @@ -246,7 +263,7 @@ describe('batch support', function() { let documentName = firestore.doc('col/doc'); return writeBatch - .set(documentName, {}) + .set(documentName, {foo: Firestore.FieldValue.serverTimestamp()}) .update(documentName, {}) .create(documentName, {}) .delete(documentName)