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

YAML parses okay on CRuby, raises exception on JRuby #4847

Closed
r-obert opened this Issue Nov 10, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@r-obert

r-obert commented Nov 10, 2017

testcase.yml

---
  !!seq [
    !!str "https://www.youtube.com/watch?v=DzpKasJJtRs",
    !!str "2Pac - Dont Care What Ya\'ll Think Remix Music Video 2017",
  ]

JRuby

$ jruby -v -ryaml -e "YAML.load_file('./testcase.yml')"
jruby 9.1.13.0 (2.3.3) 2017-09-06 8e1c115 Java HotSpot(TM) 64-Bit Server VM 25.144-b01 on 1.8.0_144-b01 +jit [darwin-x86_64]
Psych::SyntaxError: (./testcase.yml): found unknown escape character '(39) while scanning a double-quoted scalar at line 4 column 37

CRuby

ruby -v -ryaml -e "YAML.load_file('./testcase.yml')"
ruby 2.4.2p198 (2017-09-14 revision 59899) [x86_64-darwin15]
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 10, 2017

Member

Huh, this is pretty simple YAML too. I've confirmed it's valid YAML (the parser we use can be finicky) so it definitely should be ok.

The only escape character is \\ which is ASCII 39, so the error actually just doesn't like that the single quote is escaped, perhaps because it doesn't need to be in a double-quoted string. This may be a grey area in YAML but more and more I suspect the library we use.

Member

headius commented Nov 10, 2017

Huh, this is pretty simple YAML too. I've confirmed it's valid YAML (the parser we use can be finicky) so it definitely should be ok.

The only escape character is \\ which is ASCII 39, so the error actually just doesn't like that the single quote is escaped, perhaps because it doesn't need to be in a double-quoted string. This may be a grey area in YAML but more and more I suspect the library we use.

@headius headius added this to the JRuby 9.1.15.0 milestone Nov 10, 2017

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 10, 2017

Member

Well I've confirmed it's not fixed in the latest SnakeYAML (the library we use). I'll bring it to them.

Member

headius commented Nov 10, 2017

Well I've confirmed it's not fixed in the latest SnakeYAML (the library we use). I'll bring it to them.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 10, 2017

Member

So it turns out I've dealt with this before, but never followed up to get it fixed in SnakeYAML 😞

https://bitbucket.org/asomov/snakeyaml/issues/350/failure-to-parse-escaped-single-quote-in-a

This was originally reported in #2199 where @asomov pointed out that the single quote is not a valid character in the YAML spec, and I confirmed that neither YAML 1.1 nor 1.2 list it as valid.

http://www.yaml.org/spec/1.2/spec.html#id2776092

I do not know why MRI passes this. I had not noticed the online YAML tester I used (http://www.yamllint.com/) is "optimized for ruby" so that explains in why it said it was ok. Another tester, based on Python, rejects it (http://yaml-online-parser.appspot.com/).

So, I think maybe we need to pull in @hsbt and @tenderlove. This does not appear to be valid YAML according to any spec, and yet MRI accepts it. Why?

Member

headius commented Nov 10, 2017

So it turns out I've dealt with this before, but never followed up to get it fixed in SnakeYAML 😞

https://bitbucket.org/asomov/snakeyaml/issues/350/failure-to-parse-escaped-single-quote-in-a

This was originally reported in #2199 where @asomov pointed out that the single quote is not a valid character in the YAML spec, and I confirmed that neither YAML 1.1 nor 1.2 list it as valid.

http://www.yaml.org/spec/1.2/spec.html#id2776092

I do not know why MRI passes this. I had not noticed the online YAML tester I used (http://www.yamllint.com/) is "optimized for ruby" so that explains in why it said it was ok. Another tester, based on Python, rejects it (http://yaml-online-parser.appspot.com/).

So, I think maybe we need to pull in @hsbt and @tenderlove. This does not appear to be valid YAML according to any spec, and yet MRI accepts it. Why?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 10, 2017

Member

I will close the older bug in favor of this new active bug.

Member

headius commented Nov 10, 2017

I will close the older bug in favor of this new active bug.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 29, 2017

Member

I've filed an additional bug with libyaml. Hopefully I won't have to also file a bug with Psych to get some movement on this! (cc @hsbt, @tenderlove)

yaml/libyaml#68

Member

headius commented Nov 29, 2017

I've filed an additional bug with libyaml. Hopefully I won't have to also file a bug with Psych to get some movement on this! (cc @hsbt, @tenderlove)

yaml/libyaml#68

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 29, 2017

Member

If we fix this at all, it won't be in 9.1.15.0.

Member

headius commented Nov 29, 2017

If we fix this at all, it won't be in 9.1.15.0.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Dec 5, 2017

Member

A PR has been created for libyaml (which seems very likely to be merged), which means this is going to start percolating back into MRI.

Given that fact I'm going to close this as Won't Fix and let MRI know what's happening.

Member

headius commented Dec 5, 2017

A PR has been created for libyaml (which seems very likely to be merged), which means this is going to start percolating back into MRI.

Given that fact I'm going to close this as Won't Fix and let MRI know what's happening.

@headius headius closed this Dec 5, 2017

@headius headius modified the milestones: JRuby 9.1.16.0, Won't Fix Dec 5, 2017

@r-obert

This comment has been minimized.

Show comment
Hide comment
@r-obert

r-obert Dec 5, 2017

cheers, great @headius

r-obert commented Dec 5, 2017

cheers, great @headius

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