propagate converter into error message

Author: Brian Chen

packages/firestore/lite/src/api/reference.ts

@@ -456,11 +456,7 @@ export function setDoc<T>(
 ): Promise<void> {
   const ref = cast(reference, DocumentReference);
 
-  const [convertedValue] = applyFirestoreDataConverter(
-    ref._converter,
-    data,
-    'setDoc'
-  );
+  const convertedValue = applyFirestoreDataConverter(ref._converter, data);
   const dataReader = newUserDataReader(ref.firestore);
   const parsed = dataReader.parseSetData(
     'setDoc',
@@ -548,14 +544,16 @@ export function addDoc<T>(
   const collRef = cast(reference, CollectionReference);
   const docRef = doc(collRef);
 
-  const [convertedValue] = applyFirestoreDataConverter(
-    collRef._converter,
-    data,
-    'addDoc'
-  );
+  const convertedValue = applyFirestoreDataConverter(collRef._converter, data);
 
   const dataReader = newUserDataReader(collRef.firestore);
-  const parsed = dataReader.parseSetData('addDoc', docRef._key, convertedValue);
+  const parsed = dataReader.parseSetData(
+    'addDoc',
+    docRef._key,
+    convertedValue,
+    {},
+    docRef._converter !== null
+  );
 
   return collRef.firestore
     ._getDatastore()

packages/firestore/lite/src/api/transaction.ts

@@ -95,16 +95,13 @@ export class Transaction implements firestore.Transaction {
     options?: firestore.SetOptions
   ): Transaction {
     const ref = validateReference(documentRef, this._firestore);
-    const [convertedValue] = applyFirestoreDataConverter(
-      ref._converter,
-      value,
-      'Transaction.set'
-    );
+    const convertedValue = applyFirestoreDataConverter(ref._converter, value);
     const parsed = this._dataReader.parseSetData(
       'Transaction.set',
       ref._key,
       convertedValue,
-      options
+      options,
+      ref._converter !== null
     );
     this._transaction.set(ref._key, parsed);
     return this;

packages/firestore/lite/src/api/write_batch.ts

@@ -59,17 +59,14 @@ export class WriteBatch implements firestore.WriteBatch {
     this.verifyNotCommitted();
     const ref = validateReference(documentRef, this._firestore);
 
-    const [convertedValue] = applyFirestoreDataConverter(
-      ref._converter,
-      value,
-      'WriteBatch.set'
-    );
+    const convertedValue = applyFirestoreDataConverter(ref._converter, value);
 
     const parsed = this._dataReader.parseSetData(
       'WriteBatch.set',
       ref._key,
       convertedValue,
-      options
+      options,
+      ref._converter !== null
     );
     this._mutations = this._mutations.concat(
       parsed.toMutations(ref._key, Precondition.none())

packages/firestore/src/api/database.ts

@@ -744,17 +744,17 @@ export class Transaction implements firestore.Transaction {
       this._firestore
     );
     options = validateSetOptions('Transaction.set', options);
-    const [convertedValue, functionName] = applyFirestoreDataConverter(
+    const convertedValue = applyFirestoreDataConverter(
       ref._converter,
       value,
-      'Transaction.set',
       options
     );
     const parsed = this._firestore._dataReader.parseSetData(
-      functionName,
+      'Transaction.set',
       ref._key,
       convertedValue,
-      options
+      options,
+      ref._converter !== null
     );
     this._transaction.set(ref._key, parsed);
     return this;
@@ -851,17 +851,17 @@ export class WriteBatch implements firestore.WriteBatch {
       this._firestore
     );
     options = validateSetOptions('WriteBatch.set', options);
-    const [convertedValue, functionName] = applyFirestoreDataConverter(
+    const convertedValue = applyFirestoreDataConverter(
       ref._converter,
       value,
-      'WriteBatch.set',
       options
     );
     const parsed = this._firestore._dataReader.parseSetData(
-      functionName,
+      'WriteBatch.set',
       ref._key,
       convertedValue,
-      options
+      options,
+      ref._converter !== null
     );
     this._mutations = this._mutations.concat(
       parsed.toMutations(ref._key, Precondition.none())
@@ -1051,17 +1051,17 @@ export class DocumentReference<T = firestore.DocumentData>
   set(value: T | Partial<T>, options?: firestore.SetOptions): Promise<void> {
     validateBetweenNumberOfArgs('DocumentReference.set', arguments, 1, 2);
     options = validateSetOptions('DocumentReference.set', options);
-    const [convertedValue, functionName] = applyFirestoreDataConverter(
+    const convertedValue = applyFirestoreDataConverter(
       this._converter,
       value,
-      'DocumentReference.set',
       options
     );
     const parsed = this.firestore._dataReader.parseSetData(
-      functionName,
+      'DocumentReference.set',
       this._key,
       convertedValue,
-      options
+      options,
+      this._converter !== null
     );
     return this._firestoreClient.write(
       parsed.toMutations(this._key, Precondition.none())
@@ -2597,9 +2597,8 @@ function resultChangeType(type: ChangeType): firestore.DocumentChangeType {
 export function applyFirestoreDataConverter<T>(
   converter: UntypedFirestoreDataConverter<T> | null,
   value: T,
-  functionName: string,
   options?: firestore.SetOptions
-): [firestore.DocumentData, string] {
+): firestore.DocumentData {
   let convertedValue;
   if (converter) {
     if (options && (options.merge || options.mergeFields)) {
@@ -2610,11 +2609,10 @@ export function applyFirestoreDataConverter<T>(
     } else {
       convertedValue = converter.toFirestore(value);
     }
-    functionName = 'toFirestore() in ' + functionName;
   } else {
     convertedValue = value as firestore.DocumentData;
   }
-  return [convertedValue, functionName];
+  return convertedValue;
 }
 
 function contains(obj: object, key: string): obj is { key: unknown } {

packages/firestore/src/api/user_data_reader.ts

@@ -173,6 +173,11 @@ interface ContextSettings {
    * If not set, elements are treated as if they were outside of arrays.
    */
   readonly arrayElement?: boolean;
+  /**
+   * Whether or not a converter was specified in this context. If set, error
+   * messages will reference the converter when invalid data is provided.
+   */
+  readonly hasConverter?: boolean;
 }
 
 /** A "context" object passed around while parsing user data. */
@@ -259,7 +264,8 @@ export class ParseContext {
       reason,
       this.settings.methodName,
       this.path,
-      this.settings.targetDoc
+      this.settings.targetDoc,
+      this.settings.hasConverter
     );
   }
 
@@ -314,14 +320,16 @@ export class UserDataReader {
     methodName: string,
     targetDoc: DocumentKey,
     input: unknown,
-    options: firestore.SetOptions = {}
+    options: firestore.SetOptions = {},
+    hasConverter: boolean
   ): ParsedSetData {
     const context = this.createContext(
       options.merge || options.mergeFields
         ? UserDataSource.MergeSet
         : UserDataSource.Set,
       methodName,
-      targetDoc
+      targetDoc,
+      hasConverter
     );
     validatePlainObject('Data must be an object, but it was:', context, input);
     const updateData = parseObject(input, context)!;
@@ -494,15 +502,17 @@ export class UserDataReader {
   private createContext(
     dataSource: UserDataSource,
     methodName: string,
-    targetDoc?: DocumentKey
+    targetDoc?: DocumentKey,
+    hasConverter = false
   ): ParseContext {
     return new ParseContext(
       {
         dataSource,
         methodName,
         targetDoc,
         path: FieldPath.emptyPath(),
-        arrayElement: false
+        arrayElement: false,
+        hasConverter
       },
       this.databaseId,
       this.serializer,
@@ -803,10 +813,16 @@ function createError(
   reason: string,
   methodName: string,
   path?: FieldPath,
-  targetDoc?: DocumentKey
+  targetDoc?: DocumentKey,
+  hasConverter = false
 ): Error {
   const hasPath = path && !path.isEmpty();
   const hasDocument = targetDoc !== undefined;
+  let message = `Function ${methodName}() called with invalid data`;
+  if (hasConverter) {
+    message += ' (via `toFirestore()`)';
+  }
+  message += '. ';
 
   let description = '';
   if (hasPath || hasDocument) {
@@ -823,7 +839,7 @@ function createError(
 
   return new FirestoreError(
     Code.INVALID_ARGUMENT,
-    `Function ${methodName}() called with invalid data. ` + reason + description
+    message + reason + description
   );
 }
 

packages/firestore/test/integration/api/database.test.ts

@@ -1379,9 +1379,9 @@ apiDescribe('Database', (persistence: boolean) => {
         expect(() =>
           batch.set(ref, { title: 'olive' }, { merge: true })
         ).to.throw(
-          'Function toFirestore() in WriteBatch.set() called with invalid ' +
-            'data. Unsupported field value: undefined (found in field author ' +
-            'in document posts/some-post)'
+          'Function WriteBatch.set() called with invalid ' +
+            'data (via `toFirestore()`). Unsupported field value: undefined ' +
+            '(found in field author in document posts/some-post)'
         );
       });
     });