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

Invalid data is read in JSONGet when a struct with Unicode strings is passed into JSONSet. #56

Closed
sheophe opened this issue Dec 8, 2021 · 1 comment · Fixed by #57
Closed

Comments

@sheophe
Copy link

sheophe commented Dec 8, 2021

Describe the bug
JSONGet does not produce the correct output for Cyrillic text.

To Reproduce
Steps to reproduce the behavior:

  1. Use JSONSet to write a structure with string fields. Fill the fields with Unicode characters.
  2. Use res, err := JSONGet() to read the structure.
  3. Convert res into []byte either manually (res.[]byte), or using redigo.Bytes(res).
  4. Unmarshall the []byte result via json.Unmarshall() into the structure.
  5. Compare values you have written with values json.Unmarshal produced from JSONGet result.
  6. New structure will contain fields with different (seemingly random) characters.

Expected behavior
Fields in first structure (which we have written) and the second one (which was read) should match.

Additional context
The problem I found lies within rjs.StringToBytes function, which is called from JSONGet. There are the following lines (_lst is a string, by is []byte) :

for _, s := range _lst {
    by = append(by, byte(s))
}

Here, s is a rune, which is an alias for int32. When we convert it into byte, we loose all but the least significant byte. Fix is pretty straightforward, we just need to convert string into []byte directly, without looping over each rune:

by = []byte(_lst)

I've copied JSONGet in my own code and applied this fix, and my Unicode problem was solved.

@breno12321
Copy link
Contributor

I was having problems with Brazilian Portuguese accentuation and it worked!

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

Successfully merging a pull request may close this issue.

2 participants