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

Raise the correct exception in fast_serialize_string #633

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

eregon
Copy link
Member

@eregon eregon commented Oct 21, 2024

Comment on lines 452 to 460
# These pass on the pure-Ruby generator but not with the native extension
# assert_raise(Encoding::UndefinedConversionError) do
# "\x82\xAC\xEF".b.to_json
# end
#
# assert_raise(Encoding::UndefinedConversionError) do
# JSON.dump("\x82\xAC\xEF".b)
# end
Copy link
Member

Choose a reason for hiding this comment

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

You can conditionally run it with if defined?(JSON::Pure).

I'll be trying to make implementations converge over time, but short term it's helpful to test the divergent behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, done.

Copy link
Member

Choose a reason for hiding this comment

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

Actually there is already a test for it, so just remove the commented out code I think:

if defined?(JSON::Ext::Generator) and RUBY_PLATFORM != "java"
def test_valid_utf8_in_different_encoding
utf8_string = "€™"
wrong_encoding_string = utf8_string.b
# This behavior is historical. Not necessary desirable. We should deprecated it.
# The pure and java version of the gem already don't behave this way.
assert_equal utf8_string.to_json, wrong_encoding_string.to_json
assert_equal JSON.dump(utf8_string), JSON.dump(wrong_encoding_string)
end

Copy link
Member Author

@eregon eregon Oct 21, 2024

Choose a reason for hiding this comment

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

That's different, it's for valid UTF-8 bytes, this is invalid UTF-8 bytes (both cases using the BINARY encoding on the String).
Also that test only checks the C ext behavior, and this tests the pure-Ruby behavior.

@eregon eregon force-pushed the generator-error-fast_serialize_string branch 2 times, most recently from 9c4d459 to 5492c8c Compare October 21, 2024 19:43
@byroot byroot added the bug label Oct 21, 2024
@byroot byroot force-pushed the generator-error-fast_serialize_string branch from 5492c8c to 35407d6 Compare October 22, 2024 11:56
@byroot byroot merged commit c4a6e77 into ruby:master Oct 22, 2024
73 checks passed
casperisfine pushed a commit to casperisfine/json that referenced this pull request Nov 5, 2024
Followup: ruby#633

That's what was raised historically. You could argue that this new
exception is more precise, but I've encountered some real production
code that expected the old behavior and that was broken by this change.
casperisfine pushed a commit to casperisfine/json that referenced this pull request Nov 5, 2024
Followup: ruby#633

That's what was raised historically. You could argue that this new
exception is more precise, but I've encountered some real production
code that expected the old behavior and that was broken by this change.
byroot added a commit to byroot/json that referenced this pull request Nov 5, 2024
Followup: ruby#633

That's what was raised historically. You could argue that this new
exception is more precise, but I've encountered some real production
code that expected the old behavior and that was broken by this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON.dump("\x82\xAC\xEF".b) no error with the C extension
2 participants