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

[Discussion] Schema Definition using the IDL #727

Closed
xuorig opened this issue May 17, 2017 · 18 comments
Closed

[Discussion] Schema Definition using the IDL #727

xuorig opened this issue May 17, 2017 · 18 comments

Comments

@xuorig
Copy link
Contributor

xuorig commented May 17, 2017

Discussion on a PR to auto-camelize fields recently has brought up some valid concerns:
#555 (comment)
#555 (comment)

This seems to be part of a bigger discussion on wether or not we want users write actual graphql, or ruby when defining a GraphQL Schema.

Currently, defining a schema usually looks like this:

QueryType = GraphQL::ObjectType.define do
  name "Query"
  description "The query root of this schema"

  field :posts do
    type types.[PostType]
    description "Get all Posts"
    resolve ->(obj, args, ctx) { Post.all }
  end
end

The resulting IDL for this, the source of truth, looks like this:

# The query root of this schema
type Query {
  # Get all Posts
  posts: [Post!]!
}

With this approach, the ruby defined schema != resulting IDL. A good thing about it is that users can write a schema without knowing the GraphQL Langage. At the same time, like discussed in the issue linked above, we don't help users learning GraphQL.

Another argument against this approach is that we're mixing the Schema / Interface with the business logic. Maybe what we want is using the IDL (the best tool we got to define a Schema) and then using plain Ruby to define the resolvers / business logic. With a proper set of conventions, this could be quite powerful. A mix of plain .graphql files and their ruby object counter parts.

Defining a Schema using the IDL

A simple approach to this could be as simple as having something like this:

# post.graphql

type Post {
  title: String!
  description: String
}
# post.rb

class Post < GraphQL::Resolver
  def initialize(post, args, ctx)
    @post = post
    @args = args
    @ctx = ctx
  end

  # These can even be delegated instead by default
  def title
    post.title
  end

  def description
    post.description
  end
end

But this approach limits us in certain ways. @ Shopify, we extracted a lot of common logic into field definition arguments. For example, if a field needs to be batched or preloaded, it may look like something like this:

field :posts, [Post], preload: :posts

It's also possible to extend graphql-ruby's DSL using accepts_definition and Instrumenters to come up with something like this:

field :posts do
  type types.[Post]
  preloads :posts
end

However, with an IDL approach, we lose that DSL and we need a way to extend resolvers. What if I wanted to preload posts in this example?

class Author < GraphQL::Resolver
  def posts
  end
end

we can somehow have config at the class level to define behaviours or have some kind of before and after filters?

class Author < GraphQL::Resolver
  preload :posts, on_fields: [:posts]

  def posts
  end
end

Doesn't seem very elegant to me ^

Just throwing ideas here, but another possible way would be to have something more like this?

module Author
  class Posts < GraphQL::FieldResolver
    preloads :posts

    def resolve
      # ....
    end
  end
end

Or hacky decorators?

class Author < GraphQL::Resolver
  preloads(:posts)
  def posts
    # ...
  end
end

Another approach is to instead use the IDL to define these behaviours! As discussed in graphql/graphql-spec#300. (This is not in the Spec currently, and would be a custom graphql-ruby behaviour)

type Author {
  posts: [Post] @preloads("posts")
}

There's many approaches we can take, but I just wanted to kickstart a discussion. I'm pretty excited about having the schema defined using the IDL personally, but can't seem to find the best solution for custom business logic 🤔

What do you all think❓

@rmosolgo
Copy link
Owner

👍 I really want to make this a thing!

Thanks especially to @f and @emilebosch for their work on the IDL-based schemas so far.

Personally, I like the IDL-based approach to modifying field behavior, like

# :+1: 
type Author {
  posts: [Post] @preloads(hasMany: "posts")
}

I think making schema.graphql the "source of truth" for the Schema would be an improvement.

However, since the schema would "magically" map to objects in Ruby, it will be very important to expose clearly how they map together.

For example, if you add something to schema.graphql but something is missing from Ruby, we should provide very good messaging about what's missing, why, and how to add it.

Additionally, I think the various behavior modifications (such as authorization or preloads above) should somehow be available to a user, so that someone reading the DSL can understand how they modify the system. Imagine a command line interface to that data:

$ rake graphql:describe[Author.posts]
# @return [Post]
# @description All the posts that this person has written 
# @preloads `Author.has_many(:posts)` is preloaded for this field 
# @authorization only `:publisher?` users can access these values. 

@rmosolgo rmosolgo added this to the 1.7.0 milestone May 17, 2017
@theorygeek
Copy link
Contributor

theorygeek commented May 17, 2017

I'm definitely a big fan also of writing your schema in IDL, and then having a convention map it to the underlying code.

Perhaps we should start by thinking about the use-case here. How is the IDL going to be written, and how will it be consumed? I think it's quite likely that these IDL's could be written (in whole or in part) by people that are not going to write the implementation.

For example, it seems like for many teams (mine included), developing the schema is a collaborative process, and much of the work can happen in other environments. I think that the Apollo team said that when they built Optics, they had both front and back end engineers contributing to the schema, and they first ran it through some tool that built a GraphQL server returning mock data while the back-end team built the real implementation.

That leads me to three things that I think counsel against the @preloads pattern:

  • I think it's important that the IDL can be understood by outside tools (eg, Relay Modern). So we should avoid introducing extra syntax, unless we can get it PR'd into the GraphQL Spec.
  • Also, the IDL might be sourced from outside tools. So there should be, at the very least, an alternative way to get these same behaviors.
  • The person writing the IDL might not understand when, why, or how they should use @preloads, particularly if it's a front-end engineer writing it without much knowledge about the internal implementation.

The last point is really a different way of saying that it's a mixing of responsibilities - namely, it brings implementation details into the IDL. ☹️ Sometimes that's probably OK (eg, @authorization) but sometimes it's really hard to see how it adds value to the IDL itself (eg, @preloads).

So I think I would lean more toward the decorators. Even though they're kinda gross. It makes me wish that Ruby had something like C#'s attributes:

[Preloads("Author")]
public Author Author() {
  return Post.Author();
}

@emilebosch
Copy link
Contributor

That leads me to three things that I think counsel against the @preloads pattern:

I think whatever loaders we have, or things that are tied to implementation should be split in a separate gem. I don't by default think its bad to annotate your schema with implementation specifics. Schemas are also used for code generation or hinting.

In my PR i’v solved partially of it in a big hash map like JS graphl-tools does. Im still in doubt often between DSL and IDL. I do like writing IDL and it allows me to iterate fast on the graphql schema, and just hooking up some lambdas with a big resolver hash allows for fast prototyping of APIs.

Things I like about the DSL:

  • Like that I have a quick overview of my types/mutations in the file explorer.
  • In the current situation its only option to camelize the input vs the output params via as:
  • Mutations/types etc have their own files, good for quick glance over things.

Things I don’t like about the DSL:

  • Writing documentation in the DSL
  • Bit of a hassle /w types enums and lot of files
  • Not easy to see in one go what the IDL is.

Things I like about IDL:

  • Fast and pretty to write. Documentation comes easy. One repository for the truth.
  • A hash resolver map is usually all you need for small APIs
  • Documentation seems like its in the right spot
  • The place to annotate stuff for generators. I generate views/migrations out of IDL. Want to do it via custom directives implementation, but isn’t fully supported yet in graphql-ruby so i use comments now.

Things I don’t like about IDL/Resolver map:

  • Everything is in one hash. Which becomes crowded and cumbersome to read at some point, and in the end you want to split it in multiple files anyway.
  • Doesn’t look super elegant.

Some use cases I thought about:

  • The fast small api writer, that just need a backend for their js apps. Think of tiny api’s. Fast time to market. Think Sinatra under the APIS. The faster you can cram your API out the better. This is what we do mostly.
  • The mid sized/larger teams, think a bit more break things a bit less.
  • The composer, use different gems to compose/extend the current graphql API. If you to see some really cool things /inspiration check out how Scaphold does their integrations (When you click the integrate with stripe, they basically add extra mutations and queries to your graphql API)

Things that need to be decided still when going the IDL way:

  • Camlizing. I’m pro writing IDL the IDL way with jsStyle, but getting snake_case arguments and mapping.
  • How to we make the hash les cumbersome and make it look ruby ish?
  • How do we do extension points on the schema? A bit like how people extend activerecord with acts_as_commentable etc?

Ideas?

@rmosolgo
Copy link
Owner

@theorygeek's point about compatibility with other GraphQL tools is a great one. Even if we extended the IDL for our use case, you could achieve a compatible result but going through Ruby:

input custom IDL 
    |
    V
parse IDL with GraphQL::Schema.from_definition
    |
    V
dump IDL with GraphQL::Schema#to_definition
    |
   V
output "normal" IDL 

But this is pretty cumbersome and having two schema.graphql is error-prone. Maybe we should stick to the spec for this reason.

Another option to put meaningful info in the schema is to use the descriptions for metadata, eg

# @model(name: "Author")
type Author {
  #  @preloads(hasMany: "posts")
  posts: [Post]
}

@emilebosch
Copy link
Contributor

@rmosolgo That's what i am doing my schema, but its ugly (no syntax highliting, no type checking)
Graphl-up uses custom directives. And afaik, Byron also has a RFC outstanding where he allows custom directives to be attached to all elements of the schema this purpose. (https://github.com/facebook/graphql/pull/90/files#diff-fe406b08746616e2f5f00909488cce66R1192)

https://github.com/graphcool/graphql-up

type Tweet {
  id: ID!
  text: String!
  author: User! @relation(name: "Tweets")
}

type User {
  id: ID!
  name: String!
  tweets: [Tweet!]! @relation(name: "Tweets")
}

Also i'd like to separate the discussion in two parts:

  1. How do we do IDL /w ruby resolvers in a standard way. (No model, preloads etc etc, thats resolver specific, not all resolvers get the data from activerecord)
  2. How do implement loader specific behavior/hinting. Which can be directives or comments or whatever. Comments having the downside of no typing (which directives have afaik) and no syntax highliting.

@rmosolgo
Copy link
Owner

How to we make the hash less cumbersome and make it look ruby ish?

The default_resolve object in GraphQL::Schema.from_definition doesn't have to be a hash. It can be any object that responds to the set of required methods.

In fact, when you pass a hash as default_resolve, it's wrapped by another object which maps the keys and values to execution behaviors.

As we build this out, I think a different default_resolve object will come in handy. We could make one that uses the Ruby namespace to find execution behaviors instead of using a hash. For example, imagine this:

# default_resolve's field resolution hook: 
def call(type, field, obj, args, ctx)
  # get a module for this type, eg `type Post` => `module Post`: 
  type_module = Object.const_get(type.name)
  # get a method for the field, eg `topComments` => `Post#top_comments`
  method_name = field.name.underscore
  field_method = type_module.instance_method(method_name)
  # call the method: 
  field_method.call(obj, args, ctx)
end 

Something along those lines would support one-type-per-file, like the DSL uses. (And since it uses proper Ruby constructs, it wouldn't be subject to Rails loading bugs.)

@rmosolgo
Copy link
Owner

Another crazy thing is that we could implement #call(...) above to be backwards-compatible. You could handle NameError by looking for a GraphQL::ObjectType, then using the DSL-defined object in that case.

It would allow a gradual transition from DSL to the new way.

@theorygeek
Copy link
Contributor

Another option to put meaningful info in the schema is to use the descriptions for metadata

This definitely seems worth exploring. It seems like processing that metadata could happen in application code: for example, just write instrumentation that can parse out the metadata from the description.

So perhaps one option would be to provide an instrumentation class that essentially did just that?

# Block is invoked for each type + field + metadata combo that was found.
instrumenter = MetadataInstrumenter.new do |type, field, metadata|
  case metadata.name
  when 'preloads'
    # redefine the field with preloads support
  end
end

Schema.define do
  instrument(:field, instrumenter)
end
 # call the method: 
 field_method.call(obj, args, ctx)

OK so this might be crazy, and shoot me if it is. But what if we could come up with a way to just pass those arguments to your field as normal arguments? Ruby provides all the metadata we need to do it:

# Imagine that this is the field's resolver method:
def my_field(object, some_argument, some_other_argument, context:)
end

# You can ask what its parameters are:
method(:my_field).parameters
# => [[:req, :object], [:req, :some_argument], [:req, :some_other_argument], [:keyreq, :context]] 

Now you have enough information to map the arguments defined in the IDL to the parameters that the method accepts:

  • The first positional parameter is always the object
  • The remaining positional parameters must map to the arguments in the IDL. They may not be omitted, and there may not be extras. (Maybe if you have a billion arguments, you could take an args: kwarg to get the GraphQL::Arguments instead)
  • The method may accept a context: kwarg if it wants the context object

The really nice thing about this solution is that your field resolution methods are all plain Ruby, AND if there is a mismatch between your method signature and the IDL definition, it explodes during boot time instead of in the middle of a query.

Plus, it's probably easier to write tests for this. You don't have to construct an args object to test your methods. As a side note, I think that if you're using the positional version, you should get fully unwrapped (and underscore'd) argument objects: normal arrays and hashes, coerced scalars, etc. But the args: kwarg should give you the wrapped arguments, although maybe it underscore's them.

@rmosolgo
Copy link
Owner

write instrumentation that can parse out the metadata from the description

:mind_blown:

pass those arguments ... it explodes during boot time

image

This is amazing to me. I didn't quite see the light on GraphQL arguments -> Ruby arguments before but I think I can see it now 😆

  • Based on the schema definition, you know exactly what inputs a field will get (based on args + metadata)
  • Based on Ruby introspection, you know exactly what inputs the method will take
  • Check at build time, if they don't match, 💥

@eapache
Copy link
Contributor

eapache commented May 29, 2017

The last point is really a different way of saying that it's a mixing of responsibilities - namely, it brings implementation details into the IDL. ☹️ Sometimes that's probably OK (eg, @authorization) but sometimes it's really hard to see how it adds value to the IDL itself (eg, @preloads).

I agree with this: moving that kind of implementation detail into the IDL bloats the interface with irrelevant information that makes it harder to consume. It also potentially makes it harder to expose publicly, since you now have to generate a copy of the IDL without those annotations if they contain quasi-sensitive information.

# Imagine that this is the field's resolver method:
def my_field(object, some_argument, some_other_argument, context:)
end

I like the breaking out of arguments, but this reads to me like C. If we go this route we should consider:

  • make my_field just a method on the object, instead of passing it as the first parameter
  • using named_arguments: so that you don't get bugs from messing up the ordering

There's a lot of great conversations and ideas happening here, but to me the best argument for the DSL is still locality: it keeps all of the facts about the definition and implementation of a field in a single place. If I'm writing a resolve block/method I don't want to have to flip to another file to figure out what type it should be returning.

@rmosolgo
Copy link
Owner

I took a try at comment-based custom behavior in #789, I'd love to hear your thoughts if you get a chance to look!

@rmosolgo
Copy link
Owner

Let's imagine a world where you build your schema by writing a schema.graphql file.

How do connection types work in that case? Do you write each one by hand? Or is there some way to "generate" that boilerplate?

@theorygeek
Copy link
Contributor

@rmosolgo How do connection types work

You're referring to things like, how do we wire up the behaviors for the paging/slicing of nodes? I think you could either annotate the field with something like:

type SomeType {
  # @connection 
  myField: SomeConnectionType
}

Or we could detect connections and wire them up automatically. But that seems like a bad idea (what if I don't want the default behavior?)

@rmosolgo
Copy link
Owner

Sorry, I didn't express myself too clearly 😬

Let's say you're writing a Relay backend and you have a bunch of different object types. You'll also have connection types:

type Thing {
  name: String!
}

type ThingConnection {
  edges: [ThingEdge!]!
  pageInfo: PageInfo!
}

type ThingEdge {
  node: Thing!
  cursor: String!
}

If you were treating schema.graphql as the source of truth, how would you cope with the boilerplate code for type *Connection { ... } and type *Edge { ... } definitions?

  • Copy-and-paste?
  • Generate code with .erb ? (eg <%= connection("Thing") %>)
  • Machine-readable comments as macros? (eg # @macro connection("Thing"))

Or, something else? I looked at graphql-tools but they don't have any special support for this.

@rmosolgo
Copy link
Owner

I tried ERB 😬 #810

@emilebosch
Copy link
Contributor

Well I've learned to love the DSL, exactly for this, and the meta typing/programming dynamic type generation. I think that IDL is cool for small projects. but it quickly starts to get difficult. Another option is to generate an annotations.rb file from the IDL which allows for annotations on the IDL file, for loaders, etc etc.

But then again the logic is spread over multiple sources.

Isn't there an easy way to load parts from IDL and then just redefine things that need to be adjusted or have additional logic in them?

@rmosolgo
Copy link
Owner

More thoughts about the Ruby part of this system (along the lines of @xuorig's Ruby snippets above): #820

@rmosolgo
Copy link
Owner

rmosolgo commented Nov 2, 2017

Thanks again to everyone who participated in this discussion. After a lot of trial and error, I think #1037 will be my next take at a schema definition API!

@rmosolgo rmosolgo closed this as completed Nov 2, 2017
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

5 participants