nodejs-firestore: DocumentReference.update() is wrongly parameterized

Environment details

• @google-cloud/firestore version: 5.0.2

Issue description

In #1595, the type of the DocumentReference.update() method was updated to get a UpdateData<T> instead of the previously unparameterized UpdateData.

This was presumably done to make the method type-safe and more consistent with get() and set() methods.

However, the parameter T does not refer to the database format of the objects, but to the “converted” format that is returned by the converter’s fromFirestore method (if any is configured). While this makes sense for get() and set(), since they call the appropriate converter methods, the same does not hold true for the update() method. The update() method does not call the configured converter, and instead operates directly on the database values (see reference#update() which delegates to write-batch#update).

This means that the newly introduced parameter to UpdateData should reflect the storage format of the collection, not the mapped type defined by the converter. Unfortunately, such a type is currently not present in the library’s types.

Demonstration

To demonstrate the issue, consider this fictitious collection where the data is stored as an object with a count: number property, but the converter converts it to a progress: string consisting of count asterisks:

type DatabaseFormat = {
  'foo': string;
  'count': number;
}

type EntityFormat = {
  'foo': string;
  'progress': string;
}

const collection = firestore.collection('update-type-problem').withConverter(<FirestoreDataConverter<EntityFormat>> {
  fromFirestore: (snapshot: QueryDocumentSnapshot<DatabaseFormat>): EntityFormat => {
    const data = snapshot.data();
    return {
      foo: data.foo,
      progress: Array(data.count).fill('*').join(''),
    };
  },
  toFirestore: (modelObject: PartialWithFieldValue<EntityFormat>): PartialWithFieldValue<DatabaseFormat> => {
    const result: PartialWithFieldValue<DatabaseFormat> = {};
    if (modelObject.foo) {
      result.foo = modelObject.foo;
    }
    if (modelObject.progress) {
      result.count = modelObject.progress instanceof FieldValue
        ? modelObject.progress // as FieldValue
        : modelObject.progress.length;
    }
    return result;
  },
});

Now let’s operate on a document foo in that collection:

Setting works as expected

  const ref = collection.doc('foo');

  await ref.set({
    foo: 'bar',
    progress: '****',
  });
  // collection contains now: foo: { foo: 'bar', count: 4 }

Setting only the progress property to a new value (with merge:true) works just as well:

  await ref.set({
    progress: '******',
  }, { merge: true });
  // collection contains now: foo: { foo: 'bar', count: 6 }

Using a increment on the property works just the same, even though it sounds a bit strange to increase the progress, but the converter hands that through to the count property:

  await ref.set({
    progress: FieldValue.increment(2),
  }, { merge: true });
  // collection contains now: foo: { foo: 'bar', count: 8 }

  console.log(await ref.get().then((doc) => doc.data()));
  // { foo: 'bar', progress: '******' }

⚠️ But now with the update method it becomes tricky:

  // this is actually correct, but the new types forbid it.
  /*
    without a cast to any, it yields:
  
    TS2345: Argument of type '{ count: firestore.FieldValue; }' is not assignable to parameter of type '{ foo?: string | FieldValue | undefined; progress?: string | FieldValue | undefined; }'.   Object literal may only specify known properties, and 'count' does not exist in type '{ foo?: string | FieldValue | undefined; progress?: string | FieldValue | undefined; }'.
  */
  await ref.update({
    count: FieldValue.increment(3),
  } as any);

  // collection contains now: foo: { foo: 'bar', count: 11 }

⚠️ If we use the expected type, it gets even more wrong:

  await ref.update({
    progress: 'broken',
  });
  // collection contains now: foo: { foo: 'bar', count: 11, progress: 'broken' }

The database entry now contains a property progress, which should not exist on that level.

Proposed solution:

1. Revert the changes to the update-Method and UpdateData type, as they are incorrect

and/or

1. Consider introducing a second type on the Collection/Reference/Document/Snapshot, a DatabaseFormat type (which needs to be specified manually on the collection, and defaults to DocumentData), in addition to the existing optional EntityFormat type (which is taken from the Converter, and defaults to the DatabaseFormat)

or

1. Try to modify the update method to work on the Entity type and use the configured converter. This would greatly increase the complexity of any converter, since they would have to handle not only FieldValues, but also FieldPaths etc.