APN errors not retrieved properly + doc update?

[sinedied]

I had a hard time understanding why my iOS push notifications suddenly did not work, and went through multiple issues:

• The topic property is now REQUIRED since node-apn changed the way to connect to APNs (thanks for the semver breaking BTW...)

• To find the previous issue, I had to dig in the node-apn code to reveal the error, as node-pushnotifications did not show the error and only shows a successful promise with a failure count and no error message, even though the error message is return from node-apn. It seems that node-apn may have changed the way it returns errors, and the result is that we cannot see the underlying error when a notification sending fails.

It would be nice to fix the doc regarding the topic property and to investigate why error messages are not reported correctly.

[appfeel]

I'm little confused, what is required? topic or token? Please, could you check with node-apn if this is really required (or point in this issue the link where you found that this is now required)?

Regarding the error, this may be because it is an error in Apple platform, not in node-apn, neither in node-pushnotifications, so I'm sorry but the plugins can't detect them if Apple does not provide more information about the error. But I'm not sure that Apple is not providing enought info...

[sinedied]

Sorry I made a typo, this is the topic that is now required.

There is only a single line in their doc indicating this: https://github.com/node-apn/node-apn/blob/master/doc/notification.markdown#notificationtopic

The example have not been updated...

I found this essentially by tracing the error in the node-apn lib and found that all my notification were rejected with a MissingTopic error, then found this :/ Node apn report the error correctly in their promise (rejection with a 400 status and error message to MissingTopic), but node-pushnotifications does not expose it.

[appfeel]

Hmm... that's a little bit messing. Regarding to documentate it in readme.md it's easy.

Regarding to catch the error, the main purpose of the library was to standarize the response:

            (response.failed || []).forEach((failure) => {
                resumed.failure += 1;
                if (failure.error) {
                    // A transport-level error occurred (e.g. network problem)
                    resumed.message.push({
                        regId: failure.device,
                        error: failure.error,
                    });
                } else {
                    // `failure.status` is the HTTP status code
                    // `failure.response` is the JSON payload
                    resumed.message.push({
                        regId: failure.device,
                        error: new Error(failure.response.reason || failure.response),
                    });
                }
            });

So when there is an error in node-apn the error is directly streamed to the resumed.messages. If there is no failure.error info then a new error is generated with the failure.response info (comming from Apple).

I can't find where the MissingTopic error is raised. Could you point me to the line in node-apn where this error is generated? Maybe in this way I could try to figure out how to stream the error...

Also would be nice to have the failure object (if you could place a console.log of it) when this error is produced, in order to understand what is comming from node-apn. If the error is not comming, node-pushnotifications is not able to catch it and so it cannot be emmited...

[sinedied]

The error is not written by node-apn it is reported by the push API endpoint and reported by node-apn. I did not investigate why the second handler in the code you posted did not catch the message, but it's definitely somewhere around.

[jamesholcomb]

Seeing this as well for error response BadDeviceToken. I just get "error": {} in the return value.

[flipace]

I also just lost quiet some time due to this issue - i temporarily added a console log in the apn module so i could debug better.

[alex-friedl]

Hello @sinedied, @appfeel, @miqmago regarding the topic parameter I created a PR for the README update (https://github.com/appfeel/node-pushnotifications/pull/68) as a follow-up to https://github.com/appfeel/node-pushnotifications/issues/67

[alex-friedl]

Hello @flipace , @jamesholcomb , @sinedied ,

I looked into this old issue again and cannot quite make sense of it. The implementation

(response.failed || []).forEach((failure) => {
                resumed.failure += 1;
                if (failure.error) {
                    // A transport-level error occurred (e.g. network problem)
                    resumed.message.push({
                        regId: failure.device,
                        error: failure.error,
                    });
                } else {
                    // `failure.status` is the HTTP status code
                    // `failure.response` is the JSON payload
                    resumed.message.push({
                        regId: failure.device,
                        error: new Error(failure.response.reason || failure.response),
                    });
                }
            });

looks good to me upon first glance, after looking at https://github.com/node-apn/node-apn/blob/master/doc/provider.markdown#connectionsendnotification-recipients

Maybe the error response is really empty or there is a bug or wrong documentation in node-apn? Unfortunately I do not have an active Apple subscription. Could you provide more detailed logs from the actual apn response you are receiving?

Thank you for your support!

[sinedied]

I'm not working on the project anymore so I don't have the sources to show, but from memory the error was swallowed at a lower level than what you code shows, and the issue was that the low level error was incorrectly transformed and transformed resoluting in an empty error message.

[alex-friedl]

@sinedied at a lower level meaning the node-apn dependency then I guess?

[sinedied]

It's been more than a year so I'm not sure, but I think that failure.response was always returning undefined, so it may be related to node-apn, either the doc is outdated or something else occurred.

[blhio]

Changing line 97 in sendAPN.js from

error: new Error(failure.response.reason || failure.response)

to

error: failure.response.reason || failure.response

gives me the apn error as expected:

[{"method":"apn","success":0,"failure":1,"message":[{"regId":"-","error":"TopicDisallowed"}]}]

@alex-friedl thoughts on removing new Error()?

[alex-friedl]

Hi @blhio, thank you for your support! Your discovery actually makes sense.

Could you try if const err = failure.response ? new Error(failure.response.reason) : failure.error and using that in line 97 does the trick?

Thanks!

[alex-friedl]

I looked into the code in question again and now I am confused again. I forgot that the failure.error case is handled further up in the code.

To not pass an Error object would break the API, because that's what is the expected type of the error property.

Could you simply try error: new Error(failure.response.reason) ?

If you have the means to debug apn, maybe also look into the failure object and check if it looks as expected?

[blhio]

Hey thanks for answering, I'm not too well versed in JS so I figured it might break something I didn't realise.

error: new Error(failure.response.reason) gives an empty error object as well.

The failure object:

{ device: 'e0203120dd162d639d19qwertyasdf',
  status: '400',
  response: { reason: 'TopicDisallowed' } }

Not sure if helpful but console.log(new Error(failure.response.reason)) returns Error: TopicDisallowed

[alex-friedl]

@blhio thanks again for your help with this! I thought about it some more. Considering that the tests for the line in question assert that the Error object is created correctly I was wondering if the behaviour maybe relates to logging behaviour of the Error object in javascript.

Could do set a breakpoint at the point where you receive the results array and inspect if the Error object has been created correctly with the proper error message? If yes and the log output shows the empty object, it probably has something to do with the console logger only printing enumerable properties of objects and Error behaving differently in that regard.

Edit: If the behaviour is related to logging, which I currently think, then https://stackoverflow.com/questions/18391212/is-it-not-possible-to-stringify-an-error-using-json-stringify is probably the reason. I guess JSON.stringify is called on the array containing the results and the error object is being printed as empty ?

[alex-friedl]

Hello @sinedied , @blhio , I created https://github.com/appfeel/node-pushnotifications/pull/86 in which the error object is replaced in favor of a simple string with the error message. This should ensure that the error response is printed clearly when using JSON.stringify to print the result. Could you test if this change satisfy your needs?

I am not quite sure about this change though because it might break applications which have a custom workaround in place and expect an Error object. @appfeel what do you think? can we move forward with that PR?

[alex-friedl]

Hello @sinedied , @blhio , @miqmago and I worked on a change that introduces a new errorMsg property which provides the plain error message and should work well with JSON.stringify(). The error property still holds an Error instance with stack etc. for detailed debugging. v1.0.22 provides the new property.