Permalink
Browse files

Fix generation of wrong json string when field has multiple errors

  • Loading branch information...
1 parent 76a15dd commit a9b666b51d28b2e74da630c31327dee7cbe96d37 @krekoten krekoten committed with josevalim Nov 30, 2010
Showing with 20 additions and 0 deletions.
  1. +10 −0 activemodel/lib/active_model/errors.rb
  2. +10 −0 activemodel/test/cases/errors_test.rb
@@ -167,6 +167,16 @@ def to_xml(options={})
def as_json(options=nil)
self
end
+
+ def encode_json(encoder)
+ errors = []
+ each_pair do |key, value|
+ value = value.first if value.size == 1
+ errors << "#{encoder.encode(key.to_s)}:#{encoder.encode(value, false)}"
+ end
+
+ "{#{errors * ','}}"
+ end
# Adds +message+ to the error messages on +attribute+, which will be returned on a call to
# <tt>on(attribute)</tt> for the same attribute. More than one error can be added to the same
@@ -61,5 +61,15 @@ def self.lookup_ancestors
assert_equal ["name can not be blank", "name can not be nil"], person.errors.to_a
end
+
+ test 'to_json should return valid json string' do
+ person = Person.new
+ person.errors.add(:name, "can not be blank")
+ person.errors.add(:name, "can not be nil")
+
+ hash = ActiveSupport::OrderedHash[:name, ["can not be blank", "can not be nil"]]
+
+ assert_equal person.errors.to_json, hash.to_json
+ end
end

5 comments on commit a9b666b

Contributor

krekoten replied Dec 1, 2010

Yes, there is logical incompatibility between xml and json output. This fix lets us at least recreate objects from json correctly. If field has more then one error we would miss all error messages except last.

Contributor

asanghi replied Dec 1, 2010

https://rails.lighthouseapp.com/projects/8994/tickets/5615-support-multiple-errors-per-attribute-in-activemodel-errors-json-serialization

Another ticket on which the same issue was being raised and patch suggested. The patch contained in that ticket makes better sense to me somehow.

Member

josevalim replied Dec 1, 2010

Yes, it makes more sense to me as well. krekoten, does it make more sense to you as well?

Contributor

krekoten replied Dec 1, 2010

Yes it does :) My bad, I didn't search LH.

Please sign in to comment.