Read and persist state returned in ARTLocalDevice/ ARTDeviceDetails

[ben-xD]

In trying to make sure behaviour is correct in push notifications, I noticed that the DeviceDetails.DevicePushDetails.state is always null on Android and iOS. A user can get the LocalDevice by calling ARTRealtime#device, to get back ARTLocalDevice, which extends from ARTDeviceDetails, which contains ARTDevicePushDetails. Unfortunately, @property (nullable, nonatomic) NSString *state; is always nil.

It looks like it was never implemented? This value is not null when using the http api/ dashboard, its just the fact that ably-cocoa and ably-java doesn’t ever use it. This value is definitely returned to SDKs which call POST /push/deviceRegistrations. However, it is also returned by other HTTP requests, e.g. PUT rest.ably.io/push/deviceRegistrations/<deviceId>.

Similar ably-java issue: ably/ably-java#697 Note, in ably-java it is an enum: active, failing and failed. It might be worth switching to use an enum. However, it is also null on Ably-java.

┆Issue is synchronized with this Jira Bug by Unito

[maratal]

From slack discussion:

ben.butterworth:
it should be set more often than just in tests? It should be updated when data is received from Ably, so that devices know what state they are? However, it's a field that may change based on the ably servers.
This happens when ably tries to send a message to APNs but if it returns with error, then this state will be updated

ben.butterworth:
that method you showed in screenshot is not used when the device registers/ subscribes, that is used by push admin API when listing device registrations. Push admin is probably a very uncommon feature being used on cocoa (edited)
Maybe we should remove the state field altogether?
its not a very useful field if the client never gets told the error, when the failure happens on the server. Unless the ably-cocoa client can download the latest state from the ably server

So, yes, this field is only updated manually by calling methods with push/deviceRegistrations/<deviceId>. Since there is no way to immediately inform a client app that there was an error while sending push (unless we directly send this error via ARTRealtime object via some diagnostics/system channel) state field will be useless. That's why these methods placed in the Admin namespace, because normally user should not use any of that. All interactions with APNs should be performed on the server, client app only must be subscribed properly (without error during subscription process). I can move ARTDevicePushDetails.h(m) files into the Admin file group, so it would be obvious these are not for customer use. @ben-xD

[ben-xD]

We expose ARTDevicePushDetails to users, I'm not sure if its just an "admin" feature. Here is how we expose it: ARTRealtime and ARTRest expose push property, which is ARTLocalDevice, a subclass of ARTDeviceDetails, which in turn has a property, push, which is ARTDevicePushDetails. This what I mean, to the user, when they check the device details, this push.state is always null.

Regarding your comments:

• I'm not sure which methods specifically you're talking about in the admin file group/ directory. The .push is not an Push Admin feature.
• I'm also not sure when you say "for customer use" Push Admin API is still for customer used, but designed for use on the server side. They are not internal to ably SDK.

[maratal]

Anyways, I mean there is no way to update state property immediately. If users want to know its value they must explicitly call GET push/deviceRegistrations/<deviceId>.

[ben-xD]

• Then perhaps ARTPush#device should be a method that makes that network request to get the state from Ably instead of a property. This way, the user can know for sure if the state/ error related to APNs and FCM is failed when they get the device details. After all, the device details is a network dependent state stored in Ably servers, much like presence, so it should make a network request. We should do this on Ably-java as well.
• My preference: Or perhaps a new method ARTDeviceDetails#getPushState() which should make the network request check this FCM/ APNs state. This would affect the activation state machine less.
• Alternatively, we should completely remove this state and errorReason field from being available when a user calls ARTPush#device because it is currently useless to a user. Note: It is still useful when being used by Push Admin though. ARTDevicePushDetails type is being used in both push admin and the normal push API. We would have to create separate types to do this, or use interfaces? (I'm not great at Obj-c)

@QuintinWillison and @paddybyers, this would be a breaking change but I think its much better to make a network request somewhere, so that this field is not null all the time (except for Push Admin use).

[maratal]

One of possible solutions would be to have a system channel ARTRealtimeChannel through which Ably server could send a diagnostics/control messages to its clients, including any push state changes.

[ben-xD]

Are you suggesting automatically making the client set up a realtime channel permanently to receive any state changes?This is because state change can happen not just when an Ably client is not instantiated, but also when when the app is off/ terminated.

Of course, we cannot use push notifications to update the failed states of Push, since if the state is failed, the push message won't be delivered. If by nature we cannot have it pushed to us, we should request it. Hence my network request suggestion :).

[maratal]

but also when when the app is off/ terminated

True, but server can send all actual information upon system channel successfully attached.

[ben-xD]

What's the point of all this overhead and complexity in both and realtime when we can update it when it matters by making a network request? (the user calls the device method)

[maratal]

Apparently we already have such a method in ARTPushDeviceRegistrations: get:callback:. It takes deviceId as an argument.

[ben-xD]

Yes absolutely, that's what I meant by

Note: It is still useful when being used by Push Admin though. ARTDevicePushDetails type is being used in both push admin and the normal push API.

I think listing a device/ clients own subscriptions (device or client ID) should not require Push Admin, but it currently does because it uses the same exact endpoint.

Please read #1162 (comment)

A concern is if we need to add a new endpoint in realtime (or modify the existing one) so that non push admins can get this information too.

[ben-xD]

I had a call with @paddybyers, here's what I thought he's trying to say:

Concern: HTTP Request synchronizing with Activation State Machine State

Paddy had concerns with my suggestion of making an HTTP request to get device details from the server. This is because we would probably want to synchronise the device details data received with the local device state used by the activation state machine. I think this state is server state, not local device state, so we should be fine with synchronizing data from the server.

Solution

To avoid synchronizing or having the theoretical inconsistency between this method and local device, clients could make a network request to with ARTPush#getPublishError(). You may think state is important, but if an errorReason is non null, then we can assume it failed. Push#getPublishError() just to get the error reason if there was one. We could still include state if you prefer, calling a method called Push#getPublishState() instead. This is specifically getting state related to push publishing: e.g. if it is successful or the errorReason.

Additional things Paddy said:

• Users are expected to call activate on every launch so we validate their device details anyway. (This is not documented anywhere, and I believed we ask users to only call activate once, since the state is saved between app launches. Furthermore, the activation documentation states that it is idempotent, not mentioning any state update:

calling it again when device is already activated has the sole effect of calling its callback.

• state is not a useless field if the ARTPush:activate: updates this state when it completes.
  • I personally think it would be very confusing if state would only change is activate was called. We didn't discuss any current "expected behaviour" mechanism to update this state. (currently its always null when calling Push.device())
  • paddy also suggested also adding an extra case to the DeviceDetails.Push.State enum, to signal that the activation state was not yet ready, e.g. SYNC or IN_PROGRESS. (those specific cases are my ideas)

My thoughts:

I think the bullet points above are problematic. He had to go to another call so I could not discuss further:

• It is confusing if state is only updated when activate is called. users would keep checking the state, and nothing would happen. If a user wants to check on an error, they would have to call activate. This would be confusing for the developer, and future developers/ readability: the code would need comment like: // Calling activate again to check push error state.

So my current suggestion is similar to my previous one, but slightly improved:

• Implement a new method ARTDeviceDetails#getPushState() or ARTPush#getPublishState/Error which should make the network request check this FCM/ APNs state/ errorReason. It does no synchronisation with the state machine to keep it simple.
• Create a new type to be returned for ARTPush.device, same as before, but excluding state and errorReason (they are always null). Keep the existing type to be returned by Push Admin APIs. As @maratal has said in a different way, there is no way that this state is meaningfully updated in the activation state machine for the user.

[maratal]

@ben-xD As a temporary solution I can spit a diagnostic message to the console like "To update 'state' field to an actual value you must call 'ARTPushDeviceRegistrations: get' first". Diagnostic messages is a common practice in Apple to notify API users about incorrect usage of the framework (a lot of them in AutoLayout f.e.).

[ben-xD]

That won't update the field though? Calling ARTPushDeviceRegistrations:get: you mention will just return different instances of ARTLocalDevice/ ARTDeviceDetails?

[QuintinWillison]

Diagnostic messages is a common practice in Apple to notify API users about incorrect usage of the framework (a lot of them in AutoLayout f.e.).

Please don't use this practice. Auto Layout is a very different kettle of fish - and, actually, Apple's 'need' to emit those kind of diagnostics when people get it wrong is perhaps more indicative of problems in their API design. 🤔 In our context, being an SDK that gets embedded into somebody else's app, we can't be brushing stuff like this under the covers like that. Nobody is ever going to see those diagnostics - I guarantee it - as these kind of edge cases are much more likely to be happening at runtime on a customer's device.

[maratal]

@ben-xD You are right, it only gets data, so looks like the new method really needed.

[ben-xD]

Please see ably/specification#26 for the upcoming discussion with Realtime team. We have a video meeting to discuss this on Tuesday 9th November 2021.

[jamienewcomb]

@maratal is this unblocked now based on the discussion in ably/specification#26 ?

[maratal]

@jamienewcomb Yes, seems like it can be unblocked

[maratal]

As mentioned by Ben earlier here - ably/specification#26, updated server endpoint now supports retrieving push state for the current device without Push-Admin capability. Now I'm not sure whether we should move DeviceRegistrations API out of Admin namespace, but meanwhile it is a mistake in one repo or another (cocoa vs java). Shout out to the server and spec/documentation teams for clarification. See #1186 (comment) for the context.

What is now needed to push this issue forward:

1. A test device similar to one with the id "testDeviceDetails" (f.e. "testFailedDeviceDetails") which would always return failed state with some error, so I could write a test for the updated endpoint. Currently tests always consider a success.

2. An update in the docs about the new method and that you don't need Push-Admin capability for the local device.

Alternatively, we can skip adding the new method, as the old one (from DeviceRegistrations API) works now just fine without Push-Admin capability if you pass local device id to it. WDYT @QuintinWillison

[lawrence-forooghian]

I think we need to come to a cross-platform conclusion on how we want to solve this issue. I've created ably/specification#25, should we continue the conversation there?

[maratal]

Good summary, thanks @lawrence-forooghian (let's continue there).

[maratal]

Related issues #1264, #1268, ably/specification#25

[QuintinWillison]

I don't feel it's helpful to keep this issue open, now that the discussion has been moved to docs/spec. Thanks, @lawrence-forooghian and @maratal. If new work comes out of the spec discussion then that will create a new, focussed, well-scoped issue in this repository.