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

Rescue LoadError #1500

Closed Grafikart closed 6 years ago

Grafikart commented 6 years ago

This fix an issue I had when the system tried to constantize labels

codecov-io commented 6 years ago

Codecov Report

Merging #1500 into master will increase coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1500      +/-   ##
==========================================
+ Coverage   96.83%   96.83%   +<.01%     
==========================================
  Files         205      205              
  Lines       12663    12665       +2     
==========================================
+ Hits        12262    12264       +2     
  Misses        401      401
Impacted Files Coverage Δ
spec/e2e/node_wrapping_spec.rb 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f66ba2e...7c0ae40. Read the comment docs.

cheerfulstoic commented 6 years ago

Thanks for this!

What specifically is the LoadError that you're getting? I'd like to know more about it before we rescue from it.

It would also be good to have a spec which causes this to both explain the issue and to prevent regressions

Grafikart commented 6 years ago

@cheerfulstoic I'm not able to produce the rspec but I can explain how the error is triggered.

Within my rails application :

"FORMATION".constantize
# LoadError: Unable to autoload constant FORMATION, expected /some/random/path/formation.rb to define it

Rails autoloader seems to trigger a LoadError instead of a NameError if it finds a file with the same name. That's why it's hard to create a spec to test this feature.

cheerfulstoic commented 6 years ago

When I try that in my Rails app I get a NameError. Maybe it's a version of Rails thing? What version are you using?

It looks like LoadError is what you get when you try to require something that isn't there:

2.5.1 :003 > require 'aoeundaeound'
Traceback (most recent call last):
        1: from (irb):3
LoadError (cannot load such file -- aoeundaeound)

So I feel like there's something I'm not completely understanding.

Also, I believe that it's ActiveSupport specifically that loads files based on namespace/class conventions and that's a dependency of the neo4j gem, so maybe there is some sort of tool that could be used from the gem to test this.

Grafikart commented 6 years ago

@cheerfulstoic Here are the steps to produce a LoadError on constantize from a fresh Rails app

  1. Create a new rails app (without ORM) rails new app -O
  2. Create a simple Demo model

app/models/demo.rb

class Demo
end
  1. rails c, then "DEMO".constantize will throw a LoadError instead of a NameError
cheerfulstoic commented 6 years ago

OK, thanks. I see how there's an issue where two different class names (Demo and DEMO) map to the same file and then it fails because it can't find DEMO in the file.

It's a bit weird to me because it seems like LoadError comes from Ruby but ActiveSupport is using it for a somewhat separate thing. But it does make me feel better that this is rescuing from a base Ruby exception rather than ActiveSupport. And since the line is so targeted, I don't see a possibility of the other sort of LoadError situation happening erroneously here. So :+1: thanks!

cheerfulstoic commented 6 years ago

I was able to add specs for this. It gave me the opportunity to test for the NameError case as well. Check out this commit: https://github.com/neo4jrb/neo4j/commit/bcae1b4c5fb250fa6b56a82ec4a4fe836b5d56bf

cheerfulstoic commented 6 years ago

Released as 9.2.3. Thanks again!