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

ablyMessage.Data type changed after encryption was added #608

Closed
codegoalie opened this issue Jul 14, 2023 · 11 comments
Closed

ablyMessage.Data type changed after encryption was added #608

codegoalie opened this issue Jul 14, 2023 · 11 comments

Comments

@codegoalie
Copy link

codegoalie commented Jul 14, 2023

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 4672dd4 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 4672dd4 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
Copy link

sync-by-unito bot commented Jul 14, 2023

➤ Automation for Jira commented:

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

@sacOO7
Copy link
Collaborator

sacOO7 commented Jul 17, 2023

@codegoalie we are looking into the issue and will fix it asap 👍

@sacOO7
Copy link
Collaborator

sacOO7 commented Jul 18, 2023

Related to #370

@sacOO7
Copy link
Collaborator

sacOO7 commented Jul 18, 2023

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
Copy link
Author

codegoalie commented Jul 18, 2023

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
Copy link
Collaborator

sacOO7 commented Jul 18, 2023

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

@lmars
Copy link
Member

lmars commented Jul 18, 2023

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).

@sacOO7
Copy link
Collaborator

sacOO7 commented Jul 19, 2023

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
Copy link
Collaborator

sacOO7 commented Jul 21, 2023

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
Copy link
Author

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

Looking forward to v2!

@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 1, 2023

Closing the issue since the change has been reverted and new implementation will be done as per #615

@sacOO7 sacOO7 closed this as completed Aug 1, 2023
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

3 participants