[1.8] Objects inheriting Interface definition methods

[swalkinshaw]

This is in reference to the feature added here: https://github.com/rmosolgo/graphql-ruby/pull/1635. Objects that implement an interface also get their definition methods now.

I'm curious about the reasoning behind this feature. I don't really agree with the idea that Interfaces definition methods should be included in objects that implement them. Although Interfaces are Ruby Modules, it seems to be conflating them. If you want shared behaviour between an Interface and an Object, why not use a plain Module and include them in both places?

Background: I'm migrating our codebase to use the new modules and this feature has caused large issues. We have common methods in both interfaces and objects and now they are getting overridden by the Interface's version. I'm running a fork right now where I've removed this, and it fixes (most) of our issues.

cc @joostverdoorn as well. Maybe you have some valid uses cases you can share.

[joostverdoorn]

Hi there,

Here's a simplified use case similar to what we use on our systems:

module ScopedInterface
  include GraphQL::Schema::Interface

  definition_methods do
    def base_scope(&resolve_block)
      ... 
    end

    def scope(name, type, &resolve_block)
      ...
    end

    def scoped_list_field(name, type)
      field(name, field: GraphQL::Schema::Field.new(...))
    end
  end

end

module MessageInterface
  include ScopedInterface
  scope(:read, Boolean) { |base_scope, arg| base_scope.where(read: arg) }

  field :body, String
end

class CommentType < GraphQL::Schema::Object
  implements MessageInterface
  base_scope { Comment.all }
end

class PostType < GraphQL::Schema::Object
  implements MessageInterface
  base_scope { Post.all }

  scoped_list_field :comments, CommentType
end

class UserType < GraphQL::Schema::Object
  implements ScopedInterface

  scoped_list_field :posts, PostType
  scoped_list_field :comments, CommentType
end

query($userId: ID!) {
  user(id: $userId) {
    posts(read: true) {
      comments(read: true) {
        body
      }
    }
  }
}

IMO this is a valid use case where definition methods carrying over to implementing classes is beneficial. It may not be the only way of doing things, but it makes sense when definition_methods is already offered for interfaces.

[swalkinshaw]

🤔 do ScopedInterface or MessageInterface actually have fields on them? Or just definition_methods?

[joostverdoorn]

MessageInterface would have actual fields on them, such as the body of text associated with the message. Updated the example to reflect this.

[rmosolgo]

Happy to reconsider this in some way, maybe implements(...) should accept definition_methods: true|false ? Then a default value could be specified in the base object class. What do you think?

[swalkinshaw]

@joostverdoorn thanks for the example! But ScopedInterface does not have fields? If so, I think it should just be a plain module.

@rmosolgo that would solve our particular issue, but I still feel like this is conflating concepts and might encourage some not ideal patterns. This is still pretty new though, so I haven't thought about it in depth.

[joostverdoorn]

@swalkinshaw you may be right, but the way I see it is that it's kind of the point of definition_methods to add definition methods, i.e. methods that define something on an existing type. scoped_list_field in my example above seems to be a proper example hereof :).

[swalkinshaw]

Oh interesting. That's taking the name very literally 😄

I think the intention was more generic. It's the equivalent of class_methods from AS::Concern but @rmosolgo can clarify that.

[eapache]

🤔

We have common methods in both interfaces and objects and now they are getting overridden by the Interface's version.

This is the core issue isn't it? A method definition on an object should take precedence over any method definitions provided by any interfaces? But I'm confused why this is an issue, because Ruby Modules already work this way (a local method takes precedence over one in a module).

[swalkinshaw]

A method definition on an object should take precedence over any method definitions provided by any interfaces? But I'm confused why this is an issue, because Ruby Modules already work this way (a local method takes precedence over one in a module).

It's not local methods to the object classes which are the problem. It's methods from the parent/inherited class.

class BaseObject < GraphQL::Schema::Object
  class << self
    def to_graphql
      # something custom
    end
  end
end

module MyInterface
  include GraphQL::Schema::Interface

  definition_methods do
    def to_graphql
      # something else custom
    end
  end
end

class MyObjectType < BaseObject
  implements MyInterface # this triggers `self.extend(MyInterface::DefinitionMethods)`
end

MyObjectType will now have to_graphl from the interface. This is pretty fundamentally broken 😞

[swalkinshaw]

Going back to @joostverdoorn's example, I still think that behaviour should be done via plain modules:

module ScopedField
  def base_scope(&resolve_block)
    ... 
  end

  def scope(name, type, &resolve_block)
    ...
  end

  def scoped_list_field(name, type)
    field(name, field: GraphQL::Schema::Field.new(...))
  end
end

module MessageInterface
  definition_methods do
    include ScopedField
  end

  scope(:read, Boolean) { |base_scope, arg| base_scope.where(read: arg) }

  field :body, String
end

class UserType < GraphQL::Schema::Object
  include ScopedField

  scoped_list_field :posts, PostType
  scoped_list_field :comments, CommentType
end

In the previous example, implements is being used to solely include behaviour and not actually implement a GraphQL interface I think?

🤔 actually thinking that implements shouldn't be only allowed for GraphQL interface modules.

[swalkinshaw]

I opened a PR to revert this behaviour: https://github.com/rmosolgo/graphql-ruby/pull/1716

I'm open to alternatives or a non-breaking solution as well.

[rmosolgo]

Yes, I think I underestimated the consequences and didn't think enough about the use case when I initially supported this feature. @joostverdoorn sorry for the back-and-forth, but maybe what you're looking for could be accomplished with plain ruby, as @swalkinshaw pointed out!

[joostverdoorn]

@rmosolgo No worries, I had it implemented a different way and can just revert to that. Just implemented it like this after there seemed to be some interest :).