ably / ably-go

Go client library SDK for Ably realtime messaging service
https://ably.com/download
Apache License 2.0
82 stars 31 forks source link

`ablyMessage.Data` type changed after encryption was added #608

Closed codegoalie closed 1 year ago

codegoalie commented 1 year ago

In v1.2.12, the stream I am connecting to provided messages with a Data attribute of type string (a string of JSON text). However, after upgrading to the latest commit (specifically 4672dd4480a3b8e3f11dd075f9de224440563f71 or later), I'm getting messages with Data of type map[string]interface{}. Nothing has changed with the publisher. In fact, if I revert back to v1.2.12, the Data type goes back to string.

Here's the relevant subscription code:

unsubscribe, err := channel.SubscribeAll(context.Background(), func(msg *ably.Message) {
    log.Info("received message", "data_type", reflect.TypeOf(msg.Data))
}

And the output on v1.2.12:

2023/07/14 14:26:13 INFO received message data_type=string

And on 4672dd4480a3b8e3f11dd075f9de224440563f71 or later:

2023/07/14 14:28:57 INFO received message data_type="map[string]interface {}"

Am I doing something wrong, or is this a regression?

Thanks so much!

┆Issue is synchronized with this Slack message by Unito

sync-by-unito[bot] commented 1 year ago

➤ Automation for Jira commented:

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

sacOO7 commented 1 year ago

@codegoalie we are looking into the issue and will fix it asap :+1:

sacOO7 commented 1 year ago

Related to https://github.com/ably/ably-go/pull/370

sacOO7 commented 1 year ago

Hi, @codegoalie I have added test cases to check for encoding/decoding of JSON string data with/without cipher. You can check here https://github.com/ably/ably-go/pull/609/files. Encoding/Decoding for JSON in string format seems to be working just fine 🤔. Can you recheck if the bug still persists? if it does, can you post a working snippet that can help me reproduce the issue from my side, WDYT?

codegoalie commented 1 year ago

That did not appear to work. Here's a minimal code from our side. I'm working with our vendor to see about getting a sample of the producer-side:

unsubscribe, err := channel.SubscribeAll(context.Background(), func(msg *ably.Message) {
    log.Info("received message", "data_type", reflect.TypeOf(msg.Data), "data", msg.Data)

    var event Event
    err = json.Unmarshal([]byte(msg.Data.(string)), &event)
    // ...
})

The log used to read that msg.Data was a string and so the interface conversion on the Unmarshal line worked. Now the logs show msg.Data is a map[string]interface{} and the Unmarshal line panics with

2023/07/18 14:45:42 [ERROR] EventEmitter: panic in event handler: interface conversion: interface {} is map[string]interface {}, not string
sacOO7 commented 1 year ago

Yeah, it will be good if we can see the code from producer side, mainly it will be a publish method.

lmars commented 1 year ago

Hi @codegoalie

the stream I am connecting to provided messages with a Data attribute of type string (a string of JSON text).

I suspect the message being received also has msg.Encoding set to json, which is typically an indication to an Ably SDK that the message data should be decoded from a JSON string into a native object (which in Go is a map[string]interface{}). You can confirm this by logging msg.Encoding.

If so, then the issue is that, before commit 4672dd4, the Go realtime client was not automatically decoding received data as it should do, and so the data was always coming through as a string, but since that commit, the client is now decoding the data, so stringified JSON is decoded into a map[string]interface{}.

@sacOO7

This can be reproduced by publishing a map[string]string, which will be encoded as JSON, and checking the received message:

client, err := ably.NewRealtime(
    ably.WithKey("<key>"),
)
if err != nil {
    return err
}
channel := client.Channels.Get("test")
msgC := make(chan *ably.Message)
unsub, err := channel.SubscribeAll(context.Background(), func(msg *ably.Message) {
    msgC <- msg
})
if err != nil {
    return err
}
defer unsub()
log.Println("publishing JSON message")
if err := channel.Publish(context.Background(), "test", map[string]string{"foo": "bar"}); err != nil {
    return err
}
log.Println("waiting for message")
msg := <-msgC
log.Printf("got message data_type=%T data=%v encoding=%q", msg.Data, msg.Data, msg.Encoding)
return nil

In release v1.2.12 you'll get back a string with msg.Encoding set to json:

2023/07/18 21:41:10 publishing JSON message
2023/07/18 21:41:10 waiting for message
2023/07/18 21:41:10 got message data_type=string data={"foo":"bar"} encoding="json"

and on main you'll get back a map[string]interface{} with an empty msg.Encoding:

2023/07/18 21:41:36 publishing JSON message
2023/07/18 21:41:36 waiting for message
2023/07/18 21:41:36 got message data_type=map[string]interface {} data=map[foo:bar] encoding=""

I think we need to treat this as a breaking change, revert from the v1.x branch, and queue up automatically decoding data for ably-go v2 (although we'll likely want something type safe in v2, see https://github.com/ably/ably-go/issues/584).

sacOO7 commented 1 year ago

Hi @codegoalie

the stream I am connecting to provided messages with a Data attribute of type string (a string of JSON text).

I suspect the message being received also has msg.Encoding set to json, which is typically an indication to an Ably SDK that the message data should be decoded from a JSON string into a native object (which in Go is a map[string]interface{}). You can confirm this by logging msg.Encoding.

If so, then the issue is that, before commit 4672dd4, the Go realtime client was not automatically decoding received data as it should do, and so the data was always coming through as a string, but since that commit, the client is now decoding the data, so stringified JSON is decoded into a map[string]interface{}.

@sacOO7

This can be reproduced by publishing a map[string]string, which will be encoded as JSON, and checking the received message:

client, err := ably.NewRealtime(
  ably.WithKey("<key>"),
)
if err != nil {
  return err
}
channel := client.Channels.Get("test")
msgC := make(chan *ably.Message)
unsub, err := channel.SubscribeAll(context.Background(), func(msg *ably.Message) {
  msgC <- msg
})
if err != nil {
  return err
}
defer unsub()
log.Println("publishing JSON message")
if err := channel.Publish(context.Background(), "test", map[string]string{"foo": "bar"}); err != nil {
  return err
}
log.Println("waiting for message")
msg := <-msgC
log.Printf("got message data_type=%T data=%v encoding=%q", msg.Data, msg.Data, msg.Encoding)
return nil

In release v1.2.12 you'll get back a string with msg.Encoding set to json:

2023/07/18 21:41:10 publishing JSON message
2023/07/18 21:41:10 waiting for message
2023/07/18 21:41:10 got message data_type=string data={"foo":"bar"} encoding="json"

and on main you'll get back a map[string]interface{} with an empty msg.Encoding:

2023/07/18 21:41:36 publishing JSON message
2023/07/18 21:41:36 waiting for message
2023/07/18 21:41:36 got message data_type=map[string]interface {} data=map[foo:bar] encoding=""

I think we need to treat this as a breaking change, revert from the v1.x branch, and queue up automatically decoding data for ably-go v2 (although we'll likely want something type safe in v2, see #584).

@lmars I think this is the only case I can think of! Basically, rest API is being used to publish data, and realtime subscribers to decode the data of different types. I think the new implementation shows the expected behavior where the sent data type is equivalent to the received data type.

sacOO7 commented 1 year ago

Hi, @codegoalie since changes on the main are breaking for you and might reoccur for other users, we have reverted the change on the main branch. We will soon be implementing real-time encryption support on the v2 branch (or branch with similar name). Let us know if you need anything else : )

codegoalie commented 1 year ago

Ok! Sounds good. Thanks for looking into it and keeping backwards compatibility.

Looking forward to v2!

sacOO7 commented 1 year ago

Closing the issue since the change has been reverted and new implementation will be done as per https://github.com/ably/ably-go/issues/615