Skip to content

Commit

Permalink
Addressing review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@schmidt-sebastian
schmidt-sebastian committed Oct 10, 2017
1 parent 33e9c47 commit e0c5eb3
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 71 deletions.
45 changes: 31 additions & 14 deletions src/convert.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,20 @@ const is = require('is');

const validate = require('./validate')();

/*!
* @module firestore/convert
* @private
*
* This module contains utility functions to convert
* `firestore.v1beta1.Documents` from Proto3 JSON to their equivalent
* representation in Protobuf JS. Protobuf JS is the only encoding supported by
* this client, and dependencies that use Proto3 JSON (such as the Google Cloud
* Functions SDK) are supported through this conversion and its usage in
* {@see Firestore#snapshot_}.
*/

/**
* Converts an ISO 8601 or Protobuf JS 'timestampValue' into Protobuf JS.
* Converts an ISO 8601 or google.protobuf.Timestamp proto into Protobuf JS.
*
* @private
* @param {*=} timestampValue - The value to convert.
Expand Down Expand Up @@ -51,7 +63,8 @@ function convertTimestamp(timestampValue) {

if (isNaN(seconds) || isNaN(nanos)) {
// This error should only ever be thrown if the end-user specifies an
// invalid 'lastUpdateTime'.
// invalid 'lastUpdateTime' in a precondition (hence we use
// 'lastUpdateTime' here instead of the actual argument name).
throw new Error(
'Specify a valid ISO 8601 timestamp for "lastUpdateTime".'
);
Expand All @@ -73,7 +86,7 @@ function convertTimestamp(timestampValue) {
}

/**
* Converts an API JSON or Protobuf JS 'bytesValue' field into Protobuf JS.
* Converts a Proto3 'bytesValue' field into Protobuf JS.
*
* @private
* @param {*} bytesValue - The value to convert.
Expand All @@ -88,10 +101,10 @@ function convertBytes(bytesValue) {
}

/**
* Detects 'valueType' from an API JSON or Protobuf JS 'Field' proto.
* Detects 'valueType' from a Proto3 JSON `firestore.v1beta1.Value` proto.
*
* @private
* @param {object} proto - The 'field' proto.
* @param {object} proto - The `firestore.v1beta1.Value` proto.
* @return {string} - The string value for 'valueType'.
*/
function detectValueType(proto) {
Expand Down Expand Up @@ -145,12 +158,13 @@ function detectValueType(proto) {
}

/**
* Converts an API JSON or Protobuf JS 'Field' proto into Protobuf JS.
* Converts a `firestore.v1beta1.Value` in Proto3 JSON encoding into the
* Protobuf JS format expected by this client.
*
* @private
* @param {object} fieldValue - The 'Field' value in API JSON or Protobuf JS
* @param {object} fieldValue - The `firestore.v1beta1.Value` in Proto3 JSON
* format.
* @return The 'Field' proto in Protobuf JS format.
* @return {object} The `firestore.v1beta1.Value` in Protobuf JS format.
*/
function convertValue(fieldValue) {
let valueType = detectValueType(fieldValue);
Expand Down Expand Up @@ -196,13 +210,16 @@ function convertValue(fieldValue) {
return Object.assign({valueType}, fieldValue);
}
}

/**
* Converts an API JSON or Protobuf JS 'Document' into Protobuf JS. This
* conversion creates a copy of underlying document.
* Converts a `firestore.v1beta1.Document` in Proto3 JSON encoding into the
* Protobuf JS format expected by this client. This conversion creates a copy of
* the underlying document.
*
* @private
* @param {object} document - The 'Document' in API JSON or Protobuf JS format.
* @return {object} The 'Document' in Protobuf JS format.
* @param {object} document - The `firestore.v1beta1.Document` in Proto3 JSON
* format.
* @return {object} The `firestore.v1beta1.Document` in Protobuf JS format.
*/
function convertDocument(document) {
let result = {};
Expand All @@ -217,6 +234,6 @@ function convertDocument(document) {
}

module.exports = {
convertDocument,
convertTimestamp,
documentFromJson: convertDocument,
timestampFromJson: convertTimestamp,
};
29 changes: 2 additions & 27 deletions src/document.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const assert = require('assert');
const is = require('is');

const path = require('./path');
const convertTimestamp = require('./convert').convertTimestamp;
const timestampFromJson = require('./convert').timestampFromJson;

/*!
* @see {ResourcePath}
Expand Down Expand Up @@ -1033,32 +1033,7 @@ class Precondition {
let proto = {};

if (is.defined(this._lastUpdateTime)) {
let date = new Date(this._lastUpdateTime);
let seconds = Math.floor(date.getTime() / 1000);
let nanos = null;

let nanoString = this._lastUpdateTime.substring(
20,
this._lastUpdateTime.length - 1
);

if (nanoString.length === 3) {
nanoString = `${nanoString}000000`;
} else if (nanoString.length === 6) {
nanoString = `${nanoString}000`;
}

if (nanoString.length === 9) {
nanos = parseInt(nanoString);
}

if (isNaN(seconds) || isNaN(nanos)) {
throw new Error(
'Specify a valid ISO 8601 timestamp for "lastUpdateTime".'
);
}

proto.updateTime = convertTimestamp(this._lastUpdateTime);
proto.updateTime = timestampFromJson(this._lastUpdateTime);
} else if (is.defined(this._exists)) {
proto.exists = this._exists;
}
Expand Down
33 changes: 27 additions & 6 deletions src/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@ const FieldPath = path.FieldPath;
*/
const FieldValue = require('./field-value');

const convertTimestamp = convert.convertTimestamp;
const convertDocument = convert.convertDocument;

/*!
* @see CollectionReference
*/
Expand Down Expand Up @@ -336,20 +333,44 @@ class Firestore extends commonGrpc.Service {

/**
* Creates a [DocumentSnapshot]{@link DocumentSnapshot} from a
* `Document` proto (or from a resource name for missing documents).
* `firestore.v1beta1lDocument` proto (or from a resource name for missing
* documents).
*
* This API is used by Google Cloud Functions.
* This API is used by Google Cloud Functions and can be called with both
* Proto3 JSON and Protobuf JS encoded data.
*
* @private
* @param {object} documentOrName - The Firestore `Document` proto or the
* resource name of a missing document.
* @param {object=} readTime - A `Timestamp` proto indicating the time this
* document was read.
* @param {string=} encoding - One of 'json' or 'protobufJS'. Applies to both
* the 'document' Proto and 'readTime'. Defaults to 'protobufJS'.
* @returns {DocumentSnapshot} - A DocumentSnapshot.
*/
snapshot_(documentOrName, readTime) {
snapshot_(documentOrName, readTime, encoding) {
let document = new DocumentSnapshot.Builder();

encoding = encoding || 'protobufJS';

let convertTimestamp;
let convertDocument;

if (!is.defined(encoding) || encoding === 'protobufJS') {
convertTimestamp = data => data;
convertDocument = data => data;
} else if (encoding === 'json') {
// Google Cloud Functions calls us with Proto3 JSON format data, which we
// must convert to Protobuf JS.
convertTimestamp = convert.timestampFromJson;
convertDocument = convert.documentFromJson;
} else {
throw new Error(
`Unsupported encoding format. Expected 'json' or 'protobufJS', ` +
`but was ${encoding}.`
);
}

if (is.string(documentOrName)) {
document.ref = new DocumentReference(
this,
Expand Down
11 changes: 0 additions & 11 deletions test/document.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -696,17 +696,6 @@ describe('deserialize document', function() {
.then(verifyAllSupportedTypes);
});

it('deserializes all supported types from API JSON', function() {
firestore.api.Firestore._batchGetDocuments = function() {
return stream(found(allSupportedTypesApiJson));
};

return firestore
.doc('collectionId/documentId')
.get()
.then(verifyAllSupportedTypes);
});

it('ignores intermittent stream failures', function() {
let attempts = 1;

Expand Down
40 changes: 27 additions & 13 deletions test/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,25 +320,35 @@ describe('snapshot_() method', function() {
assert.equal('1970-01-01T00:00:05.000000006Z', doc.readTime);
});

it('handles API JSON', function() {
it('handles Proto3 JSON', function() {
// Google Cloud Functions must be able to call snapshot_() with Proto3 JSON
// data.
let doc = firestore.snapshot_(
{
name: `${DATABASE_ROOT}/documents/collectionId/doc`,
fields: {foo: {bytesValue: 'AQI='}},
createTime: '1970-01-01T00:00:01.000000002Z',
updateTime: '1970-01-01T00:00:03.000000004Z',
fields: {
a: {bytesValue: 'AQI='},
b: {timestampValue: '1985-03-18T07:20:00.000Z'},
c: {stringValue: 'foobar'},
},
createTime: '1970-01-01T00:00:01.002Z',
updateTime: '1970-01-01T00:00:03.000004Z',
},
'1970-01-01T00:00:05.000000006Z'
'1970-01-01T00:00:05.000000006Z',
'json'
);

assert.equal(true, doc.exists);
assert.deepEqual({foo: bytesData}, doc.data());
assert.equal('1970-01-01T00:00:01.000000002Z', doc.createTime);
assert.equal('1970-01-01T00:00:03.000000004Z', doc.updateTime);
assert.deepEqual(
{a: bytesData, b: new Date('1985-03-18T07:20:00.000Z'), c: 'foobar'},
doc.data()
);
assert.equal('1970-01-01T00:00:01.002000000Z', doc.createTime);
assert.equal('1970-01-01T00:00:03.000004000Z', doc.updateTime);
assert.equal('1970-01-01T00:00:05.000000006Z', doc.readTime);
});

it('handles invalid API JSON', function() {
it('handles invalid Proto3 JSON', function() {
assert.throws(() => {
firestore.snapshot_(
{
Expand All @@ -347,7 +357,8 @@ describe('snapshot_() method', function() {
createTime: '1970-01-01T00:00:01.000000002Z',
updateTime: '1970-01-01T00:00:03.000000004Z',
},
'1970-01-01T00:00:05.000000006Z'
'1970-01-01T00:00:05.000000006Z',
'json'
);
}, /Unable to infer type value fom '{}'./);

Expand All @@ -359,7 +370,8 @@ describe('snapshot_() method', function() {
createTime: '1970-01-01T00:00:01.000000002Z',
updateTime: '1970-01-01T00:00:03.000000004Z',
},
'1970-01-01T00:00:05.000000006Z'
'1970-01-01T00:00:05.000000006Z',
'json'
);
}, /Unable to infer type value fom '{"stringValue":"bar","integerValue":42}'./);

Expand All @@ -371,15 +383,17 @@ describe('snapshot_() method', function() {
createTime: '1970-01-01T00:00:01.NaNZ',
updateTime: '1970-01-01T00:00:03.000000004Z',
},
'1970-01-01T00:00:05.000000006Z'
'1970-01-01T00:00:05.000000006Z',
'json'
);
}, /Specify a valid ISO 8601 timestamp for "lastUpdateTime"./);
});

it('handles missing document ', function() {
let doc = firestore.snapshot_(
`${DATABASE_ROOT}/documents/collectionId/doc`,
'1970-01-01T00:00:05.000000006Z'
'1970-01-01T00:00:05.000000006Z',
'json'
);

assert.equal(false, doc.exists);
Expand Down

0 comments on commit e0c5eb3

Please sign in to comment.