Skip to content

Commit

Permalink
renderer calls object.to_json when rendering :json => object [#5655 s…
Browse files Browse the repository at this point in the history
…tate:resolved]

Signed-off-by: José Valim <jose.valim@gmail.com>
  • Loading branch information
dcrec1 authored and josevalim committed Sep 27, 2010
1 parent 4966b91 commit 72f37bd
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 1 deletion.
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/metal/renderers.rb
Expand Up @@ -71,7 +71,7 @@ def self._write_render_options
end

add :json do |json, options|
json = ActiveSupport::JSON.encode(json, options) unless json.respond_to?(:to_str)
json = json.to_json(options) unless json.respond_to?(:to_str)
json = "#{options[:callback]}(#{json})" unless options[:callback].blank?
self.content_type ||= Mime::JSON
self.response_body = json
Expand Down
13 changes: 13 additions & 0 deletions actionpack/test/controller/render_json_test.rb
Expand Up @@ -9,6 +9,10 @@ def as_json(options={})
hash.except!(*options[:except]) if options[:except]
hash
end

def to_json(options = {})
super :except => [:c, :e]
end
end

class TestController < ActionController::Base
Expand Down Expand Up @@ -49,6 +53,10 @@ def render_json_with_render_to_string
def render_json_with_extra_options
render :json => JsonRenderable.new, :except => [:c, :e]
end

def render_json_without_options
render :json => JsonRenderable.new
end
end

tests TestController
Expand Down Expand Up @@ -109,4 +117,9 @@ def test_render_json_forwards_extra_options
assert_equal '{"a":"b"}', @response.body
assert_equal 'application/json', @response.content_type
end

def test_render_json_calls_to_json_from_object
get :render_json_without_options
assert_equal '{"a":"b"}', @response.body
end
end

3 comments on commit 72f37bd

@brianmario
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we were trying to get rid of to_json entirely? (or as much as possible at least)

@dcrec1
Copy link
Contributor Author

@dcrec1 dcrec1 commented on 72f37bd Sep 28, 2010

Choose a reason for hiding this comment

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

what is the problem with to_json?

@brianmario
Copy link
Contributor

Choose a reason for hiding this comment

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

The JSON gem, ActiveSupport and yajl-ruby's JSON gem compatibility API all override that method. If someone includes 'yajl/json_gem' after ActiveSupport has been loaded, the above code will work but not the way the caller expects since the options hash for ActiveSupport's version of to_json is completely different than that of the one in yajl-ruby's JSON gem compatibility API.

I was under the assumption as_json was added to help get around this by allowing one to provide a JSON encodable version of their object using primitives that are directly mappable to JSON from Ruby, and the other way around. Then to actually encode the object, it would be passed through ActiveSupport::JSON.encode which has no chance of being overridden elsewhere and as such would have consistent behavior everywhere.

Rails actually has a hack for this from long-standing issues in the past with the JSON gem and ActiveSupport both overriding to_json.

Please sign in to comment.