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

jimnor0xF · 2024-03-03T19:55:17Z

New Pull Request Checklist

I am not disclosing a vulnerability.
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"¹ and "data"² key.
This was changed to use the "android" ³ 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

Test push with an Android device with rawPayload and a GCM-compatible payload.
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 · 2024-03-03T19:55:21Z

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 · 2024-03-03T19:55:55Z

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 ⚠️

Additional details and impacted files

@@            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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jimnor0xF · 2024-03-03T19:56:36Z

@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 · 2024-03-05T12:53:51Z

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 · 2024-03-05T13:12:59Z

@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 · 2024-03-05T13:16:37Z

@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 · 2024-03-05T13:26:26Z

@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 · 2024-03-05T14:59:03Z

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 · 2024-03-05T15:34:09Z

@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 · 2024-03-05T19:17:00Z

@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 · 2024-03-05T19:36:16Z

@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:
parse-community/parse-server#6369 (comment)

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

mtrezza · 2024-03-06T09:13:44Z

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

jimnor0xF · 2024-03-12T19:49:38Z

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 · 2024-03-21T18:30:18Z

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

spec/FCM.spec.js

spec/support/fakeServiceAccount.json

mtrezza · 2024-03-21T18:39:17Z

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 · 2024-03-21T18:40:53Z

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

jimnor0xF · 2024-03-22T13:08:20Z

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.

mtrezza

Looks good! Now we are just waiting for a device test, correct?

jimnor0xF · 2024-03-26T01:00:55Z

@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 · 2024-03-26T05:29:27Z

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 · 2024-03-28T23:05:36Z

@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 },
  );

mtrezza

Looks good; is this ready for merge?

jimnor0xF · 2024-03-30T12:15:30Z

Looks good; is this ready for merge?

Yup 👍

# [5.2.0](5.1.1...5.2.0) (2024-03-30) ### Features * Add backwards compatibility with APNS payloads when using Firebase Cloud Messaging API (HTTP v1) ([#234](#234)) ([b9a41d1](b9a41d1))

parseplatformorg · 2024-03-30T21:34:17Z

🎉 This change has been released in version 5.2.0

jimnor0xF added 3 commits March 3, 2024 20:34

Backwards compatability with APNS and some tests

e6e390c

vim mistake

29e7b86

fix

ccb5e5d

JSDoc

af7445d

remove junk and add some checks for rawpayload

d48204a

Merge branch 'master' into backwards_compatability_with_apns_and_tests

245a64b

mtrezza requested changes Mar 21, 2024

View reviewed changes

spec/FCM.spec.js Outdated Show resolved Hide resolved

spec/support/fakeServiceAccount.json Outdated Show resolved Hide resolved

mtrezza changed the title ~~Add backwards compatability with APNS payloads and tests~~ feat: Add backwards compatibility with APNS payloads Mar 21, 2024

mtrezza changed the title ~~feat: Add backwards compatibility with APNS payloads~~ feat: Add backwards compatibility with APNS payloads when using FCMv1 API Mar 21, 2024

mtrezza changed the title ~~feat: Add backwards compatibility with APNS payloads when using FCMv1 API~~ feat: Add backwards compatibility with APNS payloads when using Firebase Cloud Messaging API (HTTP v1) Mar 21, 2024

jimnor0xF added 2 commits March 25, 2024 20:02

Use dummy domains

a2560bc

Remove done()

1a9cd96

mtrezza previously approved these changes Mar 25, 2024

View reviewed changes

Fix to allow a payload that uses gcm+apns keys

3338ad9

jimnor0xF dismissed mtrezza’s stale review via 3338ad9 March 28, 2024 22:41

Delete key instead

488914e

mtrezza approved these changes Mar 29, 2024

View reviewed changes

mtrezza merged commit b9a41d1 into parse-community:master Mar 30, 2024
8 checks passed

parseplatformorg added the state:released label Mar 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jimnor0xF commented Mar 3, 2024 •

edited

parse-github-assistant bot commented Mar 3, 2024 •

edited

codecov bot commented Mar 3, 2024 •

edited

jimnor0xF commented Mar 3, 2024 •

edited

pcg92 commented Mar 5, 2024

jimnor0xF commented Mar 5, 2024 •

edited

pcg92 commented Mar 5, 2024

jimnor0xF commented Mar 5, 2024 •

edited

pcg92 commented Mar 5, 2024

jimnor0xF commented Mar 5, 2024

mtrezza commented Mar 5, 2024 •

edited

jimnor0xF commented Mar 5, 2024

mtrezza commented Mar 6, 2024

jimnor0xF commented Mar 12, 2024 •

edited

mtrezza commented Mar 21, 2024

mtrezza commented Mar 21, 2024 •

edited

mtrezza commented Mar 21, 2024

jimnor0xF commented Mar 22, 2024 •

edited

mtrezza left a comment •

edited

jimnor0xF commented Mar 26, 2024 •

edited

mtrezza commented Mar 26, 2024

jimnor0xF commented Mar 28, 2024

mtrezza left a comment

jimnor0xF commented Mar 30, 2024

parseplatformorg commented Mar 30, 2024

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

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

Conversation

jimnor0xF commented Mar 3, 2024 • edited

New Pull Request Checklist

Issue Description

Approach

Changes to how GCM payloads are represented in FCM

TODOs before merging

Footnotes

parse-github-assistant bot commented Mar 3, 2024 • edited

Thanks for opening this pull request!

codecov bot commented Mar 3, 2024 • edited

Codecov Report

jimnor0xF commented Mar 3, 2024 • edited

pcg92 commented Mar 5, 2024

jimnor0xF commented Mar 5, 2024 • edited

pcg92 commented Mar 5, 2024

jimnor0xF commented Mar 5, 2024 • edited

pcg92 commented Mar 5, 2024

jimnor0xF commented Mar 5, 2024

mtrezza commented Mar 5, 2024 • edited

jimnor0xF commented Mar 5, 2024

mtrezza commented Mar 6, 2024

jimnor0xF commented Mar 12, 2024 • edited

mtrezza commented Mar 21, 2024

mtrezza commented Mar 21, 2024 • edited

mtrezza commented Mar 21, 2024

jimnor0xF commented Mar 22, 2024 • edited

mtrezza left a comment • edited

Choose a reason for hiding this comment

jimnor0xF commented Mar 26, 2024 • edited

mtrezza commented Mar 26, 2024

jimnor0xF commented Mar 28, 2024

mtrezza left a comment

Choose a reason for hiding this comment

jimnor0xF commented Mar 30, 2024

parseplatformorg commented Mar 30, 2024

jimnor0xF commented Mar 3, 2024 •

edited

parse-github-assistant bot commented Mar 3, 2024 •

edited

codecov bot commented Mar 3, 2024 •

edited

jimnor0xF commented Mar 3, 2024 •

edited

jimnor0xF commented Mar 5, 2024 •

edited

jimnor0xF commented Mar 5, 2024 •

edited

mtrezza commented Mar 5, 2024 •

edited

jimnor0xF commented Mar 12, 2024 •

edited

mtrezza commented Mar 21, 2024 •

edited

jimnor0xF commented Mar 22, 2024 •

edited

mtrezza left a comment •

edited

jimnor0xF commented Mar 26, 2024 •

edited