Feature/fix aggregate issue with converted queries #291
Conversation
| @@ -0,0 +1,16 @@ | |||
| import 'package:cloud_firestore_platform_interface/cloud_firestore_platform_interface.dart'; | |||
|
|
|||
| extension AggregateTypeExtension on AggregateType { | |||
lib/src/fake_aggregate_query.dart
Outdated
| @@ -28,10 +31,18 @@ class FakeAggregateQuery implements AggregateQuery { | |||
| @override | |||
| AggregateQuery count() => _query.count(); | |||
|
|
|||
| Map<String, dynamic> _getRawDocDataMap(QueryDocumentSnapshot<Object?> s) { | |||
| if (s is MockQueryDocumentSnapshot && s.snapshot is MockDocumentSnapshot) { | |||
There was a problem hiding this comment.
Why not use an assert statement like you did in the other PR? flutter test does fail if the assert fails. If something does fails for unexpected reasons, let it fail so we can fix it properly next time. :)
There was a problem hiding this comment.
Oh, it's because I want to check the type with the is operator so that we can use the instance method without casting it again.
Let me think and revise it. ;)
There was a problem hiding this comment.
after using DocumentSnapshot.get we don't need this method anymore, haha. Thank you!
lib/src/fake_aggregate_query.dart
Outdated
| AggregateQuerySnapshotPlatform _getAggregateQuerySnapshotPlatform({ | ||
| required QuerySnapshot<Object?> snapshot, | ||
| }) { | ||
| final dataMaps = snapshot.docs.map((e) => e.data() as Map<String, dynamic>); | ||
| final dataMaps = snapshot.docs.map((s) => _getRawDocDataMap(s)); |
There was a problem hiding this comment.
Have you considered using DocumentSnapshot.get? Then you don't need to expose the internal map. It's better to use the public API whenever possible.
You would replace
final value = dataMap[field.field];by
final value = documentSnapshot.get(field.field);It also has the added benefit to make the loop down there clearer. Instead of this:
for (final dataMap in dataMaps) {
for (final field in fields) {you could write:
for (final field in fields) {
for (final documentSnapshot in documentSnapshots) {Also would you mind renaming valueMap into something more meaningful like aggregateValues? Since you already use the variable name value when referring to a document value, value and valueMap was a bit confusing.
|
@atn832 Thanks for the review. |
|
I published your fix to https://pub.dev/packages/fake_cloud_firestore/changelog#247. I also switched to Dart 3 and used pattern matching to rewrite https://dart.dev/language/patterns#switch-statements-and-expressions |
This PR has fixed the aggregate query problem on converted queries.
#290 (comment)
The error log would be like: