We do not explain how a client lib should populate `LocalDevice.push`

[lawrence-forooghian]

What's the issue?

LocalDevice extends DeviceDetails, which has a push: DevicePushDetails property. The features spec does not provide any guidance on how a client library should populate this property for a LocalDevice instance.

(This only applies to LocalDevice instances. A non-LocalDevice instance of DeviceDetails is the result of a call to one of the push admin REST APIs, in which all of these properties will have been populated by the backend.)

Let's look at each of the properties of DevicePushDetails in more detail:

• errorReason: ErrorInfo? and state: .Active | .Failing | .Failed: I believe these are meant to describe whether the Realtime backend was able to successfully send push notifications to that device.
• recipient: JsonObject: This describes the parameters that the Realtime backend needs in order to send push notifications to that specific device ("push transport details" as the features spec calls them). Its value is best described by the REST API documentation for the "register a device for receiving push notifications" operation. For example, on iOS, it's { transportType: "apns", deviceToken: <string> }, where deviceToken is a value that was originally provided to the client library by iOS.

What does the features spec imply?

recipient property

The features spec implies, but does not explicitly state, that the LocalDevice.push.recipient property should be populated when the client library receives updated push transport details from its environment (OS, browser, …), and should be used to store these details. This is how the current iOS implementation behaves.

In the features spec, this requirement is primarily implied by (emphasis mine):

• RSH3a2c:

  If the local device has the necessary push details (registration token, etc.), sends a GotPushDeviceDetails event.

• RSH3a2d:

  If the local device does not have the necessary push details, it initiates a request to the underlying platform (or otherwise generates them)

• RSH3d3b:

  On event GotPushDeviceDetails (note that this will only happen on platforms whose push device details, after first set, can change, e. g. FCM’s registration token refresh) … make an asynchronous PATCH HTTP request to /push/deviceRegistrations/:deviceId using the local DeviceDetails ’s push details as body (but only the changed fields

• RSH8i:

  Each time the library is instantiated, if the LocalDevice has push device details (eg an APNS deviceToken), and if the platform supports it, it must verify the validity of those details (eg by requesting a token from the platform and comparing that with the already-known token).

errorReason and state properties

The features spec makes no mention of how LocalDevice.push.{errorReason, state} should be populated.

Suggested solution

recipient property

We should explicitly document how a client library is expected to populate this property, in line with what’s currently implied. We should also use this opportunity to make sure that the guidance we give about storing push transport details locally is compatible with the advice of client platform vendors, for example Apple’s guidance to “never cache device tokens in local storage” (which I think needs to be interpreted not as “don’t store”, but “make sure to validate your stored data”).

errorReason and state properties

This data is potentially useful for a client library to expose, as described in "When is this useful?" of #26.

We have two options for exposing it:

1. Explain a process by which the client library fetches this data from the server and uses it to populate these properties. We would also need to design a process for keeping these values up to date.
2. Remove these properties from the LocalDevice interface, and then describe a mechanism for explicitly fetching this data separately. This would be a (in theory, but not in practice since these properties never did anything useful) breaking change to the public API of the library. We’d also have to find a way to handle the fact that these properties exist in its parent DeviceDetails interface (e.g. remove the inheritance).

My preference would be the latter. I can’t think of a use case that requires this data to be updated in real time, and it would add substantial complexity.

Other conversations / issues

• Push#getDevicePushDetails method to make HTTP request #26 – here it’s decided that the backend will update the "get details from a registered device" REST API operation to work for the current device without requiring push admin, hence partly implementing solution 2 above. Related client SDK issues:

  • LocalDevice.push.state and DeviceDetails.push.state is always null ably-flutter#146
  • Read and persist state returned in ARTLocalDevice/ ARTDeviceDetails ably-cocoa#1162
  • Read and persist state returned in LocalDevice/ DeviceDetails ably-java#697

• New ARTPush/getDevicePushDetails method for simplifying push state access ably-cocoa#1186 – ably-cocoa attempt to address this issue, by taking advantage of the backend change described above.

┆Issue is synchronized with this Jira Task by Unito

[ably-sync-bot]

➤ Phil Leggetter commented:

is blocking SDK work. See

https://ably-real-time.slack.com/archives/C01LUEUC9GT/p1652888449704039 ( https://ably-real-time.slack.com/archives/C01LUEUC9GT/p1652888449704039|smart-card )

[lawrence-forooghian]

As mentioned on Slack, @paddybyers, it would be great to get your opinion on this one.

[paddybyers]

We should explicitly document how a client library is expected to populate this property, in line with what’s currently implied

Agreed

errorReason and state

These were intended to capture the terminal state of the activation state machine. In particular, if activation fails and the stae machine ends up in a failed state, then these values are captured in the local device. Then, any attempted operations that are contingent on activation having happened (eg anything relating to local device subscriptions, that depend on local device authentication) can return the appropriate error.

That was the idea. I can't look further into the linked discussions right now, but lmk if this helps

[maratal]

1. Explain a process by which the client library fetches this data from the server and uses it to populate these properties. We would also need to design a process for keeping these values up to date.

In according what @paddybyers said

These were intended to capture the terminal state of the activation state machine. In particular, if activation fails and the stae machine ends up in a failed state, then these values are captured in the local device.

I could just save these values upon error in the activation process both on Apple or Ably side, and then when calling methods "relating to local device subscriptions" I can check whether an error was saved into the LocalDevice and immediately throw this error to the user via callback. But I don't think this solution is very reliable. The only place where an error can occur on Apple side is failing to receive device token. As Apple suggests not to keep it locally, absence of it in therecipient property when calling methods related to push subscriptions can be considered invalid and a callback with an error like "You should obtain device token first" can be called for a library user. In conjunction with the ARTPushRegistererDelegate.didActivateAblyPush(_ error:) error information it's enough for both library user and for the end user to understand what happened. Whereas failure on Ably side can get a proper error from the server every time. So there is no need to save any errors IMHO.

2. Remove these properties from the LocalDevice interface, and then describe a mechanism for explicitly fetching this data separately. This would be a (in theory, but not in practice since these properties never did anything useful) breaking change to the public API of the library. We’d also have to find a way to handle the fact that these properties exist in its parent DeviceDetails interface (e.g. remove the inheritance).

I also keen to this solution, but I think removing errorReason and state properties (moving them to another structure) is enough, no need to remove inheritance from DeviceDetails. I'll make a "preview" PR with this changes, and if we agree on the implementation we'll update docs accordingly and land this PR with all the fixes (if needed). WDYT @lawrence-forooghian @paddybyers

[lawrence-forooghian]

@maratal I've taken a look at your draft PR ably/ably-cocoa#1441, which, effectively, removes errorReason and state from LocalDevice. I think that, if we're happy to remove those properties, then your approach is a fine one (people might have opinions about the details but it's gonna roughly look like that, I think; also it's worth bearing in mind that this would be a breaking API change and we’d need to handle the implications of that).

But, I just wanted to also explore @paddybyers's idea for how LocalDevice’s errorReason and state might work, and your objections to it.

First, to summarise what Paddy is suggesting, I think it’s the following:

• LocalDevice's state property would be directly derived from the current state of the push activation state machine – i.e. there would be a mapping from state machine state to one of Active / Failing / Failed.
• When the push activation state machine transitions to a state that represents failure, it would store information about the failure in LocalDevice’s error property.

And then, as for how that might benefit us, it would mean that when a user calls a method that requires the local device to already be activated for push notifications – specifically, these are PushChannel.subscribeDevice and PushChannel.unsubscribeDevice – we would be able to fail with a specific error instead of, for example, the generic "cannot use device before device activation has finished" error that ably-cocoa currently uses.

I'd like to understand better this part of your objection:

Whereas failure on Ably side can get a proper error from the server every time. So there is no need to save any errors IMHO.

If I've understood correctly, you're saying that, each time the user calls PushChannel.subscribe/unsubscribeDevice, if the device is not already registered for push notifications then the SDK will make a call to the Ably backend to try and register the device, and if that fails, then the error can be returned to the user directly. Have I got that right? But that's not how the push activation process works as defined by our feature spec – it's more appropriate to think of it as a background process that occurs independently of whatever other calls the user is making to our SDK. That is, calling PushChannel.subscribe/unsubscribeDevice on an unregistered device will not trigger an attempt to register the device. Which, I think, is why Paddy is suggesting keeping around a copy of the error that last caused device activation to fail. Does that make sense / change your opinion on this?

[lawrence-forooghian]

Ah, one other thing I wanted to mention. I think that, if we do keep the state and error properties on LocalDevice, I wonder whether the inheritance from DeviceDetails would make sense. From a polymorphism point of view, I’d expect these properties to have the same meaning for DeviceDetails and LocalDevice. But I think they wouldn't – on DeviceDetails, they'd provide information about whether the backend was able to send notifications to that device, and on LocalDevice, they’d provide information about whether the local device was able to register itself for notifications with the Ably backend.

[maratal]

If I've understood correctly, you're saying that, each time the user calls PushChannel.subscribe/unsubscribeDevice, if the device is not already registered for push notifications then the SDK will make a call to the Ably backend to try and register the device, and if that fails, then the error can be returned to the user directly. Have I got that right? But that's not how the push activation process works as defined by our feature spec – it's more appropriate to think of it as a background process that occurs independently of whatever other calls the user is making to our SDK. That is, calling PushChannel.subscribe/unsubscribeDevice on an unregistered device will not trigger an attempt to register the device. Which, I think, is why Paddy is suggesting keeping around a copy of the error that last caused device activation to fail. Does that make sense / change your opinion on this?

Yeah, seems like I've jumped into conclusions here, looks like saving error is still useful, will do.

[maratal]

There is a problem with this - not all failures end up in a failed state of the state machine. There are 4 events representing failure, and only 2 states for it. After, for example, ARTPushActivationEventGettingPushDeviceDetailsFailed event SM ends up in the ARTPushActivationStateNotActivated state. With described approach I shouldn't save an error in this case, and call to PushChannel.subscribe/unsubscribeDevice will still got no proper error. @lawrence-forooghian

[maratal]

Regarding inheritance - there are still a lot of common properties between these two classes:

@property (strong, nonatomic) ARTDeviceId *id;
@property (strong, nullable, nonatomic) NSString *clientId;
@property (strong, nonatomic) NSString *platform;
@property (strong, nonatomic) NSString *formFactor;
@property (strong, nonatomic) NSDictionary<NSString *, NSString *> *metadata;
@property (strong, nonatomic) NSMutableDictionary<NSString *, NSString *> *pushRecipient;

I think inheritance still makes sense. I've added these two

@property (nullable, nonatomic, readonly) ARTErrorInfo *activationError;
@property (nullable, nonatomic, readonly) ARTPushActivationPersistentState *activationState;

in a private part of the ARTLocalDevice.