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.

Sign up for GitHub

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

Make extract! accept objects with internal hash implementation #440

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Make extract! accept objects with internal hash implementation #440

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2543bf2
Make json.extract! accept objects with internal hash implementation
bacchir Sep 27, 2017
4c3d6f3
Fix json.extract! to use first the hash implementation and public_sen…
bacchir Dec 2, 2018
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
14 changes: 4 additions & 10 deletions lib/jbuilder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,8 @@ def array!(collection = [], *attributes)
#
# json.(@person, :name, :age)
def extract!(object, *attributes)
if ::Hash === object
_extract_hash_values(object, attributes)
else
_extract_method_values(object, attributes)
attributes.each do |key|
_set_value key, _extract_value(object, key)
end
end

Expand Down Expand Up @@ -252,12 +250,8 @@ def target!

private

def _extract_hash_values(object, attributes)
attributes.each{ |key| _set_value key, object.fetch(key) }
end

def _extract_method_values(object, attributes)
attributes.each{ |key| _set_value key, object.public_send(key) }
def _extract_value(object, attribute)
object.respond_to?(attribute) ? object.public_send(attribute) : object.fetch(attribute)
Copy link
Member

Choose a reason for hiding this comment

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

The risk is pretty low but still, it should rather be:

object.respond_to?(:fetch) ? object.fetch(attribute) : object.public_send(attribute)

Otherwise, if a hash or hash-like object has a :object_id, :hash or any of its method as a key, the wrong value may be returned.

Could you fix this and add a test to ensure that a Hash with such key would be properly handled please ?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry @robin850 I just saw your comment now. Sure I'll change it 😊

end

def _merge_block(key)
Expand Down
22 changes: 22 additions & 0 deletions test/jbuilder_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ def initialize(name, age)
end
end

class PersonWithHash
attr_reader :name, :collection

def initialize(name, age)
@collection = { age: age }
@name = name
end

delegate :[], :fetch, to: :@collection
end

class RelationMock
include Enumerable

Expand Down Expand Up @@ -121,6 +132,17 @@ class JbuilderTest < ActiveSupport::TestCase
assert_equal 34, result['age']
end

test 'extracting from object with internal hash' do
person = PersonWithHash.new('David', 32)

result = jbuild do |json|
json.extract! person, :name, :age
end

assert_equal 'David', result['name']
assert_equal 32, result['age']
end

test 'nesting single child with block' do
result = jbuild do |json|
json.author do
Expand Down