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

Inheriting from Grape::API breaks basic Ruby functionality #1829

Closed
sos4nt opened this issue Nov 30, 2018 · 7 comments
Closed

Inheriting from Grape::API breaks basic Ruby functionality #1829

sos4nt opened this issue Nov 30, 2018 · 7 comments

Comments

@sos4nt
Copy link

sos4nt commented Nov 30, 2018

Grape seems to be messing with build-in methods resulting in self schizophrenia issues. Here are some examples: (using version 1.2.1)

require 'grape'

class MyAPI < Grape::API
end

MyAPI
#=> MyAPI

MyAPI.itself
#=> #<Class:0x00007faccf943300>

MyAPI.eql?(MyAPI)
#=> false

MyAPI == MyAPI
#=> false

MyAPI.itself == MyAPI
#=> false

MyAPI == MyAPI.itself
#=> true

MyAPI.itself == MyAPI.itself
#=> true

Please fix this. Better yet, don't do this in the first place. Ruby's object model is complex enough by itself.

@myxoh
Copy link
Member

myxoh commented Nov 30, 2018

OK, not sure whether this reflects a specific issue or is just a general comment.
Nevertheless it triggered me reinspecting the code around remounting, and it indeed was possible to make a less risky approach.

#1830
I've made all of these into specs and created a PR

@myxoh
Copy link
Member

myxoh commented Nov 30, 2018

Overriding new is still something I need to do in order to preserver backwards compatibility with a new fundamentally different underyling implemention, but other than that, I was able to restore all other class methods

@sos4nt
Copy link
Author

sos4nt commented Nov 30, 2018

OK, not sure whether this reflects a specific issue or is just a general comment.

Somehow specific. One of our checks for a specific endpoint failed for no apparent reason after the 1.2 upgrade:

if env['api.endpoint'].options[:for].base == Some::Grape::Endpoint

Investigating further lead to the aforementioned oddities.

If you are referring to "Better yet, don't do this in the first place"? That's more like an advice in general. I couldn't figure out what was going on with the object on hand. It somehow looked like a mixture of instance and subclass with a bunch of overridden methods hidden in the anonymous classes' singleton class (yikes). It just felt way too complex, hence my comment. Don't get me wrong, Grape is great, I love it 🤗

Overriding new is still something I need to do [...]

That's how Struct works – Struct.new returns a new (anonymous) class which inherits from Struct.

[...] other than that, I was able to restore all other class methods

Retrieving the method names is much better than hardcoding them. But why do you have to batch-override methods at all? Why do you need this delegation? Wouldn't it be possible / easier to use Ruby's object model, i.e. casual classes and plain inheritance? I'm just curious, trying to understand Grape's architecture.

dblock added a commit that referenced this issue Nov 30, 2018
restores self_sanity Addresses #1829
@dblock
Copy link
Member

dblock commented Nov 30, 2018

I merged #1830.

@dblock
Copy link
Member

dblock commented Nov 30, 2018

Retrieving the method names is much better than hardcoding them. But why do you have to batch-override methods at all? Why do you need this delegation? Wouldn't it be possible / easier to use Ruby's object model, i.e. casual classes and plain inheritance? I'm just curious, trying to understand Grape's architecture.

Yes, but it wasn't as obvious to implement. Try to make a PR on top of what we have now @sos4nt along those lines. If you can get all the specs to pass you'll be my hero.

@myxoh
Copy link
Member

myxoh commented Nov 30, 2018

As @dblock said, I'm sure with a very large change it could be done. With a reasonably sized change, we couldn't figure out a better way.
Would love it if you could 😄

@sos4nt
Copy link
Author

sos4nt commented Nov 30, 2018

I'm closing this for now, I'll open a PR if I find a better solution. Thanks guys! 😄

@sos4nt sos4nt closed this as completed Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants