FirebaseMessaging: RemoteMessage.fromMap(): regression, faulty handling of 'contentAvailable' & 'mutableContent'

spacejunky commented 3 years ago

Bug report

Commit: 8a58dac3afbc2a80bfd18398158fdc0f75af1210 (part of PR #4909)

In RemoteMessage.fromMap():

Apparent regression in handling of missing 'contentAvailable' & 'mutableContent' message fields in message map.

These fields are now initialised to 'false' (and are therefore boolean). If the fields are not present in the map, the code attempts to set the value to 'null' (the return from map[] when the key is not present).

This fails as null is not a subtype of bool, although the exception is seemingly not propagated to the application.

Earlier versions of this had, for example,

contentAvailable: map['contentAvailable']??false,

which maintains the bool type. For whatever reason this commit removed the '??false'.

This results in silent failure to deliver messages to the package user (at least for messages where these optional fields are not present).

Expected behavior

Restore the '??false' to maintain type correctness. Local experimentation shows that this seems to work.

I would write a PR but I'm not familiar enough with the structure and workflows of flutterfire to do it within any reasonable time

Flutter doctor

Run flutter doctor and paste the output below:

Click To Expand

``` Doctor summary (to see all details, run flutter doctor -v): [√] Flutter (Channel beta, 1.25.0-8.3.pre, on Microsoft Windows [Version 10.0.19041.746], locale en-US) [√] Android toolchain - develop for Android devices (Android SDK version 29.0.2) [√] Chrome - develop for the web [√] Android Studio (version 4.1.0) [√] VS Code (version 1.52.1) [√] Connected device (3 available) • No issues found! ```

Flutter dependencies

Run flutter pub deps -- --style=compact and paste the output below:

Click To Expand

``` Dart SDK 2.12.0-133.7.beta Flutter SDK 1.25.0-8.3.pre no_words 1.0.0-nullsafety.0 dependencies: - cloud_firestore 0.17.0-1.0.nullsafety.0 [cloud_firestore_platform_interface cloud_firestore_web firebase_core firebase_core_platform_interface flutter meta] - cloud_functions 0.9.1-1.0.nullsafety.1 [cloud_functions_platform_interface cloud_functions_web firebase_core firebase_core_platform_interface flutter] - cupertino_icons 1.0.2 - firebase_auth 0.21.0-1.1.nullsafety.1 [firebase_auth_platform_interface firebase_auth_web firebase_core firebase_core_platform_interface flutter meta] - firebase_core 0.8.0-1.0.nullsafety.1 [firebase_core_platform_interface firebase_core_web flutter meta] - firebase_messaging 9.0.0-1.0.nullsafety.0 [firebase_core firebase_core_platform_interface firebase_messaging_platform_interface firebase_messaging_web flutter meta] - flutter 0.0.0 [characters collection meta typed_data vector_math sky_engine] - freezed_annotation 0.13.0-nullsafety.0 [collection json_annotation meta] - hooks_riverpod 0.13.0-nullsafety.1 [collection flutter flutter_hooks flutter_riverpod riverpod state_notifier] - logging 1.0.0-nullsafety.0 - meta 1.3.0-nullsafety.6 - rxdart 0.26.0-nullsafety.1 dev dependencies: - build_runner 1.11.1 [args async build build_config build_daemon build_resolvers build_runner_core code_builder collection crypto dart_style glob graphs http_multi_server io js logging meta mime path pedantic pool pub_semver pubspec_parse shelf shelf_web_socket stack_trace stream_transform timing watcher web_socket_channel yaml] - effective_dart 1.3.0 - flutter_test 0.0.0 [flutter test_api path fake_async clock stack_trace vector_math async boolean_selector characters charcode collection matcher meta source_span stream_channel string_scanner term_glyph typed_data] - freezed 0.13.0-nullsafety.2 [analyzer build build_config meta source_gen freezed_annotation] transitive dependencies: - _fe_analyzer_shared 14.0.0 [meta] - analyzer 0.41.2 [_fe_analyzer_shared args cli_util collection convert crypto glob meta package_config path pub_semver source_span watcher yaml] - args 1.6.0 - async 2.5.0-nullsafety.3 [collection] - boolean_selector 2.1.0-nullsafety.3 [source_span string_scanner] - build 1.6.2 [analyzer async convert crypto glob logging meta path] - build_config 0.4.5 [checked_yaml json_annotation meta path pubspec_parse yaml] - build_daemon 2.1.7 [built_collection built_value http_multi_server logging pedantic path pool shelf shelf_web_socket stream_transform watcher web_socket_channel] - build_resolvers 1.5.3 [analyzer build crypto graphs logging meta path package_config pool pub_semver] - build_runner_core 6.1.7 [async build build_config build_resolvers collection convert crypto glob graphs logging meta path package_config pedantic pool timing watcher yaml] - built_collection 4.3.2 [collection quiver] - built_value 7.1.0 [built_collection collection fixnum quiver] - characters 1.1.0-nullsafety.5 - charcode 1.2.0-nullsafety.3 - checked_yaml 1.0.4 [json_annotation source_span yaml] - cli_util 0.2.0 [path] - clock 1.1.0-nullsafety.3 - cloud_firestore_platform_interface 4.0.0-1.0.nullsafety.0 [collection firebase_core flutter meta plugin_platform_interface] - cloud_firestore_web 0.4.0-1.0.nullsafety.0 [cloud_firestore_platform_interface firebase_core firebase_core_web flutter flutter_web_plugins js] - cloud_functions_platform_interface 4.0.2-1.0.nullsafety.1 [firebase_core flutter meta plugin_platform_interface] - cloud_functions_web 3.1.4-1.0.nullsafety.1 [cloud_functions_platform_interface firebase_core firebase_core_web flutter flutter_web_plugins js] - code_builder 3.6.0 [built_collection built_value collection matcher meta] - collection 1.15.0-nullsafety.5 - convert 2.1.1 [charcode typed_data] - crypto 2.1.5 [collection convert typed_data] - dart_style 1.3.10 [analyzer args path source_span] - fake_async 1.2.0-nullsafety.3 [clock collection] - firebase_auth_platform_interface 4.0.0-1.1.nullsafety.1 [firebase_core flutter meta plugin_platform_interface] - firebase_auth_web 0.4.0-1.1.nullsafety.1 [firebase_auth_platform_interface firebase_core firebase_core_web flutter flutter_web_plugins http_parser intl js meta] - firebase_core_platform_interface 4.0.0-1.0.nullsafety.1 [flutter meta plugin_platform_interface] - firebase_core_web 0.3.0-1.0.nullsafety.1 [firebase_core_platform_interface flutter flutter_web_plugins js meta] - firebase_messaging_platform_interface 2.0.0-1.0.nullsafety.0 [firebase_core flutter meta plugin_platform_interface] - firebase_messaging_web 0.2.0-1.0.nullsafety.0 [firebase_core firebase_core_web firebase_messaging_platform_interface flutter flutter_web_plugins js meta] - fixnum 0.10.11 - flutter_hooks 0.16.0-nullsafety.0 [flutter] - flutter_riverpod 0.13.0-nullsafety.1 [collection flutter meta riverpod state_notifier] - flutter_web_plugins 0.0.0 [flutter js characters collection meta typed_data vector_math] - glob 1.2.0 [async collection node_io path pedantic string_scanner] - graphs 0.2.0 - http_multi_server 2.2.0 [async] - http_parser 4.0.0-nullsafety [charcode collection source_span string_scanner typed_data] - intl 0.17.0-nullsafety.2 [clock path] - io 0.3.4 [charcode meta path string_scanner] - js 0.6.3-nullsafety.3 - json_annotation 4.0.0-nullsafety.0 - matcher 0.12.10-nullsafety.3 [stack_trace] - mime 0.9.7 - node_interop 1.2.1 [js] - node_io 1.1.1 [node_interop path] - package_config 1.9.3 [path charcode] - path 1.8.0-nullsafety.3 - pedantic 1.10.0-nullsafety.3 - plugin_platform_interface 1.1.0-nullsafety.2 [meta] - pool 1.5.0-nullsafety.3 [async stack_trace] - pub_semver 1.4.4 [collection] - pubspec_parse 0.1.7 [checked_yaml json_annotation pub_semver yaml] - quiver 2.1.5 [matcher meta] - riverpod 0.13.0-nullsafety.1 [collection freezed_annotation meta state_notifier] - shelf 1.0.0-nullsafety.0 [async collection http_parser path stack_trace stream_channel] - shelf_web_socket 0.2.4 [shelf stream_channel web_socket_channel] - sky_engine 0.0.99 - source_gen 0.9.10+1 [analyzer async build dart_style glob meta path pedantic source_span] - source_span 1.8.0-nullsafety.4 [charcode collection path term_glyph] - stack_trace 1.10.0-nullsafety.6 [path] - state_notifier 0.7.0-nullsafety.0 [meta] - stream_channel 2.1.0-nullsafety.3 [async] - stream_transform 1.2.0 - string_scanner 1.1.0-nullsafety.3 [charcode source_span] - term_glyph 1.2.0-nullsafety.3 - test_api 0.2.19-nullsafety.6 [async boolean_selector collection meta path source_span stack_trace stream_channel string_scanner term_glyph matcher] - timing 0.1.1+3 [json_annotation] - typed_data 1.3.0-nullsafety.5 [collection] - vector_math 2.1.0-nullsafety.5 - watcher 0.9.7+15 [async path pedantic] - web_socket_channel 1.1.0 [async crypto stream_channel] - yaml 2.2.1 [charcode collection string_scanner source_span] ```

spacejunky commented 3 years ago

So, I'm fixing to find time to do a PR for this. Thinking more carefully about the issue though:

the original intent (which my fix-the-regression proposal above tried to maintain) was apparently to have these two fields always present, but defaulting to 'false', presumably to support higher level processing on Apple platforms (but I don't have one to confirm or deny). For android they are completely irrelevant, as I understand it (and with the V1 API and admin sdk I seem unable to include them in the message to be sent to an android device in any case).
so a more appropriate change would be to make them nullable -- from the viewpoint of reflecting the protocol.
this would leave it up to higher layers (on apple'ish platforms) to null-check, or null cast-away, the values, depending on the context (I know 'null' about apple platforms, and I don't have one to experiment with).

Which approach do the experts deem to be appropriate?

PS: sorry if I'm a PITA, but the very simple PoC app I'm working on could (and therefore should, if possible) be sound-null-safe. But that makes it imperative to fix this issue, for android platform usage. I think the messaging example only works now because it is running unsound-null-safe, which disguises the issue? Sadly, some dependencies are missing to try in proper null-safe ...

eugeniyk commented 3 years ago

Any update on this one?

Once migrated to null-safety I've received

I/flutter (16763): FlutterFire Messaging: An error occurred in your background messaging handler: I/flutter (16763): type 'Null' is not a subtype of type 'bool'

Using admin java SDK I don't see the workaround rather than fix it in the lib

spacejunky commented 3 years ago

No public progress. Amongst other things, I'm still waiting to hear what the preferred approach is for any general fix.

For my own purposes I have patched RemoteMessage.dart locally to put back the original "??false" for mutableContent & contentAvailable.

For example:

contentAvailable:map['contentAvailable'] ?? false,

To be honest the thing that is holding up proposing a PR for this simple fix is the need to create a test that will fail first. It seems that all the tests so far check only interfaces, not functionality (at least I can't find one that should have failed on this). That's a whole new rabbit hole, and together with being unable to get "Melos" to run error free on my system it means the PR has got bumped from the top of my list in favour of completing other aspects of my project. The infrastructure is too complex fo4r a newcomer at this point, at least for my small brain.

eugeniyk commented 3 years ago

@spacejunky thanks, patching cached plugins code works for now (contentAvailable:map['contentAvailable'] ?? false,) until the next update =)

Tests should be pretty straightforward if you know the original format ;) specially what fields are optional, what fields are not

firebase / flutterfire