Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fixes an issue when decoding a json string which looks like a date bu…

…t is invalid. This DateTime parse error is now caught and the original string is instead passed back [#6286 state:resolved]

Signed-off-by: Santiago Pastorino <santiago@wyeworks.com>
  • Loading branch information...
commit b17d8d727fb510ad8b6eb4302984d290dc2e53b0 1 parent f23bf7d
Josh Kalderimis joshk authored spastorino committed
6 activesupport/lib/active_support/json/backends/jsongem.rb
View
@@ -26,7 +26,11 @@ def convert_dates_from(data)
when nil
nil
when DATE_REGEX
- DateTime.parse(data)
+ begin
+ DateTime.parse(data)
+ rescue ArgumentError
+ data
+ end
when Array
data.map! { |d| convert_dates_from(d) }
when Hash
6 activesupport/lib/active_support/json/backends/yajl.rb
View
@@ -23,7 +23,11 @@ def convert_dates_from(data)
when nil
nil
when DATE_REGEX
- DateTime.parse(data)
+ begin
+ DateTime.parse(data)
+ rescue ArgumentError
+ data
+ end
when Array
data.map! { |d| convert_dates_from(d) }
when Hash
12 activesupport/lib/active_support/json/backends/yaml.rb
View
@@ -36,7 +36,7 @@ def convert_json_to_yaml(json) #:nodoc:
quoting = char
pos = scanner.pos
elsif quoting == char
- if json[pos..scanner.pos-2] =~ DATE_REGEX
+ if valid_date?(json[pos..scanner.pos-2])
# found a date, track the exact positions of the quotes so we can
# overwrite them with spaces later.
times << pos
@@ -94,6 +94,16 @@ def convert_json_to_yaml(json) #:nodoc:
output
end
end
+
+ private
+ def valid_date?(date_string)
+ begin
+ date_string =~ DATE_REGEX && DateTime.parse(date_string)
+ rescue ArgumentError
+ false
+ end
+ end
+
end
end
end
2  activesupport/test/json/decoding_test.rb
View
@@ -19,6 +19,8 @@ class TestJSONDecoding < ActiveSupport::TestCase
%({"a": "2007-01-01 01:12:34 Z"}) => {'a' => Time.utc(2007, 1, 1, 1, 12, 34)},
# no time zone
%({"a": "2007-01-01 01:12:34"}) => {'a' => "2007-01-01 01:12:34"},
+ # invalid date
+ %({"a": "1089-10-40"}) => {'a' => "1089-10-40"},
# needs to be *exact*
%({"a": " 2007-01-01 01:12:34 Z "}) => {'a' => " 2007-01-01 01:12:34 Z "},
%({"a": "2007-01-01 : it's your birthday"}) => {'a' => "2007-01-01 : it's your birthday"},

1 comment on commit b17d8d7

jobstar-se

Thanks for fixing this problem. However, I suggest that you should never convert string values to DateTime in the JSON parser. Generally, the current logic violates the Keep it Simple Smartypants (KISS) principle. Specifically, I will have to do the following unintuitive and cumbersome programming pattern because of the current logic:

hash = ActiveSupport::JSON.decode(json_string)
hash['reference'] = hash['reference'].to_s # make sure to convert DateTime to string if the string matched the datetime regexp in the parser.

I will have to convert all string values to strings in the hash because otherwise something like the following might happen:
subject = "Product reference: #{hash['reference']}" # => "Product reference: 2011-02-12T00:00:00+00:00"
which would be incorrect. The expected value of subject should be: "Product reference: 2011-02-12" because the reference string just happens to look like a date but really isn't.

So by converting string values to DateTime in the parser you:
1) require a unintuitive and cumbersome programming pattern
2) make it hard to avoid bugs that are hard to find and debug and appear seldom.

Also, by converting strings to DateTime the parse code become unnecessarily complicated which makes it harder to maintain increases the risk of bugs and makes it slower.

Please sign in to comment.
Something went wrong with that request. Please try again.