Skip to content

adding utf8 encoding to documents data #239

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

Merged
merged 6 commits into from
May 27, 2025

Conversation

MohKamal
Copy link

Encode JSON data as UTF-8 before sending requests to properly support non-ASCII characters.

@poissoncorp
Copy link
Contributor

Hi @MohKamal, thanks for the contribution. Could you check code format? Try using black on the modified file.

Also, could you provide a simple test that verifies correct behavior?

I've seen this one in the codebase, is it passing after the change?

@unittest.skip("todo: Not passing on CI/CD")
def test_can_put_document_using_command_with_surrogate_pairs(self):
name_with_emojis = "Gracjan \ud83d\ude21\ud83d\ude21\ud83e\udd2c\ud83d\ude00😡😡🤬😀"
user = User(name=name_with_emojis, age=31)
node = Utils.entity_to_dict(user, self.store.conventions.json_default_method)
command = PutDocumentCommand("users/2", None, node)
self.store.get_request_executor().execute_command(command)
result = command.result
self.assertEqual("users/2", result.key)
self.assertIsNotNone(result.change_vector)
with self.store.open_session() as session:
loaded_user = session.load("users/2", User)
self.assertEqual(loaded_user.name, name_with_emojis)

@poissoncorp
Copy link
Contributor

poissoncorp commented May 23, 2025

@MohKamal To proceed to merge, please address my previous comment - once the test (verifying expected behavior) is created and passing (and the skip is removed from test_can_put_document_using_command_with_surrogate_pairs just for a check), we'll be able to approve merge the proposed change.

@MohKamal
Copy link
Author

@MohKamal To proceed to merge, please address my previous comment - once the test (verifying expected behavior) is created and passing (and the skip is removed from test_can_put_document_using_command_with_surrogate_pairs just for a check), we'll be able to approve merge the proposed change.

Hi @poissoncorp, Thank you for your feedback, I will do it, I was busy a little bit.

@poissoncorp
Copy link
Contributor

@MohKamal could you amend after black?

@MohKamal
Copy link
Author

@MohKamal could you amend after black?

Yes, done, sorry forgot applying black before pushing!

@poissoncorp
Copy link
Contributor

To proceed, please fix failing tests. Many thanks 🙏

@MohKamal
Copy link
Author

To proceed, please fix failing tests. Many thanks 🙏

I see that the emojis are correctly saved and retrieved, do i change the test string from "\ud83d\ude21\ud83d\ude21\ud83e\udd2c\ud83d\ude00" to "😡😡🤬😀" ?

@poissoncorp
Copy link
Contributor

To proceed, please fix failing tests. Many thanks 🙏

I see that the emojis are correctly saved and retrieved, do i change the test string from "\ud83d\ude21\ud83d\ude21\ud83e\udd2c\ud83d\ude00" to "😡😡🤬😀" ?

@MohKamal, any idea why those aren't passing? If that's not something critical, we can safely remove that.

@MohKamal
Copy link
Author

To proceed, please fix failing tests. Many thanks 🙏

I see that the emojis are correctly saved and retrieved, do i change the test string from "\ud83d\ude21\ud83d\ude21\ud83e\udd2c\ud83d\ude00" to "😡😡🤬😀" ?

@MohKamal, any idea why those aren't passing? If that's not something critical, we can safely remove that.

i can escape them ""\ud83d\ude21\ud83d\ude21\ud83e\udd2c\ud83d\ude00"

@poissoncorp
Copy link
Contributor

I've researched on this myself. Remove the Pythonic \u signs and leave just emojis

Like this:

"\ud83d\ude21\ud83d\ude21\ud83e\udd2c\ud83d\ude00" to "😡😡🤬😀"

I'll run tests, and merge if passing 👍

@poissoncorp poissoncorp merged commit dc175e1 into ravendb:v7.0 May 27, 2025
4 checks passed
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 this pull request may close these issues.

2 participants