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

Data sent as []byte should be decoded as []byte #581

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Data sent as []byte should be decoded as []byte #581

wants to merge 15 commits into from

Conversation

amnonbc
Copy link
Contributor

@amnonbc amnonbc commented Jan 10, 2023

Fixes #574

The spec states:

(RSL6a) All messages received will be decoded automatically based on the encoding field and the payloads will be converted into the format they were originally sent using i.e. binary, string, or JSON

In this case the message was sent as []byte so should have been received as as []byte.

This PR fixes the behaviour when receiving a []byte message.

The problem is that merging this PR could cause user code to panic at runtime, if it assumes the old incorrect behaviour
and does something like

payload := msg.Data.(string)

Is this a breaking change which requires a new major version?
How many customers are using ably-go to receive realtime messages?
The SDK team will need to decide.

@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 10, 2023 13:38 Inactive
@sacOO7 sacOO7 self-requested a review January 11, 2023 06:32
Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add one more msgpack integration test that sends binary data and checks on another side if data is received in the required format?

@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 11, 2023 09:49 Inactive
@amnonbc
Copy link
Contributor Author

amnonbc commented Jan 11, 2023

Can you add one more msgpack integration test that sends binary data and checks on another side if data is received in the required format?

done in 68c0afe

@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 13, 2023 11:28 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 13, 2023 15:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 19, 2023 11:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 19, 2023 11:41 Inactive
@sacOO7
Copy link
Collaborator

sacOO7 commented Jan 20, 2023

Are we waiting for anymore changes on this PR? I remember there was issue the msgpack library for encoding/decoding data?

@amnonbc
Copy link
Contributor Author

amnonbc commented Jan 20, 2023

Are we waiting for anymore changes on this PR? I remember there was issue the msgpack library for encoding/decoding data?

Yes, I am adding some more fixture tests. Should be done by the end of the weekend.

But the SDK team will need to discuss what we do with this PR, as it will break current behaviour (see PR comment).

…nd encoding of binary messages of different payload types and lengths.
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 22, 2023 13:01 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 22, 2023 13:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 22, 2023 17:37 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 23, 2023 07:13 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 23, 2023 15:08 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 23, 2023 16:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 24, 2023 17:47 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 24, 2023 17:50 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 24, 2023 17:59 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 25, 2023 16:30 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 26, 2023 14:05 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 26, 2023 14:11 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/581/godoc January 26, 2023 15:13 Inactive
@amnonbc
Copy link
Contributor Author

amnonbc commented Mar 7, 2023

I am going to close this for now as it is a breaking change, which will cause currently working code to panic at runtime.
This change is too small to justify a v2 api on its own. So I will close the PR for now, but link it to the bug, as an example of how the bug can be fixed when the SDK team does a v2.

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

Successfully merging this pull request may close these issues.

ably-go should not convert []byte messages to strings
2 participants