Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error in @dispatch_cache default value #188

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

srawlins
Copy link

@srawlins srawlins commented Apr 1, 2014

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.

@tenderlove
Copy link
Member

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

@srawlins
Copy link
Author

srawlins commented Apr 2, 2014

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

@tenderlove
Copy link
Member

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

@srawlins
Copy link
Author

srawlins commented Apr 9, 2014

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.

@wdiechmann
Copy link

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) ]

:)

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

Successfully merging this pull request may close these issues.

3 participants