Permalink
Browse files

Force YAML to use syck rather than psych. Eventually delayed_job shou…

…ld be made compatible with both.
  • Loading branch information...
1 parent 374c3fe commit cbb4060c8dad886f59d77deab444c94ad61e09a9 @pda pda committed with gaffneyc Feb 3, 2011
Showing with 1 addition and 0 deletions.
  1. +1 −0 lib/delayed/yaml_ext.rb
View
1 lib/delayed/yaml_ext.rb
@@ -2,6 +2,7 @@
# Classes, Modules and Structs
require 'yaml'
+YAML::ENGINE.yamler = "syck"
@tenderlove
tenderlove added a line comment Aug 29, 2011

Hi, can you tell me why this change was made? Setting the yamler isn't a good idea as it's a global (and destructive) change.

I'd like to get this line removed, but I need to know why it was added in the first place. Thanks!

@betamatt
betamatt added a line comment Aug 29, 2011

Yeah, not a great idea but it was an expedience. Lots of things broke with Psych as I recall. Would love to get that fixed.

@gaffneyc
Collective Idea member
gaffneyc added a line comment Aug 29, 2011

It has to with some of the magic we're trying to do with Yaml serialization that works with the Syck API but we haven't spent the time to figure out what it would take to update it to work with Psych as well.

#201

@tenderlove
tenderlove added a line comment Aug 29, 2011

Is this actually a requirement? It seems you would want to know about poorly formatted YAML. Psych will raise an exception:

[aaron@higgins ~]$ ruby -rpsych -ryaml -e'YAML.load("{default: {<<: *login}}")'
/Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:204:in `block in visit_Psych_Nodes_Alias': Unknown alias: login (Psych::BadAlias)
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:204:in `fetch'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:204:in `visit_Psych_Nodes_Alias'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/visitor.rb:15:in `visit'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/visitor.rb:5:in `accept'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:16:in `accept'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:217:in `block in revive_hash'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:211:in `each'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:211:in `each_slice'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:211:in `revive_hash'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:123:in `visit_Psych_Nodes_Mapping'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/visitor.rb:15:in `visit'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/visitor.rb:5:in `accept'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:16:in `accept'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:226:in `block in revive_hash'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:211:in `each'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:211:in `each_slice'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:211:in `revive_hash'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:123:in `visit_Psych_Nodes_Mapping'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/visitor.rb:15:in `visit'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/visitor.rb:5:in `accept'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/visitors/to_ruby.rb:16:in `accept'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych/nodes/node.rb:35:in `to_ruby'
    from /Users/aaron/.local/lib/ruby/1.9.1/psych.rb:116:in `load'
    from -e:1:in `<main>'
[aaron@higgins ~]$ ruby -rsyck -ryaml -e'YAML.load("{default: {<<: *login}}")'
[aaron@higgins ~]$
@tenderlove
tenderlove added a line comment Aug 29, 2011

Are these tests required too? Other serialization strategies (notably Marshal) can't support it. I think we can work around it, but I want to question assumptions first. :-)

@gaffneyc
Collective Idea member
gaffneyc added a line comment Aug 29, 2011

No, it really isn't. That got added as a response to a lot of bug reports we were getting. Since we're overriding #to_yaml (or was it .yaml_new?) on a couple core classes we would get bugs reported against DJ that were really poorly written config files in people's applications.

It's not a great fix. We've talked a bit about dropping some of the magic, and especially the monkey patching for DJ 3.0 while trying to maintain the public api.

@tenderlove
tenderlove added a line comment Aug 29, 2011

If we're willing to say "you must require your structs before deserialization", then yaml_ext.rb can be removed (when using Psych):

irb(main):001:0> require 'psych'
=> true
irb(main):002:0> module Foo; end
=> nil
irb(main):003:0> class Bar; end
=> nil
irb(main):004:0> Bar == Psych.load(Psych.dump(Bar))
=> true
irb(main):005:0> Foo == Psych.load(Psych.dump(Foo))
=> true
irb(main):006:0> Psych.dump(Bar)
=> "--- !ruby/class 'Bar'\n"
irb(main):007:0> Psych.dump(Foo)
=> "--- !ruby/module 'Foo'\n"
irb(main):008:0>
@gaffneyc
Collective Idea member
gaffneyc added a line comment Aug 29, 2011

Yeah, that makes sense. I'll have to see if it will break existing applications, or maybe we can rejigger job loading a bit so we're not trying to do class autoloading inside yaml.

@tenderlove
tenderlove added a line comment Aug 29, 2011

The other thing I noticed is ActiveRecord YAML serialization. That is baked in to Rails 3.1, but it differs slightly from the DJ implementation in that it won't raise an exception if the record is no longer in the database.

@pda
pda added a line comment Aug 30, 2011

@tenderlove There's a lot of information in issue #199 which led to that pull request #201.

I hate to submit short-sighted workarounds, but at the time some updates to bundler/rubygems had broken previously working version of DJ. Glad to see it getting some proper attention.

Cheers,
Paul

@yob
yob added a line comment Sep 14, 2011

@tenderlove I don't get the same results on my system (linux, 1.9.2p290)

⚡ irb
irb(main):001:0> require 'psych'
=> true
irb(main):002:0> module Foo; end
=> nil
irb(main):003:0> class Bar; end
=> nil
irb(main):004:0> Bar == Psych.load(Psych.dump(Bar))
TypeError: can't dump anonymous class Class
        from /usr/lib/ruby/1.9.1/psych/visitors/yaml_tree.rb:211:in `visit_Class'
        from /usr/lib/ruby/1.9.1/psych/visitors/yaml_tree.rb:63:in `accept'
        from /usr/lib/ruby/1.9.1/psych/visitors/yaml_tree.rb:36:in `<<'
        from /usr/lib/ruby/1.9.1/psych.rb:165:in `dump'
        from (irb):4
        from /usr/bin/irb:12:in `<main>'
irb(main):005:0> Foo == Psych.load(Psych.dump(Foo))
=> false
@yob
yob added a line comment Sep 14, 2011

and

irb(main):006:0> Psych.dump(Bar)
TypeError: can't dump anonymous class Class
        from /usr/lib/ruby/1.9.1/psych/visitors/yaml_tree.rb:211:in `visit_Class'
        from /usr/lib/ruby/1.9.1/psych/visitors/yaml_tree.rb:63:in `accept'
        from /usr/lib/ruby/1.9.1/psych/visitors/yaml_tree.rb:36:in `<<'
        from /usr/lib/ruby/1.9.1/psych.rb:165:in `dump'
        from (irb):6
        from /usr/bin/irb:12:in `<main>'
irb(main):007:0> Psych.dump(Foo)
=> "--- !ruby/object:Module {}\n"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
class Module
yaml_as "tag:ruby.yaml.org,2002:module"

0 comments on commit cbb4060

Please sign in to comment.