iOS Title property is not received when Alert property is a string

[chadpav]

New Issue Checklist

• I am not disclosing a vulnerability.
• I am not just asking a question.
• I have searched through existing issues.
• I can reproduce the issue with the latest versions of Parse Server and the Parse Server Push Adapter.

Issue Description

iOS devices were only receiving the alert body. The alert title was not showing up. This is the payload I tested with.

const result = await Parse.Push.send(
    {
      where: pushQuery,
      // WORKS FOR ANDROID
      notification: {
        title: 'Android title',
        body: 'Android notification body',
      },
      sound: 'default',
      // WORKS FOR IOS
      data: {
        // alert: { title: 'iOS data alert title', body: 'iOS data alert body' }, // <-- this works, avoids the bug in the adapter code, and sets both title+body
        title: 'iOS data title',
        alert: 'iOS data alert', // <-- will overwrite the title key
        priority: '10',
        collapse_id: '1',
      },
    },
    {
      useMasterKey: true,
    }
  )

Steps to reproduce

1. Configure project using FCM to send iOS push notifications.
2. Send notification to an iOS device with the payload provided above.

Actual Outcome

• Push notification is received, however the title was missing.

Expected Outcome

• Should have received both title and alert message.

Environment

• "@parse/push-adapter": "^6.5.0"
• "parse-server": "~7.2.0"

Logs

• not relevant, logs stated that push notification was sent successfully.

[parse-github-assistant]

Thanks for opening this issue!

• 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

[chadpav]

I stepped into the push-adapter code and figured out the issue. The code was initializing the apns.payload.aps.alert key before setting the body key. This is a code snippet from FCM.js in _APNStoFCMPayload():

    case 'alert':
      if (typeof coreData.alert == 'object') {
        // When we receive a dictionary, use as is to remain
        // compatible with how the APNS.js + node-apn work
        apnsPayload['apns']['payload']['aps']['alert'] = coreData.alert;
      } else {
        // When we receive a value, prepare `alert` dictionary
        // and set its `body` property
        apnsPayload['apns']['payload']['aps']['alert'] = {};
        apnsPayload['apns']['payload']['aps']['alert']['body'] = coreData.alert;
      }
      break;

I learned through reading the code that I could send both alert/title as an object as a workaround.

I'm submitting a PR with a failing test + patch.

[mman]

@chadpav Quickly chiming in... If I remember correctly, there are two ways to send a push...

1. Body only by setting alert: "body text"
2. Body and title by setting alert: { title: "title", body: "body text" }.

This is in line with how Apple evolved the JSON payload over time, there was/is no other way - so trying to pass alert and title in a flat structure is not expected to work. And never actually worked before...

I may be missing something, but your example should never have worked before, or was it?
Martin

[chadpav]

You are correct that it wasn't working when passing alert as a string instead of an object. I just lost an afternoon troubleshooting why it wasn't working. I did discover the alternate method by reading the adapter source code but felt that I could save someone else the pain by providing a simple fix.

Reading the source code you can tell that it was supposed to work. There is a case statement that specifically handles the title and alert keys as strings.

It was not obvious what payload the adapter is expecting in order to send notifications to iOS and Android devices. Maybe I'll provide a more complete code sample in the readme with my PR.

[mman]

@chadpav The point is that currently supported syntax is the one officially supported by Apple APNS, and only passed one to one via FCM to APNS.

The one you are proposing is artificial new syntax that is not expected by anybody...

am I missing something?

[chadpav]

@mman ok, I hear what you are saying. Let me check the Apple APNS docs and get back to you. On the surface it makes no sense to support setting title and alert as a string and then silently eating the title key. It looks like a bug. If your point is that is just how APNS works and we want to mirror that then I understand your point.

If I have to study the Parse adapter source + docs, FCM docs, and APNS docs in order to work out what the correct payload is then maybe we could provide more code samples or even a validator to give feedback?

[chadpav]

@mman I checked the APNS docs and I can see where you are coming from. However, I don't think that's the whole story.

I have my project configured to send notifications through FCM for both iOS and Android (i.e., not through APNS). I think the docs I should be reading are the FCM Notification message docs. FCM is "adapting" my message to APNS so that I shouldn't be reading the APNS docs.

If I were to be very literal about keeping Parse as close to the underlying platform API spec then we should support the FCM "common fields" as documented. In fact, that is what I really wanted... to provide one title/body in Parse but target both iOS/Android devices without having to do platform specific stuff.

Thoughts?

this should work:

const result = await Parse.Push.send(
    {
      where: pushQuery,
      // ONLY WORKS FOR ANDROID BUT FCM SAYS IS SHOULD WORK FOR BOTH
      notification: {
        title: 'common title',
        body: 'common body',
      },
    },
    {
      useMasterKey: true,
    }
  )

[mman]

Yeah, I have been there as well multiple times.

First came iOS with some simple syntax, then they amended the syntax to support title, subtitle, and then they were adding more and more. My favourite was content-available: 1 where you can not pass anything else but number otherwise stuff breaks... we have even tests for this.

Then came Android with their own GCM syntax... and Parse Push adapter added code to send iOS formatted pushes properly to Android as well via GCM... while skipping unsupported fields. Then came FCM with their own syntax and code was added to transform it to iOS payload, or Android payload... depending on destination. and this is where we are now.

So it is absolutely possible to send one payload to multiple destinations regardless of what adapter you talk to - once you stick to currently supported payload format - but what is missing is proper documentation so that you do not need to lookup the code, and you do not hit obstacles as you did.

Adding more potentially valid input variations will only make "adding the proper documentation" task more complex...

So I think what is needed is to add more detailed documentation of the currently supported payload format while documenting all caveats and features supported on all platforms... and/or possibly add a validator that may catch invalid payload during runtime... that would definitely help...

Otherwise we end up in a state where we have

1. APNS payload syntax
2. FCM payload syntax
3. Parse Server Push Adapter payload syntax

with 1. and 2. being used in the wild with no easy/clear path to migrate towards 3. So we will have to support 1. and 2. indefinitely anyway...

so +1 for docs, and +1 for validator

just my .2 cents,
Martin

[chadpav]

@mman following up with what the Parse Server guide says... what I was trying to do is documented here. Although that documentation is really light and old.

The parse-server-push-adapter doesn't cover any of this syntax or what is supported. It only covers how to configure the adapter.

curl -X POST \
  -H "X-Parse-Application-Id: you_app_id" \
  -H "X-Parse-Master-Key: your_master_key" \
  -H "Content-Type: application/json" \
  -d '{
        "where": {
          "deviceType": {
            "$in": [
              "ios",
              "android"
            ]
          }
        },
        "data": {
          "title": "The Shining",
          "alert": "All work and no play makes Jack a dull boy."
        }
      }'\   http://your_server_address/parse/push

[mman]

@chadpav Excellent catch, exactly as you say... this is one of the oldest docs that was not updated in ages... and more docs missing... although existence of this example may prove that it actually was supposed to work this way at some point in the past...

[chadpav]

@mman I can see where this slowly got out of hand over the years.

Lets decide what we should do with this issue/PR. Are there others that should weigh in? I see some options, we could close the issue and document the "alert object" syntax here. I almost hate to add a code sample in the readme because I'm not testing every supported configuration. Or we could merge this fix since it's still backward compatible with existing clients and matches the current docs.

I see another, parallel option forward that addresses issue #245. Fork this repo, rename it to parse-server-fcm-push-adapter, remove all other configuration options, add support for the full FCM syntax, add a link in the readme pointing to the FCM documentation that says "that's the api". It's clean and doesn't affect existing users.

☝🏼 that's what I really wanted to exist.

[mman]

I'll let @mtrezza to chime in what he sees as the best approach. I think the Adapter interface should document and support same interface no matter what the underlying implementation may be... so making different push adapters support different syntax of payload is a NO GO for me... as you can not swap them easily... but there may be a precedent that I do not see where different adapter implementations may support different features... (Mongo versus PostgreSQL comes to mind)...

I honestly kind of like the validator approach, and merging your PR to make the existing example work... seems like a good way forward...

[chadpav]

I think that means that Parse Server should have an opinionated, documented API that the adapter translates to it's destination.

I'd also argue that the validation should happen in Parse Server and not each individual adapter if you want to make them truly swappable.

One more thought, you could make a "push v2" api so that you don't have to support legacy baggage. e.g.:

Parse.Pushv2.send(...);

[mtrezza]

Interesting discussion, we have been here before. I think we want to distinguish between 2 types of APIs:

• a) Parse push adapter API:

  • Syntax is designed and documented by Parse
  • Payload is modified by the adapter as needed before being sent to the push provider.
  • Changes influence the module version (e.g. breaking changes)
  • Purpose is to make life easier for developers by providing a unified syntax across push providers.
  • The API is our responsibility to manage.

• b) 3rd party APIs where the adapter just passes through the payload without (much) validation. All the points above do not apply here.

Maybe we just need to define which of the current APIs (or adapter configs / syntax parts) fall into which of the 2 categories above. And then polish the docs a bit.

Note that any breaking changes in the adapter will not be merged until end of the year, because Parse Server comes bundled with the push adapter, and we would not be able to upgrade it there if it contained a breaking change.