DocumentReference<T>.update() is wrongly parameterized

[oxc]

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

2. 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

3. 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.

[tom-andersen]

Thank you for bringing this to our attention. I will take a closer look.

[swftvsn]

Any news on this? It is blocking our firebase-admin upgrade.

[justinfagnani]

Option 2 would be wonderful here, IMO.

[PieterDexper]

There's another issue with the UpdateData typing as well, it can't handle mapped types.

Environment:

• [email protected]
• @google-cloud/[email protected]

Given this type as a model for the data:

type TestType = {
    foo: {
        [key: string]: {
            bar: string;
        }
    }
}

The UpdateData type doesn't accurately reflect allowed values:

const firestore = getFirestore();
const key = "aKey";
const ref: DocumentReference<TestType> = firestore.doc("testCollection/testDoc") as unknown as DocumentReference<TestType>;
ref.update({
    [`foo.${key}.bar`]: "test",
});

resulting in:

Argument of type '{ [x: string]: string; }' is not assignable to parameter of type '{ foo?: FieldValue | ({ [x: string]: { bar?: string | FieldValue | undefined; } | FieldValue | undefined; } & AddPrefixToKeys<string, { bar?: string | FieldValue | undefined; }>) | undefined; } & AddPrefixToKeys<...>'.
  Type '{ [x: string]: string; }' is not assignable to type 'AddPrefixToKeys<"foo", { [x: string]: { bar?: string | FieldValue | undefined; } | FieldValue | undefined; } & AddPrefixToKeys<string, { bar?: string | FieldValue | undefined; }>>'.
    'string' and '`foo.${string}`' index signatures are incompatible.
      Type 'string' is not assignable to type '{ bar?: string | FieldValue | undefined; } | FieldValue | undefined'.ts(2345)

The ${string} halfway the path matches everything that follows, including values after the next dot. That forces the type of foo.${string}.bar to be a union of the types of foo.anything.bar and foo.anything.

To accurately parse this construct Typescript would need to have regex or similar typing that can narrow down ${string} to ${'any valid key without dots'}, but that is not supported by the language afaik.

[swftvsn]

@PieterDexper This is exactly our problem too. We ended up using really suboptimal way to resolve it:

// TODO: FIXME: DELETE as any CAST WHEN TYPES ARE FIXED!
...update(valueObject as any)

Fortunately we have only handful of these for now, so it's manageable.

[swftvsn]

(And while we're at it, could we please have firestore.doc<T>(...): DocumentReference<T>)

[dconeybe]

@swftvsn Can you please open a separate issue to request firestore.doc<T>(...): DocumentReference<T>)

[swftvsn]

@swftvsn Can you please open a separate issue to request firestore.doc<T>(...): DocumentReference<T>)

Created #1792

[FBuervenich]

@dconeybe Sorry to bump this but could you give us a rough estimation on when/if this will be approached further?
Would be nice to decide wether or not it's worth it to build a custom solution around the problem (that is not just casting the update-data to any).

[MarkDuckworth]

@FBuervenich, I can't give a commitment or a timeline to deliver this. However, we have been investigating it. I will say that it needs to be treated as a breaking change and included in a major version release.

[Gsiete]

Good to know I'm not the only one. Luckily I can use .set({ field: true }, { merge: true })

[0xJem]

I ran into this problem, too. Neither the function docs nor published documentation mention this limitation, which can affect the consistency of data in the DB! I highly recommend updating the docs to mention this limitation.

[VictorLeach96]

Do we have to wait for the fix made in firebase/firebase-js-sdk to be released here?

[dconeybe]

Do we have to wait for the fix made in firebase/firebase-js-sdk to be released here?

Yes. We are planning to have the change released here in the coming months, but, unfortunately, it's not our top priority and we cannot promise any specific release date. We also have to release it carefully because it is a "breaking API change" and requires a major version upgrade. We will post here once the fix is released.

[VictorLeach96]

Looks like a fix for this was pushed out, but I'm still getting an error when assigning a whole object to the property.

Type 'Record<string, { id: string; }>' is not assignable to type 'FieldValue | ({ [x: string]: { id?: string | FieldValue | undefined; } | FieldValue | undefined; } & AddPrefixToKeys<string, { id?: string | FieldValue | undefined; }>) | undefined'.
  Property 'isEqual' is missing in type 'Record<string, { id: string; }>' but required in type 'FieldValue'.

const update: firestore.UpdateData<{ prop: Record<string, { id: string }> }> = {}

const value: Record<string, { id: string }> = {
  key: { id: '' }
}

update.prop = value

[MarkDuckworth]

This issue was fixed in commit #1887 and released in v7.0.0. CollectionReference and DocumentReference have been updated to have two type parameters, one for the AppModelType and one for the DbModelType.

class CollectionReference<AppModelType = DocumentData, DbModelType extends DocumentData = DocumentData>;

class DocumentReference<AppModelType = DocumentData, DbModelType extends DocumentData = DocumentData>;

The DocumentReference.update(data: UpdateData<DbModelType>), now operates on the DbModelType.

[MarkDuckworth]

@VictorLeach96, the issue you reported using UpdateData with index signature types is covered by #1890.