flutterfire icon indicating copy to clipboard operation
flutterfire copied to clipboard

🐛 [firestore] BatchWrite.update does not implement type-safe read & write operations.

Open paul-appliquette opened this issue 2 years ago • 1 comments

Bug report

WriteBatch.update does not implement withConverter Type handling.

This seems like a bug to me since WriteBatch.set does implement withConverter Type handling, and it appears that implementing the same in the update method has been overlooked.

Some may say this should be a Feature Request rather than a Bug Report. If that turns out to be the general consensus I'll change it.

Steps to reproduce

Steps to reproduce the behavior:

  1. Use a Flutter app configured for Firestore access.
  2. Define withConverter ToFirestore & FromFirestore functions to provide type-safe read & write operations for a data class.
  3. Attempt to perform BatchWrite.set and BatchWrite.update using withConverter and supplying the relevant data Object.

Result

BatchWrite.set has no issues. BatchWrite.update results in error "The argument type '[Object]' can't be assigned to the parameter type 'Map<String, dynamic>'."


Expected behavior

Both BatchWrite.set and BatchWrite.update using withConverter should be allowed.


Additional context

Current implementation for BatchWrite.set:

void set<T>(
  DocumentReference<T> document,
  T data, [
  SetOptions? options,
]) {
  assert(
    document.firestore == _firestore,
    'the document provided is from a different Firestore instance',
  );

  Map<String, dynamic> firestoreData;
  if (document is _JsonDocumentReference) {
    firestoreData = data as Map<String, dynamic>;
  } else {
    final withConverterDoc = document as _WithConverterDocumentReference<T>;
    firestoreData = withConverterDoc._toFirestore(data, options);
  }

  return _delegate.set(
    document.path,
    _CodecUtility.replaceValueWithDelegatesInMap(firestoreData)!,
    options,
  );
}

Current implementation for BatchWrite.update:

void update(DocumentReference document, Map<String, dynamic> data) {
  assert(
    document.firestore == _firestore,
    'the document provided is from a different Firestore instance',
  );
  return _delegate.update(
    document.path,
    _CodecUtility.replaceValueWithDelegatesInMap(data)!,
  );
}

paul-appliquette avatar Sep 05 '23 04:09 paul-appliquette

Thanks for the report. Treating this as an enhancement to support set and update to support withConverter.

/cc @Lyokone

darshankawar avatar Sep 05 '23 10:09 darshankawar