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 bug symbolize_names: true with null column yaml #423

Closed

Conversation

kibitan
Copy link

@kibitan kibitan commented Nov 24, 2019

Trying to parse YAML which contains null column symbolize_names: true would raise NoMethodError

sample code

$ docker run --entrypoint sh --rm -it ruby:2.7-rc

# ruby -v
ruby 2.7.0preview2 (2019-10-22 master 02aadf1032) [x86_64-linux]
# irb
irb(main):001:0> require 'yaml'
=> true
irb(main):002:0>  Psych.load("null: nullnull", symbolize_names: true)
Traceback (most recent call last):
        8: from /usr/local/bin/irb:23:in `<main>'
        7: from /usr/local/bin/irb:23:in `load'
        6: from /usr/local/lib/ruby/gems/2.7.0/gems/irb-1.1.0.pre.3/exe/irb:11:in `<top (required)>'
        5: from (irb):2
        4: from /usr/local/lib/ruby/2.7.0/psych.rb:276:in `load'
        3: from /usr/local/lib/ruby/2.7.0/psych.rb:606:in `symbolize_names!'
        2: from /usr/local/lib/ruby/2.7.0/psych.rb:606:in `each'
        1: from /usr/local/lib/ruby/2.7.0/psych.rb:607:in `block in symbolize_names!'
NoMethodError (undefined method `to_sym' for nil:NilClass)

```
$ rake test

....

..................................................................................................................................E......................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 0.434459s, 1309.6748 runs/s, 3622.8965 assertions/s.

  1) Error:
TestPsych#test_symbolize_names:
NoMethodError: undefined method `to_sym' for nil:NilClass
    /repo/psych/lib/psych.rb:611:in `block in symbolize_names!'
    /repo/psych/lib/psych.rb:610:in `each'
    /repo/psych/lib/psych.rb:610:in `symbolize_names!'
    /repo/psych/lib/psych.rb:611:in `block in symbolize_names!'
    /repo/psych/lib/psych.rb:610:in `each'
    /repo/psych/lib/psych.rb:610:in `symbolize_names!'
    /repo/psych/lib/psych.rb:280:in `load'
    /repo/psych/test/psych/test_psych.rb:324:in `test_symbolize_names'

569 runs, 1574 assertions, 0 failures, 1 errors, 0 skips
```
@jeremyevans
Copy link
Contributor

This patch only fixes the nil case, it doesn't fix cases where the key is a value other than a String or nil. For example, this is still broken:

YAML.load("1: n", :symbolize_names=>true)

The patch should be changed to only call to_sym if the value is a String, I think.

@kibitan
Copy link
Author

kibitan commented Nov 26, 2019

@jeremyevans thank you for the reply, appreciate.
yes, you are right, I haven't notice about it. I'm going to fix patch 😊

@kibitan
Copy link
Author

kibitan commented Nov 28, 2019

@jeremyevans I reflect your suggestion by 1271aa7, can you have another look? also, windows build on github action were failed, do you have any idea for it?

@jeremyevans
Copy link
Contributor

This looks good to me. @tenderlove should probably review.

@kibitan
Copy link
Author

kibitan commented Dec 2, 2019

@jeremyevans thank you for your support! 😀

@tenderlove could you have a look when you have time?

@jeremyevans
Copy link
Contributor

This no longer applies. It looks like #480 will fix this.

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.

None yet

2 participants