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

Uninitialized constant error for nested class of Grape::API class when using bootsnap and sidekiqswarm #2258

Closed
guigs opened this issue May 30, 2022 · 7 comments · Fixed by #2328
Labels

Comments

@guigs
Copy link

guigs commented May 30, 2022

The error only happens when booting a Rails app in a particular way (sidekiqswarm) when loading a nested class whose parent class inherits from Grape::API.

For example:

# app/api/thing.rb
class Thing < Grape::API; end

# app/api/thing/nested_thing.rb
class Thing
  class NestedThing; end
end

When started with sidekiqswarm, the application crash with:

/home/guigs/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/grape-1.6.2/lib/grape/api.rb:87:in `const_get': uninitialized constant #<Class:0x00007f5b1c6cb2d0>::NestedThing (NameError)
Did you mean?  MyApi::Thing::NestedThing
        from /home/guigs/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/grape-1.6.2/lib/grape/api.rb:87:in `const_missing'
        from /home/guigs/grapeboot/config/routes.rb:8:in `block in <main>'
        from /home/guigs/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/actionpack-7.0.3/lib/action_dispatch/routing/route_set.rb:428:in `instance_exec'
        from /home/guigs/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/actionpack-7.0.3/lib/action_dispatch/routing/route_set.rb:428:in `eval_block'
        from /home/guigs/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/actionpack-7.0.3/lib/action_dispatch/routing/route_set.rb:410:in `draw'
        from /home/guigs/grapeboot/config/routes.rb:1:in `<main>'
        from /home/guigs/.rbenv/versions/3.0.3/lib/ruby/gems/3.0.0/gems/bootsnap-1.11.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:39:in `load'

The problem does not happen if DISABLE_BOOTSNAP_LOAD_PATH_CACHE=1 is used.

I've created a minimum Rails app to demonstrate the problem: https://github.com/heyjobs/grapeboot

Related Sidekiq issue: sidekiq/sidekiq#5346
Related Bootsnap issue: Shopify/bootsnap#416

This has been fixed now in Bootsnap, but it was suggested that behavior should be fixed in Grape.

@casperisfine
Copy link

To clarify, this is the constant I have a problem with:

NON_OVERRIDABLE = (Class.new.methods + %i[call call! configuration compile! inherited]).freeze

If any code loaded after grape/api.rb defines some methods on Module or Class they end up being gimped. I think grape could do without that cache, just call Class.instance_methods every time.

@dblock
Copy link
Member

dblock commented Jun 2, 2022

@casperisfine Makes sense. Try adding an integration test in Grape and fix this?

@dblock dblock added the bug? label Jun 2, 2022
@casperisfine
Copy link

I may do it at some point, but it's super low priority for me as I'm not a grape user.

@allisonphillips
Copy link

allisonphillips commented May 13, 2023

I came across this while looking into a Zeitwerk thing, but given the available information this seems like implementation error to me.

Here: https://github.com/heyjobs/grapeboot/tree/main/app/api

You are defining:

# app/api/thing.rb
class Thing < Grape::API
end

and

# app/api/thing/nested_thing.rb
class Thing
  class NestedThing
  end
end

Given that the inheritance from Grape::API is commented out I imagine you were trying it with that value present at some point, but without it that inner class is not inheriting from Grape::API. For example:

class Thing < Grape::API
end
class Thing
  class NestedThing
  end
end

# subclass <= superclass will evaluate to true:
Thing <= Grape::API
=> true                  
Thing::NestedThing <= Grape::API
=> nil
Thing::NestedThing <= Thing
=> nil
Thing::NestedThing <= Object
=> true

Apart from Thing::NestedThing not being defined as a subclass of Grape::API, the non-commented out content of your config/routes.rb file is referencing the class name in a void context:

Rails.application.routes.draw do
  Thing::NestedThing
end

The Grape docu will tell you to either use #compile! or to mount your top-level API class there, so I'm not sure why it's written that way but it is another reason I would expect this configuration to not work.

Your original comment mentions MyApi which isn't in your test project, and in the history it looks like you had a folder named MyApi but never actually defined it as a module? I have to wonder if this wasn't a combination of not defining the module/class/inheritance/routing correctly (as demonstrated above) and improper directory structure given the files that were defined (which is a common trigger for this issue, e.g., https://stackoverflow.com/a/29307359 and needing to have the structure app/api/api/*). While I understand a change was made in Shopify/bootsnap#416 that may have stopped it from erroring, it seems like the implementation triggering it was not utilizing grape correctly.

@dblock
Copy link
Member

dblock commented May 13, 2023

Thanks for the detailed call outs @allisonphillips! @guigs care to check it out?

@guigs
Copy link
Author

guigs commented May 15, 2023

@allisonphillips Thank you for investigating. You're right in your analysis.

I deliberate removed the inheritance from NestedThing and removed the mounting in order to "simplify" my demonstration, to show that neither the mounting nor the inheritance in NestedThing are cause of this bug.

These were actually from of my attempts to debug this issue and a way to rule out they were not the cause.

But now I see how this can make it more confusing, as it is not doing the proper thing.

I just added back the inheritance and the mounting and the problem can still be reproduced.

casperisfine pushed a commit to casperisfine/grape that referenced this issue May 16, 2023
Fix: ruby-grape#2258

Some gems or app code may define methods on `Class` after `grape` is
loaaded but before `override_all_methods!` is called.

As such it's better to check `Class.method_defined?` when doing the override.
casperisfine pushed a commit to casperisfine/grape that referenced this issue May 16, 2023
Fix: ruby-grape#2258

Some gems or app code may define methods on `Class` after `grape` is
loaaded but before `override_all_methods!` is called.

As such it's better to check `Class.method_defined?` when doing the override.
@casperisfine
Copy link

Sorry, totally forgot about this one. Proposed fix: #2328

casperisfine pushed a commit to casperisfine/grape that referenced this issue May 17, 2023
Fix: ruby-grape#2258

Some gems or app code may define methods on `Class` after `grape` is
loaaded but before `override_all_methods!` is called.

As such it's better to check `Class.method_defined?` when doing the override.
casperisfine pushed a commit to casperisfine/grape that referenced this issue May 18, 2023
Fix: ruby-grape#2258

Some gems or app code may define methods on `Class` after `grape` is
loaaded but before `override_all_methods!` is called.

As such it's better to check `Class.method_defined?` when doing the override.
casperisfine pushed a commit to casperisfine/grape that referenced this issue May 18, 2023
Fix: ruby-grape#2258

Some gems or app code may define methods on `Class` after `grape` is
loaaded but before `override_all_methods!` is called.

As such it's better to check `Class.method_defined?` when doing the override.
casperisfine pushed a commit to casperisfine/grape that referenced this issue May 19, 2023
Fix: ruby-grape#2258

Some gems or app code may define methods on `Class` after `grape` is
loaaded but before `override_all_methods!` is called.

As such it's better to check `Class.method_defined?` when doing the override.
casperisfine pushed a commit to casperisfine/grape that referenced this issue May 19, 2023
Fix: ruby-grape#2258

Some gems or app code may define methods on `Class` after `grape` is
loaaded but before `override_all_methods!` is called.

As such it's better to check `Class.method_defined?` when doing the override.
@dblock dblock closed this as completed in 4f27305 May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants