Add psych as dependency and fix schema load on psych >= 4.0.1 #1685
Conversation
From version 4 psych, has changed the API for load_safe. Resolves neo4jrb#1684
Codecov ReportBase: 93.49% // Head: 93.45% // Decreases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #1685 +/- ##
==========================================
- Coverage 93.49% 93.45% -0.05%
==========================================
Files 128 128
Lines 5812 5817 +5
==========================================
+ Hits 5434 5436 +2
- Misses 378 381 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@chytreg please help me understand. When psych is not specified in the gemspec how does it get pulled in? Furthermore, we probably shouldn't be changing the required version like that. It might break other people's code if they rely on an earlier version. We should have some type of conditional statement. |
since ruby 1.9.2 psych is a part of MRI (note from psych project), usually you don't need to upgrade the version which comes with ruby, but in my project when we switched to ruby ~> 3 we had problems with builtin psych version and explicit upgrade solves our compatibility issues. Currently the projects uses Dependabot to keep all the gems updated, and we noticed that newer version of psych breaks activegraph, I think in some point you may hit that problem of psych compatibility issues.
That's true I will put the conditional instead since it's the only place it breaks. My current situation: without psych ~> 3 in Gemfile I get, when executing with psych ~> 3 it works, but with psych >=4.0.1 it causes what I've described here: #1684 Hmm, but what I see, in the source of my ruby is: # /Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/3.2.0/psych.rb:322:in `safe_load'
def self.safe_load yaml, permitted_classes: [], permitted_symbols: [], aliases: false, filename: nil, fallback: nil, symbolize_names: false, freeze: false, strict_integer: false
result = parse(yaml, filename: filename)So by adding Im not sure where to check which psych version ruby uses, but will try to figure it out. |
|
According to this source they did psych update in ruby version 3.1 https://www.ctrl.blog/entry/ruby-psych4.html |
|
So from ruby 3.1.0 MRI comes with https://www.ruby-lang.org/en/news/2021/12/25/ruby-3-1-0-released/ psych 4.0.1 Ruby 3.2.0 comes with psych 5.0.1 https://www.ruby-lang.org/en/news/2022/12/25/ruby-3-2-0-released/ So it we like to make a conditional, we should do it on ruby version |
Since ruby 1.9.2 psych is a part of ruby MRI. From ruby 3.1.0 it comes with psych 4.0.1, which introduced a braking change into safe_load method. Ruby 3.2.0 comes with psych 5.0.1 refs: - https://www.ruby-lang.org/en/news/2021/12/25/ruby-3-1-0-released/ - https://www.ruby-lang.org/en/news/2022/12/25/ruby-3-2-0-released/
|
Thank you @chytreg for the detailed explanation. Why |
On psych 4.0.0 it raises another sort of error, it looks like the 4.0.0 it's buggy that's why ruby team included 4.0.1 BTW the psych team on Github issues recommends the update to 4.0.1 as a fix for that ruby/psych#504 |
|
Yes, but we don't want to add our own error on 4.0.0 but rather have it fail on errors that are out of our control. So this version should use the new api as well. |
|
Yes you are right, best would be have 4.0.0 instead of 4.0.1. Should I reopen the PR? |
Add psych as dependency and fix schema load on psych >= 4.0.1, because from version 4 psych, has changed the API for load_safe.
Resolves #1684