Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Unserializing YAML attributes can silently fail in development mode #9975

Merged
merged 2 commits into from

3 participants

@mmangino

If you use a serialized attribute on an ActiveRecord model and the object to be unserialized has not yet been loaded, AR will silently return the YAML string. This can be confusing to new users and apparently to me, even though I've been bitten by this three or four times in the past.

@mmangino

The above patch will at least re-raise the argument error. I'm not convinced we should ever swallow these exceptions though. When would we want to get a string of bad YAML back instead of being told our object is invalid? That seems surprising.

If we want to keep swallowing some errors, it might be nice to create another custom error class that gives the missing class name and suggests that the user require_dependency it. I'm happy to do that if there is interest.

@egilburg

Is this really an ArgumentError? Seems more like TypeError or similar

@mmangino

The error comes from YAML, so I can't control that. We could wrap it, I'm just not sure if it makes sense.

@mmangino

@tenderlove This has been updated as we discussed.

@tenderlove tenderlove merged commit 22fee7c into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
20 activerecord/lib/active_record/coders/yaml_column.rb
@@ -23,19 +23,15 @@ def dump(obj)
def load(yaml)
return object_class.new if object_class != Object && yaml.nil?
return yaml unless yaml.is_a?(String) && yaml =~ /^---/
- begin
- obj = YAML.load(yaml)
-
- unless obj.is_a?(object_class) || obj.nil?
- raise SerializationTypeMismatch,
- "Attribute was supposed to be a #{object_class}, but was a #{obj.class}"
- end
- obj ||= object_class.new if object_class != Object
-
- obj
- rescue ArgumentError, Psych::SyntaxError
- yaml
+ obj = YAML.load(yaml)
+
+ unless obj.is_a?(object_class) || obj.nil?
+ raise SerializationTypeMismatch,
+ "Attribute was supposed to be a #{object_class}, but was a #{obj.class}"
end
+ obj ||= object_class.new if object_class != Object
+
+ obj
end
end
end
View
14 activerecord/test/cases/coders/yaml_column_test.rb
@@ -43,10 +43,20 @@ def test_load_handles_other_classes
assert_equal [], coder.load([])
end
- def test_load_swallows_yaml_exceptions
+ def test_load_doesnt_swallow_yaml_exceptions
coder = YAMLColumn.new
bad_yaml = '--- {'
- assert_equal bad_yaml, coder.load(bad_yaml)
+ assert_raises(Psych::SyntaxError) do
+ coder.load(bad_yaml)
+ end
+ end
+
+ def test_load_doesnt_handle_undefined_class_or_module
+ coder = YAMLColumn.new
+ missing_class_yaml = '--- !ruby/object:DoesNotExistAndShouldntEver {}\n'
+ assert_raises(ArgumentError) do
+ coder.load(missing_class_yaml)
+ end
end
end
end
Something went wrong with that request. Please try again.