Report the attribute on ActiveRecord::SerializationTypeMismatch #27773

Merged
merged 1 commit into from Jan 30, 2017

Projects

None yet

7 participants

@kirs
Contributor
kirs commented Jan 23, 2017

Today when you get ActiveRecord::SerializationTypeMismatch it doesn't give you a tip about what attribute caused the error. This is not user friendly:

screen shot 2017-01-22 at 22 39 03

I think we should report it together with the error.
Regarding the implementation, I had to catch SerializationTypeMismatch on one level up in Attribute because that's where we know the attribute name. SerializationTypeMismatch is raised from a coder that has no idea about the attribute name.

@sgrif @kaspth

@rails-bot

r? @chancancode

(@rails-bot has picked a reviewer for you, use r? to override)

@maclover7
Member

Please take a look at the Travis CI build when you get a chance.

@eileencodes
Member

@maclover7 the failure isn't related to this branch, it's failing on all new PRs. Something on the env side got updated.

activerecord/lib/active_record/errors.rb
+ @value = value
+ end
+
+ def to_s
@kaspth
kaspth Jan 23, 2017 Member

Generally I'd expect message to hold this for an exception. Why are we using to_s?

@@ -37,6 +37,9 @@ def value
# `defined?` is cheaper than `||=` when we get back falsy values
@value = type_cast(value_before_type_cast) unless defined?(@value)
@value
+ rescue ActiveRecord::SerializationTypeMismatch => error
+ error.attribute = name
@kaspth
kaspth Jan 23, 2017 Member

Mutating the error is a bit of a smell to me. And, like you said, having the top level attribute know about this error seems off too.

We need to find some other way to extract this. Maybe there's some way of making the YAMLColumn aware of the attribute (or just it's column name) when it's instantiated?

+ Topic.serialize :content, Hash
+ topic = Topic.create!(content: { zomg: true })
+
+ Topic.serialize :content, Array
@kirs kirs Report the attribute on ActiveRecord::SerializationTypeMismatch
99820fb
@kirs
Contributor
kirs commented Jan 29, 2017

@kaspth thanks for review! I updated the code. Now YAMLColumn accepts attribute name.

@rafaelfranca rafaelfranca merged commit 6bf6957 into rails:master Jan 30, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maclover7 maclover7 removed the needs work label Jan 30, 2017
@kirs
Contributor
kirs commented Jan 31, 2017

cc @jules2689

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment