Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Do not expose internal state in the public encoder API (i.e. as_json) #12785

Merged
merged 2 commits into from

2 participants

Godfrey Chan Jeremy Kemper
Godfrey Chan
Owner

See 1 for why this is not a good idea.

As part of this refactor, circular reference protection in as_json has
been removed and the corresponding error class has been deprecated.

As discussed with @jeremy, circular reference error is considered
programmer errors and protecting against it is out of scope for
the encoder.

This is again based on the excellent work by @sergiocampama in #11728.

activesupport/lib/active_support/core_ext/object/json.rb
@@ -139,14 +139,11 @@ def as_json(options = nil) #:nodoc:
class Array
def as_json(options = nil) #:nodoc:
- # use encoder as a proxy to call as_json on all elements, to protect from circular references
- encoder = options && options[:encoder] || ActiveSupport::JSON::Encoding::Encoder.new(options)
- map { |v| encoder.as_json(v, options) }
+ map{ |v| v.as_json(options && options.dup) }
Jeremy Kemper Owner
jeremy added a note

map{ -> map {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jeremy Kemper jeremy was assigned
activesupport/lib/active_support/core_ext/object/json.rb
((8 lines not shown))
end
def encode_json(encoder) #:nodoc:
- # we assume here that the encoder has already run as_json on self and the elements, so we run encode_json directly
- "[#{map { |v| v.encode_json(encoder) } * ','}]"
+ "[#{map { |v| v.as_json.encode_json(encoder) } * ','}]"
Jeremy Kemper Owner
jeremy added a note

Indentation

Godfrey Chan Owner

Oops, fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activesupport/lib/active_support/json/encoding.rb
@@ -138,6 +100,28 @@ def escape(string)
json.force_encoding(::Encoding::UTF_8)
json
end
+
+ # Deprecate CircularReferenceError
+ def const_missing(name)
+ if name == :CircularReferenceError
+ message = "The JSON encoder in Rails 4.1 no longer offer protection from circular references. " \
Jeremy Kemper Owner
jeremy added a note

"no longer offers"

Godfrey Chan Owner

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jeremy Kemper jeremy commented on the diff
activesupport/lib/active_support/json/encoding.rb
@@ -138,6 +100,28 @@ def escape(string)
json.force_encoding(::Encoding::UTF_8)
json
end
+
+ # Deprecate CircularReferenceError
+ def const_missing(name)
+ if name == :CircularReferenceError
+ message = "The JSON encoder in Rails 4.1 no longer offers protection from circular references. " \
+ "You are seeing this warning because you are rescuing from (or otherwise referencing) " \
+ "ActiveSupport::Encoding::CircularReferenceError. In the future, this error will be " \
+ "removed from Rails. You should remove these rescue blocks from your code and ensure " \
+ "that your data structures are free of circular references so they can be properly " \
+ "serialized into JSON.\n\n" \
+ "For example, the following Hash contains a circular reference to itself:\n" \
+ " h = {}\n" \
+ " h['circular'] = h\n" \
+ "In this case, calling h.to_json would not work properly."
Jeremy Kemper Owner
jeremy added a note

ActiveSupport::JSON.encode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
chancancode added some commits
Godfrey Chan chancancode Moved AS::JSON::DATE_REGEX as it's only used for decoding 6da5ff4
Godfrey Chan chancancode Do not expose internal state in the public encoder API (i.e. as_json)
See [1] for why this is not a good idea.

As part of this refactor, circular reference protection in as_json has
been removed and the corresponding error class has been deprecated.

As discussed with @jeremy, circular reference error is considered
programmer errors and protecting against it is out of scope for
the encoder.

This is again based on the excellent work by @sergiocampama in #11728.

[1]: intridea/multi_json#138 (comment)
798881e
Jeremy Kemper jeremy merged commit 240863a into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 7, 2013
  1. Godfrey Chan
  2. Godfrey Chan

    Do not expose internal state in the public encoder API (i.e. as_json)

    chancancode authored
    See [1] for why this is not a good idea.
    
    As part of this refactor, circular reference protection in as_json has
    been removed and the corresponding error class has been deprecated.
    
    As discussed with @jeremy, circular reference error is considered
    programmer errors and protecting against it is out of scope for
    the encoder.
    
    This is again based on the excellent work by @sergiocampama in #11728.
    
    [1]: intridea/multi_json#138 (comment)
This page is out of date. Refresh to see the latest.
5 activesupport/CHANGELOG.md
View
@@ -1,3 +1,8 @@
+* Removed circular reference protection in JSON encoder, deprecated
+ ActiveSupport::JSON::Encoding::CircularReferenceError.
+
+ *Godfrey Chan*, *Sergio Campamá*
+
* Add `capitalize` option to Inflector.humanize, so strings can be humanized without being capitalized:
'employee_salary'.humanize # => "Employee salary"
19 activesupport/lib/active_support/core_ext/object/json.rb
View
@@ -139,14 +139,11 @@ def as_json(options = nil) #:nodoc:
class Array
def as_json(options = nil) #:nodoc:
- # use encoder as a proxy to call as_json on all elements, to protect from circular references
- encoder = options && options[:encoder] || ActiveSupport::JSON::Encoding::Encoder.new(options)
- map { |v| encoder.as_json(v, options) }
+ map { |v| v.as_json(options && options.dup) }
end
def encode_json(encoder) #:nodoc:
- # we assume here that the encoder has already run as_json on self and the elements, so we run encode_json directly
- "[#{map { |v| v.encode_json(encoder) } * ','}]"
+ "[#{map { |v| v.as_json.encode_json(encoder) } * ','}]"
end
end
@@ -165,19 +162,11 @@ def as_json(options = nil) #:nodoc:
self
end
- # use encoder as a proxy to call as_json on all values in the subset, to protect from circular references
- encoder = options && options[:encoder] || ActiveSupport::JSON::Encoding::Encoder.new(options)
- Hash[subset.map { |k, v| [k.to_s, encoder.as_json(v, options)] }]
+ Hash[subset.map { |k, v| [k.to_s, v.as_json(options && options.dup)] }]
end
def encode_json(encoder) #:nodoc:
- # values are encoded with use_options = false, because we don't want hash representations from ActiveModel to be
- # processed once again with as_json with options, as this could cause unexpected results (i.e. missing fields);
-
- # on the other hand, we need to run as_json on the elements, because the model representation may contain fields
- # like Time/Date in their original (not jsonified) form, etc.
-
- "{#{map { |k,v| "#{encoder.encode(k.to_s)}:#{encoder.encode(v, false)}" } * ','}}"
+ "{#{map { |k,v| "#{k.as_json.encode_json(encoder)}:#{v.as_json.encode_json(encoder)}" } * ','}}"
end
end
3  activesupport/lib/active_support/json/decoding.rb
View
@@ -7,6 +7,9 @@ module ActiveSupport
mattr_accessor :parse_json_times
module JSON
+ # matches YAML-formatted dates
+ DATE_REGEX = /^(?:\d{4}-\d{2}-\d{2}|\d{4}-\d{1,2}-\d{1,2}[T \t]+\d{1,2}:\d{2}:\d{2}(\.[0-9]*)?(([ \t]*)Z|[-+]\d{2}?(:\d{2})?))$/
+
class << self
# Parses a JSON string (JavaScript Object Notation) into a hash.
# See www.json.org for more info.
64 activesupport/lib/active_support/json/encoding.rb
View
@@ -10,7 +10,6 @@
require 'active_support/core_ext/time/conversions'
require 'active_support/core_ext/date_time/conversions'
require 'active_support/core_ext/date/conversions'
-require 'set'
module ActiveSupport
class << self
@@ -21,9 +20,6 @@ class << self
end
module JSON
- # matches YAML-formatted dates
- DATE_REGEX = /^(?:\d{4}-\d{2}-\d{2}|\d{4}-\d{1,2}-\d{1,2}[T \t]+\d{1,2}:\d{2}:\d{2}(\.[0-9]*)?(([ \t]*)Z|[-+]\d{2}?(:\d{2})?))$/
-
# Dumps objects in JSON (JavaScript Object Notation).
# See www.json.org for more info.
#
@@ -34,56 +30,22 @@ def self.encode(value, options = nil)
end
module Encoding #:nodoc:
- class CircularReferenceError < StandardError; end
-
class Encoder
attr_reader :options
def initialize(options = nil)
@options = options || {}
- @seen = Set.new
- end
-
- def encode(value, use_options = true)
- check_for_circular_references(value) do
- jsonified = use_options ? value.as_json(options_for(value)) : value.as_json
- jsonified.encode_json(self)
- end
end
- # like encode, but only calls as_json, without encoding to string.
- def as_json(value, use_options = true)
- check_for_circular_references(value) do
- use_options ? value.as_json(options_for(value)) : value.as_json
- end
- end
-
- def options_for(value)
- if value.is_a?(Array) || value.is_a?(Hash)
- # hashes and arrays need to get encoder in the options, so that
- # they can detect circular references.
- options.merge(:encoder => self)
- else
- options.dup
- end
+ def encode(value)
+ value.as_json(options.dup).encode_json(self)
end
def escape(string)
Encoding.escape(string)
end
-
- private
- def check_for_circular_references(value)
- unless @seen.add?(value.__id__)
- raise CircularReferenceError, 'object references itself'
- end
- yield
- ensure
- @seen.delete(value.__id__)
- end
end
-
ESCAPED_CHARS = {
"\x00" => '\u0000', "\x01" => '\u0001', "\x02" => '\u0002',
"\x03" => '\u0003', "\x04" => '\u0004', "\x05" => '\u0005',
@@ -138,6 +100,28 @@ def escape(string)
json.force_encoding(::Encoding::UTF_8)
json
end
+
+ # Deprecate CircularReferenceError
+ def const_missing(name)
+ if name == :CircularReferenceError
+ message = "The JSON encoder in Rails 4.1 no longer offers protection from circular references. " \
+ "You are seeing this warning because you are rescuing from (or otherwise referencing) " \
+ "ActiveSupport::Encoding::CircularReferenceError. In the future, this error will be " \
+ "removed from Rails. You should remove these rescue blocks from your code and ensure " \
+ "that your data structures are free of circular references so they can be properly " \
+ "serialized into JSON.\n\n" \
+ "For example, the following Hash contains a circular reference to itself:\n" \
+ " h = {}\n" \
+ " h['circular'] = h\n" \
+ "In this case, calling h.to_json would not work properly."
Jeremy Kemper Owner
jeremy added a note

ActiveSupport::JSON.encode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ ActiveSupport::Deprecation.warn message
+
+ SystemStackError
+ else
+ super
+ end
+ end
end
self.use_standard_json_time_format = true
12 activesupport/test/json/encoding_test.rb
View
@@ -146,19 +146,25 @@ def test_wide_utf8_roundtrip
def test_exception_raised_when_encoding_circular_reference_in_array
a = [1]
a << a
- assert_raise(ActiveSupport::JSON::Encoding::CircularReferenceError) { ActiveSupport::JSON.encode(a) }
+ assert_deprecated do
+ assert_raise(ActiveSupport::JSON::Encoding::CircularReferenceError) { ActiveSupport::JSON.encode(a) }
+ end
end
def test_exception_raised_when_encoding_circular_reference_in_hash
a = { :name => 'foo' }
a[:next] = a
- assert_raise(ActiveSupport::JSON::Encoding::CircularReferenceError) { ActiveSupport::JSON.encode(a) }
+ assert_deprecated do
+ assert_raise(ActiveSupport::JSON::Encoding::CircularReferenceError) { ActiveSupport::JSON.encode(a) }
+ end
end
def test_exception_raised_when_encoding_circular_reference_in_hash_inside_array
a = { :name => 'foo', :sub => [] }
a[:sub] << a
- assert_raise(ActiveSupport::JSON::Encoding::CircularReferenceError) { ActiveSupport::JSON.encode(a) }
+ assert_deprecated do
+ assert_raise(ActiveSupport::JSON::Encoding::CircularReferenceError) { ActiveSupport::JSON.encode(a) }
+ end
end
def test_hash_key_identifiers_are_always_quoted
Something went wrong with that request. Please try again.