feat: Add backwards compatibility with APNS payloads when using Firebase Cloud Messaging API (HTTP v1)

jimnor0xF commented 4 months ago

New Pull Request Checklist

[x] I am not disclosing a vulnerability.
[x] I am creating this PR in reference to an issue.

Issue Description

As discussed in #219, we want to remain backwards compatible with both APNS and GCM payloads. For a more seamless transition for developers. A user can also specify a "raw payload" according to https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages which will be used "as-is" when sent without any payload conversion.

Approach

Looked at the code for APNS and GCM around how payloads are generated.
Wrote test specs to ensure that payloads are converted from GCM/APNS to a compatible FCMv1 payload. The input payload used in each test spec match the ones used in GCM.spec.js and APNS.spec.js.

Changes to how GCM payloads are represented in FCM

In current master branch, the GCM to FCMv1 payload conversion uses the FCM "notification"^1 and "data"^2 key. This was changed to use the "android" ^3 version of this since what we're doing is android-specific.

If people want to use the FCM "notification" key which can be used across all supported platforms in FCM, they should use a raw payload.

TODOs before merging

[x] Test push with an Android device with rawPayload and a GCM-compatible payload.
[x] Would be good to do a real test with an Apple device (I unfortunately don't have one on hand currently).
[ ] Add changes to documentation (guides, repository pages, in-code descriptions)

parse-github-assistant[bot] commented 4 months ago

Thanks for opening this pull request!

❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 84.84848% with 15 lines in your changes are missing coverage. Please review.

Project coverage is 90.65%. Comparing base (6ceaccc) to head (488914e).

Files	Patch %	Lines
src/FCM.js	84.53%	15 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #234 +/- ## ========================================== + Coverage 81.38% 90.65% +9.26% ========================================== Files 6 6 Lines 360 428 +68 ========================================== + Hits 293 388 +95 + Misses 67 40 -27 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jimnor0xF commented 4 months ago

@mtrezza For documentation on usage, do we put that in README.md? Or is it better to update: http://docs.parseplatform.org/parse-server/guide/#push-notifications

As mentioned before, I think it would be good to hint to users that using rawPayload is preferred when using FCMv1 in the current documentation, even if the old payload structures are supported. The main reason I added support for FCMv1 to begin with was because it was flaky/confusing supporting apple&android devices at the same time using the same payload with the info written in the current docs. Especially if you needed data payloads to be received and handled when the app is in background.

pcg92 commented 3 months ago

Hello, I am using this branch to set up the push notifications for my project. So far, everything is going well with Android, but I have started working with iOS and am encountering some issues receiving push notifications on the simulator, so I still can't test if it works on a device. The error I'm getting is: {"code":"messaging/registration-token-not-registered","message":"Requested entity was not found."}

I'm checking if the key I added in Firebase is correct to see if I can resolve this and will update accordingly, regards.

jimnor0xF commented 3 months ago

@pcg92

Hello, I am using this branch to set up the push notifications for my project. So far, everything is going well with Android, but I have started working with iOS and am encountering some issues receiving push notifications on the simulator, so I still can't test if it works on a device. The error I'm getting is: {"code":"messaging/registration-token-not-registered","message":"Requested entity was not found."}

I'm checking if the key I added in Firebase is correct to see if I can resolve this and will update accordingly, regards.

As far as I know, you have to test push notifications with Apple on a real device. Cannot use a simulator. You need to have a record with a deviceToken populated under the _Installation collection as well.

pcg92 commented 3 months ago

@pcg92

Hello, I am using this branch to set up the push notifications for my project. So far, everything is going well with Android, but I have started working with iOS and am encountering some issues receiving push notifications on the simulator, so I still can't test if it works on a device. The error I'm getting is: {"code":"messaging/registration-token-not-registered","message":"Requested entity was not found."} I'm checking if the key I added in Firebase is correct to see if I can resolve this and will update accordingly, regards.

As far as I know, you have to test push notifications with Apple on a real device. Cannot use a simulator. You need to have a record with a deviceToken populated under the _Installation collection as well.

Yes, in development I was obtaining a deviceToken for installations from iOS, but I'm not receiving any push notifications, even from the Firebase panel I don't receive any notifications. In a while, I'm going to test with a real device to see what happens.

jimnor0xF commented 3 months ago

@pcg92

Hello, I am using this branch to set up the push notifications for my project. So far, everything is going well with Android, but I have started working with iOS and am encountering some issues receiving push notifications on the simulator, so I still can't test if it works on a device. The error I'm getting is: {"code":"messaging/registration-token-not-registered","message":"Requested entity was not found."} I'm checking if the key I added in Firebase is correct to see if I can resolve this and will update accordingly, regards.

As far as I know, you have to test push notifications with Apple on a real device. Cannot use a simulator. You need to have a record with a deviceToken populated under the _Installation collection as well.

Yes, in development I was obtaining a deviceToken for installations from iOS, but I'm not receiving any push notifications, even from the Firebase panel I don't receive any notifications. In a while, I'm going to test with a real device to see what happens.

Would be great if we can test a payload as described here as well: https://docs.parseplatform.org/parse-server/guide/#api

I am using rawPayload in our deployment of Parse which is working great for both apple and android, but not sure whether the above documented payload works well yet for Apple (this PR aims to make that work).

pcg92 commented 3 months ago

Okay, I've been testing and here are my conclusions: With an iPhone in release mode, push notifications are received, but I only see the notification when I send it from the Firebase console. If I send it from the Parse console, it says 'push sent' but the notification doesn't arrive on the phone. Could you provide me with the rawPayload so I can try sending a push from a cloud function?

jimnor0xF commented 3 months ago

@pcg92

Okay, I've been testing and here are my conclusions: With an iPhone in release mode, push notifications are received, but I only see the notification when I send it from the Firebase console. If I send it from the Parse console, it says 'push sent' but the notification doesn't arrive on the phone. Could you provide me with the rawPayload so I can try sending a push from a cloud function?

Sure, this is how I'm using it currently:

  const q = new Parse.Query(Parse.Installation);
  q.containedIn('user', userIds);

  await Parse.Push.send(
    {
      where: q,
      rawPayload: {
        notification: {
          title: title,
          body: body,
        },
        data: {
          type: type,
          payload: payload,
        },
        android: {
          priority: 'high',
        },
        apns: {
          headers: {
            'apns-priority': '5',
          },
          payload: {
            aps: {
              contentAvailable: true,
            },
          },
        },
      },
    },
    { useMasterKey: true },
  );

mtrezza commented 3 months ago

@jimnor0xF

For documentation on usage, do we put that in README.md? Or is it better to update: http://docs.parseplatform.org/parse-server/guide/#push-notifications

Better to update http://docs.parseplatform.org/parse-server/guide/#push-notifications I think

I added support for FCMv1 to begin with was because it was flaky/confusing supporting apple&android devices at the same time using the same payload with the info written in the current docs. Especially if you needed data payloads to be received and handled when the app is in background.

Is this a bug in the current implementation? Or why is it confusing / flaky? I think the original intention of Parse was to offer a common interface / payload despite of the different ecosystems. In a way, Firebase is doing the same with their API. But I don't see a reason for us to adapt Firebase's abstraction, if ours works as well.

jimnor0xF commented 3 months ago

@mtrezza

@jimnor0xF

For documentation on usage, do we put that in README.md? Or is it better to update: http://docs.parseplatform.org/parse-server/guide/#push-notifications

Better to update http://docs.parseplatform.org/parse-server/guide/#push-notifications I think

I added support for FCMv1 to begin with was because it was flaky/confusing supporting apple&android devices at the same time using the same payload with the info written in the current docs. Especially if you needed data payloads to be received and handled when the app is in background.

Is this a bug in the current implementation? Or why is it confusing / flaky? I think the original intention of Parse was to offer a common interface / payload despite of the different ecosystems. In a way, Firebase is doing the same with their API. But I don't see a reason for us to adapt Firebase's abstraction, if ours works as well.

IIRC it had to do with the "data" property for background notifications. Might have been the documentation being wrong and us setting a payload that doesn't work though.

There's a related comment to that here: https://github.com/parse-community/parse-server/issues/6369#issuecomment-721095800

Suppose I'm fine with keeping it as it is, but just mentioning that you can use a raw payload if wanted.

mtrezza commented 3 months ago

Sounds good, kindly let me know when this PR is ready for review :-)

jimnor0xF commented 3 months ago

Sounds good, kindly let me know when this PR is ready for review :-)

@mtrezza If you can have a look now that would be great :)

mtrezza commented 3 months ago

@jimnor0xF Sorry, I didn't notice your message, I'll take a look

mtrezza commented 3 months ago

In current master branch, the GCM to FCMv1 payload conversion uses the FCM "notification"1 and "data"2 key. This was changed to use the "android" 3 version of this since what we're doing is android-specific.

If people want to use the FCM "notification" key which can be used across all supported platforms in FCM, they should use a raw payload.

Could you explain a bit more what that practically means for developers? I guess this should be in the docs as well?

This PR is not a breaking change, right?

mtrezza commented 3 months ago

@pcg92 could you give this a shot and test it out on a real iPhone?

jimnor0xF commented 3 months ago

In current master branch, the GCM to FCMv1 payload conversion uses the FCM "notification"1 and "data"2 key. This was changed to use the "android" 3 version of this since what we're doing is android-specific. If people want to use the FCM "notification" key which can be used across all supported platforms in FCM, they should use a raw payload.

Could you explain a bit more what that practically means for developers? I guess this should be in the docs as well?

This PR is not a breaking change, right?

It's an internal change only. Still works the same as before (I tested). The top level notification/data keys in the FCMv1 API is supposed to be used for cross-platform push notifications. The "notification" key is basically for the alert body and title of a push notification and the "data" is used for silent data payloads (to for example trigger something in the background on receive).

There is an android scoped version of these keys in the FCMv1 API, so this is what is being used now for android devices. Reason for the change is because I think it's more appropriate to use the android scoped version of these keys to match how things were done before. Since, the GCM.js module can only be used with android and the APNS.js module with apple.

jimnor0xF commented 3 months ago

@mtrezza

Yeah. I'll try and see if I can get a coworker to test it out for me :)

Also, any directions you can give around updating docs? Is that done in some repo?

mtrezza commented 3 months ago

Sure, the docs are updated in https://github.com/parse-community/docs. The file structure can be confusing at first, but if you just search the files for keywords you should find the parts to update.

jimnor0xF commented 3 months ago

@mtrezza Managed to do some testing on iOS now. Works.

Noticed it was not working for Android when you use a combined APNS+GCM payload in the case where you have APNS-specific keys that have integer values under "data".

For latest commit I added a quick fix to remove those APNS-specific keys when device type is android.

Used a payload that looks like this:

  await Parse.Push.send(
    {
      where: q,
      data: {
        alert: "Test alert",
        badge: 666,
        title: 'Test title'
      },
      notification: {
        title: 'Test title',
        body: 'Test body',
      },
    },
    { useMasterKey: true },
  );

jimnor0xF commented 3 months ago

Looks good; is this ready for merge?

Yup 👍

parseplatformorg commented 3 months ago

🎉 This change has been released in version 5.2.0

parse-community / parse-server-push-adapter