Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

batchSet and transactionSet should rely on castedReference like set #38

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gmarizy
Copy link
Contributor

@gmarizy gmarizy commented Dec 10, 2024

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
Contributor

Thanks! Could you add tests for those?

batchSet and transactionSet code generation changed
@gmarizy
Copy link
Contributor Author

gmarizy commented Dec 10, 2024

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
Copy link
Contributor Author

gmarizy commented Dec 12, 2024

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

@andreasmpet
Copy link

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

@gmarizy
Copy link
Contributor Author

gmarizy commented Feb 27, 2025

Still waiting review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batches and transactions broken
3 participants