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

Add tests for the behavior of JSON.generate with base types subclasses #679

Merged
merged 1 commit into from
Nov 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ext/json/ext/generator/generator.c
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,11 @@ json_object_i(VALUE key, VALUE val, VALUE _arg)
VALUE key_to_s;
switch(rb_type(key)) {
case T_STRING:
key_to_s = key;
if (RB_LIKELY(RBASIC_CLASS(key) == rb_cString)) {
key_to_s = key;
} else {
key_to_s = rb_funcall(key, i_to_s, 0);
}
Comment on lines +758 to +762
Copy link
Author

Choose a reason for hiding this comment

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

We had regressed here on the C implementation.

break;
case T_SYMBOL:
key_to_s = rb_sym2str(key);
Expand Down
66 changes: 66 additions & 0 deletions test/json/json_generator_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,72 @@ def test_to_json_called_with_state_object
assert_instance_of JSON::State, argument
end

module CustomToJSON
def to_json(*)
%{"#{self.class.name}#to_json"}
end
end

module CustomToS
def to_s
"#{self.class.name}#to_s"
end
end

class ArrayWithToJSON < Array
include CustomToJSON
end

def test_array_subclass_with_to_json
assert_equal '["JSONGeneratorTest::ArrayWithToJSON#to_json"]', JSON.generate([ArrayWithToJSON.new])
assert_equal '{"[]":1}', JSON.generate(ArrayWithToJSON.new => 1)
end

class ArrayWithToS < Array
include CustomToS
end

def test_array_subclass_with_to_s
assert_equal '[[]]', JSON.generate([ArrayWithToS.new])
assert_equal '{"JSONGeneratorTest::ArrayWithToS#to_s":1}', JSON.generate(ArrayWithToS.new => 1)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem reasonable, i.e. calling to_s on an Array subclass (both semantically and performance-wise).
Inheriting from core classes is frowned upon, so without a convincing use case I see no value to respect those old weird/broken semantics.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I'm not sold yet on which behavior makes the most sense, I just want to make sure it's tested and that all implementation behave the same, and then think about what makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

I think ideally we wouldn't treat subclasses specially at all, it doesn't seem reasonable to subclass a core type and then expect different behavior than the superclass for JSON behavior and many other things (Liskov substitution principle).
If someone wants to have some custom serialization to JSON, it seems simple enough to have a custom type not inheriting from a core type and having to_json implemented on it.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not disagreeing, I'm just saying we should be very deliberate about it, because backward compatibility is very important.

Copy link
Member

Choose a reason for hiding this comment

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

Ah for this specific example I missed that to_s is only called because the ArrayWithToS is a Hash key, that seems less crazy at least 😅

end

class HashWithToJSON < Hash
include CustomToJSON
end

def test_hash_subclass_with_to_json
assert_equal '["JSONGeneratorTest::HashWithToJSON#to_json"]', JSON.generate([HashWithToJSON.new])
assert_equal '{"{}":1}', JSON.generate(HashWithToJSON.new => 1)
end

class HashWithToS < Hash
include CustomToS
end

def test_hash_subclass_with_to_s
assert_equal '[{}]', JSON.generate([HashWithToS.new])
assert_equal '{"JSONGeneratorTest::HashWithToS#to_s":1}', JSON.generate(HashWithToS.new => 1)
end

class StringWithToJSON < String
include CustomToJSON
end

def test_string_subclass_with_to_json
assert_equal '["JSONGeneratorTest::StringWithToJSON#to_json"]', JSON.generate([StringWithToJSON.new])
assert_equal '{"":1}', JSON.generate(StringWithToJSON.new => 1)
end

class StringWithToS < String
include CustomToS
end

def test_string_subclass_with_to_s
assert_equal '[""]', JSON.generate([StringWithToS.new])
assert_equal '{"JSONGeneratorTest::StringWithToS#to_s":1}', JSON.generate(StringWithToS.new => 1)
end

if defined?(JSON::Ext::Generator) and RUBY_PLATFORM != "java"
def test_valid_utf8_in_different_encoding
utf8_string = "€™"
Expand Down
Loading