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

new reject_bytes option to raise on bytes #266

Merged
merged 10 commits into from May 8, 2020

Conversation

elelay
Copy link
Contributor

@elelay elelay commented Jun 11, 2017

raise TypeError when encountering bytes in ujson.dumps() to prevent
unexpected Unicode exceptions in production.
Fixes #264

raise TypeError when encountering bytes in ujson.dumps() to prevent
unexpected Unicode exceptions in production.
Fixes ultrajson#264
was seing  ~5% drop in performance without it on 250 strings
@Jahaja
Copy link
Contributor

Jahaja commented Aug 31, 2017

Hi, thanks for the PR. This should probably be the new default as it aligns with the standard json modules behaviour.

@elelay
Copy link
Contributor Author

elelay commented Aug 31, 2017

Great! I'm looking forward to that.

python/ujson.c Outdated Show resolved Hide resolved
lib/ultrajson.h Outdated Show resolved Hide resolved
elelay and others added 2 commits March 1, 2020 15:53
Co-Authored-By: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-Authored-By: Hugo van Kemenade <hugovk@users.noreply.github.com>
@hugovk
Copy link
Member

hugovk commented May 6, 2020

@elelay Please could you resolve the merge conflicts?

And also have it with reject_bytes = True by default, or perhaps flip it to allow_bytes = False by default?

No need to support Python 2.7 any more, so PyString_AS_STRING -> PyBytes_AS_STRING.

The project is now using pytest, so perhaps something like:

def test_allow_bytes_default():
    data = {"a": b"b"}
    with pytest.raises(TypeError):
        ujson.dumps(data)


def test_allow_bytes_false():
    data = {"a": b"b"}
    with pytest.raises(TypeError):
        ujson.dumps(data, allow_bytes=False)


def test_allow_bytes_true():
    data = {"a": b"b"}
    assert ujson.dumps(data, allow_bytes=True) == '{"a":"b"}'

Sorry this has taken a while! Because this is a breaking change, it would be good to get this in the 3.0 release due out in the next week or so, if possible.

Thank you!

@elelay
Copy link
Contributor Author

elelay commented May 7, 2020

OK. I'll do this on Friday.

@elelay
Copy link
Contributor Author

elelay commented May 8, 2020

inverting the logic will break those tests:

  • test_encode_unicode_4_bytes_utf8_fail
  • test_encode_big_escape

@elelay elelay requested a review from hugovk May 8, 2020 16:37
@hugovk hugovk added the changelog: Added For new features label May 8, 2020
@hugovk hugovk merged commit 81feb55 into ultrajson:master May 8, 2020
@hugovk
Copy link
Member

hugovk commented May 8, 2020

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Added For new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

should not serialize bytes
3 participants