fix(firestore,windows): report unimplemented for sum/average aggregations - #18628
fix(firestore,windows): report unimplemented for sum/average aggregations#18628m1roxx wants to merge 1 commit into
Conversation
…ions
The Firebase C++ SDK only implements count() aggregations, so the Windows
plugin printed "Sum is not supported on C++" to stdout and dropped the
aggregation. A mixed request such as `aggregate(count(), sum('foo'))`
therefore resolved successfully with the count filled in and `getSum()`
returning null, which is indistinguishable from a query that legitimately
has no sum. A sum-only request left the AggregateQuery default constructed,
so `Get()` returned a failed future and surfaced an opaque error.
Reject requests containing sum() or average() with an explicit
`unimplemented` error instead, matching how Windows already reports
unsupported Storage list() operations.
`MethodChannelAggregateQuery.get` did not convert platform exceptions, so
the error reached callers as a raw PlatformException rather than a
FirebaseException like every other Firestore API. Wrap it the same way
`MethodChannelQuery.get` does.
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
Description
On Windows,
sum()andaverage()aggregations are silently unsupported.The Firebase C++ SDK exposes only
Query::Count()— there is no sum or average aggregation infirebase/firestore/aggregate_query.h. The Windows plugin handled that by printing to stdout and moving on:Two user-visible consequences:
collection.aggregate(count(), sum('foo'))resolves successfully with the count filled in andgetSum('foo')returningnull.getSumis typeddouble?, so this is indistinguishable from a query that legitimately has no sum — the app silently reads wrong data.collection.aggregate(sum('foo'))leavesaggregate_querydefault constructed, soAggregateQuery::Get()returnsFailedFuture(aggregate_query.cc) and the caller gets an opaque error with no indication that the operation is unsupported on this platform.This PR rejects any aggregation request containing
sum()oraverage()with an explicitunimplementederror, following the precedent set for Windows Storage listing in #18449 ("throw unimplemented instead of returning misleading empty results").count()is unaffected.While adding the integration test I found that
MethodChannelAggregateQuery.getis the only Firestore method-channel call not wrapped inconvertPlatformException, so the error reached callers as a rawPlatformExceptioninstead of aFirebaseException. That is fixed here too, the same wayMethodChannelQuery.getdoes it — it also affects Android/iOS, where any aggregate-query failure (for examplepermission-denied) currently surfaces in the wrong exception type.Related Issues
No existing issue — found while auditing the Windows plugin. Happy to file one if you'd prefer the report tracked separately.
Checklist
///).melos run analyze) does not report any problems on my PR.Breaking Change
An app that today reads
nullfromgetSum()on Windows will now see aFirebaseException. That is the point of the change: the previous value was wrong, not absent.