diff --git a/dev/src/document-change.ts b/dev/src/document-change.ts index bdb73a0fc..8465ab22e 100644 --- a/dev/src/document-change.ts +++ b/dev/src/document-change.ts @@ -14,9 +14,8 @@ * limitations under the License. */ -import {FirestoreDataConverter} from '@google-cloud/firestore'; import {QueryDocumentSnapshot} from './document'; -import {DocumentData} from './types'; +import {DocumentData, FirestoreDataConverter} from './types'; import {defaultConverter} from './watch'; export type DocumentChangeType = 'added' | 'removed' | 'modified'; @@ -32,7 +31,6 @@ export class DocumentChange { private readonly _document: QueryDocumentSnapshot; private readonly _oldIndex: number; private readonly _newIndex: number; - private readonly _converter?: FirestoreDataConverter; /** * @hideconstructor @@ -48,15 +46,12 @@ export class DocumentChange { type: DocumentChangeType, document: QueryDocumentSnapshot, oldIndex: number, - newIndex: number, - converter?: FirestoreDataConverter + newIndex: number ) { this._type = type; this._document = document; this._oldIndex = oldIndex; this._newIndex = newIndex; - this._converter = - converter || (defaultConverter as FirestoreDataConverter); } /** @@ -186,8 +181,7 @@ export class DocumentChange { this._type === other._type && this._oldIndex === other._oldIndex && this._newIndex === other._newIndex && - this._document.isEqual(other._document) && - this._converter === other._converter + this._document.isEqual(other._document) ); } } diff --git a/dev/src/document.ts b/dev/src/document.ts index f70b9ed6f..501a1ab04 100644 --- a/dev/src/document.ts +++ b/dev/src/document.ts @@ -28,8 +28,6 @@ import {ApiMapValue, DocumentData, UpdateMap} from './types'; import {isEmpty, isObject} from './util'; import api = google.firestore.v1; -import {FirestoreDataConverter} from '@google-cloud/firestore'; -import {defaultConverter} from './watch'; /** * Returns a builder for DocumentSnapshot and QueryDocumentSnapshot instances. @@ -39,7 +37,7 @@ import {defaultConverter} from './watch'; */ export class DocumentSnapshotBuilder { /** The reference to the document. */ - ref?: DocumentReference; + ref: DocumentReference; /** The fields of the Firestore `Document` Protobuf backing this document. */ fieldsProto?: ApiMapValue; @@ -53,11 +51,12 @@ export class DocumentSnapshotBuilder { /** The time when this document was last updated. */ updateTime?: Timestamp; - private readonly _converter: FirestoreDataConverter; - - constructor(converter?: FirestoreDataConverter) { - this._converter = - converter || (defaultConverter as FirestoreDataConverter); + /** + * We include the DocumentReference in the constructor in order to allow the + * DocumentSnapshotBuilder to be typed with when it is constructed. + */ + constructor(ref: DocumentReference) { + this.ref = ref; } /** @@ -82,16 +81,14 @@ export class DocumentSnapshotBuilder { this.fieldsProto!, this.readTime!, this.createTime!, - this.updateTime!, - this.ref!._converter + this.updateTime! ) : new DocumentSnapshot( this.ref!, undefined, this.readTime!, undefined, - undefined, - this.ref!._converter + undefined ); } } @@ -117,7 +114,6 @@ export class DocumentSnapshot { private _readTime: Timestamp | undefined; private _createTime: Timestamp | undefined; private _updateTime: Timestamp | undefined; - private readonly _converter: FirestoreDataConverter; /** * @hideconstructor @@ -137,8 +133,7 @@ export class DocumentSnapshot { fieldsProto?: ApiMapValue, readTime?: Timestamp, createTime?: Timestamp, - updateTime?: Timestamp, - converter?: FirestoreDataConverter + updateTime?: Timestamp ) { this._ref = ref; this._fieldsProto = fieldsProto; @@ -146,8 +141,6 @@ export class DocumentSnapshot { this._readTime = readTime; this._createTime = createTime; this._updateTime = updateTime; - this._converter = - converter || (defaultConverter as FirestoreDataConverter); } /** @@ -408,7 +401,7 @@ export class DocumentSnapshot { for (const prop of Object.keys(fields)) { obj[prop] = this._serializer.decodeValue(fields[prop]); } - return this._converter.fromFirestore(obj); + return this.ref._converter.fromFirestore(obj); } /** @@ -515,8 +508,7 @@ export class DocumentSnapshot { this === other || (other instanceof DocumentSnapshot && this._ref.isEqual(other._ref) && - deepEqual(this._fieldsProto, other._fieldsProto, {strict: true}) && - this._converter === other._converter) + deepEqual(this._fieldsProto, other._fieldsProto, {strict: true})) ); } } @@ -554,10 +546,9 @@ export class QueryDocumentSnapshot extends DocumentSnapshot< fieldsProto: ApiMapValue, readTime: Timestamp, createTime: Timestamp, - updateTime: Timestamp, - converter?: FirestoreDataConverter + updateTime: Timestamp ) { - super(ref, fieldsProto, readTime, createTime, updateTime, converter); + super(ref, fieldsProto, readTime, createTime, updateTime); } /** diff --git a/dev/src/index.ts b/dev/src/index.ts index fa038b546..60c847660 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -42,6 +42,7 @@ import {Timestamp} from './timestamp'; import {parseGetAllArguments, Transaction} from './transaction'; import { ApiMapValue, + DocumentData, GapicClient, GrpcError, ReadOptions, @@ -60,7 +61,6 @@ import { import {WriteBatch} from './write-batch'; import api = google.firestore.v1; -import {DocumentData} from '@google-cloud/firestore'; export { CollectionReference, @@ -661,20 +661,22 @@ export class Firestore { ); } - const document = new DocumentSnapshotBuilder(); - + let ref; + let document; if (typeof documentOrName === 'string') { - document.ref = new DocumentReference( + ref = new DocumentReference( this, QualifiedResourcePath.fromSlashSeparatedString(documentOrName) ); + document = new DocumentSnapshotBuilder(ref); } else { - document.ref = new DocumentReference( + ref = new DocumentReference( this, QualifiedResourcePath.fromSlashSeparatedString( documentOrName.name as string ) ); + document = new DocumentSnapshotBuilder(ref); document.fieldsProto = documentOrName.fields ? convertFields(documentOrName.fields as ApiMapValue) : {}; diff --git a/dev/src/reference.ts b/dev/src/reference.ts index 6d800f9ac..6a72909bc 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -41,6 +41,7 @@ import {Serializable, Serializer, validateUserInput} from './serializer'; import {Timestamp} from './timestamp'; import { DocumentData, + FirestoreDataConverter, OrderByDirection, Precondition, SetOptions, @@ -59,7 +60,6 @@ import {defaultConverter, DocumentWatch, QueryWatch} from './watch'; import {validateDocumentData, WriteBatch, WriteResult} from './write-batch'; import api = proto.google.firestore.v1; -import {FirestoreDataConverter} from '@google-cloud/firestore'; /** * The direction of a `Query.orderBy()` clause is specified as 'desc' or 'asc' @@ -497,12 +497,12 @@ export class DocumentReference implements Serializable { } // The document is missing. - const document = new DocumentSnapshotBuilder(); - document.ref = new DocumentReference( + const ref = new DocumentReference( this._firestore, this._path, this._converter ); + const document = new DocumentSnapshotBuilder(ref); document.readTime = readTime; onNext(document.build()); }, onError || console.error); @@ -675,7 +675,6 @@ export class QuerySnapshot { private _materializedChanges: Array> | null = null; private _docs: (() => Array>) | null = null; private _changes: (() => Array>) | null = null; - private readonly _converter: FirestoreDataConverter; /** * @hideconstructor @@ -695,13 +694,10 @@ export class QuerySnapshot { private readonly _readTime: Timestamp, private readonly _size: number, docs: () => Array>, - changes: () => Array>, - converter?: FirestoreDataConverter + changes: () => Array> ) { this._docs = docs; this._changes = changes; - this._converter = - converter || (defaultConverter as FirestoreDataConverter); } /** @@ -901,10 +897,6 @@ export class QuerySnapshot { ); } - if (this._converter !== other._converter) { - return false; - } - // Otherwise, we compare the changes first as we expect there to be fewer. return ( isArrayEqual(this.docChanges(), other.docChanges()) && @@ -1040,7 +1032,7 @@ export class QueryOptions { */ export class Query { private readonly _serializer: Serializer; - private readonly _converter: FirestoreDataConverter; + readonly _converter: FirestoreDataConverter; /** * @hideconstructor @@ -1724,13 +1716,10 @@ export class Query { () => { const changes: Array> = []; for (let i = 0; i < docs.length; ++i) { - changes.push( - new DocumentChange('added', docs[i], -1, i, this._converter) - ); + changes.push(new DocumentChange('added', docs[i], -1, i)); } return changes; - }, - this._converter + } ) ); }); @@ -2178,7 +2167,7 @@ export class CollectionReference extends Query { ); } - return new DocumentReference(this.firestore, path); + return new DocumentReference(this.firestore, path, this._converter); } /** @@ -2258,13 +2247,6 @@ export function validateQueryOrder( return op as OrderByDirection | undefined; } -export function defaultDataConverter(): FirestoreDataConverter { - return { - toFirestore: data => data, - fromFirestore: data => data as DocumentData, - }; -} - /** * Validates the input string as a field comparison operator. * diff --git a/dev/src/types.ts b/dev/src/types.ts index 52cd8787d..4d85bf5bf 100644 --- a/dev/src/types.ts +++ b/dev/src/types.ts @@ -40,6 +40,61 @@ export class GrpcError extends Error { code?: number; } +/** + * Converter used by `withConverter()` to transform user objects of type T + * into Firestore data. + * + * Using the converter allows you to specify generic type arguments when + * storing and retrieving objects from Firestore. + * + * @example + * ```typescript + * class Post { + * constructor(readonly title: string, readonly author: string) {} + * + * toString(): string { + * return this.title + ', by ' + this.author; + * } + * } + * + * const postConverter = { + * toFirestore(post: Post): FirebaseFirestore.DocumentData { + * return {title: post.title, author: post.author}; + * }, + * fromFirestore( + * data: FirebaseFirestore.DocumentData + * ): Post { + * return new Post(data.title, data.author); + * } + * }; + * + * const postSnap = await Firestore() + * .collection('posts') + * .withConverter(postConverter) + * .doc().get(); + * const post = postSnap.data(); + * if (post !== undefined) { + * post.title; // string + * post.toString(); // Should be defined + * post.someNonExistentProperty; // TS error + * } + * ``` + */ +export interface FirestoreDataConverter { + /** + * Called by the Firestore SDK to convert a custom model object of type T + * into a plain Javascript object (suitable for writing directly to the + * Firestore database). + */ + toFirestore(modelObject: T): DocumentData; + + /** + * Called by the Firestore SDK to convert Firestore data into an object of + * type T. + */ + fromFirestore(data: DocumentData): T; +} + /** * Settings used to directly configure a `Firestore` instance. */ diff --git a/dev/src/watch.ts b/dev/src/watch.ts index 407600c67..00125a4c7 100644 --- a/dev/src/watch.ts +++ b/dev/src/watch.ts @@ -25,11 +25,10 @@ import {DocumentReference, Firestore, Query} from './index'; import {logger} from './logger'; import {QualifiedResourcePath} from './path'; import {Timestamp} from './timestamp'; -import {DocumentData, GrpcError, RBTree} from './types'; +import {DocumentData, FirestoreDataConverter, GrpcError, RBTree} from './types'; import {requestTag} from './util'; import api = google.firestore.v1; -import {FirestoreDataConverter} from '@google-cloud/firestore'; export const defaultConverter = { toFirestore( @@ -637,9 +636,10 @@ abstract class Watch { if (changed) { logger('Watch.onData', this.requestTag, 'Received document change'); - const snapshot = new DocumentSnapshotBuilder(this._converter); const ref = this.firestore.doc(relativeName); - snapshot.ref = ref.withConverter(this._converter); + const snapshot = new DocumentSnapshotBuilder( + ref.withConverter(this._converter) + ); snapshot.fieldsProto = document.fields || {}; snapshot.createTime = Timestamp.fromProto(document.createTime!); snapshot.updateTime = Timestamp.fromProto(document.updateTime!); diff --git a/dev/src/write-batch.ts b/dev/src/write-batch.ts index cf53a44f5..7df07877a 100644 --- a/dev/src/write-batch.ts +++ b/dev/src/write-batch.ts @@ -185,16 +185,15 @@ export class WriteBatch { this.verifyNotCommitted(); - const firestoreData = documentRef._converter - ? documentRef._converter.toFirestore(data) - : (data as T); + const firestoreData = documentRef._converter.toFirestore(data); const transform = DocumentTransform.fromObject(documentRef, firestoreData); transform.validate(); const precondition = new Precondition({exists: false}); const op = () => { - const document = DocumentSnapshot.fromObject(documentRef, data); + // TODO: chenbrian why is this data and not firestoreDAta + const document = DocumentSnapshot.fromObject(documentRef, firestoreData); const write = !document.isEmpty || transform.isEmpty ? document.toProto() : null; @@ -296,11 +295,11 @@ export class WriteBatch { validateSetOptions('options', options, {optional: true}); const mergeLeaves = options && options.merge === true; const mergePaths = options && options.mergeFields; - validateDocumentReference('documentRef', documentRef); + let firestoreData: DocumentData = documentRef._converter.toFirestore(data); validateDocumentData( 'data', - data, + firestoreData, /* allowDeletes= */ !!(mergePaths || mergeLeaves) ); @@ -308,8 +307,6 @@ export class WriteBatch { let documentMask: DocumentMask; - let firestoreData = documentRef._converter.toFirestore(data); - if (mergePaths) { documentMask = DocumentMask.fromFieldMask(options!.mergeFields!); firestoreData = documentMask.applyTo(firestoreData); diff --git a/dev/test/collection.ts b/dev/test/collection.ts index e25272535..fb2b30111 100644 --- a/dev/test/collection.ts +++ b/dev/test/collection.ts @@ -14,13 +14,20 @@ import {expect} from 'chai'; -import {DocumentReference, Firestore, setLogFunction} from '../src'; +import { + DocumentData, + DocumentReference, + Firestore, + setLogFunction, +} from '../src'; import { ApiOverride, createInstance, DATABASE_ROOT, document, InvalidApiUsage, + Post, + postConverter, verifyInstance, } from './util/helpers'; @@ -167,4 +174,26 @@ describe('Collection interface', () => { expect(coll1.isEqual(coll1Equals)).to.be.ok; expect(coll1.isEqual(coll2)).to.not.be.ok; }); + + it('for CollectionReference.withConverter()', async () => { + const docRef = firestore + .collection('posts') + .withConverter(postConverter) + .doc(); + + await docRef.set(new Post('post', 'author')); + const postData = await docRef.get(); + const post = postData.data(); + expect(post).to.not.equal(undefined); + expect(post!.byline()).to.equal('post, by author'); + }); + + it('drops the converter when calling CollectionReference.parent()', () => { + const postsCollection = firestore + .collection('users/user1/posts') + .withConverter(postConverter); + + const usersCollection = postsCollection.parent; + expect(usersCollection!.isEqual(firestore.doc('users/user1'))).to.be.true; + }); }); diff --git a/dev/test/document.ts b/dev/test/document.ts index 4a6f62c27..f9d8e3509 100644 --- a/dev/test/document.ts +++ b/dev/test/document.ts @@ -32,6 +32,8 @@ import { found, InvalidApiUsage, missing, + Post, + postConverter, remove, requestEquals, retrieve, @@ -2020,3 +2022,28 @@ describe('listCollections() method', () => { }); }); }); + +describe('withConverter() support', () => { + let firestore: Firestore; + + beforeEach(() => { + return createInstance().then(firestoreInstance => { + firestore = firestoreInstance; + }); + }); + + afterEach(() => verifyInstance(firestore)); + + it('for DocumentReference.withConverter()', async () => { + const docRef = firestore + .collection('posts') + .doc() + .withConverter(postConverter); + + await docRef.set(new Post('post', 'author')); + const postData = await docRef.get(); + const post = postData.data(); + expect(post).to.not.equal(undefined); + expect(post!.byline()).to.equal('post, by author'); + }); +}); diff --git a/dev/test/query.ts b/dev/test/query.ts index 03ff08cb1..39e79d5de 100644 --- a/dev/test/query.ts +++ b/dev/test/query.ts @@ -25,6 +25,7 @@ import { createInstance, document, InvalidApiUsage, + postConverter, stream, verifyInstance, } from './util/helpers'; @@ -42,11 +43,11 @@ function snapshot( data: DocumentData ): Promise { return createInstance().then(firestore => { - const snapshot = new DocumentSnapshotBuilder(); const path = QualifiedResourcePath.fromSlashSeparatedString( `${DATABASE_ROOT}/documents/${relativePath}` ); - snapshot.ref = new DocumentReference(firestore, path); + const ref = new DocumentReference(firestore, path); + const snapshot = new DocumentSnapshotBuilder(ref); snapshot.fieldsProto = firestore['_serializer']!.encodeFields(data); snapshot.readTime = Timestamp.fromMillis(0); snapshot.createTime = Timestamp.fromMillis(0); @@ -614,6 +615,21 @@ describe('query interface', () => { }); }); }); + + it('for Query.withConverter()', async () => { + await firestore + .doc('postings/post1') + .set({title: 'post1', author: 'author1'}); + await firestore + .doc('postings/post2') + .set({title: 'post2', author: 'author2'}); + const posts = await firestore + .collectionGroup('postings') + .withConverter(postConverter) + .get(); + expect(posts.size).to.equal(2); + expect(posts.docs[0].data()!.byline()).to.equal('post1, by author1'); + }); }); describe('where() interface', () => { diff --git a/dev/test/util/helpers.ts b/dev/test/util/helpers.ts index d47a8ff67..7bd8dcac4 100644 --- a/dev/test/util/helpers.ts +++ b/dev/test/util/helpers.ts @@ -20,7 +20,7 @@ import * as through2 from 'through2'; import * as proto from '../../protos/firestore_v1_proto_api'; import {Firestore} from '../../src'; import {ClientPool} from '../../src/pool'; -import {GapicClient, GrpcError} from '../../src/types'; +import {DocumentData, GapicClient, GrpcError} from '../../src/types'; import api = proto.google.firestore.v1; @@ -378,3 +378,19 @@ export function stream( return stream; } + +export class Post { + constructor(readonly title: string, readonly author: string) {} + byline(): string { + return this.title + ', by ' + this.author; + } +} + +export const postConverter = { + toFirestore(post: Post): DocumentData { + return {title: post.title, author: post.author}; + }, + fromFirestore(data: DocumentData): Post { + return new Post(data.title, data.author); + }, +}; diff --git a/dev/test/watch.ts b/dev/test/watch.ts index 99b9d37e1..c9754edd1 100644 --- a/dev/test/watch.ts +++ b/dev/test/watch.ts @@ -144,8 +144,7 @@ function snapshot( ref: DocumentReference, data: DocumentData ): QueryDocumentSnapshot { - const snapshot = new DocumentSnapshotBuilder(); - snapshot.ref = ref; + const snapshot = new DocumentSnapshotBuilder(ref); snapshot.fieldsProto = ref.firestore._serializer!.encodeFields(data); snapshot.readTime = new Timestamp(0, 0); snapshot.createTime = new Timestamp(0, 0);