Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ably-go's messages are not type safe #584

Open
amnonbc opened this issue Jan 23, 2023 · 2 comments
Open

ably-go's messages are not type safe #584

amnonbc opened this issue Jan 23, 2023 · 2 comments

Comments

@amnonbc
Copy link
Contributor

amnonbc commented Jan 23, 2023

The ably.Message structure contains a field Data which contains the message payload.
This field is defined as interface{} implying that it can contain any type of data.

In practice, ably supports string []byte and any structure or array which can be represented as JSON.

The problems with this approach are:

  1. The API type definitions do not give the programmer any hints about what data they can to a channel, and what they will receive when they subscribe to a channel.
  2. The programmer does not benefit from compile-time type-checking
  3. The programmer will need to cast the msg.Data field to the right type in order to use it.
  4. The API is asymmetrical - it will allow a publisher to publish a struct, but the receiver will get a string containing the json representation of the struct.

The Generics feature which was introduced in Go 1.18 should allow us to create the API to solve some of these problems.

#582 provides a proof of concept implementation in ably-go, and
ably-labs/chessdemo#7 demonstrates the changes to the chessdemo to show what these changes look like from a user perspective.

The API parameterises the Message type to give a MessageOf[T any] type, where the msg.Data field is of type T.
We also introduce a parameterised RealtimeChannel

  • RealtimeChannelOf[T].
    To get one, you call the function GetChannelOf[T any](client *Realtime, name string)

This RealtimeChannelOf[T] has strongly a strongly typed Publish method.
Publish(ctx context.Context, name string, o T) error
The compiler will complain if you try to publish a message of a type other than T.

The Subscribe method, now takes a strongly typed handle function, which unlike the current API, also decodes
json encoded messages,.
Subscribe(ctx context.Context, name string, handle func(*MessageOf[T], error)) (func(), error)
handle is called with an error, if the message payload can not be decoded into a T.

Current Limitations

  • only one message type per channel - we create a RealtimeChannelOf[T], and it will be a compile time error to Publish anything but a T. When we subscribe, our handle will get called with a MessageOf[T] parameter, or with an error, if anything which is not a T gets sent over the channel. If we do not want to create a channel per type, we could add some routing of types to handler in the library, or leave routing to the calling application.
  • Ably generally json encodes user data, but strings and []byte are treated differently and transmitted as is.
    Go Generics does not have a type switch, so in order to treat string and []byte differently, we need to cast msg.Data to an interface and then use Reflection or a type switch to handle these cases.

┆Issue is synchronized with this Jira Task by Unito

@sync-by-unito
Copy link

sync-by-unito bot commented Jan 23, 2023

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3268

@sacOO7
Copy link
Collaborator

sacOO7 commented Jul 21, 2023

Based on the internal discussion, some points to be raised

  1. This has nothing to do with spec
  2. Type safety for publish and subscribe doesn't exist in other SDK's yet.
  3. People migrating to new libraries will need to define duplicate POJO or structs in publishers and subscribers.
  4. Though this would improve compile time improvisation for data types, the chances of subscribers failing to decode for newly added keys or missing keys will also increase ( depending on the implementation )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants