neo4jrb / activegraph

An active model wrapper for the Neo4j Graph Database for Ruby.
http://neo4jrb.io
MIT License
1.4k stars 276 forks source link

Add psych as dependency and fix schema load on psych >= 4.0.1 #1685

Closed chytreg closed 1 year ago

chytreg commented 1 year ago

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

codecov-commenter commented 1 year ago

Codecov Report

Base: 93.49% // Head: 93.45% // Decreases project coverage by -0.05% :warning:

Coverage data is based on head (16289bb) compared to base (203f272). Patch coverage: 0.00% of modified lines in pull request are covered.

:mega: 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 ```diff @@ 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 ``` | [Impacted Files](https://codecov.io/gh/neo4jrb/activegraph/pull/1685?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=neo4jrb) | Coverage Δ | | |---|---|---| | [lib/active\_graph/tasks/migration.rake](https://codecov.io/gh/neo4jrb/activegraph/pull/1685?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=neo4jrb#diff-bGliL2FjdGl2ZV9ncmFwaC90YXNrcy9taWdyYXRpb24ucmFrZQ==) | `32.72% <0.00%> (-0.61%)` | :arrow_down: | | [lib/active\_graph/node/query/query\_proxy\_link.rb](https://codecov.io/gh/neo4jrb/activegraph/pull/1685?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=neo4jrb#diff-bGliL2FjdGl2ZV9ncmFwaC9ub2RlL3F1ZXJ5L3F1ZXJ5X3Byb3h5X2xpbmsucmI=) | `98.71% <0.00%> (-1.29%)` | :arrow_down: | | [lib/active\_graph/shared/enum.rb](https://codecov.io/gh/neo4jrb/activegraph/pull/1685?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=neo4jrb#diff-bGliL2FjdGl2ZV9ncmFwaC9zaGFyZWQvZW51bS5yYg==) | `94.93% <0.00%> (-1.22%)` | :arrow_down: | | [lib/active\_graph/relationship/rel\_wrapper.rb](https://codecov.io/gh/neo4jrb/activegraph/pull/1685?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=neo4jrb#diff-bGliL2FjdGl2ZV9ncmFwaC9yZWxhdGlvbnNoaXAvcmVsX3dyYXBwZXIucmI=) | `100.00% <0.00%> (ø)` | | | [lib/active\_graph/node/id\_property.rb](https://codecov.io/gh/neo4jrb/activegraph/pull/1685?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=neo4jrb#diff-bGliL2FjdGl2ZV9ncmFwaC9ub2RlL2lkX3Byb3BlcnR5LnJi) | `88.54% <0.00%> (+1.04%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=neo4jrb). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=neo4jrb)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

klobuczek commented 1 year ago

@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.

chytreg commented 1 year ago

When psych is not specified in the gemspec how does it get pulled in?

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.

It might break other people's code if they rely on an earlier version. We should have some type of conditional statement.

That's true I will put the conditional instead since it's the only place it breaks.

My current situation:

# Gemfile
ruby 3.2.0
activegraph 11.2.0

without psych ~> 3 in Gemfile I get, when executing rake neo4j:schema:load --trace

ArgumentError: wrong number of arguments (given 2, expected 1)
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/3.2.0/psych.rb:322:in `safe_load'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/activegraph-11.2.0/lib/active_graph/tasks/migration.rake:86:in `block (3 levels) in <top (required)>'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/task.rb:281:in `block in execute'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/task.rb:281:in `each'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/task.rb:281:in `execute'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/sentry-ruby-5.8.0/lib/sentry/rake.rb:26:in `execute'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/task.rb:219:in `block in invoke_with_call_chain'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/task.rb:199:in `synchronize'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/task.rb:199:in `invoke_with_call_chain'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/task.rb:188:in `invoke'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/application.rb:160:in `invoke_task'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/application.rb:116:in `block (2 levels) in top_level'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/application.rb:116:in `each'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/application.rb:116:in `block in top_level'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/application.rb:125:in `run_with_threads'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/application.rb:110:in `top_level'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/application.rb:83:in `block in run'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/application.rb:186:in `standard_exception_handling'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/lib/rake/application.rb:80:in `run'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/rake-13.0.6/exe/rake:27:in `<top (required)>'
/Users/chytreg/.asdf/installs/ruby/3.2.0/bin/rake:25:in `load'
/Users/chytreg/.asdf/installs/ruby/3.2.0/bin/rake:25:in `<main>'

with psych ~> 3 it works, but with psych >=4.0.1 it causes what I've described here: https://github.com/neo4jrb/activegraph/issues/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 psych ~> 3 I probably downgrade the version

Im not sure where to check which psych version ruby uses, but will try to figure it out.

chytreg commented 1 year ago

According to this source they did psych update in ruby version 3.1 https://www.ctrl.blog/entry/ruby-psych4.html

chytreg commented 1 year ago

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 It was core team decision to stay with that https://bugs.ruby-lang.org/issues/17866

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

klobuczek commented 1 year ago

Thank you @chytreg for the detailed explanation. Why 4.0.1 and not 4.0.0?

chytreg commented 1 year ago

Why 4.0.1 and not 4.0.0?

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 Psych 4.0.0 does not come with any ruby by default, so the only chance to get this error is having lock in the Gemfile to version 4.0.0 I would consider it as a very rare edge case.

BTW the psych team on Github issues recommends the update to 4.0.1 as a fix for that https://github.com/ruby/psych/issues/504

Psych::DisallowedClass: Tried to load unspecified class: Symbol
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/class_loader.rb:99:in `find'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/class_loader.rb:28:in `load'
(eval):2:in `symbol'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/class_loader.rb:32:in `symbolize'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/class_loader.rb:84:in `symbolize'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/scalar_scanner.rb:74:in `tokenize'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:65:in `deserialize'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:128:in `visit_Psych_Nodes_Scalar'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/visitor.rb:30:in `visit'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/visitor.rb:6:in `accept'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:35:in `accept'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:345:in `block in revive_hash'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:343:in `each'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:343:in `each_slice'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:343:in `revive_hash'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:167:in `visit_Psych_Nodes_Mapping'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/visitor.rb:30:in `visit'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/visitor.rb:6:in `accept'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:35:in `accept'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:318:in `visit_Psych_Nodes_Document'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/visitor.rb:30:in `visit'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/visitor.rb:6:in `accept'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych/visitors/to_ruby.rb:35:in `accept'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych.rb:334:in `safe_load'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych.rb:587:in `block in safe_load_file'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych.rb:586:in `open'
/Users/chytreg/.asdf/installs/ruby/3.2.0/lib/ruby/gems/3.2.0/gems/psych-4.0.0/lib/psych.rb:586:in `safe_load_file'
klobuczek commented 1 year ago

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.

chytreg commented 1 year ago

Yes you are right, best would be have 4.0.0 instead of 4.0.1. Should I reopen the PR?