Support annotations

[SimonWoolf]

Spec equivalent of ably/ably-js#1953
PUB-1549

[sacOO7]

Wow, big PR, will take some time to review it properly 👍
Thanks for adding !

[lawrence-forooghian]

Reviewed everything except for the docstrings; will look at those shortly.

[lawrence-forooghian]

A few thoughts about this one:

1. Does this make adding the listener become an asynchronous thing, i.e. that the listener is only added once the channel modes have been validated? The current convention (at least, as far as I understand it) for the subscribe methods is that the listener is synchronously added, and that if you call unsubscribe(listener) before subscribe completes the implicit attach, the listener will get removed.
2. Presumably should be "if the channel is already ATTACHED or once the channel has been attached if necessary".
3. This check can only be a best effort, given that there might be no attach if attachOnSubscribe is disabled, or the implicit attach might fail.

[SimonWoolf]

I initially did want it to only add the listener after the validation, but andy persuaded me to change it, see discussion ably/ably-js#1953 (comment) . I don't like that we still add the listener even if the promise is rejected, but reluctantly I figure the least bad option is to be consistent with the existing behaviour, at least until we change both at once.

That convention doesn't appear to be specified for realtimechannel#subscribe so I don't think the additional condition changes anything?

good point about 3. that clause is already getting a bit complex so I think i'll add that caveat as a sub-item

[lawrence-forooghian]

That convention doesn't appear to be specified for realtimechannel#subscribe so I don't think the additional condition changes anything?

That is true, but I guess it doesn't hurt to make it explicit here?

More broadly, I'm still on the fence about whether we should have this error, given that it's primarily of value in the mental model in which there exists some relation between attaching a channel and adding an event listener for a channel event. We seem to be questioning this mental model internally (moving away from it in Chat, adding an opt-out for it in the core SDKs) and have to go out of our way in documentation to explain that subscribing and attaching are different things. And I worry that by adding this error, we're actually doubling down on this mental model, which we're elsewhere trying to move away from; this could be confusing to users.

[SimonWoolf]

I'm still on the fence about whether we should have this error, given that it's primarily of value in the mental model in which there exists some relation between attaching a channel and adding an event listener for a channel event.

I absolutely agree with you about implicit attach, I was arguing against it years ago.

...But I don't agree that this error is "primarily" about implicit attach. The check is equally valid if you subscribe after being attached for hours. I could even have chosen to make the check on the channel modes being requested in channel options, which is purely local and has nothing to do with your attachment state at all; I only didn't because (a) it's possible to request channel modes in a way that's opaque to the sdk and (b) this way you also catch a user who doesn't have the mode because they don't have the capabilities.

[lawrence-forooghian]

As you've said, you can only throw this error if you are sure of which channel modes are actually active, which requires you to be attached. So, I find it hard to understand how, when writing API documentation for Annotations.subscribe, we'd describe this potential error without mentioning the channel being attached, which then cements this attach / subscribe relation in the user's mind.

[SimonWoolf]

I find it hard to understand how, when writing API documentation for Annotations.subscribe, we'd describe this potential error without mentioning the channel being attached

The exception is not the potential error. Rather, the exception is telling you about the potential error. The potential error, that we will indeed need to document, is calling Annotations.subscribe() without having requested the appropriate annotations subscribe mode.

It is ultimately the user's responsibility to specify the correct subscribe mode. If they don't, their code will not work.

But as a convenience, in some circumstances, the library can detect that they've made this mistake and throw an exception as a way of alerting the developer to the problem. The idea is to be helpful to devs while they are building out an implementation, by saving them a trip to the documentation to figure out why their code doesn't yet work. Once they fix their implementation to request the right modes, that will never happen again.

(I could also have just printed a warning log and carried on; I went for an exception just because I don't think there's ever a legitimate reason for someone to do this, so why not highlight it in the most noticeable way? But if you feel strongly that it should be a log rather than an exception I'm happy to change it, I don't mind either way)

I wasn't planning to document the exact circumstances in which this the can happen any more other than maybe a short parenthetical like "(The library may be able to detect that you have called subscribe() without the appropriate mode, if so the subscribe() call may throw an error)". Similarly we document that you shouldn't use a token literal with no way of renewing it in production, we don't document the exact circumstance in which the library will warn you about it.

[lawrence-forooghian]

My preference would be that we allow this to be a warn log message, at the discretion of the platform. I was trying to figure out how we'd implement this in Swift, which distinguishes between two types of errors:

• programmer error: the caller is using an API incorrectly; the expected behaviour here is to trap (i.e. crash the program)
• runtime error: not a logic error; something has gone "expectedly wrong" at runtime; the language forces users to catch and handle these errors

This error doesn't really fall neatly into either of these categories:

• we can't treat it as a programmer error, because the programmer who called subscribe() might be relying on the correct channel mode being given via a server-issued token, and we shouldn't be crashing an app because the server misbehaved
• if we treat it as a runtime error, then we have a subscribe() which now throws errors for two different reasons (1. implicit attach failed, 2. channel modes are wrong), and the programmer, as mentioned above, has to catch and handle this thrown error, thus forcing them to be able to distinguish between the different kinds of errors, and to have error-handling code for an error that is unlikely to ever actually happen in a properly-configured program

[SimonWoolf]

Sure, I'll make it discretionary.

(JS of course used to have that distinction as well -- exception for programmer errors and callback with err for runtime errors -- now that it's an async function those are both collapsed into promise rejection. Which in some ways makes things easier, but in other ways makes the subscribe function now even less intuitive: when there were two separate types of error we could document that if an exception was not thrown, the subscription has been added, even if the callback was called with an error; now that it's an async function we can no longer do that, which makes the api inconsistent)

[lawrence-forooghian]

In the JS typings it seems like this type does aim to be exhaustive — it doesn't offer the possibility of receiving an arbitrary JSON object in the Message.summary property.

[SimonWoolf]

This is kindof a limitation of typescript. I initially had type SummaryEntry = SummaryDistinctValues | ... | unknown. The trouble is that unknown is a collapsing type; x | y | unknown is just equivalent to unknown -- which means you don't get any type hints, autocompletion etc. So I ended up removing the | unknown, and expecting we'd have to tell people in the docs to use (msg.summary as any).xxx for summary types the ably-js types don't include yet.

If you happen to think of any better suggestions for that than what I did that I'm all ears, I realise it's not great.

[lawrence-forooghian]

which means you don't get any type hints, autocompletion etc

I'm not sure I understand to what extent you currently get these (i.e. even without the unknown)? Presumably, given a SummaryEntry, the only thing that autocompletion can offer you are properties which exist on the union of all possible SummaryEntry types, which doesn't seem hugely useful?

Perhaps the thing to do here in TypeScript is that which resembles option 2 of #292 (comment) — i.e. keep SummaryDistinctValues etc but remove SummaryEntry and change Message.summary to a JSON object?

[SimonWoolf]

Yeah, fair point.

You do get one thing with the current typings, which is you can use go-to-type to go to the relevant bit of the type definitions and look at them. Adding | unknown kills that: the type is collapsed into unknown and drops all metadata. But now I come to think more, a better solution would be | {[k: string]: unknown}. (Which is essentially JSONObject).

Not sure I see the argument for removing SummaryEntry as a union type, it gives you that thing and doesn't really harm anything. Either way you'd still get that option 2 through a type assertions.

[lawrence-forooghian]

Ah, so if you go with the {[k: string]: unknown} then it doesn't collapse it, I see. That sounds good. Might be worth adding a comment alongside the TypeScript type definition to explain why the union type exists; I can quite easily see someone otherwise coming along and going "this type adds no value, I'll remove it".

[SimonWoolf]

Adding more notes to the ably-js typings seems useful sure. (Also considering providing type assertoin functions). But unless I'm misunderstanding, you're not suggesting any change to this spec PR, so that can happen async in the ably-js repo.

[lawrence-forooghian]

I worry that we're pushing quite a lot of work onto users by using such a loose type here. It's not too bad in an untyped language or something like TypeScript where you can tell the compiler "assume that this JSONObject has this given (potentially nested) shape". But there isn't a similar mechanism in a language like Swift in which the type system cannot represent arbitrarily-shaped JSON objects; we'd need to give the user a dictionary with untyped values and they'd need to convert it into some strongly typed object they can work with (i.e. something that looks like one of the SummaryEntry values that you've given in the docstrings). I think that expecting them to do this for non-user-supplied data (i.e. data whose shape Ably controls) would be quite a faff for them.

The ideal would be for the SDK to provide these possible SummaryEntry values as part of its public API, so that one of the following can happen:

1. the SDK, using a rule for mapping from a Message.summary key to a SummaryEntry value type, creates the appropriate SummaryEntry value type by calling a constructor that accepts a JSONObject (or returns the JSONObject if there's no rule for a given key)
2. same thing as 1. but the mapping is performed by the user (this one might be better so that the type we emit in the message summary doesn't suddenly change when we add support to the SDK for some new aggregation method)

[SimonWoolf]

I did consider 1 but, like you point out, it kindof sucks because it means that when we do add support the type changes suddenly in what's supposed to be a non-breaking library change. And we really do want to be free to add new aggregations serverside without having to coordinate half a dozen SDK releases each time.

But 2 seems much more reasonable, that's not a bad idea..

[lawrence-forooghian]

If you're happy with 2, then I think that the thing to do is to add the TypeScript types (SummaryDistinctValues, SummaryUniqueValues, SummaryMultipleValues, SummaryClientIdList, SummaryTotal) into the spec, and have a spec point explaining that there should be a mechanism for a user to convert a Message.summary into a user-selected one of these (this mechanism might just be a type assertion e.g. in TypeScript, or a constructor that takes a JSONObject e.g. Swift). What do you think?

[SimonWoolf]

Done, wdyt?
Slight tweaks to naming (added the version to the constructor name to match the version being in the annotation type) that I'll need to do in ably-js too.

[lawrence-forooghian]

Looks good, please could you update the docstrings and IDL to match?

[sacOO7]

Adding to this, can we also have public methods that returns boolean like
isSummaryDistinctValues, isSummaryUniqueValues etc, so it's easier for user to cast/convert into appropriate type. Same pattern is followed in java gson library

https://github.com/google/gson/blob/dd2fe59c0d3390b2ad3dd365ed6938a5c15844cb/gson/src/main/java/com/google/gson/JsonElement.java#L119-L149

I guess this has to follow pattern where Summary is a base class and SummaryUniqueValues, SummaryMultipleValues, ... extends Summary

class Message {
    // other fields
    Summary summary;  // type can be SummaryUniqueValues, SummaryDistinctValues etc
}

// Summary.java
public abstract class Summary {
    private Map<string, JsonObject>? value; // TM2q

    public boolean isSummaryDistinctValues() {
        return this instanceof SummaryDistinctValues;
    }

    public boolean isSummaryUniqueValues() {
        return this instanceof SummaryUniqueValues;
    }
   
    public SummaryUniqueValues getAsSummaryUniqueValues() {
        return (SummaryUniqueValues) this;
    }
}

// SummaryUniqueValues.java
public class SummaryUniqueValues extends Summary {
    // Implementation specific to unique values summary
}

// SummaryDistinctValues.java
public class SummaryDistinctValues extends Summary {
    // Implementation specific to distinct values summary
}

I think we can establish this standard pattern across SDKs. Currently, the usage of static factory methods is not entirely clear and related implementation is different in ably-js.

[sacOO7]

One more related comment -> https://github.com/ably/specification/pull/292/files#r2137836289

[SimonWoolf]

I guess this has to follow pattern where Summary is a base class and SummaryUniqueValues, SummaryMultipleValues, ... extends Summary

The pattern Lawrence proposed, and I then adopted, for languages where it is not easy to work with plain objects, is a function SummaryDistinctV1() that takes a JsonObject (the value of some key in the summary dict), and returns a structure that is specific to a distinct v1 summary. The user is expected to know what aggregation type they're using and call the correct function. In this pattern, there is no subclassing.

I think you're imagining an api where the library detects what kind of summary entry it is and does the decoding itself automatically, which would then need subclassing. That is a possible api we considered, but we decided against it.

In ably-js working with plain objects is easy and idiomatic, so the sdk just returns objects, and the user can use type assertions to get the correct type. (We could potentially also provide type assertion functions).

[SimonWoolf]

@lawrence-forooghian any further comments?

[lawrence-forooghian]

Was away for a couple of weeks — will take a look now.

[lawrence-forooghian]

Nothing beyond the current unresolved conversations.

[sacOO7]

Suggested change

	| summary: `Dict<string, JsonObject>`? ||| TM2q | A summary of all the annotations that have been made to the message, whose keys are the `type` fields from any annotations that it includes. For summary types the SDK knows about, static factor methods on `Message` exist to parse the values into strongly-typed objects. |
	| summary: `Dict<string, JsonObject>`? ||| TM2q | A summary of all the annotations that have been made to the message, whose keys are the `type` fields from any annotations that it includes. For summary types the SDK knows about, static factory methods on `Message` exist to parse the values into strongly-typed objects. |

[sacOO7]

Or can be updated to
A collection summarizing all annotations attached to the message, where each entry is keyed by the type field from the corresponding annotation. For known summary types, the SDK provides static factory methods on the Message class to parse these values into strongly-typed objects.

[ttypic]

Can we define this more loosely in the spec? Not require static factory methods on Message, but suggest providing some utility methods, idiomatic to the SDK, to perform these casts to strongly-typed objects. I’m just not sure that Message is the right place to put those static methods.

[sacOO7]

Yeah, in case of ably-java, we added them as static methods of Summary.java (PR => Java annotations) or can we define them as public utility methods, wdyt

[SimonWoolf]

Can we define this more loosely in the spec? Not require static factory methods on Message, but suggest providing some utility methods, idiomatic to the SDK, to perform these casts to strongly-typed objects. I’m just not sure that Message is the right place to put those static methods.

That's pretty much exactly what the spec already says. I can modify TM7 to make clear that it doesn't have to be static methods on Message to make that explicit, sure. (They won't be in ably-js for example).

[sacOO7]

Added few typo comments.
Other than this, do we need to mention that annotations introduces mutable messages.
I could see this change as a part of java annotations PR
AppSpec.json#Mutable=true

[sacOO7]

requested few changes

[SimonWoolf]

Other than this, do we need to mention that annotations introduces mutable messages.

Eh, that's effectively a detail on how this functionality is tested; I've never been a big fan of having the features spec specify that sort of thing, I don't think it's necessary. The features spec doesn't lay out exactly what any of the other namespaces created by test-app-setup.json fixtures are or what needs them, why would it for this?

[maratal]

Should it contain somethig similar to RestAnnotations? @SimonWoolf