Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split QueryType into separate files #1825

Closed
cabello opened this issue Aug 30, 2018 · 27 comments
Closed

Split QueryType into separate files #1825

cabello opened this issue Aug 30, 2018 · 27 comments

Comments

@cabello
Copy link

@cabello cabello commented Aug 30, 2018

Using old style definitions we have been splitting QueryType into multiple files using the following solution:

QueryType = GraphQL::ObjectType.new.tap do |query|
  query.name = 'Query'
  query.description = 'The query root of this schema. See available queries.'
  query.interfaces = []

  query_types = [
    ClientQueryType,
    AccountQueryType,
    InstitutionQueryType,
  ]

  query.fields = FieldCombiner.combine(query_types)
end

Example of *QueryType:

ClientQueryType = GraphQL::ObjectType.define do
  field :clients, types[ClientType] do
    description 'Find clients by ID'
    argument :ids, types[!types.ID]
    resolve ->(_, args, _) {
      Client.where(id: args['ids'])
    }
  end
end

And the field combiner:

class FieldCombiner
  def self.combine(query_types)
    Array(query_types).inject({}) do |acc, query_type|
      acc.merge!(query_type.fields)
    end
  end
end

I am having a hard time using this concept (or coming up with a similar one) for class based types, when I execute an empty query {} I get the following:

NoMethodError: undefined method `edges?' for #<GraphQL::Schema::Field:0x00007fbeb7f51900>

And it happens on the first field that gets added to the schema, I believe is something related to the fact that fields returning old style definition?

I will be investigating more, is there a blessed approach for breaking up a huge QueryType into multiple objects?

@NicholasLYang

This comment has been minimized.

Copy link

@NicholasLYang NicholasLYang commented Aug 31, 2018

Not sure this is a good way of doing it, but I'm attempting to add a DSL that allows for something like

class Article
  queryable_on :id, :title
end

queryable_on will automatically generate the following queries: article(id: ID), articleByTitle(id: ID) (breaking consistency for id cause articleByID seems redundant)

The way I'm doing it is by reopening QueryType using class_eval. Metaprogramming is evil though, so it's probably not the most reusable or readable solution.

This is what I have right now:

  QueryType.class_eval do
    field model, Module.const_get("#{model}Type"), null: true do
      description "Query #{model} by ID"
      argument :id, query_type, required: true
      resolve -> (obj, args, ctx) { PostsResolver.show(id) }
    end
  end
@Amnesthesia

This comment has been minimized.

Copy link

@Amnesthesia Amnesthesia commented Sep 18, 2018

I'm also really interested in this — should be possible to do with concerns, somehow? Perhaps some sort of GraphQL::Concern to extend would be really nice, the query_type file gets very large in big projects

@paweljw

This comment has been minimized.

Copy link

@paweljw paweljw commented Sep 19, 2018

I wouldn't necessarily agree that a concern would be the right approach here, IMHO mixins/concerns that one only mixes in in one place are a bit of an antipattern ;) But some sort of composition helper, so one could say

class QueryType < ...
  exposes ThisLogicalUnitQueries
  exposes ThatLogicalUnitQueries
  # ...
end

Would be very nice indeed. As a matter of fact, this helper could actually run class_eval under the hood as shown in @NicholasLYang's example above :)

Same goes for MutationType too, the one we have isn't very large yet, but it is definitely going to amass quite a bit of stuff soon.

@thefliik

This comment has been minimized.

Copy link
Contributor

@thefliik thefliik commented Sep 19, 2018

My interpretation of this issue is the desire to break up a graphql type across multiple files, rather than across multiple objects (which is the way the issue is phrased). So maybe I'm misunderstanding something, but I think you're overthinking this problem (or maybe I'm underthinking it)?

I've found it very easy to define a graphql type across multiple files (both in the current class-based API, and the old .define API).

Example:

In my project I have a MutationRoot class with the base definition in app/graphql/root/mutation_root.rb

module Types
  class MutationRoot < BaseObject
    graphql_name "Mutation"
  end
end

And I have a folder app/graphql/mutations with individual mutation files such as app/graphql/mutations/person.rb

require_relative '../root/mutation_root'

module Types
  class MutationRoot
    field :personUpdate, PersonType, null: true do
      description "Update a person's properties"
      argument :person_id, ID, required: true
      argument :property, Inputs::Person::PropertyUpdate, required: true
    end
  end
end

This works fine for splitting up files. I just make sure the mutation_root.rb file is loaded before the individual mutation files.

For this matter, in the pre-class version of graphql-ruby, I similarly split up my mutation type by calling .define on the MutationRoot class multiple times, which works fine. I.e.

In app/graphql/mutations/person_mutations.rb

require_relative '../root/mutation_root'

GraphTypes::MutationRoot.define do
  field :personUpdate, !GraphTypes::Person do
    description "Update a person's properties"
    argument :personId, !types.ID, as: :person_id
    argument :property, !GraphInputs::PersonPropertyUpdate
    resolve ->(obj,args,ctx) {
      result = Person::Update.(args.to_h, 'current_user' => ctx[:current_user])

      if result.success?
        ::PersonInstance.new(result[:person], ctx[:current_perspective])
      else
        GraphQL::ExecutionError.new(result[:errors].join(' '))        
      end
    }
  end
end

In app/graphql/mutations/contact_list_mutations.rb

require_relative '../root/mutation_root'

GraphTypes::MutationRoot.define do
  field :contactListCreate, !GraphTypes::ContactList do
    description "Create a contactcontactLlist"
    argument :property, !GraphInputs::ContactListPropertyCreate
    resolve ->(obj,args,ctx) {
      params = args.to_h
      params[:owner_ids] = [ctx[:current_perspective].id]

      result = ContactList::Create.(params, 'current_user' => ctx[:current_user])

      if result.success?
        result[:contact_list]
      else
        GraphQL::ExecutionError.new(result[:errors].join(' '))        
      end
    }
  end
end

This worked because .define mutates the caller.

I don't understand the desire to create additional DSL to accomplish this. Seems unnecessary?

@NicholasLYang

This comment has been minimized.

Copy link

@NicholasLYang NicholasLYang commented Sep 19, 2018

At least for me, I'm attempting to do what Rails did for resource routes: make a concise, declarative DSL for explaining the queries in types. I agree that if you're simply trying to lower the amount of code in query_type.rb, then reopening the class is perfectly fine. But in my case, I'm trying to make touching QueryType a rare occurrence (much like manually defining resources in config/routes.rb versus just using resources :articles)

@thefliik

This comment has been minimized.

Copy link
Contributor

@thefliik thefliik commented Sep 19, 2018

@NicholasLYang gotcha. That makes sense. Personally, I don't think that approach makes sense for my own app, and I don't immediately like the idea of searching through every object type definition file to see how the query object is defined, but maybe I would have designed my graphql API differently had DSL like this been around (and hence it would make more sense).

@NicholasLYang

This comment has been minimized.

Copy link

@NicholasLYang NicholasLYang commented Sep 21, 2018

Yeah, my eventual goal is to make something that's like Rails + GraphQL Ruby, but minus all the stuff I don't use anymore (views, asset pipeline, etc.)

@bryantbj

This comment has been minimized.

Copy link

@bryantbj bryantbj commented Oct 25, 2018

@thefliik

This works fine for splitting up files. I just make sure the mutation_root.rb file is loaded before the individual mutation files.

Please excuse my ignorance. How does one ensure the load order of these files?

@Loschcode

This comment has been minimized.

Copy link

@Loschcode Loschcode commented Nov 22, 2018

I'm testing things around with your gem for a little while now and there's something I really don't get, and it's about what everyone's talking about in this issue, and code scalability.

For mutations you have this sort of things: http://graphql-ruby.org/mutations/mutation_classes.html

This is perfect, it splits up things into different files and you limit mutation_type.rb to the bare minimum. You can add something like /mutations/create_something.rb

module Mutations
  class CreateSomething < Mutations::BaseMutation
    argument :something
    field :result_field, String, null: true

    def resolve(something:)
    end
  end
end

Then you just have to add this into the mutation_type.rb

module Types
  class MutationType < Types::BaseObject
    field :createSomething, mutation: Mutations::CreateSomething
  end
end

Why isn't there any /queries/ folder doing the exact same thing for queries ? I'm new in the GraphQL world so I tried random things like

module Queries
  class GetCurrentUser < GraphQL::Schema::Queries
    # some logic abstracted
  end
end

Then you could write something like

module Types
  class QueryType < Types::BaseObject
    extend ActiveSupport::Concern

    field :currentUser, Types::User, null: true, query: Queries::GetCurrentUser
  end
end

Obviously, it did not work because it's not implemented, or maybe I missed something in the documentation.

Anyway, aren't we suppose to have some kind of cohesion between mutations and queries in the structure ? Why can't i abstract the logic of my queries into multiple files ? Is it under progress ?

I'm trying to integrate your gem to a fairly small project and my query_type.rb file is over 100 lines and start to be unreadable and full of logic, not at all like a route file because it contains several methods. We are talking about 3 models integrated top right now, I can't imagine when it grows.

If I lead myself into the wrong direction, don't hesitate to correct me 😄

@hooopo

This comment has been minimized.

Copy link

@hooopo hooopo commented Nov 23, 2018

It works for me:

module Queries
  module Companies
    extend ActiveSupport::Concern

    included do
      field :companies, Types::CompanyType.connection_type, '工商公司列表', connection: true, null: false do 
        argument :name_contains, String, '注册工商名包含', required: false
      end
    end

    def companies(**args)
    end
  end
end

then

class QueryType < Types::BaseObject
  include Queries::Companies
end
@bryantbj

This comment has been minimized.

Copy link

@bryantbj bryantbj commented Nov 23, 2018

@Loschcode
I was facing the same problem. rmosolgo pointed out the usefulness of resolvers for queries and they feel almost exactly like the Mutation interface, which is what I was looking for. Let me know if you have anymore questions.

Here's an example:

Folder structure:

 app /
 |__ graphql /
      |__ mutations /
      |__ queries /
      |   |- base_query.rb
      |   |- some_resource.rb
      |__ types /
      |   |- query_type.rb
      |   |- some_resource_type.rb      
      |- schema.rb

Resolvers:

module Queries
  class BaseQuery < GraphQL::Schema::Resolver
    # methods that should be inherited can go here.
    # like a `current_tenant` method, or methods related
    # to the `context` object
  end
end
module Queries
  class SomeResource < BaseQuery
    type Types::SomeResourceType, null: <false|true>
    description "Query all SomeResources"

    argument :name, String, required: false, default_value: nil

    def resolve(name:)
      return SomeResource.all if name.nil?
      
      SomeResource.where('name like ?', "%#{name}%")
    end
  end
end

This way, your root QueryType file can still act as the single source of truth for what you can query, but the logic is split up into their respective module/file components.

module Types
  class QueryType < GraphQL::Schema::Object
    field :some_resource, resolver: Queries::SomeResource
  end
end
@Loschcode

This comment has been minimized.

Copy link

@Loschcode Loschcode commented Nov 25, 2018

Thanks for your input @hooopo but that's not a parallel solution, just a workaround and I wish to avoid tricks as much as I can

@bryantbj this actually gets really close from a more solid solution, I made it work within my project pretty easily, it's nice enough for now 👍

Would be really best to have a similar structure to Mutations though, but I guess i'll go for this now 😄

@egonm12

This comment has been minimized.

Copy link

@egonm12 egonm12 commented Nov 26, 2018

@bryantbj when implementing your solution I get the following error:

uninitialized constant Types::QueryType::Queries
/app/graphql/types/query_type.rb:5:in `<class:QueryType>'
/app/graphql/types/query_type.rb:4:in `<module:Types>'
/app/graphql/types/query_type.rb:3:in `<top (required)>'

And then

A copy of Types has been removed from the module tree but is still active!

I am using Rails 5.2.1 with Ruby 2.5.1 and graphql-ruby 1.8.11
Could it be related to this?

@bryantbj

This comment has been minimized.

Copy link

@bryantbj bryantbj commented Nov 26, 2018

@Loschcode Happy to help! After rmosolgo pointed me in that direction, I noticed a reference to that pattern in the docs. I don't know how I missed it!

@egonm12
Regarding the load error:
It seems like Rails is looking for Queries in the local namespace. You could try preceding your reference to Queries with ::, making it ::Queries::YourResolverNameHere to reset the namespace. If that doesn't work, would you mind pasting the block of code when it's convenient for you so we have some context?

Regarding the Types error:

And then
A copy of Types has been removed from the module tree but is still active!

I get this regularly. It's usually a typo somewhere. Most recently, it was me typing resovler: instead of resolver:. It's a frustrating error (like that linked issue describes) because it doesn't really indicate where the problem is.

@SeanRoberts

This comment has been minimized.

Copy link

@SeanRoberts SeanRoberts commented Nov 30, 2018

@bryantbj @rmosolgo This is exactly what I've been looking for! I've been using the mixin approach for a while now but my app is getting complicated enough that I was starting to worry about method name collisions and all the other limitations you run into with module mixins. This resolver approach is perfect, thank you!

@Fleick

This comment has been minimized.

Copy link

@Fleick Fleick commented Dec 3, 2018

When refactoring from the .define to the class variant I also hit this problem. The QueryType becomes too big and unmaintainable. @hooopo's solution is just a workaround - but that was the "old" field-combiner method, too. It would be nice to create a real solution here, which even with the resolver method of @bryantbj the QueryType becomes huge in larger projects - just think on 200-300 queries.
Also the definition of a resolver which only does the following at the end:

::User.find(user_id)

seems to me to generate a lot of code and makes the whole thing rather confusing than writing one query file per module and putting them together at the end.

@rmosolgo is this really the best way to handle the Query and MutationType in larger projects? After all, any lint-test etc. will alarm at these file lengths. If I need the entire QueryType for an overview, I export the schema and use an appropriate viewer program, which is clearer anyway.

@SeanRoberts

This comment has been minimized.

Copy link

@SeanRoberts SeanRoberts commented Dec 3, 2018

Because query ultimately is a type in my GraphQL schema it makes sense to me to have all the field definitions in one place. Maybe add the same exception for file length on your linter that you would for something like schema.rb or routes.rb?

@Fleick

This comment has been minimized.

Copy link

@Fleick Fleick commented Dec 6, 2018

@SeanRoberts ok, if you see it as a comparison with the routes.rb it makes sense. I was just astonished, because you generally try not to let Rails classes get too big.
Thanks for answering

@Amnesthesia

This comment has been minimized.

Copy link

@Amnesthesia Amnesthesia commented Dec 7, 2018

I think one solution to this, would be to allow defining Queries the same way mutations can be defined:

class Types::MutationType < Types::BaseObject
  field :createThing, mutation: Mutations::Thing::CreateThign
end

Similarly, field definitions - that used to use resolve: keyword argument, but now uses methods, could use a method reference, e.g:

module Types::Queries::Things
  def get_things
      # This is the resolver for a list of things
  end
end

class Types::QueryType < Types::BaseObject
  include Types::Queries::Things

  field :things, [Types::Thing], null: false, query: :get_things
end

or even just structure them almost identically:

class Types::Queries::ListThings < Types::BaseQuery
  description "Fetch a list of things"
  argument :arg1, String, required: false

  def resolve(arg1: nil)
      # This is the resolver for a list of things
  end
end

class Types::QueryType < Types::BaseObject
  include Types::Queries::Things

  field :things, [Types::Thing], null: false, query: Types::Queries::ListThings
end

This would allow you to either use a very big QueryType class, with all the fields + resolver methods in the same file (which I think works very well for smaller projects), but also allow you to split up the code base and avoid having a very big bloated QueryType which has often been a problem for me personally

@rmosolgo

This comment has been minimized.

Copy link
Owner

@rmosolgo rmosolgo commented Dec 14, 2018

@Amnesthesia you might like GraphQL::Schema::Resolver! It looks just like what you posted, and it's the base class of mutations: http://graphql-ruby.org/fields/resolvers.html

I think the analogy to routes.rb is quite good. Types::Query isn't a normal Rails model or controller, it's more like a router that takes API requests and passes them to business logic. Hopefully the adapter code between GraphQL API inputs and the corresponding business logic is pretty minimal!

@Amnesthesia

This comment has been minimized.

Copy link

@Amnesthesia Amnesthesia commented Dec 14, 2018

Thank you @rmosolgo! I didn't see @bryantbj had commented about that further up too — I just rewrote everything using resolvers and cleaned up my code a lot

@AndrewRayCode

This comment has been minimized.

Copy link

@AndrewRayCode AndrewRayCode commented Feb 15, 2019

I believe the pattern of breaking up your graphql schema/resolvers into verticals and merging them together in a single place is easier in other graphql implementations. I don't think graphql-ruby should impose restrictions that prevents the same pattern. I think a vertical module should be able to define its entire graphql structure, like a rails engine. we've learned that the rails's horizontal separations leads to difficult to maintain apps. I would love to see first class support for merging of schemas that doesn't require a large root query.rb with all of the definitions in it.

@rmosolgo

This comment has been minimized.

Copy link
Owner

@rmosolgo rmosolgo commented Apr 10, 2019

Yeah, I guess I'm not opposed to something like that in GraphQL-Ruby, but it's not a priority for me.

Maybe there could be an abstraction like a GraphQL Interface, but without a name. (When you implement interfaces, the interface becomes part of your schema, but maybe we could have an even more abstract "container of fields" which doesn't even exist in the GraphQL side of things, only in Ruby!)

@dkniffin

This comment has been minimized.

Copy link

@dkniffin dkniffin commented Jul 17, 2019

@rmosolgo I like this Resolver pattern as a solution to the problem described here, but I also see in the docs that using Resolvers in general is discouraged. Can you give your thoughts on that? Is it simply discouraged because it's easy to end up with fat resolvers? If that's the case, I don't think it's any worse than doing it with methods, right?

Assuming I'm not missing something, I would love to see the docs updated to:

  1. Not discourage resolvers so much
    And 2. Mention this pattern on the Resolvers doc page

I'd even go so far as having this type of set up be the generated example, because it's more consistent with Mutations, and it's cleaner/more maintainable

@hueter

This comment has been minimized.

Copy link
Contributor

@hueter hueter commented Sep 3, 2019

In agreement with @dkniffin, it wasn't clear to me why the resolver pattern is discouraged in the docs, as it would probably lead to a more modular code structure.

@rmosolgo

This comment has been minimized.

Copy link
Owner

@rmosolgo rmosolgo commented Oct 23, 2019

I'd happily accept an improvement to the docs as mentioned above. You'll find an "Edit" button on the bottom of the page which will start a web-based PR workflow. Please give it a try!

Thanks for all the discussion here. I don't have any other work planned for this, so I'm closing it.

@rmosolgo rmosolgo closed this Oct 23, 2019
@acro5piano

This comment has been minimized.

Copy link

@acro5piano acro5piano commented Nov 28, 2019

For those who are looking for how to implement the feature using Resolver, try this:

types/query_type.rb

class Types::QueryType < Types::BaseObject
  field :searchBooks, resolver: Queries::SearchBooks
end

queries/search_books.rb

class Queries::SearchBooks < Queries::BaseQuery
  type [Types::BookType]
  argument :q, String

  def resolve(q:)
    context[:viewer].books.search(q)
  end
end

queries/base_query.rb

class Queries::BaseQuery < GraphQL::Schema::Resolver
  extend GraphQL::Schema::Member::HasFields
  extend GraphQL::Schema::Resolver::HasPayloadType

  class << self
    # Override this method to handle legacy-style usages of `MyMutation.field`
    def field(*args, &block)
      if args.empty?
        raise ArgumentError, "#{name}.field is used for adding fields to this mutation. Use `mutation: #{name}` to attach this mutation instead."
      else
        super
      end
    end

    def visible?(context)
      true
    end
  end
end

keep in mind that the error RuntimeError: Unexpected parent_type: will occur if there are fields with the same model name, like class User < ActiveRecord::Base and field :user, resolver: Queries::User.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.