New Mutation API Brainstorming

[rmosolgo]

GraphQL::Relay::Mutation is really showing its age, and I'd like to replace it with a new class-based API for structuring mutations. I want to answer a few specific questions that have come up over the years. Here are some issues and the current best practice:

• How do you authorize a mutation? Current best practice is to put auth code at the top of the resolve function.
• How do you convert ids to objects in a consistent way? Current best practice is to load ids as needed.
• How do you express errors to clients? No current best practice. Maybe top-level "errors" key, maybe return "errors": as part of the mutation response, maybe a dedicated ClientError type.

So, I have some thoughts about how to solve this which I will add below as a comment, and I'd love to hear if anyone else has other issues to consider or suggestions to propose.

[rmosolgo]

cc others who expressed interest in this before:

@swalkinshaw @eapache @xuorig @cjoudrey @etripier @Pallinder @chadwilken @cisacke

(If you're not interested, you can click "unsubscribe" on the right!)

[rmosolgo]

I'd like to solve this in "layers", where:

• a base class provides some core behavior like generating *Input and *Payload types
• a child class provides opinionated, robust defaults (demo'd below)
• another child class provides Relay Classic support

Here's one idea for an API:

class Mutations::UpdatePost < GraphQL::Schema::Mutation 
  description "Update the attributes of a blog post by ID" 
  argument :post_id, ID, null: false, description: "The ID of the post to update", inject: :post 
  argument :attributes, Types::PostAttributes, null: false
  
  # Return fields must be `null: true`, because if client_errors is present, the rest will be null
  returns :post, Types::Post, null: true 
  # Any returned `GraphQL::ClientError` instances will go here 
  returns_client_errors 
  # equivalent to 
  # returns :client_errors, [GraphQL::Mutation::ClientErrorType], null: true, description: "..."

  # This method corresponds to `post_id...inject: :post` above, 
  # it will be called _before_ `resolve`  and the return value will be passed to `resolve`.
  # So, it loads the post from ID, or returns an error. 
  # It's hooked up to GraphQL-Ruby's `lazy_values` API (aka: works with graphql-batch!). 
  def post(post_id) 
    Loaders::ObjectLoader.find("Post", post_id).then do |post| 
      if post.nil? 
        GraphQL::ClientError.new("Post ##{post_id} not found") 
      else 
        post 
      end 
    end 
  end 
  
  # This is called after arguments are loaded, but before resolve. 
  # It can return true, false, or client errors 
  def authorize(post:, attributes:)
    if post.author_id == @context[:current_user].id 
      true 
    else
      # could also return `false` here to fail silently 
      GraphQL::ClientError.new("You can only update posts which you have written.")
    end     
  end 

  # Finally, this is called. It may return a Hash or client errors 
  def resolve(post:, attributes:) 
    if post.update(attributes)
      { post: post }
    else 
      post.errors.full_messages.map { |m| GraphQL::ClientError.new(m) } 
    end 
  end 
end 

Using classes means you can use familiar techniques to DRY your code and instill conventions in your codebase. For example, a lot of the code above could be generalized to an UpdateInstanceMutation.

[eapache]

I like a lot of what's in that example; here are some notes based on what we've got internally:

Authorization

The auth use case you demonstrate is legitimate, but there is another case worth mentioning: when the auth check does not depend on anything in the arguments (e.g. only depends on the context). In this case, it needs to be run before arguments are resolved since in the case of a failed auth we don't want the mutation to return "Post x not found" (the client shouldn't get that information). I wonder if this means we need two different methods, a "pre_auth" and "post_auth" or something?

Errors

I find the name ClientError here a bit confusing. We call them UserError internally and use that as a distinguishing feature: anything that goes in a UserError is a message that can/should be shown to the human user in the case that the client is acting on behalf of a human. This lets us reserve the top-level errors for developer or "client" errors which should be logged, generate an alert, etc. and get masked as just "Something went wrong" when presenting to the human user (if any).

We also follow the ActiveRecord/GraphQL errors approach of providing fields on our errors when possible:

type UserError {
  message: String!
  field: [String!]
}

We have a bunch of helpers and logic around translating and validating AR errors, so that we can do UserError.errors_from(active_record_object) and it will do the right thing, including letting us know when the AR field that the error was on does not line up with the shape of the actual mutation input and needs adjusting.

One other tiny note on the topic of errors: we went with making our field :user_errors as null: false since there didn't seem to be a strong reason to distinguish null from [] in this case.

[swalkinshaw]

Should also note that the specific UserError type @eapache posted above seems to be used quite widely now. Someone at GraphQL Summit presented the exact same type I believe.

I think this gem could even include a simple version of UserError.errors_from(active_record_object) since it will be very common.

The example looks great 👍 . I wonder if it makes sense to provide a separate "hook" method for errors specifically. A kind of "post_resolve".

[xuorig]

I find the name ClientError here a bit confusing. We call them UserError internally and use that as a distinguishing feature: anything that goes in a UserError is a message that can/should be shown to the human user in the case that the client is acting on behalf of a human. This lets us reserve the top-level errors for developer or "client" errors which should be logged, generate an alert, etc. and get masked as just "Something went wrong" when presenting to the human user (if any).

We do the exact same, except ClientError is the name we've picked for "UserError". I personally don't have a strong opinion on the naming. I've always seen it as DeveloperError vs ClientError but I can see how Client could sound like it's an error for the "graphql client" and not the current user.

Again, no strong opinion, just thought I'd mention we're talking about the same thing 👍

[eapache]

We also don't have an equivalent to returns_client_errors; all mutations get it whether they want it or not. It has turned out to be handy because usually clients are already fetching/checking it when the server decides it actually has something it wants to return.

[tb]

How do you express errors to clients? No current best practice. Maybe top-level "errors" key, maybe return "errors": as part of the mutation response, maybe a dedicated ClientError type.

I used to return validation errors as part of data, but had major issues:

• My type definitions had to include field :errors, Types::JSONType - that was not the right thing to have in type definition.
• Frontend code was not aware the mutation failed, so I had to wrap the call and check of model contains errors and then reject Promise. That method had to know however what is the name of mutation to extract the errors and reject.
• Finally, it did not address the errors in general. If there was some server error, it was not handled via data errors.

Better approach is to pass errors via top-level errors. This is also the approach that graphene has.

Here is how I changed it in my code tb/northwind-graphql-ruby#33 and here is example mutation spec for testing it. I use fields to pass the fields validation errors to client. I still need to improve this code, in particular replace mutationAsPromise with apollo link middleware.

[xuorig]

@tb there are several limitations to that approach also, so I wouldn't say it's a better approach just yet.

For example, this part of the spec is worrying:

GraphQL servers may provide additional entries to error as they choose to produce more helpful or machine‐readable errors, however future versions of the spec may describe additional entries to errors.

Meaning that the fields field in the errors may be broken by the spec at any time.

Another point to consider is that the errors key is not typed, making it a way less attractive option for clients. It's also very difficult to manage its evolution without breaking clients.

There are pros, and cons unfortunately, to both approaches.

[tb]

@xuorig ... in fact graphql spec already includes it https://github.com/facebook/graphql/blob/master/spec/Section%207%20--%20Response.md#errors

[xuorig]

@tb sorry, includes what? Edit: The GraphQL spec includes errors, the discussion here is whether it's the best place to display user facing errors.

[tb]

@xuorig Specs include official approach for errors and answer to @rmosolgo question:

How do you express errors to clients? No current best practice. Maybe top-level "errors" key, maybe return "errors": as part of the mutation response

[cjoudrey]

I love these discussions about authorization and errors, don't mean to derail them but do we feel these need to be solved at the same time as making mutations definable via the class-based API?

In other words, would it make sense to include a version of GraphQL::Schema::Mutation in v1.8 without bells and whistles, and then build authorization and errors on top of that in a subsequent version once we've gotten to experiment with the class-based API?

[xuorig]

Maybe the minimal things we need to discuss for a good base class are generation of input / payload types and how extensible the base class is.

These days im leaning towards have a barebones base class mutation which doesn't generate an input and payload types. The single input requirement and payload type are gone since RelayModern.

My hopes are we can support mutations like updatePost(id: ID, postPayload: PostInput) and not necessarily impose updatePost(input: UpdatePostInput). Input and payload generation is possible by subclassing the first approach, the opposite is not true.

[bwillis]

Just wanted to add our (hackerone) two cents:

How do you express errors to clients?
👍 to structuring errors. We formalized our ErrorConnection and it makes me hopeful for the future. Gave an example of our interface below.

How do you convert ids to objects in a consistent way?
Many inconsistent ways, but (hopefully) these steps:

• unsafe_object = ctx.schema.object_from_id(...)
• safe_object = User.access_to_object(unsafe_object)

Any help in consistency here would be 💯

How do you authorize a mutation?
We're mixing a bit of upstream authorization with some basic object access as answered above.

Structure response
One thing we found difficult for developers to understand was how the response would be constructed, returning a hash that had to map to return_fields was not as straightforward as one might expect. We added a builder pattern as a wrapper around the response in order to make it easier to understand how to send back errors, or send back response fields. There are other patterns that you could apply, but making the interface a bit simpler was useful-it also added that we could surface developer feedback sooner, eg. "Required field :x wasn't sent back", "Field of type String was Object".

Reference
Sample mutation:

Mutations::UpdateMeMutation = GraphQL::Relay::Mutation.define do
  name 'UpdateMe'

  input_field :tshirt_size, !types.String

  return_field :me, -> { UserType }
  standardized_mutation # this is how you opt-in to the error handling

  resolve ->(_, args, ctx) {
    user = ctx[:current_user]
    if user.nil?
      ctx[:response_builder].add_authentication_required_error
    else
      user.update(tshirt_size: args.tshirt_size)

      ctx[:response_builder]
        .add_return_field(:me, user)
        .add_errors_from_model(user)
    end
  }
end

Sample schema snippet:

type UpdateMePayload implements MutationResult {
  # A unique identifier for the client performing the mutation.
  clientMutationId: String
  errors(types: [ErrorTypeEnum]): ErrorConnection!
  me: User
  was_successful: Boolean!
}

interface MutationResult {
  errors(types: [ErrorTypeEnum]): ErrorConnection!
  was_successful: Boolean!
}

enum ErrorTypeEnum {
  ARGUMENT
  AUTHORIZATION
  THROTTLE
}

[etripier]

@eapache For what it's worth, I think there are already a few ways to do ahead-of-time authorization - enough so that we may not need a special case for mutations.

For one, you can use a schema whitelist. The Authorization DSL also does some ahead-of-time checking; you can recreate it by combining a query analyzer with some custom instrumentation.

Some of my team members have been using the schema whitelist approach. Personally, I think a more general, non-mutation-specific tool makes a bit more sense in an ahead-of-time context because you don't need any information obtained within the resolver.

[etripier]

@rmosolgo That looks fantastic! I particularly like that the authorization functionality works well with GraphQL::Batch. Please let me know if I can help in any way (where should I start?).

[eapache]

Whitelist visibility approach is not desirable for us because we don't want to hide the mutations from introspection or anything like that; just prevent them from being run. Analyzers and instrumentation operate at the request level and don't provide a way to reject a single mutation while permitting the rest of the request to execute.

[rmosolgo]

Thanks everyone for this discussion! It sounds like we're on to something 😎

I have a few responses:

auth before arguments are resolved

What if we used (😱 ) #initialize for this? For example, initialize could raise an error to halt the mutation.

ClientError...UserError

Glad to hear we're basically on the same page. I don't have a strong opinion on the name, so we can work out those details down the line.

all mutations get returns_client_errors whether they want it or not

That's 👍 with me. We'll have layers, so people can mess with it if they want.

GraphQL::Schema::Mutation in v1.8 without bells and whistles

For me, the requirement is that bells and whistles fit in nicely. I don't think I can confidently build the base model without also building the luxury model, so I'd like to build them together.

barebones base class mutation which doesn't generate an input and payload types

👍

our (hackerone) two cents

Thanks @bwillis, that looks pretty similar to the github flow: fetch object, then authorize. One trick is that we put auth in our object_from_id function. I want to make that more widespread because I think it has really worked well here.

schema whitelist

I think these two things are complementary: sometimes you want to completely hide a mutation, but other times you want to conditionally allow/disallow it (eg, you can only update your own comments). But for that to work, the mutation must be visible.

Better approach is to pass errors via top-level errors

I think the design goal of GraphQL is:

• GraphQL-level runtime errors go in "errors"
• Application errors go in "data" (somewhere)

For example, I heard that Relay completely disregards "data" when "errors" is present at all, I'm not sure that's always the best behavior. But I'm glad it has worked well for you so far! I definitely won't break that option.

[jeffcarbs]

Wanted to throw another example out there for authorizing mutations.

We already had an authorization instrumentation for fields and types which we also wanted to use for mutations so all of our auth logic can live in one spot. To do that, we first added a custom resolve_root instrumentation that is essentially:

def root_resolver(original_resolver, root_resolver)
  -> (obj, args, ctx) do
    # Call the `resolve_root` proc
    root = root_resolver.call(obj, args, ctx)
    # Pass the result to the original resolve instead of `obj`
    original_resolver.call(root, args, ctx)
  end
end

Our authorization instrumentation is essentially:

def authorize_object_resolver(action, original_resolver)
  -> (obj, args, ctx) do
    # In our mutations, `obj` will be the result of `resolve_root`
    return unless ctx[:current_ability].can?(action, obj)

    original_resolver.call(obj, args, ctx)
  end
end

Then our mutations look like:

Mutations::UserUpdateMutation = GraphQL::Relay::Mutation.define do
  name 'UserUpdate'
  description 'Update a User'

  # Custom `authorization` instrumentation
  authorize :update

  input_field :id, !types.ID
  input_field :user, !Types::Input::UserInputType

  # Custom `resolve_root` instrumentation
  resolve_root -> (obj, args, ctx) { User.find(args[:id]) }

  # `user` here would normally be `nil` since all mutation fields exist on the root MutationType
  resolve -> (user, args, ctx) do
    # At this point, the user has already been authorized with:
    #   ctx[:current_ability].can?(:update, user)
    if user.update_attributes(args[:user])
      { success: true, user: user }
    else
      { success: false, errors: user.errors }
  end
end

So resolve_root finds the object that you'll be operating on and passes it as the obj to the mutation's resolve. You could also theoretically pass this as a fourth argument instead of overriding the root but in our case the root would always be nil otherwise for mutations. Then we let our normal authorization instrumentation ensure that you can perform the action on that obj before calling the original mutation resolver.

[rmosolgo]

Thanks for sharing that example, @jeffcarbs , that's a nice way to show how auth can be fitted in programmatically!

I'm going start chipping away at #1310, feel free to review there if you're interested.

[cabello]

I just read this article from the GraphQL weekly https://blog.graph.cool/graphql-directive-permissions-authorization-made-easy-54c076b5368e it's on the Javascript implementation and how to fit directives into permission checking nonetheless I find it very relatable to Mutation authorization and generally Query authorization like in the article the author mentions that costBasis is only available for managers even though is listed in the "public" schema.

[rmosolgo]

Starting to work on built-in loading/auth in #1609