Closed mman closed 1 month ago
@jimnor0xF I have intentionally left the tests to fail so that we can discuss what is right and what is wrong.
@mman Should be OK. Tested myself, although not with the Parse Android SDK.
I guess rawPayload does not work with the Android SDK either, unless a user put those keys in themselves. Not sure if we want/need to support that out of the box.
@mtrezza Can you chime in on the above comment?
Should rawPayload be supported out of the box for the Parse Android SDK? In that case we basically need to add those time/push_id keys if "android" is present in the payload as well.
Personally, I don't see the need, since I'd prefer it to not be dependent on some client side implementation.
@mman What do you think?
Not sure I fully understand what the issue is, could someone explain again, please? I'd just keep in mind that developers cannot easily change the (custom) push adapter implementation in all their rolled out apps, so whatever we can do here to make life easier for developers will likely be appreciated.
@mtrezza @jimnor0xF I will try to explain what I have learned over the years:
Apple APNS service defines an (let's call it) API that specifies what JSON payload you can push to apple servers, and what will happen on device. This has been evolving over time quite randomly. For example initially we only had alert: String
, then alert migrated to dictionary alert: {title: String, body: String}
, mutable-content: 1
, content-available: 1
, etc... It's also quite strict as in mutable-content
must contain number 1
, nothing else, otherwise it fails in random ways.
Google Cloud Messaging and later Firebase Messaging defines their own API for JSON payload to target Android devices. Android seems to be me much less strict and basically what you give it, will get sent to device where you reassemble as you wish... GCM did support arbitrary JSON it seems, whereas FCM insists on flat JSON.
Since FCM also support iOS, it further complicates that API in that it supports general payload, as well as platform specific payload for Android, and platform specific for iOS. All this mangled together in large JSON. What is general payload for? I have no idea, but I assume FCM can send more data to more destinations - like web for example.
Historically (I think) Parse Server Push Adapter tried to invent its own payload API that you pass to Parse.Push.send
and that gets later converted by the adapter to appropriate platform specific API that gets sent via push adapter to the device itself.
Now point 3. is what @jimnor0xF calls rawPayload
in that you directly specify what should be sent to FCM. Apple specific subset of rawPayload
should be what 1. expects and that should work fine.
But because of 4. we have our own iOS and Android payload API that gets passed to Parse.Push.send
and then converted to whatever is required by 1., 2., 3.
More over, Parse Server + Parse Push Adapter agreed with Parse Android SDK in the past that each Android push will have push_id
and time
always injected so that Parse Android SDK can use it despite it being largely a historic artefact that is probably no longer needed...
For now, I see one remaining problem:
Parse Server Push Adapter, should probably still support expected Parse.Push.Send
API and convert that to whatever should be sent out. Since this format was never formalised it is pretty difficult to cover it with tests.
One issue I identified is that in the past we supported alert: { title: "Pizza is on the way", body: "Expect your delivery in 25 minutes..." }
, and that no longer works however it can be converted to "alert: "Pizza is on the way"
, body: "Expect your delivery in 25 minutes..."
(reason being FCM requires "flat payload")
Second issue is that when you send to Android we misplace the push_id
and time
(subject of this PR).
More issues perhaps may pop up as more people migrate to push adapter 6.x.
I will update this PR and will test it more, and try to at least open issues against thing that no longer work - but that have a reasonable workaround - so that we can mention them in README.
Thanks for the explanation. What is the suggested solution, with the least impact for developers? Mind that we have only 4 weeks left until Google will decommission the legacy API.
Let's assume the viewpoint of a developer for an app that currently sends pushes to Apple (directly to APNS via node-apn
) and Android (directly to FCM legacy API via node-gcm
) devices. For APNS there should be no change required, neither client nor server side. For Android there should be:
Parse.Push.send
, since we control this API and can convert the provided payload if necessary.So we'd need to transform the payload that is sent to the Parse Android SDK push module so that it works properly. For apps that subclassed the ParsePushBroadcastReceiver
and implemented custom payload parsing, that means that the payload that arrives at the device must not be affected by the FCM API change.
Does that make sense and is that technically possibly?
Or does the new FCM API send a payload to the device that we cannot fully influence and therefore the Parse Android SDK cannot interpret the push correctly? That would be the worst case scenario. For example @mman wrote:
One issue I identified is that in the past we supported alert: { title: "Pizza is on the way", body: "Expect your delivery in 25 minutes..." }, and that no longer works however it can be converted to "alert: "Pizza is on the way", body: "Expect your delivery in 25 minutes..." (reason being FCM requires "flat payload")
If we change this structure, will the Parse Android SDK still interpret that correctly? What about apps that extend the ParsePushBroadcastReceiver
and parse the push payload manually, expecting that the payload is not flat and that alert
has the nested keys title
and body
?
@mtrezza You summarized it nicely. My expectation is to simply add firebaseServiceAccount
to Android push config and everything must continue to work.
I think it is technically possible and I will try to work on it next Monday. The issues I identified are:
alert
dictionarypush_id
and time
once these three are addressed it should work. I assume everybody affected will start migrating over the next few weeks so cutting 6.0.1 before the end of next week should work.
It would be amazing if we could get this done by next week. I believe this is an all-hands on deck issue, but few people are actually aware of this challenge. Once we have a release next week, we'll post this on our social media to invite more developers to test this out. Thanks @jimnor0xF and @mman for your efforts so far!
@mman @jimnor0xF So this one should be ready to go, I have verified that it works with my apps with no changes required other than adding the firebaseServiceAccount
configuration option for Android.
@mtrezza ready for review...
LGTM at least :)
So this one should be ready to go, I have verified that it works with my apps with no changes required other than adding the
firebaseServiceAccount
configuration option for Android.
@mman Great - I understand you tested this out with Android and Apple push notifications on real devices?
So this one should be ready to go, I have verified that it works with my apps with no changes required other than adding the
firebaseServiceAccount
configuration option for Android.@mman Great - I understand you tested this out with Android and Apple push notifications on real devices?
I tested on real Android devices, where push for Android goes via FCM.js
. For iOS push I do not use Firebase at all, but I have not touched and iOS related code.
@jimnor0xF Could you review as well before we merge?
Hello guys, I raised this PR https://github.com/parse-community/parse-server-push-adapter/pull/246 with the compatibility for nested objects. Could you please leave your comments?
So I'll wait with merging until we have figured out what https://github.com/parse-community/parse-server-push-adapter/pull/246 contains in the context of this PR - does it contain subset of this PR, or is it an amendment,...?
So I'll wait with merging until we have figured out what #246 contains in the context of this PR - does it contain subset of this PR, or is it an amendment,...?
I'm honestly not sure, since I started using push on Apple first and then added Android devices, I was only able to uncover the missing nested alert: {body:, title:}
issue. What @charles-ramos has added is additional support for directly sending Android notification
object. I am not 100% sure, but I think that Parse Android SDK never handled receiving Android notification
directly, and always extracted user data from only three parameters: pushId
, time
, and data
.
So I need @charles-ramos to answer whether he intents to support new functionality, or whether migration to latest FCM broke something for him...
So I'll wait with merging until we have figured out what #246 contains in the context of this PR - does it contain subset of this PR, or is it an amendment,...?
I'm honestly not sure, since I started using push on Apple first and then added Android devices, I was only able to uncover the missing nested
alert: {body:, title:}
issue. What @charles-ramos has added is additional support for directly sending Androidnotification
object. I am not 100% sure, but I think that Parse Android SDK never handled receiving Androidnotification
directly, and always extracted user data from only three parameters:pushId
,time
, anddata
.So I need @charles-ramos to answer whether he intents to support new functionality, or whether migration to latest FCM broke something for him...
The idea is to keep everything compatible with the current code (GCM) that uses "data" for sending push notifications for Android. Right now, in case I want to send push notifications to everyone (android and iOS), I must use both "data" and "notification". With the PR I raised, it will be possible to keep using "data" for both.
During my tests using the changes mentioned in this branch, I got some results here:
First of all, I tested this code for sending push notifications:
Parse.Push.send(
{
where: {},
data: {
alert: "Alert Push Notification",
badge: 123,
title: 'Title data'
}
notification: {
title: 'Title Notification',
body: 'Test body'
},
},
{ useMasterKey: true },
);
I got the following error:
RangeError: Invalid time value
at Date.toISOString (<anonymous>)
Due to this line: https://github.com/parse-community/parse-server-push-adapter/blob/8b93e22c5a24cd424709f186f07f09b2ba806d68/src/FCM.js#L265
After adding a console.log() to see what was my androidPayload, I got this:
{
android: {
priority: 'high',
notification: { title: 'Title Notification', body: 'Test body' }
}
}
Then, I removed and tested it, but I did not receive the notification for Android (only for iOS, with the content from the data
).
If I tried sending a push notification without notification
, my androidPayload
was like this:
{ android: { priority: 'high' } }
Which seems to be expected.
Testing via Dashboard, I did not receive any notifications for android, as the dashboard only sends push notifications through alert
: https://github.com/parse-community/parse-dashboard/blob/5.3.0/src/dashboard/Push/PushDetails.react.js#L42
Is there anything that I'm not concerned about?
As a reference, I used the code described in the documentation: https://docs.parseplatform.org/js/guide/#sending-pushes
And used the push options listed here: https://docs.parseplatform.org/js/guide/#sending-options
I'd like to chime in and share details about our current structure for sending push notifications. Here’s how we format our payload with a nested data object:
data: {
alert: "foo",
badge: 1,
data: {
notification_id: 1,
path: "/bar"
},
sound: "*"
}
After updating to parse-server@7.1.0-alpha.6
and using the firebaseServiceAccount
for authentication, we encountered the following error in our server logs:
ERR! parse-server-push-adapter FCM error sending push: Error: android.data must only contain string values
This error only shows when we send pushes manually through the server console. However, when our system sends pushes, they seem to fail silently without any error messages appearing in the logs.
I'd like to chime in and share details about our current structure for sending push notifications. Here’s how we format our payload with a nested data object:
data: { alert: "foo", badge: 1, data: { notification_id: 1, path: "/bar" }, sound: "*" }
After updating to
parse-server@7.1.0-alpha.6
and using thefirebaseServiceAccount
for authentication, we encountered the following error in our server logs:
ERR! parse-server-push-adapter FCM error sending push: Error: android.data must only contain string values
This error only shows when we send pushes manually through the server console. However, when our system sends pushes, they seem to fail silently without any error messages appearing in the logs.
@mman This problem should be fixed with this PR, with the following line, right?
@astroblemeal Could you test with the code in this PR?
@charles-ramos
Strange, looks like the JSON body in your example is missing a comma though. Not sure if that has anything to do with it. Did that payload work before with the legacy FCM (GCM.js)? Can you try with this payload?
const q = new Parse.Query(Parse.Installation);
q.containedIn('user', userIds);
await Parse.Push.send(
{
where: q,
data: {
alert: 'Test alert',
badge: 666,
title: 'Test title'
},
notification: {
title: 'Test title',
body: 'Test body',
},
},
{ useMasterKey: true },
);
Could you share some info about what you are running on your Android device? Are you using the Parse Android SDK to handle pushes?
On the topic of using the notification
key. Correct me if I'm wrong, this was never a "well-known" or documented key, even back when we used legacy FCM (GCM.js). You could only know it existed by reading the code.
I have to use the notification
key since I do not use the Parse Android SDK for handling pushes. You only use the notification
key if you use Firebase Cloud Messaging SDK on the client side, or perhaps if you have a customised version of the push handlers to handle that key (?). If you use the Parse Android SDK, using the data
key only for Android should work.
And like your PR mentioned #246, you have to specify notification
and data
keys to get it to work for both Apple and Android.
But this was the case in the past as well. At least in my experience with a customised setup, and according to the code in GCM.js which does not merge the data
and notification
keys.
So I do not think we are dependent on #246 to fix some backwards incompatibility issue. Please correct me if I'm wrong though.
For people that intend to use the Firebase Cloud Messaging SDK on the client side to handle pushes or that are using a customised setup, you are better off using the rawPayload
option I added here for better control.
So I'll go ahead and merge this PR and then @charles-ramos can rebase his PR on the latest commit. This may make it easier to test everything out. Please feel free to continue the discussion here, despite the PR being merged.
🎉 This change has been released in version 6.1.1
After merging this PR there is a conflict in https://github.com/parse-community/parse-server-push-adapter/pull/246#issuecomment-2112413924; if @charles-ramos could resolve this, then we can get a clearer picture of what that PR adds after this PR.
@jimnor0xF It works keeping the same payload structure with the nested data object, and setup as before using parse/push-adapter@"6.1.1"
! 🥳
Any idea when these changes will be released in parse-sever
? :)
@mtrezza Looks like parse server itself is has hard pinned the dependency on push adapter 6.0.0
, and despite my custom build depending on 6.2.0
npm
was still pulling in 6.0.0
. I needed to manually up the dependency to 6.2.0
in forked parse server to actually get latest version with the fixes. I'm not versed in npm
that much to see what is behind it, but are we sure that simple npm up
will pick up the latest push adapter?
It's actually much simpler. See Installation, Configure and Bundled with Parse Server. I rewrote the README yesterday to explain this. But we should make a Parse Server release with the new adapter as well before June.
It's actually much simpler. See Installation, Configure and Bundled with Parse Server. I rewrote the README yesterday to explain this. But we should make a Parse Server release with the new adapter as well before June.
hmm, not fully getting it at first glance. I got around it by also adding push-adapter@"6.1.1
to my project and resolving it in my package json while still using "parse-server": "^6.0.0"
.
"resolutions": {
"@parse/push-adapter": "6.1.1"
}
npm i -E @parse/push-adapter@6.2.0
Import push adapter where you init Parse Server:
import { ParsePushAdapter } from '@parse/push-adapter';
// For CommonJS replace the import statemtent above with the following line:
// const ParsePushAdapter = require('@parse/push-adapter').default;
push
is now nested under push.adapter
:
const parseServerOptions = {
push: {
adapter: new ParsePushAdapter({
ios: {
// Apple push options
},
android: {
// Android push options
},
web: {
// Web push options
},
expo: {
// Expo push options
}
})
}
// Other Parse Server options
};
New Pull Request Checklist
Issue Description
Closes: https://github.com/parse-community/parse-server-push-adapter/issues/237
Sending push notifications to Android phones does not work anymore because data are converted and sent out differently using
FCM.js
than they were using theGCM.js
.This means that Parse Android SDK is unable to parse the message correctly, and deliver it to user specified broadcast receiver, silently dropping any received messages.
Approach
Parse.Push.send
was initially built around support for iOS, and later added support for Android, and Android SDK.Parse.Push.send
accepts a dictionary that we callrequestData
.Historically sending push notifications to Apple meant passing in an
alert: "Push Notification"
key inrequestData
. Later when Apple added support for push notification titles and subtitles, push adapter offered two ways how to specifytitle
andsubtitle
. Either in top levelrequestData
, like this:or via nested
alert
dictionary inside ofrequestData
like this:In both of these cases, such
requestData
would end up being converted to the payload accepted by the APNS always getting the second form with nestedalert
dictionary.The Android side of things did not do any special formatting of user data, and would simply use whatever data user passed into
requestData
and just JSON encode them into string. For client side processing, Android SDK requires to specifypushId
andtime
. The final Android payload then looked like this:This PR adjusts the code to provide support for consuming nested
alert
dictionary as before, and it modifies the Android path to properly encode data into JSON string and decorate the payload correctly withpush_id
andtime
to receive notifications on devices currently in circulation with existing versions of Parse Android SDK.TODOs before merging