googleapis / nodejs-firestore

Node.js client for Google Cloud Firestore: a NoSQL document database built for automatic scaling, high performance, and ease of application development.
https://cloud.google.com/firestore/
Apache License 2.0
641 stars 149 forks source link

Firestore: CollectionReference.withConverter(converter).add(data) invokes converter.toFirestore twice #2173

Open CaptainCodeman opened 1 month ago

CaptainCodeman commented 1 month ago

The toFirestore method of converter is called multiple times when adding a new document to a collection which can cause issues if the object is mutated as part of the conversion (it seems reasonable to assume it should only be called once).

Similar to this client-side issue: https://github.com/firebase/firebase-js-sdk/issues/3742

Environment details

Steps to reproduce

Use the .add method on a collection ref with a converter, mutating the object:

const testConverter = {
  toFirestore(test) {
    test.hash = Blob.fromBase64String(test.hash)
    return test
  },
  fromFirestore(snapshot, options) {
    const data = snapshot.data(options)!;
    data.hash = data.hash.toBase64()
    return data
  }
};

const ref = await firestore
        .collection(`test`)
        .withConverter(testConverter)
        .add({ name: 'test', hash: 'some-base64-string-that-we-want-to-store-as-a-blob' })

The converter would work fine for regular set, and update methods, but .add causes .toFirestore to be called twice. Although it's a good idea for the converters to use immutable patterns which would avoid this, it's an easy mistake to just mutate the existing object which can cause runtime errors or data corruption and if nothing else is wasted extra work.