search

ruby-grape / grape-entity

An API focused facade that sits on top of an object model.
MIT License
715 stars 153 forks source link

`FetchableObject` can cause unexpected behavior #368

Closed danielvdao closed 1 year ago

danielvdao commented 2 years ago

Context

When delegating to a FetchableObject, we've seen that it can cause breaking changes since it's only checking object.respond_to?(:fetch, true) here as opposed to checking if the object is a Hash.

The reason this came up is because we depend on another gem StoreModel which delegates fetch to the underlying Hash. However the class wraps over a Hash with ActiveModel, so the fetch delegation isn't 100% representative of calling object.random_method.

This can cause a bug with KeyError as the underlying object is a Hash, but doesn't actually have the key on it. Instead we depend on the accessor method which does its own nil-safe handling behavior.

I'm looking into it here: https://github.com/DmitryTsepelev/store_model/pull/128, and I was curious about the inspiration for FetchableObject (since I may be missing something 😓) and whether or not it makes sense to actually remove it as I feel like fetch should only be called on a Hash. In the PR linked it can get tricky to re-implement #fetch properly 😓

I have an example commit on a fork https://github.com/danielvdao/grape-entity/commit/92df52be26f26ee7f916c5495130d841f51fe77e, and can open a PR / actually remove FetchableObject if that's the right approach, but I wanted to get y'all's thoughts first.

danielvdao commented 2 years ago

Hi @LeFnord I was wondering if you happened to have context on this? I'm happy to remove FetchableObject, but I'm curious if there's something about it that I may be missing.

LeFnord commented 2 years ago

mmh, you can try it

but have in mind, this will be used by expose, to find out how the data can be accessed, means object.fetch should be available

danielvdao commented 2 years ago

@LeFnord Thank you! I'll open a PR. I think the confusion is that some objects respond to fetch, but don't match with their public_send behavior. I would imagine that #fetch should only be used for Hash 🤔 but in that case we already have Hash.

danielvdao commented 2 years ago

@LeFnord let me know if this makes sense? Thank you!

https://github.com/ruby-grape/grape-entity/pull/369