Skip to content
This repository was archived by the owner on Apr 28, 2026. It is now read-only.

batchSet and transactionSet should rely on castedReference like set - #38

Open
gmarizy wants to merge 3 commits into
FirebaseExtended:mainfrom
gmarizy:fix/batchSet_with_castedReference
Open

batchSet and transactionSet should rely on castedReference like set#38
gmarizy wants to merge 3 commits into
FirebaseExtended:mainfrom
gmarizy:fix/batchSet_with_castedReference

Conversation

@gmarizy

@gmarizy gmarizy commented Dec 10, 2024

Copy link
Copy Markdown
Contributor

Fixes #37

In generated code, relying on batch.set(reference, json, options); for batchSet implementation is wrong because we pass a DocumentReference<T> with T a mapping class as a reference with a json data. Cloud_firestore then try to apply the converter but data is already in json. See write_batch.dart from cloud_firestore, line 56.

batch.set(reference, json, options);

Should be replaced with :

    final castedReference = reference.withConverter<Map<String, dynamic>>(
      fromFirestore: (snapshot, options) => throw UnimplementedError(),
      toFirestore: (value, options) => value,
    );

    batch.set(castedReference, json, options);

Like it's done for set method. Same for transaction.

  • Tests pass
  • Appropriate changes to README are included in PR

@rrousselGit

Copy link
Copy Markdown
Contributor

Thanks! Could you add tests for those?

batchSet and transactionSet code generation changed
@gmarizy

gmarizy commented Dec 10, 2024

Copy link
Copy Markdown
Contributor Author

Batch and transaction set are already covered in example/integration_test/document_reference_test.dart. I wanted to run those tests before and after fix for validation, but didn't succeed running them.

@gmarizy

gmarizy commented Dec 12, 2024

Copy link
Copy Markdown
Contributor Author

I should have mentioned that if I couldn't run the test suit, the proposed fix work fine on real projects.

@andreasmpet

Copy link
Copy Markdown

Any progress on this? Having this exact issue now where I wanted to use batchSet and it wouldn't work.

@gmarizy

gmarizy commented Feb 27, 2025

Copy link
Copy Markdown
Contributor Author

Still waiting review

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Batches and transactions broken

3 participants