Fix const errors being hidden by bug in const_missing

[dandehavilland]

I had not included a file and so a constant was missing. I did not receive the correct error as the custom const_missing was throwing an error of its own.

This PR resolves that for me.

NameError:
  undefined local variable or method `parent' for MyApiClass
# /repos/grape/lib/grape/api.rb:107:in `method_missing'
# /repos/grape/lib/grape/api.rb:67:in `const_missing'
# ./lib/routes/my_api_class.rb:24:in `block in <class:MyApiClass>'
# /repos/grape/lib/grape/endpoint.rb:57:in `call'
# /repos/grape/lib/grape/endpoint.rb:57:in `block (2 levels) in generate_api_method'
# /repos/grape/lib/grape/endpoint.rb:56:in `block in generate_api_method'
# /repos/grape/lib/grape/endpoint.rb:263:in `block in run'
# /repos/grape/lib/grape/endpoint.rb:243:in `run'
# /repos/grape/lib/grape/endpoint.rb:318:in `block in build_stack'
# /repos/grape/lib/grape/middleware/base.rb:31:in `call!'
# /repos/grape/lib/grape/middleware/base.rb:24:in `call'
# /repos/grape/lib/grape/middleware/error.rb:37:in `block in call!'
# /repos/grape/lib/grape/middleware/error.rb:36:in `catch'
# /repos/grape/lib/grape/middleware/error.rb:36:in `call!'
# ...

[dandehavilland]

@dblock I've added a spec for the issue I experienced (fails on master, passes with this fix). It's tied to mounted modules and is resolved by checking @base_parent rather than parent. Is it possible that parent was a typo there? I can't find a reference to it anywhere else.

[dblock]

@dblock I've added a spec for the issue I experienced (fails on master, passes with this fix). It's tied to mounted modules and is resolved by checking @base_parent rather than parent. Is it possible that parent was a typo there? I can't find a reference to it anywhere else.

Yes absolutely.

[dblock]

I know it's unrelated, but care to fix the Rails edge problem as well for 💚, please

[dandehavilland]

I know it's unrelated, but care to fix the Rails edge problem as well for 💚, please

I can take a look if you point me in the right direction - I haven't touched Rails in a long while though so no promises!

[dm1try]

Is it possible that parent was a typo there? I can't find a reference to it anywhere else.

according to ce4966d, parent could be the part of active support module introspection interface(parent/parent_name were added; I doubt that it's a typo). it is somehow related to the integration with Rails, see #1871

If this is true then parent != @base_parent but it works because the initial implementation does not have specs.

[dblock]

The Rails/edge failure seems trivial: expected NameError with "uninitialized constant Mounted::SomeRandomConst", got #<NameError: uninitialized constant SomeRandomConst> with backtrace:, so the expectation should just be a little more flexible with the message not containing the namespace. @dandehavilland I think .to raise_error NameError, /SomeRandomConst/ will do it, give it a try?

This PR seems to be fixing #1871, as it no longer uses parent, so I think this kills both birds with one stone.

Let's see what @myxoh says.

[myxoh]

Hi, thanks for tagging me! in response to @dandehavilland: when I used .parent I was making use of the ActiveSupport class method https://apidock.com/rails/ActiveSupport/CoreExtensions/Module/parent which does not behave in the same way as @base_instance_parent (Which gives you the class from which the API instance is inheriting)

I will have a deeper look at the issue and suggest an alternative

[myxoh]

I'm surprised no specs are failing after this change, I would have expected some to be. We probably need to add some coverage there.

I imagine that ActiveSupport at some point deprecated the parent method on classes altogether. The solution would be to use the new method I'd say

[myxoh]

I haven't been able to reproduce the issue with this spec, I'm assuming it's because I'm not using the right version of Active Support (I'm using the one on the gemfile lock on edge)

[myxoh]

I can't provide any extra info on the proper fix until I can reproduce the issue. After that I'm happy to help

[dandehavilland]

I'm assuming it's because I'm not using the right version of Active Support

I'm using ActiveSupport 5.2.3 - does that help?

[dandehavilland]

Also, I'm using Grape outside of Rails and it seems active_support/core_ext/module is not loaded by default. If I load that, then this issue goes away.

[myxoh]

@dandehavilland I see! yes, that sounds to be the likely cause of the issue! Could we add a Travis grid for your scenario?
As I wasn't able to get that spec to fail in any of the existing ones.

If you are right, then the solution would probably to add the reqiurement to that module, or to solve what I was trying to do without ActiveSupport (finding the base module of a class)

[dm1try]

As I wasn't able to get that spec to fail in any of the existing ones.

It's weird. The spec provided in this issue is red in my case, and this is expected behaviour as grape does not load explicitly active_support/core_ext/module/introspection in which the needed functional is implemented.

bundle exec rspec spec/grape/api_spec.rb:3873
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's default settings.
Run options: include {:locations=>{"./spec/grape/api_spec.rb"=>[3873]}}

Grape::API
  const_missing
    raises an error (FAILED - 1)

Failures:

  1) Grape::API const_missing raises an error
     Failure/Error: expect { get '/const/missing' }.to raise_error(NameError).with_message('uninitialized constant Mounted::SomeRandomConst')

       expected NameError with "uninitialized constant Mounted::SomeRandomConst", got #<NameError: undefined local variable or method `parent' for Mounted:Class
       Did you mean?  print> with backtrace:
...

[myxoh]

I don't know why yesterday I wasn't able to reproduce this, I now have been able to.
I also now understand why no test is failing when you've changed from parent to @base_parent

It seems to be the case that that branch of the if condition was no longer required, I think the issue that made it required was fixed on commit 532c08d

Therefore the proper solution for the issue you are presenting here is just removing the whole elsif as it is no longer required. It should look like:

      def const_missing(*args)
        if base_instance.const_defined?(*args)
          base_instance.const_get(*args)
        else
          super
        end
      end

I've already run the specs (including the one you've introduced) and it seems to be fixed when doing this

[myxoh]

@dandehavilland just a friendly pig whether you can update the PR, I think that will also fix the spec that's currently failing

[dandehavilland]

Ok will do that later today!

…

On Thu, 11 Apr 2019, 11:05 Nicolas Klein, ***@***.***> wrote: @dandehavilland <https://github.com/dandehavilland> just a friendly pig whether you can update the PR, I think that will also fix the spec that's currently failing — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1876 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF2bs28gDsUpKX6hNkQniGm8zinj5wyks5vfvrMgaJpZM4chlTd> .

[myxoh]

Well the spec is still failing, not sure what to make of it, I think it might be worth updating the spec in this instance as the error message is being generated on a low level, and I don't think this is indicative of a larger issue.

I'll leave this to your criteria

[dblock]

The spec failure is trivial:

1) Grape::API const_missing raises an error
     Failure/Error: expect { get '/const/missing' }.to raise_error(NameError).with_message('uninitialized constant Mounted::SomeRandomConst')
     
       expected NameError with "uninitialized constant Mounted::SomeRandomConst", got #<NameError: uninitialized constant SomeRandomConst> with backtrace:

Note the namespace in the error vs. no namespace in the expected error

Change to expect { get '/const/missing' }.to raise_error(NameError).with_message(/uninitialized constant .* SomeRandomConst/) or something similar with a regex or even just SomeRandomConst`

[dblock]

Two more minor things please. Apologies for my OCD.

[dandehavilland]

Two more minor things please. Apologies for my OCD.

I missed both of those - fixed!

[dblock]

Merged, thanks.