psych icon indicating copy to clipboard operation
psych copied to clipboard

Fix error in @dispatch_cache default value

Open srawlins opened this issue 11 years ago • 5 comments

The exception in @dispatch_cache's default value has an error( target is not a thing; raises "undefined local variable or method `target' for... "). This fixes that. (Also, the other exceptions in this file have a lowercase "can't".)

What was really fun is trying to test this. That exception should never ever ever be raised, unless Ruby does something crazy to force the accessor of @dispatch_cache to somehow return nil. The only way I could do it is with a crazy test:

diff --git a/test/psych/visitors/test_yaml_tree.rb b/test/psych/visitors/test_yaml_tree.rb
index 40702bc..a0b031d 100644
--- a/test/psych/visitors/test_yaml_tree.rb
+++ b/test/psych/visitors/test_yaml_tree.rb
@@ -38,6 +38,24 @@ module Psych
         assert(Psych.dump(Object.new) !~ /Object/, yaml)
       end

+      class Klass
+        def name
+          ObjectSpace.each_object(Psych::Visitors::YAMLTree) { |yt|
+            yt.instance_variable_get("@dispatch_cache")["foo"] = nil
+          }.to_s
+        end
+
+        def superclass; "foo"; end
+      end
+
+      def test_not_a_class
+        not_a_class = Klass.new
+        bad = Struct.new("Bad", :class)
+        assert_raises(TypeError) do
+          Psych.dump(bad.new(not_a_class))
+        end
+      end
+
       def test_struct_const
         foo = Struct.new("Foo", :bar)
         assert_cycle foo.new('bar')

Which I'm sure you don't want to commit. So only the fix is in this PR.

srawlins avatar Apr 01 '14 21:04 srawlins

How about this test?

diff --git a/test/psych/visitors/test_yaml_tree.rb b/test/psych/visitors/test_yaml_tree.rb
index 40702bc..d05ca85 100644
--- a/test/psych/visitors/test_yaml_tree.rb
+++ b/test/psych/visitors/test_yaml_tree.rb
@@ -116,6 +116,15 @@ module Psych
         assert_cycle 1...2
       end

+      def test_basic_object
+        assert_raises(TypeError) do
+          @v.accept Class.new(BasicObject) {
+            def object_id; 9001; end
+            def respond_to?(*args); false; end
+          }.new
+        end
+      end
+
       def test_anon_class
         assert_raises(TypeError) do
           @v.accept Class.new

tenderlove avatar Apr 02 '14 00:04 tenderlove

Ah ha, clever. I can add to the PR.

srawlins avatar Apr 02 '14 00:04 srawlins

@srawlins please add it, and I'll merge. Thanks!

tenderlove avatar Apr 07 '14 16:04 tenderlove

Actually, @tenderlove that test doesn't work. lib/psych/visitors/yaml_tree.rb expects the object to respond to #class on line 148, but that doesn't exist on BasicObject:

>> BasicObject.new.class
NoMethodError: undefined method `class' for #<BasicObject:0x007f8e23899a78>

and adding def class; BasicObject; end doesn't help, because BasicObject.superclass returns nil, but BasicObject.superclass.name will be called at lib/psych/visitors/yaml_tree.rb:70.

I don't see how its possible to force the accessor of @dispatch_cache to somehow return nil.

srawlins avatar Apr 09 '14 22:04 srawlins

I'm not sure whether you are trying to test my error - but this is what I get:

NoMethodError - undefined method 'name' for nil:NilClass:
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:70:in 'block in initialize'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:72:in 'block in initialize'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:72:in 'block in initialize'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:72:in 'block in initialize'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:72:in 'block in initialize'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:148:in 'accept'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:344:in 'block in visit_Hash'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:342:in 'visit_Hash'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:148:in 'accept'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:364:in 'block in visit_Array'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:364:in 'visit_Array'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:148:in 'accept'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:498:in 'block in emit_coder'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:496:in 'emit_coder'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:481:in 'dump_coder'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:146:in 'accept'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/visitors/yaml_tree.rb:112:in 'push'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych.rb:409:in 'dump'
  /Users/walther/.rbenv/versions/2.1.1/lib/ruby/2.1.0/psych/core_ext.rb:14:in 'psych_to_yaml'
  delayed_job (4.0.1) lib/delayed/backend/base.rb:85:in 'payload_object='

when I try to (well actually it's delayed_job trying but anyhow) serialize an ActiveRecord class method like this:

WageEvent.delay.build_di_stat( from_at: params[:from_at], to_at: params[:to_at], file: f )

and the file descriptor gets built like this:

f = Tempfile.new(['absence', '.esi'])

[ I know that pushing temporary file descriptors into serialized objects are not necessarily the best design option - but if you were looking for a use case .... here you have it (that is - if this is at all, what you are trying to test) ]

:)

wdiechmann avatar Apr 22 '14 10:04 wdiechmann