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

Feature/support more aggregate queries #290

Merged
merged 5 commits into from
Jan 9, 2024

Conversation

ming-chu
Copy link
Contributor

@ming-chu ming-chu commented Jan 5, 2024

Hi @atn832,

This PR is going to support aggregate queries on getSum and getAverage for cloud_firestore: ^4.14.0.
Please feel free to comment, any code refactors or edits are welcome.

Please let me know if any test cases are missed, hope this PR can help.

#289

required AggregateType type,
}) {
final nonNullFields = fields.whereNotNull();
switch (type) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using Dart 3.0's new Switch expressions but sadly it didn't work because the value wasn't a type 😞 .

final fieldType = switch (type) {
  AggregateType.sum => platform_interface.sum,
  AggregateType.average => platform_interface.average,
  AggregateType.count => platform_interface.count,
  _ => throw UnimplementedError('Unknown AggregateType: $type')
};
final nonNullFields = fields.whereNotNull();
// compiler says: The name 'fieldType' isn't a type, so it can't be used as a type argument.
// Try correcting the name to an existing type, or defining a type named 'fieldType'.
return nonNullFields.whereType<fieldType>();

https://dart.dev/language/branches#switch-expressions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a little search about it, seems we cannot pass the value as Generic Type
https://stackoverflow.com/questions/66021661/how-can-i-cast-a-variable-to-generic-type-in-dart

Copy link
Contributor Author

@ming-chu ming-chu Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, maybe we could use where instead:

return nonNullFields.where((e) => e.runtimeType == fieldType);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using Dart 3.0's new Switch expressions but sadly it didn't work because the value wasn't a type 😞 .

final fieldType = switch (type) {
  AggregateType.sum => platform_interface.sum,
  AggregateType.average => platform_interface.average,
  AggregateType.count => platform_interface.count,
  _ => throw UnimplementedError('Unknown AggregateType: $type')
};
final nonNullFields = fields.whereNotNull();
// compiler says: The name 'fieldType' isn't a type, so it can't be used as a type argument.
// Try correcting the name to an existing type, or defining a type named 'fieldType'.
return nonNullFields.whereType<fieldType>();

https://dart.dev/language/branches#switch-expressions

That's good to simplify the code which makes it easier to read. Nice!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, maybe we could use where instead:

return nonNullFields.where((e) => e.runtimeType == fieldType);

Oh! Good idea

AggregateQuerySnapshotPlatform _getAggregateQuerySnapshotPlatform({
required QuerySnapshot<Object?> snapshot,
}) {
final dataMaps = snapshot.docs.map((e) => e.data() as Map<String, dynamic>);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work for converted queries.

In fake_cloud_firestore_test.dart, I added the 'crew' integer field:

class Movie {
  late String title;
  late int crew;
}

final from =
    (DocumentSnapshot<Map<String, dynamic>> snapshot, _) => snapshot.exists
        ? (Movie()
          ..title = snapshot['title']
          ..crew = snapshot['crew'])
        : null;
final to = (Movie? movie, _) => movie == null
    ? <String, Object?>{}
    : {
        'title': movie.title,
        'crew': movie.crew,
      };

Then in mock_query_test.dart, I added this test case:

test('with Converters', () async {
  final firestore = FakeFirebaseFirestore();
  final movies = firestore
      .collection('movies')
      .withConverter(fromFirestore: from, toFirestore: to);
  await movies.add(Movie()
    ..title = 'Best Movie'
    ..crew = 30);
  await movies.add(Movie()
    ..title = 'Worst Movie'
    ..crew = 20);
  final snapshot =
      await movies.aggregate(sum('crew'), average('crew')).get();
  expect(snapshot.count, 2);
  expect(snapshot.getSum('crew'), 50);
  expect(snapshot.getAverage('crew'), 25);
});

I got:

type 'Movie' is not a subtype of type 'Map<String, dynamic>' in type cast
package:fake_cloud_firestore/src/fake_aggregate_query.dart 34:56   FakeAggregateQuery._getAggregateQuerySnapshotPlatform.<fn>
dart:_internal                                                     ListIterator.moveNext
package:fake_cloud_firestore/src/fake_aggregate_query.dart 107:27  FakeAggregateQuery.buildAggregateQueryResponseList
package:fake_cloud_firestore/src/fake_aggregate_query.dart 37:12   FakeAggregateQuery._getAggregateQuerySnapshotPlatform
package:fake_cloud_firestore/src/fake_aggregate_query.dart 21:22   FakeAggregateQuery.get

I'll merge this because it works well for non-converted queries. We can tackle converted queries in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atn832 I just made another PR to fix this problem, but I am not sure that's a good way to fix it. Please advise.
Thank you very much!😃
#291

@@ -1622,6 +1622,70 @@ void main() {
});
});

group('aggregate queries', () {
test('should return null for unspecified sum field', () async {
final firestore = FakeFirebaseFirestore();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest using setUpAll, but then I saw your last test case, which uses different data.

late FakeFirebaseFirestore firestore;
late CollectionReference collection;

setUpAll(() async {
  firestore = FakeFirebaseFirestore();
  collection = firestore.collection('firestore-example-app');

  await collection.add({'name': 'Banana', 'likes': 1, 'dislikes': 4});
  await collection.add({'name': 'Avocado', 'likes': 2, 'dislikes': 5});
  await collection.add({'name': 'Mango', 'likes': 3, 'dislikes': 6});
});

test('should return null for unspecified sum field', () async {
  final _sum = await collection.aggregate(sum('likes')).get();
  expect(_sum.getSum('dislikes'), isNull);
  expect(_sum.count, 3);
});

Copy link
Owner

@atn832 atn832 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR. It does not handle converted queries but it covers non-converted ones nicely. Thank you!

@atn832 atn832 merged commit 6a75ee4 into atn832:master Jan 9, 2024
4 checks passed
@ming-chu
Copy link
Contributor Author

ming-chu commented Jan 9, 2024

@atn832 Sorry I missed the converted queries, maybe we could create an issue to follow up later 🙇🏻

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.

None yet

2 participants