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

Object-oriented schema definition #820

Closed
rmosolgo opened this issue Jun 29, 2017 · 35 comments
Closed

Object-oriented schema definition #820

rmosolgo opened this issue Jun 29, 2017 · 35 comments

Comments

@rmosolgo
Copy link
Owner

rmosolgo commented Jun 29, 2017

Here's a proposed new API for defining GraphQL behaviors with Ruby. If this appeals to you or doesn't appeal to you, please chime in. Otherwise, I will probably implement it :)

  1. Write one or more .graphql files containing type definitions.

For example:

# app/graphql/types/post.graphql
type Post {
  title: String!
  body(truncateAt: Int): String!
  comments(moderated: Boolean = true): CommentsConnection
 }
  1. Create a matching .rb file containing a Ruby class:
# app/graphql/types/post.rb
class Types::Post < GraphQL::BaseObject
  # This is default, inherited behavior: 
  # def initialize(object, context)
  #   @object = object 
  #   @context = context 
  # end 

  # This is default field resolution behavior 
  # def title 
  #   @object.title 
  # end 
end 

By default, this object is a proxy. If you don't need custom behavior, you can bypass this object altogether.

  1. As needed, add custom behavior to the Ruby class:
# app/graphql/types/post.rb
class Types::Post < GraphQL::BaseObject 
  # - GraphQL argument -> Ruby keyword argument 
  #    - Validate the ruby method ahead of time (kwargs match graphql args)
  # - Automatic Camel -> Underscore
  def body(truncate_at: nil)
    body = @object.body 
    if truncate_at 
      body = body.first(truncate_at)
    end 
    body 
  end 
end 
  1. Put shared logic in an a ApplicationObject:
# app/graphql/types/application_object.rb 
class Types::ApplicationObject < GraphQL::Object 
  # for example, helper method to access current user:
  def current_user 
    @context[:current_user]
  end 
end 
# app/graphql/types/post.rb
class Types::Post < Types::ApplicationObject 
  # Use helper method in field resolution:
  def viewer_is_author?
    viewer == @object.author
  end 
end 
  1. graphql-ruby calls methods on these objects to resolve fields.
  • Pros:
    • No procs
    • Plain-Ruby techniques for reusing code (no procs)
    • Plain-Ruby techniques for debugging (no procs)
    • Predictable, familiar code structure & object lifecycle (no procs)
    • Better Rails autoloading
    • Less wacky DSL (no procs)
  • Cons:
    • Library support will be different; accomplishing this API without a performance loss will be hard
    • Migrating from old-to-new will be hard (but maybe we can automate it)
    • There will be lots of .graphql files?
    • Definition of GraphQL system may be spread between .rb and .graphql files
    • How to handle boilerplate (eg PostConnection, PostEdge, CommentConnection, CommentEdge, same problem as any DSL-based approach)
@timigod
Copy link

timigod commented Jun 29, 2017

If I had to choose between the current API and this proposed one, at the beginning of the very first version of graphql-ruby, I'd definitely choose this one.

I'm just not sure the migration hassle (even if it's automated) is worth it. There's so much to migrate and I haven't really faced any issues that made me wish there wasn't a "wacky" DSL.

@theorygeek
Copy link
Contributor

theorygeek commented Jun 29, 2017

❤️ 🎉 🚀 Love this! I'm curious what kinds of performance hits there would be for this. I'm guessing the biggest one is going to be instantiating those objects for every field.

I'm wondering about the Types namespace. It seems that these are not types, but rather objects. So maybe Objects::Post and put it in app/graphql/objects/post.rb.

Also, IIRC, the gem does not have ActiveSupport as a dependency. So maybe we could allow the user to inject a proc in order to fetch the correct GraphQL::BaseObject for a type that is automatically generated from IDL:

# I scope-creeped a `define` API into my comment here :p
Schema = GraphQL::Schema.define_from_idl do
  source_file 'app/graphql/schema.graphql'
  
  # Plug in your own behavior:
  resolve_object -> (type) { "Objects::#{type.name}".safe_constantize }
end

The challenge with overriding that behavior is that it has to be two-ways, but that's not really a problem; the user would have to override both resolve_object and resolve_type.

@eapache
Copy link
Contributor

eapache commented Jun 29, 2017

One additional con for the list, which is common to most IDL-based approaches: it spreads out the definition of a field across multiple files.

@cjoudrey
Copy link
Contributor

One additional con for the list, which is common to most IDL-based approaches: it spreads out the definition of a field across multiple files.

This might be a terrible idea, but what about defining the IDL within the Ruby file?

Something like:

MySchema.define_type_from_idl(<<-'GRAPHQL'
  type Post {
    title: String!
    body(truncateAt: Int): String!
    comments(moderated: Boolean = true): CommentsConnection
   }
GRAPHQL)

class Types::Post < GraphQL::BaseObject
  # This is default, inherited behavior: 
  # def initialize(object, context)
  #   @object = object 
  #   @context = context 
  # end 

  # This is default field resolution behavior 
  # def title 
  #   @object.title 
  # end 
end

@arthurschreiber
Copy link
Contributor

or even something like:

class Types::Post < GraphQL::BaseObject
  definition <<~GRAPHQL
    type Post {
      title: String!
      body(truncateAt: Int): String!
      comments(moderated: Boolean = true): CommentsConnection
     }
  GRAPHQL
end

@bswinnerton
Copy link
Contributor

This might be a terrible idea, but what about defining the IDL within the Ruby file?

I ❤️ @arthurschreiber's DSL above, but I'd also love the flexibility to do either. I appreciate being able to look at a .graphql file and know everything that I need to know about the structure of my schema.

One my favorite aspects of this new DSL is how easily it will be to mix in modules to add in custom helpers, fields, interfaces, etc.

@rmosolgo
Copy link
Owner Author

@cjoudrey , @arthurschreiber & @bswinnerton I think there must really be something to the GraphQL-in-Ruby approach, we also talked about it a bit here.

Here's the bridge that I couldn't cross: how do you determine the whole schema in that scenario? For example, what (pseudo-)code goes here:

# app/graphql/my_schema.rb
# From .graphql files:
# MySchema = Schema.from_definition(glob: "./**/*.graphql")
# 
# OR:
# From .rb files:
MySchema = # ??? 

(A bonus is something that plays well with Rails autoloading.)

@rmosolgo
Copy link
Owner Author

@eapache thanks for pointing out that extra consideration about splitting the source of truth for field definitions, I've updated the issue description so new readers will find it quickly.

(Maybe we'll find a GraphQL-in-Ruby approach that mitigates that downside.)

@rmosolgo
Copy link
Owner Author

@theorygeek regarding implementation, I had something very similar in mind!

Perhaps an API like:

MySchema = GraphQL::Schema.from_definition(
  glob: "app/graphql/**/*.graphql",
  # Specify a Ruby namespace where objects can be found:
  default_resolve: Objects,
)

The default_resolve: argument already provides special behavior for hashes; I think we could also provide special behavior to support the API described here.

Performance-wise, the issue is making one of these proxies for each object in the response. We could make a corresponding win however. Since this API doesn't support field-level context, we might be able skip the GraphQL::Query::Context::FieldResolutionContext objects which are allocated a lot in the current implementation.

@rmosolgo
Copy link
Owner Author

@timigod I'm glad to hear you don't mind the current API too much 😊

There are a few ways that it makes us less productive at GitHub:

  • Object lifecycle is not clear. For example, GraphQL::Functions are instantiated once, at schema build, then reused. Sometimes, people think that functions are instantiated "fresh" for each call, and they use instance variables for implementing behavior. In fact, this use of instance variables is not safe because the same Function object is reused for the whole time that the application runs. (People are used to ActiveRecord::Base or ActionController::Base, which are instantiated fresh over and over.)
  • Stack traces are not clear. The Proc literals don't get named entries in error traces.
  • Lexical scope is not clear. When you make a typo in a proc literal, the error message is can't define your_typo for GraphQL::ObjectType, when the normal message would be undefined method or local variable your_typo
  • Code reuse & sharing are not clear. Ruby developers are used to sharing behavior via inheritance, and inheritance doesn't work in any way with the DSL.

There are ways to mitigate these issues (for example, implementing a layer on top of graphql-ruby's DSL), but graphql-ruby doesn't guide people toward the "pit of success"!

@eapache
Copy link
Contributor

eapache commented Jun 29, 2017

There are ways to mitigate these issues (for example, implementing a layer on top of graphql-ruby's DSL)

At Shopify we have a layer on top with an API that looks very-very similar to the DSL, except implemented in a class-based way instead of using blocks. It solves a lot of the pain points without changing the usage too much.

@rmosolgo
Copy link
Owner Author

A question from @josh was: "how to add field-specific data in this API?"

  1. Add metadata to class
class Types::Post
  # field-level flag
  visibility :internal, fields: [:moderation_reason] 
end 
  1. Write an an instrumenter which uses that flag:
def instrument(type, field)
  if type.implementation_class.visibility[:internal].include?(field)
    field.redefine(...)
  else 
    field 
  end 
end 

(Maybe we could write some sugar to make this easy)

  1. OR Use your flags to meta-program your implementation classes.

@cjoudrey
Copy link
Contributor

cjoudrey commented Jun 29, 2017

how do you determine the whole schema in that scenario?

I could be missing something super obvious here, but couldn't we:

1- Start from the schema roots
2- For each field, get the type and get the definition for that type
3- Repeat 2 for each field of the newly defined type
4- Repeat 2 for orphan types

A question from @josh was: "how to add field-specific data in this API?"

Another option could be custom directives, i.e.

class Types::Post < GraphQL::BaseObject
  definition <<~GRAPHQL
    type Post {
      moderationReason: String @visibility(type: "internal")
    }
  GRAPHQL
end

You'd want a way to mask these directives from the outside world though, i.e. people would see:

type Post {
  moderationReason: String
}

This probably isn't a one-size fits all solution though.

@rmosolgo
Copy link
Owner Author

@cjoudrey traversing the schema from entry points could totally work! Personally, I was hoping to leave orphan_types behind though 😬 But you're right, that would work just as well as the current arrangement.

custom directives

I like customizing via .graphql seems nice. I took a slightly different tack at #789, using comments instead of directives. I chose comments to avoid two issues:

  • Implementing custom directives seemed hard
  • Custom directives may not be supported by other GraphQL implementations

However, I didn't recommend .graphql customizations here because, in our conversations, a different view won out:

  • .graphql files should only include data which will be visible to GraphQL clients (structure, description, deprecation)
  • Everything else goes in .rb files (including customization about how data is fetched)

One advantage is that .graphql files match what you will find in online documentation.

But personally, I'm still on the fence!

@timigod
Copy link

timigod commented Jun 29, 2017

@eapache that sounds awesome

@cjoudrey
Copy link
Contributor

.graphql files should only include data which will be visible to GraphQL clients (structure, description, deprecation)

Yeah, to be honest I also prefer leaving the rest out of the IDL.


re: #820 (comment) -- I can't put my finger on it, but there's something I dislike about having to use instrumentation.

Could an API similar to Rails before_action work?

class Types::Post < GraphQL::BaseObject
  definition <<~GRAPHQL
    type Post {
      moderationReason: String
    }
  GRAPHQL

  before_resolve :user_is_internal, fields: [:moderation_reason]
end

You'd need a way to let check_access break the chain.

@rmosolgo
Copy link
Owner Author

before_resolve :user_is_internal

😯 whoa .... 🤔

@eapache
Copy link
Contributor

eapache commented Jun 29, 2017

.graphql files should only include data which will be visible to GraphQL clients (structure, description, deprecation)

💯

@eapache that sounds awesome

We're thinking about ditching a bunch of it because it's a lot to maintain, but tbh we still haven't come up with anything better. Just writing class Foo < GraphQL::Object instead of Foo = GraphQL::ObjectType.define do is so much more natural and simplifying (especially when it comes to Rails autoloading garbage).

@malandrina
Copy link

malandrina commented Jun 29, 2017

Looking good!

With respect to step 4, "put shared logic in an ApplicationObject, I'm wondering if we could approach adding shared behavior with a preference for composition over inheritance. I can see such an ApplicationObject becoming a grab-bag of behaviors (like we sometimes see with Rails helper modules) that aren't needed by every object that inherits from it.

By way of an example, take this alternative to having Types::Post inherit from Types::ApplicationObject:

# app/graphql/types/post.rb
class Types::Post < GraphQL::Object
  def viewer_is_author?
    viewer == @object.author
  end

  private

  def current_user
    # something like
    SessionObject.new(context).current_user
  end
end

@rmosolgo
Copy link
Owner Author

@malandrina I totally share your concern that a root object will become bloated! Here's the part I can't figure out how to do:

a preference for composition over inheritance

What kind of Ruby API can express that preference? As soon as you start using class, you invite people to extend and include as means of sharing code.

(I tried to express that preference by using a function-based API, but ... here we are two years later 😆)


Unrelated note, but here are some references to previous requests for class-based API:

#91 (comment)
#122

@malandrina
Copy link

@rmosolgo thank you for linking to those older conversations! You have clearly put a lot of thought into how a preference for composition over inheritance might be expressed in graphql-ruby's API. Your comment #122 (comment) is edifying. 👍

What kind of Ruby API can express that preference? As soon as you start using class, you invite people to extend and include as means of sharing code.

This is true. I think the best one can do is expose an "opinionated" public API that serves as an example/expresses a preference for how to share behavior and enforce separation of concerns. But the value of such an approach is questionable because, Ruby being Ruby, users are going to go ahead and extend and monkeypatch when they want to, regardless of whatever perfect architecture you've established. 🤷‍♀️

tl;dr sounds like you've given a lot of thought to this and previous experiments with a "class-driven" approach were unsuccessful, so I trust your judgment that sharing behavior via ApplicationObject is the best option.

@rmosolgo
Copy link
Owner Author

the best option

I wouldn't quite go that far, or at least ... I would say, it's a familiar option. For people who want to get it done quickly, they can put all shared logic in the god class at the top of the hierarchy. It's not the most maintainable solution, but it's easy 😬

Then, if/when people want a different arrangement, they can extract and reshuffle the same way we do for ApplicationController and so on.

So in that way, I wouldn't say it's really the best, but I'm hoping to make a different tradeoff so that folks can get started and be productive with graphql-ruby more quickly.

@theorygeek
Copy link
Contributor

theorygeek commented Jul 2, 2017

On the GraphQL-in-Ruby bit, it seems like:

  • There is something really nice about the IDL and the implementation being next to each other
  • It's also nice to write IDL instead of using our field DSL (although I happen to like both of them)

But the biggest advantage I see to the IDL is that it can be shared between projects, and it is the true source of truth. The GraphQL-in-Ruby paradigm breaks that, because now you really do need to go back to doing rake graphql:schema:dump if you want to get the real schema.

Is there a real use case for sharing the IDL directly? Meaning, does anyone actually use it like it's a negotiation between client and server, rather than like it's a proclamation from the server to the client? (Sorry for the weird use of legal terms here, I couldn't think of better wording.)

@theorygeek
Copy link
Contributor

theorygeek commented Jul 2, 2017

If what we really want with the GraphQL-in-Ruby is to reduce the amount of switching between files when you're trying to implement the schema, we could accomplish that with some tooling similar to the annotate gem, that copies the IDL into your implementation class as a comment. But maybe it's more about being able to manipulate the schema as you're implementing it.

I'm starting to question whether my original "shared IDL" concept will actually be useful in the real world. It seems too waterfall-ish, and maybe fanciful to think that you really are gonna go hammer out the IDL and then it's not gonna change (much) during implementation. Nevermind, I think I was just... having a moment.

@xuorig
Copy link
Contributor

xuorig commented Jul 3, 2017

In a previous issue I gave the example of extending fields with custom arguments such as visible_if: proc or authorization: :admin. Where does this sit with the current proposal?

They won't go in the IDL, since the user should not be aware of those "extensions":

type Post {
  title: String!
  body(truncateAt: Int): String!
  comments(moderated: Boolean = true): CommentsConnection
 }

But if they have to be somewhere in the "resolver" class, where should they go? on the class? Magic decorators?

class Types::Post < GraphQL::BaseObject 
  preload :an_association, on: :body

  def body(truncate_at: nil)
    ...
  end
end 

I'm afraid if these extensions are on the class, we're now adding a third place to look at when defining fields.

EDIT: i now realize i missed this comment: #820 (comment)

@riffraff
Copy link
Contributor

riffraff commented Jul 5, 2017

My 2c: I'd be happy to move to a class-based definition, but why not keep the field definition? i.e.

class Types::Post < GraphQL::BaseObject 
   field :author, AuthorType # resolves object.author
   field :titleSlug, StringType # resolves object.title_slug
   field :body, StringType, meta: :data do |argument:, **keywords:|
     decorator.formatted_body(keywords) 
   end
   field :isRead, BooleanType do 
      current_user.read_posts.include?(object)
   end

   private

   def current_user
      # @context is set in #initialize
      context[:current_user]
   end

   def decorator
      # @object is set in #initialize. One might also trivially do
      # @object = object.decorate in a custom initializer, and always refer to this
      object.decorate
   end
end

This would be similar to how ActiveModel::Serializer works, which I've been generally happy with.

You get some of the advantages of the original proposal

  • reuse via inheritance and mixins
  • trivial definition of helpers
  • a lot less boilerplate
  • clearer lifecycle

Plus you keep the ease of use for custom field metadata, and have a trivial code update path.

The problem of stacktrace and debuggability is also still reduced, since there is no complex magic DSL (we only need field to define custom methods).

Of course, this is worse for people who like their definition in IDL, which I am not a big fan of :)

@rmosolgo
Copy link
Owner Author

rmosolgo commented Jul 5, 2017

@riffraff thanks for bringing that up! We had discussed an API like that as well, because many Ruby libraries have one (for example, MongoMapper or Sequel).

Here are some pros and cons that I saw:

  • Pro:
    • All definition is Ruby; no need to check two files for type definition
    • Similarity to current DSL (is that pro 😆 ?)
  • Con:
    • There's already a GraphQL type language; we should take advantage of it (and its documentation 😬 ) instead of writing our own
    • How will it handle circular dependencies, since it uses Ruby constants like the current definition?
    • How to define field arguments in the DSL above? (I see that do ... end is used for the resolve body, where are arguments defined? I'm worried it will become very wordy!)

I think I understand this point:

  • reuse via inheritance and mixins

    Since the object definition is in Ruby, you can share structure between graphql types by using Ruby inheritance. In an IDL-based approach, the only way to share structure is to copy-and-paste!

But, can you help me understand some of these points? I want to keep these concerns in mind but I don't quite understand it yet:

  • trivial definition of helpers

    Can you help me see the difference between the two approaches in terms of helpers?

  • a lot less boilerplate

    Can you help me see what boilerplate was initially present, but removed in an all-Ruby API?

  • clearer lifecycle

    Can you help me understand how the lifecycle in the all-Ruby API is different from the lifecycle of the initial proposal?

I want to "cover all the bases", so I don't want to miss out on these considerations!

@riffraff
Copy link
Contributor

riffraff commented Jul 5, 2017

forgive me if I wasn't clear, I meant that this points are shared advantages in both object-based approaches (with and without IDL) compared to the current pure-DSL-with-types-but-not-classes definition.

You have good points about mutually referential types and arguments, I didn't put any thought into this and mostly made a strawman proposal.

Anyway, possibly, we could just refer all types as types.Whatever, which would basically resolve by name later, and also give us consistence between builtin and user-defined types.
Obviously, it would mean type-typos would be caught at field resolution time rather than definition.

As for arguments, I guess you could abuse default values to specify types, which seems nasty, or define them as metadata

   field :body, ..., arguments: {limit: types.Int, filter: types.String} do |limit:, filter:|
     decorator.formatted_body(limit, filter) 
   end

FWIW, we could keep 100% of the current field DSL, just with classes around them instead of .define, and I'd still like it.

@eapache
Copy link
Contributor

eapache commented Jul 5, 2017

@riffraff's proposal looks a lot like what we have at Shopify right now.

How will it handle circular dependencies, since it uses Ruby constants like the current definition?

Rails autoloading has worked well for us on this point with only the occasional need for a manual require call.

How to define field arguments in the DSL above? (I see that do ... end is used for the resolve body, where are arguments defined? I'm worried it will become very wordy!)

We do approximately the following for complex fields:

field :foo, Type do |field|
  field.argument :name, Type
  field.resolve = lambda do |obj, args, ctx|
  end
end

@hooptie45
Copy link

I haven't read all the comments yet; but I think I like where this is going. Another really interesting direction this could go is something similar to the GraphQL-compose library in ruby. https://github.com/nodkz/graphql-compose

What makes that approach interesting is that it allows you abstract the shared logic as small building blocks; then use them to compose the schema. Examples of these might be a set of 'finder' modules(findOneById, findWhere, etc) which know how to do advanced filtering for which ever datastore you're using. Refer to the mongo and elasticsearch libraries built using this pattern; it's really impressive how much he was able to do with so little code.

The other really neat thing it does is compose types via the GraphQL syntax, inline with the rest of the implementation. That syntax makes creating things like enums very straightforward. I'm typing this on my mobile from the subway, so I can't give samples unfortunately. I'll follow up if anyone else is intrigued by this.

I'm all for nice a nice DSL, but it has to be built on a solid foundation. I see huge opportunity here to leverage the patterns outlined above; then slap a nice DSL on top of it.

@thebadmonkeydev
Copy link
Contributor

Something I'd like to mention that I haven't seen in here explicitly is how perspective approaches will handle (or at least behave) in terms of development and debugging. Recently I got treated with an autoload error about my root QueryType because I misspelled the name of a mutation field in MutationType 😱 This is one of the reasons why I have 👍 s all over @riffraff 's suggestion and the ruby/inheritance approaches in this thread. I feel like being structured in that way would allow for more useful information in errors and stack traces.

/my 2 cents

@pabloh
Copy link
Contributor

pabloh commented Aug 8, 2017

Will graphql-ruby be moving on this direction after all?

@rmosolgo
Copy link
Owner Author

rmosolgo commented Aug 8, 2017

Slowly but surely, I'm getting started at #871 :)

@rmosolgo
Copy link
Owner Author

Closed #871 in favor of tinkering a bit with GitHub's schema, I'll be back with another when we find something promising :)

@rmosolgo
Copy link
Owner Author

"Closed" (ok, just barely started) by #1037

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

No branches or pull requests