Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APNs: log invalid device tokens, rather than sending them #3888

Merged
merged 1 commit into from
Feb 11, 2020

Conversation

rk-for-zulip
Copy link
Contributor

APNs device tokens, as received through out iOS notifications library,
should be a hex string. Unfortunately, they're sometimes being sent to
the server as "[Object object]".

This is most likely due to the argument of handleDeviceToken
actually being an object, despite its typing as string, and being
improperly stringified by encodeParamsForUrl. However, it may
alternatively be the case that someone is mis-stringifying the object
before we get ahold of it, so log that case too.

This is work toward the resolution of #3672.

@rk-for-zulip rk-for-zulip requested a review from gnprice February 10, 2020 18:42
@rk-for-zulip
Copy link
Contributor Author

⚠️ This PR is untested, due to the difficulty (noted elsewhere) of testing iOS notifications code.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ray-kraesig ! This should be helpful.

One style comment below.

Comment on lines 213 to 221
// A device token should normally be a string of hex digits. Sometimes,
// however, we appear to receive objects here. Log this. (See issue #3672.)
if (typeof deviceToken === 'string' && deviceToken !== '[Object object]') {
this.dispatch(gotPushToken(deviceToken));
await this.dispatch(sendAllPushToken());
} else {
logging.error('Received non-string device token', { deviceToken });
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's write this error path in the early-return pattern:

  if (isBad(thing)) {
    logging.error('Thing is bad', { thing });
    return;
  }

  processNormally(thing);

This keeps the code that handles each error next to the code that tests for it. A related effect is that it helps the reader to study the behavior in a given error case, or the behavior in the "normal"/happy case, or to understand the full behavior by divide-and-conquer.

Doesn't hugely matter for a function this short with just one error case. But it's good to have the pattern consistent for if the function grows, or if another contributor copies the pattern in another context. And even at this scale I think it already helps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I actually did write it that way in the first draft; I switched to the above because the negated logic seemed more straightforward.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Yeah, I think the main thing that can make this nested style appealing is that it lets you write down the things that are true in the happy path, rather than the things that aren't.

But then I think the effect on the code's overall structure generally ends up outweighing that, even at this scale.

Thanks for the revision!

@rk-for-zulip
Copy link
Contributor Author

Done. Also, more honesty about being dishonest about types.

APNs device tokens, as received through out iOS notifications library,
should be a hex string. Unfortunately, they're sometimes being sent to
the server as "[Object object]".

This is most likely due to the argument of `handleDeviceToken`
actually being an object, despite its typing as `string`, and being
improperly stringified by `encodeParamsForUrl`. However, it may
alternatively be the case that someone is mis-stringifying the object
before we get our hands on it, so log that case too.

This is work toward the resolution of zulip#3672.
@gnprice
Copy link
Member

gnprice commented Feb 11, 2020

Thanks -- merged!

@gnprice gnprice merged commit d7b68bd into zulip:master Feb 11, 2020
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this pull request Feb 13, 2020
When no registration token is available, Android may report a null
token. This is has been observed to happen at least on an emulator
without Google Play services, and is reported to happen on physical
devices without GP, as well as on devices without Internet access.

Accept null device tokens without comment, effectively returning to
pre-zulip#3888 behavior for them. Adjust the types of `gotPushToken` _et
al._, and the docstring of `pushToken`, to reflect this.
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this pull request Feb 18, 2020
When no registration token is available, Android may report a null
token. This is has been observed to happen at least on an emulator
without Google Play services, and is reported to happen on physical
devices without GP, as well as on devices without Internet access.

Accept null device tokens without comment, effectively returning to
pre-zulip#3888 behavior for them. Adjust the types of `gotPushToken` _et
al._, and the docstring of `pushToken`, to reflect this.
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this pull request Feb 21, 2020
When no registration token is available, Android may report a null
token. This is has been observed to happen at least on an emulator
without Google Play services, and is reported to happen on physical
devices without GP, as well as on devices without Internet access.

Accept null device tokens without comment, effectively returning to
pre-zulip#3888 behavior for them. Adjust the types of `gotPushToken` _et
al._, and the docstring of `pushToken`, to reflect this.
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this pull request Feb 21, 2020
When no registration token is available, Android may report a null
token. This is has been observed to happen at least on an emulator
without Google Play services, and is reported to happen on physical
devices without GP, as well as on devices without Internet access.

Accept null device tokens without comment, effectively returning to
pre-zulip#3888 behavior for them. Adjust the types of `gotPushToken` _et
al._ and the docstring of `pushToken` to reflect this.

Additionally, as issue zulip#3672 is resolved, we no longer need an
explanatory comment pointing to it. Remove the now-confirmed-
unnecessary check for the exact string `"[Object object]"`.
@rk-for-zulip rk-for-zulip deleted the log-object-tokens branch February 22, 2020 01:23
rk-for-zulip added a commit to rk-for-zulip/zulip-mobile that referenced this pull request Mar 6, 2020
When no registration token is available, Android may report a null
token. This is has been observed to happen at least on an emulator
without Google Play services, and is reported to happen on physical
devices without GP, as well as on devices without Internet access.

Accept null device tokens without comment, effectively returning to
pre-zulip#3888 behavior for them. Adjust the types of `gotPushToken` _et
al._, and the docstring of `pushToken`, to reflect this.
gnprice pushed a commit to rk-for-zulip/zulip-mobile that referenced this pull request Mar 6, 2020
When no registration token is available, Android may report a null
token. This is has been observed to happen at least on an emulator
without Google Play services, and is reported to happen on physical
devices without GP, as well as on devices without Internet access.

Accept null device tokens without comment, effectively returning to
pre-d7b68bddc (zulip#3888) behavior for them. Adjust the types of
`gotPushToken` _et al._, and the docstring of `pushToken`, to
reflect this.
@gnprice gnprice mentioned this pull request Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants