Graphql-ruby: Field named `context` is a bit weird in the class-based API

[eapache]

Honestly not 100% sure if this counts as a bug, but if you have a field named context in the new class-based API it resolves by default to the graphql context because of the attr_reader on GraphQL::Schema::Object.

Not too terrible to work around with field :context, String, null: true, resolve: ->(obj, _args, _ctx) { obj.context }.

Also I suppose this would apply to fields named object, fields, etc. Because the resolver prioritizes methods on the definition over methods on the underlying object. I wonder if that priority should be reversed for reserved words? Or it should look for def __context or something?

fyi @rmosolgo @swalkinshaw

[rmosolgo]

Thanks for opening this issue, we found the same thing at GitHub a few places (object, to_param as well). Here are some things we've talked about:

• __Workaround:__ method:, for example:

  # Use `method:` to avoid conflict with built-in methods
  field :context, String, method: :status_context, null: false 
  def status_context 
  # ... 
  end 
  

  This is how we've handled issues so far, but we've deployed a few bugs because it's hard to know exactly which fields will have conflicts!

• __Get smart, somehow?__ We could do something tricky like:

  • Keep track of the methods GraphQL::Schema::Object responds to; these are "iffy" methods
  • If a field is defined that conflicts with an "iffy" method, take note, and don't call the "iffy" method. Instead, call a method on the underlying object
  • If an "iffy" method is overridden, remove it from the "iffy" list, since we can assume that the application overrode it on purpose

  That's similar to "reversing priority".

A convention like __ could work, but I think we'd need some prompting to the developer, because they might forget it's required, and have an unpleasant surprise!

What do you think, are there other possible approaches here?

[xuorig]

Just throwing an other approach, although I don't think its the best: being super defensive and guarding against "reserved keywords"

So you wouldn't be able to use a field that calls a method with a reserved keyword 🤷‍♂️

[eapache]

if an "iffy" method is overridden

This is also kinda scary to me, because if I override context that may screw up all the other resolvers I have defined that were using the attr_reader.

I guess executing the resolve methods in their own magical unspoiled ruby context is what we're trying to get away from in this class-based world though...

I would suggest simply: don't allow defining a field on a reserved word without an explicit resolve: or field: or function:?

[eapache]

For context specifically we could drop the attr_reader and force people to use @context, but that doesn't work for all the other pre-defined methods.

[rmosolgo]

defining a field on a reserved word without an explicit ...

👏 That seems like a great solution. Less magic, and helps the dev go to the right direction. I'd also add method: to the list of allowed options there.