search

parse-community / parse-server-push-adapter

A push notification adapter for Parse Server
https://parseplatform.org
MIT License
87 stars 99 forks source link

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

Open chadpav opened 1 month ago

chadpav commented 1 month ago

New Issue Checklist

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

Expected Outcome

Environment

Logs

parse-github-assistant[bot] commented 1 month ago

Thanks for opening this issue!

chadpav commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 4 weeks ago

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

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 commented 3 weeks ago

@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 commented 3 weeks ago

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

mtrezza commented 3 weeks ago

How can we resolve this issue in the sense of https://github.com/parse-community/parse-server-push-adapter/issues/286#issuecomment-2289294545? Update the docs? Any suggestions?

mman commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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.