Support attr readers for fields with names conflicting with Object and Kernel methods

[ElvinEfendi]

We currently rely on method_missing to provide attr readers for the fields. However, when a field has the same name as one of base Ruby methods such as method, test, method_missing method isn't triggered and instead Ruby invokes the corresponding base method. One can avoid this by to_hing first and then accessing the data like .to_h["method"].

This PR tries to implement the same access pattern for such conflicting keywords.

I am not fully convinced this is the right approach (overriding base methods and turning them into field accessor feels wrong), what if we explicitly define methods for all fields instead of relying on method_missing? Thinking out loud, while implementation-wise it would be improvement, we would still be overriding base methods. Alternatively, we can introduce a convention where we append _value suffix when there's a conflict, what do you think?

EDIT: on a second thought, seems like it's common in Ruby to override pre-existing methods like this?

[ElvinEfendi]

this feels wrong, I'll try to understand why this behaved differently when method is explicitly defined

[ElvinEfendi]

@rmosolgo what do you think?

[ElvinEfendi]

Maybe it'd be even simpler if we just deleted the methods with self.singleton_class.undef_method(:name) whenever :name conflicts with an existing method? That way everything can go through the method_missing path.

[swalkinshaw]

Considering these classes are specifically created for GraphQL responses, I think it's fine to override the built-in methods. If a consumer really wanted to use these classes for other purposes (which I'm not sure is "correct") then they could build another representation from the hash.

I do think it's weird to use undef + method_missing when we could explicitly define each method which would just override the built-in methods as you said. But considering that's a bigger refactor, I'd also be okay with this implementation since it's the same end result (I think).

[rmosolgo]

For my part, I agree the implementation is unusual Ruby, but then again, the existing implementation is pretty weird too. There's like a re-implementation of methods and inheritance here ... for my part, I'm included to include this with the expectation that that the interface (method calls retrieve fetched graphql values) will stay intact. But I'm hoping to refactor the implementation to be more "plain Ruby" soon, as part of a performance audit.

Any objections or other considerations before continuing with that approach?

[ElvinEfendi]

I'm included to include this with the expectation that that the interface (method calls retrieve fetched graphql values) will stay intact

Unless there's a strong performance or maintainability benefits of changing that interface (I can not think of one), I'd keep the interface of retrieving fetched graphql values via method calls.

[ElvinEfendi]

I tried checking with single_class.method_defined?(method_name) but while it was returning true for i.e :method, it was returning false for :test. I'm not sure what else to try here to avoid the exception handling.

[ElvinEfendi]

Closing this as I think this approach creates more headache than it solves and also introduces yet another magic. At our company, we instead updated the clients to use to_h interface to retrieve fields that have conflicting names with Ruby base methods.