Parse.Push.send() ignores ParseQuery limit and skip parameters

[shlusiak]

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 JS SDK.

Issue Description

In our attempts to deal with rate limiting when sending push notifications to a huge installation base, we tried to batch them and only send 5000 push noticiations per 30 seconds. We tried to set limit and skip for a ParseQuery to loop until all items are iterated on. This is our code:

async function batchPush(query: Parse.Query<Parse.Installation>, data: PushData, total: number) {
  let skip = 0;
  const batchSize = 5_000;
  const interBatchDelay = 30 * 1_000; // 30,000ms = 30 Seconds

  query.limit(batchSize);
  query.skip(skip);

  while (skip < total) {
    await Parse.Push.send({ where: query, data }, { useMasterKey: true });
    await sleep(interBatchDelay);
    skip = skip + batchSize;
    query.skip(skip);
  }
  return true;
}

But as it turns out, the limit and skip values from the ParseQuery are ignored by Parse.Push.send, sending it to all installations instead:

Parse-SDK-JS/src/Push.js

Lines 54 to 57 in 98674f0

	export function send(data: PushData, options?: FullOptions = {}): Promise {
	if (data.where && data.where instanceof ParseQuery) {
	data.where = data.where.toJSON().where;
	}

It seems it will only look at the where part of the ParseQuery, ignoring the limit and skip. As a result our loop still sends notifications to the entire installation base.

Steps to reproduce

Set limit and skip of a ParseQuery used in Parse.Push.send

Actual Outcome

limit and skip are ignored

Expected Outcome

Only the specified range should receive push notifications.

Environment

Client

• Parse JS SDK version: 4.1.0

[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.

[mtrezza]

I'm pretty sure that push notifications are sent to the push provider API in batches internally in the push adapter. At least for FCM I think, if not also for APNS.

In any case, if the installation query fetched all objects at once that would not be optimal, as it would need to hold potentially large amounts of data in memory. The risk of using limit and skip is of course, if the underlying data changes during batches, then devices may receive multiple pushes or no push at all; it would be the developer's responsibility to take that into account. The solution to this - at least for MongoDB - would be to search and hold on to the result pointer. The pointer can then be moved over a snapshot of the search results to retrieve the data in batches. That would be unrelated to the current skip and limit behavior and require a new feature implementation in the Parse client SDK and Parse Server.

• Add pagination with cursor parse-server#7637.

[thecolourfool]

Thanks for your thoughtful response. I'm working with @shlusiak on this. So you're saying you believe the omission of skip and limit here are intentional, and reflect the fact that they should not be used because batching is done elsewhere?

Interestingly the reason we are attempting a batching system at our end is that we observed that the number of successful deliveries of a bulk push to all users of our app, dropped by about a third overnight, and has remained at that lower level ever since. We could not find a reason for this. After some discussion on the Parse Community Slack group, we decided that we may be trying to send too many pushes at the same time, and a batching process at our end could alleviate that. If batching is already implemented in the adaptor, then we are probably incorrect in trying that.

[mtrezza]

Yes, not supporting these pagination params could have been intentional, but these risk are not unique to the Installation class, so I don't see any reason we couldn't allow them.

Sending in batches may not be supported in the push adapter for all APIs, you would need to check.

You can turn on push adapter verbose logs to see why requests are failing. In fact, if they fail with an error, that error should be logged in any case in the Parse Server logs, including the API response code and message.

[thecolourfool]

Yes, we've done that, but we're having trouble interpreting what we're seeing.

Without wanting to turn this issue report into an impromptu support discussion, I see you've been quite active in discussions of push issues. Do you know if there could be an issue with an excessive number of invalid device tokens? We've never performed a cleanup, and even before the sudden drop in deliveries, our successful deliveries were only about a quarter of our total installations. Is there, by any chance, a point at which APNS bails out due to excessive invalid tokens, or anything like that?

[mtrezza]

APNS may in fact bail out due to excessive invalid device tokens, it would return error 429 Too Many Requests.

In more detail, APNS does not in every case know whether a device token is valid or not. This is an effect of how Apple invalidates device tokens and tracks app uninstalls on end-user devices. In any case it's the end-user device that makes the final decision on whether to display the push notifications to the user. That can cause invalid device tokens sent by Parse Server to not receive a 410 Unregistered or 400 BadDeviceToken response from APNS. Parse Server will not clean these tokens up and consider them valid.

How this could affect your app varies. Invalid device tokens that are not reported as invalid by APNS and don't get cleaned up will of course increase the ratio of failed push notifications over time. This can lead to edge cases, for example, if your users often un- and then re-install your app, then an individual user may have many Parse Server Installation objects with device tokens of which only 1 token is currently valid. If your push query selects all these installations where a specific user is set, then APNS may reject push notifications on a "per device" (not per token) quota. This is different from general API throttling and may affect only specific users. It really depends on what your scenario is, what errors you're getting from APNS and what your users are reporting.

Also maybe interesting: parse-community/parse-server-push-adapter#107 (comment)

[mtrezza]

I think we may close this specific issue, unless you still think that limit and skip is something you would need for your use case.

[shlusiak]

Thank you so much for the elaborate answer, this is greatly appreciated. We are happy for this to be closed if you think this is by design. We were hoping that we could throttle the notifications in our client code and that this could fix the rate limiting, but we will see that we can clean up the invalid tokens and see if the situation improves.

[mtrezza]

I don't see how PushQuery could usefully support the standard limit and skip params due to the above mentioned side effects, but we could of course implement it. It is somewhat unintuitive that these params are not supported for push, because the limitations are not specific to the Installation class. For for consistency let's leave this issue open.

I think the broader solution for your specific issue is to look more closely at the batch algorithms in the push adapter and maybe introduce a customizable batchSize option, but that would be a different issue. The underlying issue could of course also be something specific to your app, unrelated to Parse Server.

[cbaker6]

For reference, some additional details about push with query can be found in the original REST documentation, sending pushes to queries. The doc examples mirror the current JS SDK implantation of just using where, but also gives some advice on different ways to constrain notifications.