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.

[jimnor0xF]

@chadpav
Have not read through the entire discussion
here, but you can access the raw FCM API without any abstractions by putting it inside a key rawPayload.

Example:

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

[chadpav]

@jimnor0xF I was wondering about that key but never looked into it. That is what I'll probably end up doing. Thanks!

[mtrezza]

How can we resolve this issue in the sense of #286 (comment)? Update the docs? Any suggestions?

[mman]

I am +1 for updating the docs. The syntax @chadpav used actually exists in one of the early parse server docs but was likely not supported for many years now. And we already support mix of both a/ and b/ so adding more redundant functionality to a/ will just make things more complex.

[chadpav]

I think you are right that we should document the payload that Parse Server supports. If we include the rawPayload in the docs then it unblocks most people. While I still believe the fix that I provided is a legit bug, I can agree with the logic that we shouldn't go changing things when it's not the intended syntax to begin with.

The only issue that I see is that I'm not certain that Parse Server is even consistent with its API. By that I mean that the API might be different depending on how you configure the push adapter that ships with Parse Server (e.g., APNS vs FCM configuration for iOS devices). I could be wrong on that.

Is there anything you need from me?

[chadpav]

@mtrezza side note but I'm running the latest Parse Dashboard and when I send messages to my Android device they don't display to the user. Although I do receive them, they are data only messages. I need to step into the code to see if the Parse Dashboard is using the correct syntax. I'll get back to you on that.

[mtrezza]

All good ideas. It's actually a good time to clean this up now. We're just ~4 months from Parse Server 8 release where we can introduce breaking changes.

If there is a legacy syntax that we want to remove, then let's prepare a PR for that now, since it's fresh in our minds. We'll merge it in Nov as a breaking change and then bundle it with Parse Server 8.

We could already update the docs now.

[jimnor0xF]

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.

In the long term, if breaking changes are allowed, I'd prefer to not do a) at all and only do b).

a) takes a fair amount of maintenance and keeping a consistent standard without letting it rot over time is difficult.

Easier to just link to the push provider docs for the payload structure.

For now I think updating the docs a bit more is a good idea.

[mtrezza]

I'd prefer to not do a) at all and only do b)

For developers using providers directly (instead of FCM as an intermediary for all other providers), this would require them to recreate the same logic for a universal internal API. Not sure how much sense it would make for us to remove that logic and roll the burden over to every individual developer, doesn't sound very efficient. If someone has a special use case and wants to deal with 3rd party documentation to use (b) to send a raw, unchecked payload to a specific provider, they are free to do that. But given that push notifications are an integral part of mobile apps, we should probably keep the convenience of an abstracted Parse API.

[mtrezza]

How should we proceed here?

[jimnor0xF]

@mtrezza

For developers using providers directly (instead of FCM as an intermediary for all other providers), this would require them to recreate the same logic for a universal internal API.

For me personally at least it would still be interesting if you could simply specify the exact payload for each provider you'd want to use. Sort of a "thin wrapper" approach.

Something like:

{
  "apns": {
   "key": value",
  },
  "fcm": {
    "key": "value",
  },
  "onesignal": {
    "key": "value",
  }
}

I admit this is more verbose than providing an abstracted payload. But, also means:

1. Less maintenance required and more control given to the developer. No need for updates to the abstraction when a provider adds new keys or to extend what is possible with the abstraction.

2. No payload conflicts. In current state, keys only intended for APNS will also be sent to FCM for example if you intend to use FCM for android and APNS for apple.

You'd need to be able to map between device type and when to use which provider, and have this configurable by the server admin on the backend of course. When it's an android device, use fcm, when apple device use apns, default use this and so on.

But yeah, anyway, just my opinion on things. In practice, not sure how doable above proposal would be while also avoiding breaking changes. I'm pretty happy with how things are currently since I just use FCM for everything.

[mtrezza]

I don't see a raw provider API and an abstracted API as mutually exclusive. They can live side by side. The abstracted API can cover the common denominator across all push APIs. The raw API can be used for provider specific flavors. How the overriding behavior works if both are defined, or whether one has to choose one or the other can be worked out. If we remove the abstracted API it would be less maintenance work (but how much less really?) but Parse wouldn't provide an easy API to send out pushes across multiple ecosystems. I think that is quite a neat feature of Parse.

To your point of "payload conflicts", there should be none. Maybe that's a separate issue?

[jimnor0xF]

@mtrezza
I think having a raw provider API + abstracted API is probably the way to go then. Satisfies both sides.

We would need to standardize what the abstracted API should look like if we're going forward with this. Since currently it's a bit unclear to me whether the current one that is used for APNS/FCM is good or not.

What we probably should do is to have a handler that converts the abstracted payload into whatever raw payload the configured provider(s) need before it is sent to the module that deals with the sending part. That way you only send what is necessary according to the provider.

To your point of "payload conflicts", there should be none. Maybe that's a separate issue?

Yeah, not really related to our discussion. More so related to how things are handled in the code currently.
For example here, we actually delete APNS-related keys if present when sending via FCM for android. If you have WEB configured as a provider in tandem, it will also send APNS/FCM related keys to WEB if you plan on having one single payload that fits all providers.

[mtrezza]

We could use a model like the parse-server-api-mail-adapter where many internal converters are used to convert a generic payload to provider specific payloads. To support a new provider in the future, it would just add a new converter. That creates a clear payload pipeline. I don't think this adapter is far from it.