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

Don't set message encoding for valid utf-8 string data #370

Merged
merged 4 commits into from
Aug 6, 2021

Conversation

lmars
Copy link
Member

@lmars lmars commented Aug 6, 2021

See explanation in ably/specification#28.

@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 6, 2021

@lmars , seems utf8 encoding is also used at one more place. Also, prev. comment has been added as to no idea why it's there.
Can you check if it's needed

if _, ok := m.Data.(string); ok {
// Not sure why this is useful, and can't find the requirement in the
// spec, but test fixtures expect this.
m.Encoding = mergeEncoding(m.Encoding, encUTF8)
}

@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 6, 2021

Also, we can remove this //todo comment, since proto is merged into ably

// TODO: Unexport once proto gets merged into package ably.

@lmars
Copy link
Member Author

lmars commented Aug 6, 2021

@sacOO7 thanks, I've clarified the requirement to add utf-8 encoding to encrypted strings in 99a3f68, and removed that outdated comment in a57270a.

ably/proto_message.go Outdated Show resolved Hide resolved
@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 6, 2021

@lmars , not sure but I feel when we are using cipher, we are adding extra layer of encoding for byte [] type, which increases payload size (33 %, goes exponential for next encoding iterations). I was thinking maybe you can improve on that one/ cut the part of code that does that when cipher is enabled. Although, this is not part of the fix. but I just feel we can include it as a part of fix

@lmars
Copy link
Member Author

lmars commented Aug 6, 2021

@sacOO7

this is not part of the fix. but I just feel we can include it as a part of fix

Yes, so whilst what you describe could potentially be a problem, let's deal with that separately, the priority here is fixing the behaviour we know is broken. If you're happy that we've fixed the reported issue, please approve this PR so we can merge and release it.

See explanation in https://github.com/ably/docs/issues/1165.

Signed-off-by: Lewis Marshall <lewis.marshall@ably.com>
Signed-off-by: Lewis Marshall <lewis.marshall@ably.com>
Signed-off-by: Lewis Marshall <lewis.marshall@ably.com>
Signed-off-by: Lewis Marshall <lewis.marshall@ably.com>
@lmars lmars merged commit d2b1daa into main Aug 6, 2021
@lmars lmars deleted the fix-string-encoding branch August 6, 2021 21:11
@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 14, 2021

Fixes incomplete #107

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.

2 participants