grape-roar
grape-roar copied to clipboard
ruby-grape
Telling a representer to represent nil adds all of that class's methods to nil
The code causing this behavior is https://github.com/ruby-grape/grape-roar/blob/master/lib/grape/roar/representer.rb#L10
Example
user = User.find_by(email: '[email protected]')
# user => nil
present user, with: User::Representer
# User::Representer.ancestors => [..., Grape::Roar::Representer, ...]
Obviously there's a bug in this application code, but the result is that, since nil is a singleton, now every time the application asks for nil, it gets it, but with all kinds of fun stuff mixed in, like a custom #to_hash that, for instance, mucks with the instantiation of ActionController::Parameters and causes completely unrelated pages in an app to blow up
Thoughts? I'd be happy to submit a PR, but i only have a cursory understanding of all the interplay between grape, roar, grape-roar, representable, etc. and just tossing in a nil check might not be the best approach
This is happening here, maybe we should protect the code from extending singletons, nil being one, maybe there's a better way than comparing to a list? Also is there any case where we would want to represent what would otherwise be a singleton?
It appears to be a design consequence of representable using inheritance (via mixin) instead of delegation (in the style of the Presenter pattern). One option aside from checking against a list is to see if the object is dupable.
def represent(object, _options = {})
dup = object.dup
dup.extend self
dup
rescue TypeError
fail TypeError.new "Can't represent #{object.class}"
end
This would be great, because even non-singletons aren't getting polluted when they're represented and then can continue to be used in another context. But this means that we can't represent anything that's not dupable. After a quick google, integers, symbols, true, false and floats aren't dupable and therefore would not be representable. I'm not sure if this is desirable behavior for grape-roar
Maybe we can just detect whether these are singleton types and make them non-representable?
This bit us suddenly, and very strangely. We also were mistakenly calling present nil, ... in one endpoint. A later request to the same process would fail with surprising errors.
We're attempting to patch this by simply guarding against the nil case (object.extend self unless object.nil?), but as discussed above that might not be a complete fix.
@joeyAghion, I'm sorry that this bug has affected you today! My proposed solution can be one of two things:
- We can use a blacklist, even though it is not my preferred solution, but it is well defined and I can have it out for you quickly + later reconcile it out properly if time to fix is a problem.
- We can instead decorate the singleton classes of the instances of the objects that we receive. Additionally, we have a nice predicate for the objects we should avoid touching.
While not immediately obvious, the magic is in doing checks like this:
nil.singleton_class == nil.class #=> true
We would avoid that object, but we wouldn't avoid this one:
irb(main):013:0> class Foo; end;
=> nil
irb(main):014:0> x = Foo.new
=> #<Foo:0x007fc0e00ff138>
irb(main):015:0> x.singleton_class == x.class
=> false
Thoughts?
I'm interested in this (I work with @joeyAghion here :)) - I think the next step should be to properly spec it, @mach-kernel do you think you understand this enough to code up a failing spec?
