fix(firestore): aggregate query average() is null when collection…

… is empty or collection doesn't exist or the property doesn't exist on docs (#12304)
firebase · Feb 8, 2024 · 4d3b578 · 4d3b578
1 parent 1fc89b9
commit 4d3b578
Show file tree

Hide file tree

Showing 12 changed files with 77 additions and 43 deletions.
diff --git a/...d/src/main/java/io/flutter/plugins/firebase/firestore/FlutterFirebaseFirestorePlugin.java b/...d/src/main/java/io/flutter/plugins/firebase/firestore/FlutterFirebaseFirestorePlugin.java
@@ -811,8 +811,7 @@ public void aggregateQuery(
                       new GeneratedAndroidFirebaseFirestore.AggregateQueryResponse.Builder();
                   builderAverage.setType(GeneratedAndroidFirebaseFirestore.AggregateType.AVERAGE);
                   builderAverage.setValue(
-                      Objects.requireNonNull(
-                          aggregateQuerySnapshot.get(average(queryRequest.getField()))));
+                      aggregateQuerySnapshot.get(average(queryRequest.getField())));
                   builderAverage.setField(queryRequest.getField());
 
                   aggregateResponse.add(builderAverage.build());

diff --git a/...rc/main/java/io/flutter/plugins/firebase/firestore/GeneratedAndroidFirebaseFirestore.java b/...rc/main/java/io/flutter/plugins/firebase/firestore/GeneratedAndroidFirebaseFirestore.java
@@ -1514,16 +1514,13 @@ public void setField(@Nullable String setterArg) {
       this.field = setterArg;
     }
 
-    private @NonNull Double value;
+    private @Nullable Double value;
 
-    public @NonNull Double getValue() {
+    public @Nullable Double getValue() {
       return value;
     }
 
-    public void setValue(@NonNull Double setterArg) {
-      if (setterArg == null) {
-        throw new IllegalStateException("Nonnull field \"value\" is null.");
-      }
+    public void setValue(@Nullable Double setterArg) {
       this.value = setterArg;
     }
 
@@ -1548,7 +1545,7 @@ public static final class Builder {
 
       private @Nullable Double value;
 
-      public @NonNull Builder setValue(@NonNull Double setterArg) {
+      public @NonNull Builder setValue(@Nullable Double setterArg) {
         this.value = setterArg;
         return this;
       }

diff --git a/packages/cloud_firestore/cloud_firestore/example/integration_test/query_e2e.dart b/packages/cloud_firestore/cloud_firestore/example/integration_test/query_e2e.dart
@@ -3881,6 +3881,18 @@ void runQueryTests() {
           );
         },
       );
+
+      testWidgets('count(), average() & sum() on empty collection',
+          (widgetTester) async {
+        final collection = await initializeTest('empty-collection');
+
+        final snapshot = await collection
+            .aggregate(count(), sum('foo'), average('foo'))
+            .get();
+        expect(snapshot.count, 0);
+        expect(snapshot.getSum('foo'), 0);
+        expect(snapshot.getAverage('foo'), null);
+      });
     });
 
     group('startAfterDocument', () {

diff --git a/packages/cloud_firestore/cloud_firestore/ios/Classes/FLTFirebaseFirestorePlugin.m b/packages/cloud_firestore/cloud_firestore/ios/Classes/FLTFirebaseFirestorePlugin.m
@@ -796,30 +796,41 @@ - (void)aggregateQueryApp:(nonnull FirestorePigeonFirebaseApp *)app
                          break;
                        }
                        case AggregateTypeSum: {
-                         double doubleValue = [[snapshot
+                         NSNumber *value = [snapshot
                              valueForAggregateField:[FIRAggregateField
                                                         aggregateFieldForSumOfField:[queryRequest
-                                                                                        field]]]
-                             doubleValue];
+                                                                                        field]]];
 
                          [aggregateResponses
                              addObject:[AggregateQueryResponse
                                            makeWithType:AggregateTypeSum
                                                   field:queryRequest.field
-                                                  value:[NSNumber numberWithDouble:doubleValue]]];
+                                                  // This passes either a double (wrapped in
+                                                  // NSNumber) or null value
+                                                  value:value != ((id)[NSNull null])
+                                                            ? [NSNumber
+                                                                  numberWithDouble:[value
+                                                                                       doubleValue]]
+                                                            : value]];
                          break;
                        }
                        case AggregateTypeAverage: {
-                         double doubleValue = [[snapshot
-                             valueForAggregateField:[FIRAggregateField
-                                                        aggregateFieldForAverageOfField:
-                                                            [queryRequest field]]] doubleValue];
+                         NSNumber *value = [snapshot
+                             valueForAggregateField:
+                                 [FIRAggregateField
+                                     aggregateFieldForAverageOfField:[queryRequest field]]];
 
                          [aggregateResponses
                              addObject:[AggregateQueryResponse
                                            makeWithType:AggregateTypeAverage
                                                   field:queryRequest.field
-                                                  value:[NSNumber numberWithDouble:doubleValue]]];
+                                                  // This passes either a double (wrapped in
+                                                  // NSNumber) or null value
+                                                  value:value != ((id)[NSNull null])
+                                                            ? [NSNumber
+                                                                  numberWithDouble:[value
+                                                                                       doubleValue]]
+                                                            : value]];
                          break;
                        }
                      }

diff --git a/packages/cloud_firestore/cloud_firestore/ios/Classes/FirestoreMessages.g.m b/packages/cloud_firestore/cloud_firestore/ios/Classes/FirestoreMessages.g.m
@@ -595,7 +595,7 @@ - (NSArray *)toList {
 @implementation AggregateQueryResponse
 + (instancetype)makeWithType:(AggregateType)type
                        field:(nullable NSString *)field
-                       value:(NSNumber *)value {
+                       value:(nullable NSNumber *)value {
   AggregateQueryResponse *pigeonResult = [[AggregateQueryResponse alloc] init];
   pigeonResult.type = type;
   pigeonResult.field = field;
@@ -607,7 +607,6 @@ + (AggregateQueryResponse *)fromList:(NSArray *)list {
   pigeonResult.type = [GetNullableObjectAtIndex(list, 0) integerValue];
   pigeonResult.field = GetNullableObjectAtIndex(list, 1);
   pigeonResult.value = GetNullableObjectAtIndex(list, 2);
-  NSAssert(pigeonResult.value != nil, @"");
   return pigeonResult;
 }
 + (nullable AggregateQueryResponse *)nullableFromList:(NSArray *)list {

diff --git a/packages/cloud_firestore/cloud_firestore/ios/Classes/Public/FirestoreMessages.g.h b/packages/cloud_firestore/cloud_firestore/ios/Classes/Public/FirestoreMessages.g.h
@@ -284,10 +284,10 @@ typedef NS_ENUM(NSUInteger, AggregateType) {
 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)makeWithType:(AggregateType)type
                        field:(nullable NSString *)field
-                       value:(NSNumber *)value;
+                       value:(nullable NSNumber *)value;
 @property(nonatomic, assign) AggregateType type;
 @property(nonatomic, copy, nullable) NSString *field;
-@property(nonatomic, strong) NSNumber *value;
+@property(nonatomic, strong, nullable) NSNumber *value;
 @end
 
 /// The codec used by FirebaseFirestoreHostApi.

diff --git a/packages/cloud_firestore/cloud_firestore/windows/messages.g.cpp b/packages/cloud_firestore/cloud_firestore/windows/messages.g.cpp
@@ -939,16 +939,15 @@ AggregateQuery AggregateQuery::FromEncodableList(const EncodableList& list) {
 
 // AggregateQueryResponse
 
-AggregateQueryResponse::AggregateQueryResponse(const AggregateType& type,
-                                               double value)
-    : type_(type), value_(value) {}
+AggregateQueryResponse::AggregateQueryResponse(const AggregateType& type)
+    : type_(type) {}
 
 AggregateQueryResponse::AggregateQueryResponse(const AggregateType& type,
                                                const std::string* field,
-                                               double value)
+                                               const double* value)
     : type_(type),
       field_(field ? std::optional<std::string>(*field) : std::nullopt),
-      value_(value) {}
+      value_(value ? std::optional<double>(*value) : std::nullopt) {}
 
 const AggregateType& AggregateQueryResponse::type() const { return type_; }
 
@@ -968,7 +967,13 @@ void AggregateQueryResponse::set_field(std::string_view value_arg) {
   field_ = value_arg;
 }
 
-double AggregateQueryResponse::value() const { return value_; }
+const double* AggregateQueryResponse::value() const {
+  return value_ ? &(*value_) : nullptr;
+}
+
+void AggregateQueryResponse::set_value(const double* value_arg) {
+  value_ = value_arg ? std::optional<double>(*value_arg) : std::nullopt;
+}
 
 void AggregateQueryResponse::set_value(double value_arg) { value_ = value_arg; }
 
@@ -977,18 +982,21 @@ EncodableList AggregateQueryResponse::ToEncodableList() const {
   list.reserve(3);
   list.push_back(EncodableValue((int)type_));
   list.push_back(field_ ? EncodableValue(*field_) : EncodableValue());
-  list.push_back(EncodableValue(value_));
+  list.push_back(value_ ? EncodableValue(*value_) : EncodableValue());
   return list;
 }
 
 AggregateQueryResponse AggregateQueryResponse::FromEncodableList(
     const EncodableList& list) {
-  AggregateQueryResponse decoded((AggregateType)(std::get<int32_t>(list[0])),
-                                 std::get<double>(list[2]));
+  AggregateQueryResponse decoded((AggregateType)(std::get<int32_t>(list[0])));
   auto& encodable_field = list[1];
   if (!encodable_field.IsNull()) {
     decoded.set_field(std::get<std::string>(encodable_field));
   }
+  auto& encodable_value = list[2];
+  if (!encodable_value.IsNull()) {
+    decoded.set_value(std::get<double>(encodable_value));
+  }
   return decoded;
 }
 

diff --git a/packages/cloud_firestore/cloud_firestore/windows/messages.g.h b/packages/cloud_firestore/cloud_firestore/windows/messages.g.h
@@ -554,11 +554,12 @@ class AggregateQuery {
 class AggregateQueryResponse {
  public:
   // Constructs an object setting all non-nullable fields.
-  explicit AggregateQueryResponse(const AggregateType& type, double value);
+  explicit AggregateQueryResponse(const AggregateType& type);
 
   // Constructs an object setting all fields.
   explicit AggregateQueryResponse(const AggregateType& type,
-                                  const std::string* field, double value);
+                                  const std::string* field,
+                                  const double* value);
 
   const AggregateType& type() const;
   void set_type(const AggregateType& value_arg);
@@ -567,7 +568,8 @@ class AggregateQueryResponse {
   void set_field(const std::string_view* value_arg);
   void set_field(std::string_view value_arg);
 
-  double value() const;
+  const double* value() const;
+  void set_value(const double* value_arg);
   void set_value(double value_arg);
 
  private:
@@ -578,7 +580,7 @@ class AggregateQueryResponse {
   friend class FirebaseFirestoreHostApiCodecSerializer;
   AggregateType type_;
   std::optional<std::string> field_;
-  double value_;
+  std::optional<double> value_;
 };
 
 class FirebaseFirestoreHostApiCodecSerializer

diff --git a/...d_firestore_platform_interface/lib/src/method_channel/method_channel_aggregate_query.dart b/...d_firestore_platform_interface/lib/src/method_channel/method_channel_aggregate_query.dart
@@ -44,7 +44,7 @@ class MethodChannelAggregateQuery extends AggregateQueryPlatform {
       if (query == null) continue;
       switch (query.type) {
         case AggregateType.count:
-          count = query.value.toInt();
+          count = query.value?.toInt();
           break;
         case AggregateType.sum:
           sum.add(query);

diff --git a/...es/cloud_firestore/cloud_firestore_platform_interface/lib/src/pigeon/messages.pigeon.dart b/...es/cloud_firestore/cloud_firestore_platform_interface/lib/src/pigeon/messages.pigeon.dart
@@ -505,14 +505,14 @@ class AggregateQueryResponse {
   AggregateQueryResponse({
     required this.type,
     this.field,
-    required this.value,
+    this.value,
   });
 
   AggregateType type;
 
   String? field;
 
-  double value;
+  double? value;
 
   Object encode() {
     return <Object?>[
@@ -527,7 +527,7 @@ class AggregateQueryResponse {
     return AggregateQueryResponse(
       type: AggregateType.values[result[0]! as int],
       field: result[1] as String?,
-      value: result[2]! as double,
+      value: result[2] as double?,
     );
   }
 }

diff --git a/packages/cloud_firestore/cloud_firestore_platform_interface/pigeons/messages.dart b/packages/cloud_firestore/cloud_firestore_platform_interface/pigeons/messages.dart
@@ -265,7 +265,7 @@ class AggregateQueryResponse {
 
   final AggregateType type;
   final String? field;
-  final double value;
+  final double? value;
 }
 
 @HostApi(dartHostTestHandler: 'TestFirebaseFirestoreHostApi')

diff --git a/packages/cloud_firestore/cloud_firestore_web/lib/src/interop/firestore.dart b/packages/cloud_firestore/cloud_firestore_web/lib/src/interop/firestore.dart
@@ -926,7 +926,7 @@ class AggregateQuery {
 class AggregateQuerySnapshot
     extends JsObjectWrapper<firestore_interop.AggregateQuerySnapshotJsImpl> {
   static final _expando = Expando<AggregateQuerySnapshot>();
-  late final Map<String, Object> _data;
+  late final Map<String, Object?> _data;
 
   /// Creates a new [AggregateQuerySnapshot] from a [jsObject].
   static AggregateQuerySnapshot getInstance(
@@ -942,6 +942,12 @@ class AggregateQuerySnapshot
 
   int? get count => _data['count'] as int?;
 
-  double getDataValue(platform_interface.AggregateQuery query) =>
-      _data[AggregateQuery.name(query)]! as double;
+  double? getDataValue(platform_interface.AggregateQuery query) {
+    final value = _data[AggregateQuery.name(query)];
+    if (value == null) {
+      return null;
+    } else {
+      return value as double;
+    }
+  }
 }