Don't use fetch for fields that use hash_key

[danielvdao]

This is a proposal to fix #4059.

The TL;DR of our issue is that #fetch doesn't exist on all Objects, so if inner_obj is not a Hash or doesn't respond to #fetch, this can break 😖

[danielvdao]

Actually I am worried this might not work; if the value is actually nil this will revert to @hash_key_str. In that case it could be fine anyways depending on the resolver - since we return hash_key_str if the client doesn't find hash_key.

[danielvdao]

Curious what're your thoughts @rmosolgo? Personally it's a bit weird to see hash_key not be guarded by is_a?(Hash) -- but maybe it's meant to be used this way as noticed in previous PRs.

[georgexu22]

We're also unsure how hash_key is supposed to be used. Reference in the docs: If the underlying object is a Hash, lookup a key in that hash.. Is this not the case anymore?

[rmosolgo]

I chose fetch because it would return nil if the key is present in inner_object and the value is nil -- I think we should retain that behavior.

How about:

Suggested change

	inner_object[@hash_key].nil? ? inner_object[@hash_key_str] : inner_object[@hash_key]
	inner_object.key?(@hash_key) ? inner_object[@hash_key] : inner_object[@hash_key_str]

(it looks like .key? is what the code used to use: https://github.com/rmosolgo/graphql-ruby/pull/4015/files#diff-53ed7db7a95b84a82815af3a920ccd962fce9469ef21ed25111fb149bc1406eaL658-L661)

[danielvdao]

Sorry if I'm unclear @rmosolgo or maybe we're mis-understanding, but actually I think the problem is that we have this:

if defined?(@hash_key)

check outside of checking whether or not the underlying object is a Hash, example code from today: link. The bug is in our code, but also I think exposes an issue here -- which is that clients can define hash_key even if the underlying object isn't a hash and have it regress.

So even if we use .key? I think it would break... maybe the proper fix is to actually move the @hash_key code block back into here?

[danielvdao]

I tried a bunch of different options in the commit tree. Maybe it's best to defer to you 😂

[danielvdao]

Thinking out loud when visiting this method, it's checking class type and also method resolution. Ideally we should split it into:

case inner_object
  when Hash
  ....
  else
  ....
end

And then do the behavior -- but.... this is risky 😅, as I imagine clients may have field names that are Hash / Object instance methods... Oh the joys of ruby 🤣

[rmosolgo]

Thanks for all your work on this! I've rolled your commits into #4072 and tried to address the original concern from #4059. Please let me know on that new PR (or in a new issue) if the new version doesn't properly fix your case!

[danielvdao]

Thanks Robert! Funnily enough our code was broken as we used hash_key for something that responded to the hash accessor method but wasn't actually a hash 😂

We used method to fix it -- but thought to try and fix it upstream as well. Appreciate the follow-through 💚