🐛 Fix fetch to handle nil values

[danielvdao]

Context

When relying on delegation of fetch to attributes there are some issues that can cause unexpected / mis-matched behavior due to the underlying keys in the attributes hash.

For example, when we have an object that looks like the following, we'll get different behavior due to fetch's exception handling.

class Configuration
  include StoreModel::Model

  attribute :id
end

example = Configuration.new
puts example.id # nil
puts example.fetch(:id) # KeyError: key not found: :id

See d8c468e which shows the passing behavior, and b651dbb which shows the failing behavior.

We discovered this in our usage of a gem called grape-entity. At a high-level the gem will do a large amount of delegation, but do some duck-typing here to see if the object respond_to?(:fetch, true). Whereas before it would just call send on the underlying object.

I feel like we should remove fetch from the public api, as it's not promising 1:1 behavior similar to the dot access method. I also realize it's a breaking change for any users who are using the method today, but if I recall it's only been introduced for 2 weeks, so the sooner the better 🤔

Also want to say that we really love this gem, as it's made managing our JSON backed columns so much easier 💚

Issue: #129

Edit

After some thought, we've decided to make the API safer instead, allowing it to handle nil keys!

[danielvdao]

Another option if we want to preserve backwards-compatibility is to call fetch and rescue then return nil if a KeyError comes up, but that seems a bit more tricky because we don't know if the hash actually has the keys we want before trying to do so 🤔

Example:

    # Returns an Object, similar to Hash#fetch, except it doesn't raise a KeyError
    #
    # @param attr_name [String, Symbol]
    #
    # @return Object
    def fetch(attr_name)
      attributes.fetch(attr_name.to_s)
    rescue KeyError
      nil
    end

[danielvdao]

Cc @DmitryTsepelev not sure who else to tag here 🙏🏽

[DmitryTsepelev]

Hi @danielvdao! As far as I remember, @raphox wanted to mimic ActiveModel::AttributeSet, which has the same API with fetch.

Because of that, I love the idea of keeping the method, but changing it to be safe.

def fetch(attr_name)
  if attributes.key?(attr_name)
    attributes[attr_name]
  elsif we_have_attribute_defined?(attr_name) # do not remember the exact way to check that
    nil
  else
    raise KeyError # sounds logical, right?
  end
end

WDYT?

[danielvdao]

Yeah, I agree. I thought about it more myself and making the API safer sounds like the correct thing to do. Also won't break any users today 🙏🏽

Let me give that a shot since I'm not too familiar with the inner API.

[danielvdao]

@DmitryTsepelev this seems to be breaking out internal repo, let me see if I can investigate deeper.. Ah I think this is going to be a bit more tricky, because users can define attribute, but also define a method underneath that can do something else. For example:

class Bar
  include StoreModel::Model

  attribute :foo

  def foo
    super || :fallback
  end

It seems like the default handling or nil here can be dangerous. Curious if you have any thoughts on what can be done here? 🤔 I think this might be an issue with the hash accessor as well.. I'll think on my end too.

One option is to do self.public_send(attr_name), but this kind of messes around with the idea of #fetch. That said, #fetch and accessor method calls on the object aren't 1:1 today. I'll see if I can push something up as an example, but still not sold on it yet. If methods can override, calling the method like this could be dangerous if it requires parameters 😞

[DmitryTsepelev]

What if we do something like this:

def fetch(attr_name)
  stringified_key = attr_name.to_s
  if attributes.key?(stringified_key)
    attributes.fetch(stringified_key)
  elsif attribute_names.include?(stringified_key) && responds_to?(stringified_key)
    public_send(stringified_key)
  else
    message = attr_name.is_a?(Symbol) ? "key not found: :#{attr_name}" : "key not found: #{attr_name}"
    raise KeyError, message
  end
end

Along with fetching value from attributes, it covers the scenario when attribute is defined, value is not set, but method exists. Otherwise — we raise an error. WDYT?

[danielvdao]

That might work! I'll give it a shot. The only concern I have would be if we have a method's arity > 0 or if the attributes key? returns true, but we also care about using public_send instead. In any regards - I can give it a shot and also see what tests may be breaking. I suspect it's because I added the method in the Configuration class.

Will be a bit slow as it's a holiday for me today, but should be able to try in the next day or so.

[danielvdao]

I think there was a few interesting discoveries.

1. #fetch was kind of broken, but that is because attribute_names is an array of string values, so passing in a symbol key would cause it to snap. Instead I think refactoring it to use public_send(attr_name.to_s) might work instead, as it'll rely on the StoreModel and subsequenty ActiveModel behavior.
2. I made a new factory for the test since I didn't want to mess around too much with Configuration.
3. alias_attribute doesn't list the alias-ed attribute under attribute_names, but instead, under a completely different Hash 😅

That said, I'm still trying to see if there could be any issue when using #fetch like this on an attribute name with arity > 0, as we are doing public_send, though I think the method should be called rather than #fetch-ed at that point.

[danielvdao]

@DmitryTsepelev when you get a chance, would be great to see if tests work 🙏🏽 I ran them locally, and the few that are failing match with master. I think there's some weird setup I'm probably missing.

[danielvdao]

Hey @DmitryTsepelev just checking in here! Any updates? Would love to know if I can address any comments or thoughts you may have as well.

[danielvdao]

Hey @DmitryTsepelev, apologies for pinging you again. Would love to hear your thoughts on whether or not this is a viable solution. Please let me know, thank you!

[danielvdao]

@DmitryTsepelev I saw your GitHub status saying you were slow to respond to issues, is there anyone else I can ask about this? Happy to wait to, but unfortunately there's been no guidance 😔 I understand maintaining open source gems is a lot of work and it is much appreciated!

[DmitryTsepelev]

Shoot, I somehow managed to miss notifications 😞Lookin

[DmitryTsepelev]

LGTM!