`push.listSubscriptionsImpl` method not respecting params

[uxduck]

Introduction

Users may decide to list push subscriptions by clientId or deviceId. In ably-java, this is currently not possible, and a request to listSubscriptions by clientId or deviceId will return a list of both.

Implementation/ bug

In PushChannel.java, A user has the option to pass in params into all methods related to listing subscriptions, such as listSubscriptions and listSubscriptionsAsync. They may choose to add deviceId: "some device id" as a param, or leave device ID blank. Unfortunately, all these params are completely ignored/ forgotten about in listSubscriptionsImpl. It will reset the deviceId in params, even if it were not set. This results in all subscriptions being listed and sent back to the device.

Proposed solution

If params are passed in, they should be respected, if no deviceID is added, ably-java should not add deviceId to the params sent in the HTTP request.

Optionally, we could add a convenience method, listSubscriptions() with no arguments which returns all subscriptions by the device or client ID.

┆Issue is synchronized with this Jira Bug by Unito

[martin-morek]

@ben-xd thanks for such a detailed bug description, however I still have a few follow up questions. Based on the current implementation deviceId has always be fill in also it always pick id of current device. Should this be customisable (e.g. user can specify any device id)? What should be in params if neither clientId nor deviceId is provided?

[uxduck]

Should this be customisable (e.g. user can specify any device id)?

Yes, because it's actually a push admin HTTP/ rest API. Unfortunately, you would only know this if you use a token/ API key which doesn't have the push admin capability: you'd get an error back from the server. I hope it's more obvious than that somewhere, but I don't think it is. I just tested it again, by disabling the Push Admin capability. You'd get back action not permitted on both ably-java and ably-cocoa. Can you make sure it is obvious in the future, or suggest a way? I don't think its even in the Push Admin API docs is it?

What should be in params if neither clientId nor deviceId is provided?

I initially would have said the params should still have the channel name (as currently added in Ably-java already), since this an instance method of PushChannel. And if no other params are set, an error message should be returned by the server. However, I just discovered that in Ably-cocoa, all device subscriptions will be returned, because the channel name is not set as a param automatically. I've just tested this, and compared between iOS and Android, running in Ably-flutter. Not just the ones associated with the ARTPushChannel, even though this is the instance method. In the screenshot below, both devices are subscribed by clientId to 2 push channels,push:test-push-channel and push:another-push-channel. When calling listSubscription on ARTPushChannel on iOS, you get both of them. In ably-java, when calling listSubscription(deviceId), you still get the clientId subscription returned, but not the subscription to the other channel:

So it seems like you have 2 options:

• I think this is better: Be consistent with Ably-cocoa and do not set any params, including channel param. (Edit, see next comment below, you do need to set concatFilters.
• Automatically set the channel param, since this is an instance of PushChannel after all.

[uxduck]

I also just saw RSH7e:

#listSubscriptions(params) performs a GET request to /push/channelSubscriptions and returns a paginated result with PushChannelSubscription objects filtered by the provided params, the channel name, the device ID, and the client ID if it exists, as supported by the REST API. A concatFilters param needs to be set to true as well.

The params are provided, not taken from some state. if it exists is definitely confusing. However, you do need to set concatFilters.

It would be very nice if Ably-java can take care of Param instantiation, and create something like the documentation (probably the goal initially, but never implemented):

PaginatedResult<PushChannelSubscription> listSubscriptions(String deviceId, String clientId, String deviceClientId, String channel)

Probably with @Nullable too though, or multiple method overloads. Whatever you think is right. :)

[martin-morek]

Thank you for more information. I've investigated the issue further and sent GET request to push/channelSubscriptions with only concatFilters=true parameter set. I got an error HTTP 400 from the server:
"expected parameter 'deviceId', 'clientId', 'deviceClientId', and/or 'channel'. (See https://help.ably.io/error/40000 for help.)"

It seems to me that some parameters have to be provided. Unfortunately based on what read out of documentation it's unclear to me. How should it proceed if params are empty?

[uxduck]

And if no other params are set, an error message should be returned by the server.

I mean if the params are empty, you will get the error. Just pass that error to the user. But still set concatFilters=true. WDYT? This is the same behaviour in Ably-cocoa.

[martin-morek]

That's absolutely fine for me if this is an expected behaviour. I've just thought that maybe if none params are provided some default one should be set (like we do currently).

[uxduck]

I believe it the desired behaviour, as per RSH7e mentioned here. :)