[RFC] Scoped context

[dwwoelfel]

We're working on a feature for OneGraph that allows users to query data across multiple accounts for the same provider in a single GraphQL query. For example, with our Gmail integration, you'd be able to query for both your work and your personal email in the same query.

We'll be using an argument to allow the user to specify which account to query. It would look something like:

{
  google {
    personalMessages: gmail(accountId: "daniel@gmail.com") {
      messages {
        nodes {
          id
        }
      }
    }
    workMessages: gmail(accountId: "daniel@onegraph.com") {
      messages {
        nodes {
          id
        }
      }
    }
  }
}

We'd like to use scoped context to make sure the subqueries use the correct account.

Background

Lacinia, the Clojure GraphQL library, implements scoped context: https://lacinia.readthedocs.io/en/latest/resolve/context.html

There's a short discussion in the graphql-js repo here: https://github.com/graphql/graphql-js/issues/354

Implementation

This adds a new io_field_with_set_context field that behaves like an io_field, but passes a function to the resolve function that allows it to set the context for the child fields.

In usage, it looks something like:

  io_field_with_set_context "gmail"
    ~typ:gmailObj
    ~args:[arg "accountId" ~typ:(non_null string)]
    ~resolve:(fun setChildContext  ->
                fun { ctx }  ->
                  fun ()  ->
                    fun accountId  ->
                      setChildContext
                        {
                          ctx with
                          gmailAccountId = (Some (accountId))
                        };
                      Deferred.return ((Ok (()))

  io_field "messages" ~typ:gmailMessages
    ~resolve:(fun { ctx }  -> fun ()  -> fetchGmailMessages ctx.accountId)

I don't think this is the best way to implement the feature, but I ran into troubles getting the types to work with other approaches. I'd love to get some feedback on how you'd think about implementing other approaches.

Other ideas for implementation:

1. Change the return type of io_field_with_set_context to a tuple or record with context and the normal result. This is what I initially attempted and where I got stuck.
2. Change the return type of field/io_field to be an internal type with helper functions to return either values or values with context. io_field_with_set_context above would be a normal io_field with Resolve.with_context((), {ctx with gmailAccountId = (Some (accountId))}).

[andreas]

Thanks for the PR and for writing this up, @dwwoelfel!

I think we can address a number of use cases with option 2 of the "other ideas" you mention: a new abstract type, e.g. Graphql.Resolve_result.t, with functions to create such values.

It would be great if we can make an exhaustive list of possible return values from a resolve function. Here's my first take on it:

• An error value if the resolver failed...
  • Optionally with top-level extensions.
  • Optionally with extensions on the error.
• A success value if the resolver succeeded.
  • Optionally with top-level extensions.
  • Optionally with a new context.

Does that make sense? Have I missed any cases?

[anmonteiro]

I agree with @andreas here. Adding a new type of field for each new use case creates the unfortunate explosion in combinations as you now have to add an io counterpart to each one.

@andreas are you suggesting that every resolver now returns that abstract type? I might be missing something, but it doesn't seem that it would preserve backwards compatibility (maybe there's a way I'm not seeing).

I think that the solution to this particular use case should also take into account that a GraphQL response can return both data and errors. We could perhaps do ourselves a favor here by providing a solution that also encompasses that?

[dwwoelfel]

I think that the solution to this particular use case should also take into account that a GraphQL response can return both data and errors. We could perhaps do ourselves a favor here by providing a solution that also encompasses that?

I think the only case where the resolver function could return both an error and a result is when the resolver is returning a list (e.g. [Int] could coerce to [1,error,2] and return [1, null, 2], with an error in the errors map). @anmonteiro was there another case you were thinking of?

For lists, the resolver might want a special Graphql.Resolve_list_result.t that would allow either an error if coercing the resolver result into a list results in an error or a mix of errors and results if coercing an individual item in the list results in an error. Might look something like (Graphql.Resolve_result.t list, error) Result.t

One of the more annoying things we deal with manually right now is converting [1, error, 2] into [1, null, 2] or error depending on whether the list is non-nullable. It would be nice if the graphql server would handle this (it looks like the spec expects it to https://graphql.github.io/graphql-spec/draft/#sec-Combining-List-and-Non-Null). Especially since the user of the library can't easily add arbitrary errors to the errors map.

[andreas]

@andreas are you suggesting that every resolver now returns that abstract type? I might be missing something, but it doesn't seem that it would preserve backwards compatibility (maybe there's a way I'm not seeing).

Yes, I would preserve backwards compatibility by leaving field and io_field as is. It should be possible to express the old semantics in terms of the new.

I think that the solution to this particular use case should also take into account that a GraphQL response can return both data and errors. We could perhaps do ourselves a favor here by providing a solution that also encompasses that?

I think the only case where the resolver function could return both an error and a result is when the resolver is returning a list (e.g. [Int] could coerce to [1,error,2] and return [1, null, 2], with an error in the errors map). @anmonteiro was there another case you were thinking of?

I haven't researched this in depth, but there does not appear to be consensus about whether a resolver should be allowed to return a success value and include errors. Absinthe (Elixir) apparently disallows it (discussion here), while other implementations appear to allow it (e.g. graphql-ruby or sangria). Is this what you had in mind, @anmonteiro?

About errors and lists with nullable contents, some use cases are covered (e.g. list of nullable object type as shown in this test case). It's true that a list of nullable scalars cannot easily be expressed though, but that also seems like weird use case. Can you elaborate on this, @dwwoelfel?

[sgrove]

I definitely would like to be able to return both a success and an error value - probably for scalar fields as well, but at least for lists fields (where we were able to retrieve 5/10 items, and 5/10 caused errors - I want to report on the errors, but return as many of the positive items as possible)

[dwwoelfel]

It's true that a list of nullable scalars cannot easily be expressed though, but that also seems like weird use case. Can you elaborate on this, @dwwoelfel?

We don't have a use-case for returning a list of nullable scalars. It's just a simplification of what we want to do, which is return a list of objects, where some of the objects are nulls. The test case you linked to is one way to solve the problem, but it means that you have to push the logic of when to show an error to the object's resolver instead of the resolver generating the list. I'm pretty sure that graphql-js resolvers let you return a list like [obj1, Error(error1), obj3] and will return something like data: [obj1, null, obj3], errors: [error1].

I definitely would like to be able to return both a success and an error value - probably for scalar fields as well, but at least for lists fields (where we were able to retrieve 5/10 items, and 5/10 caused errors - I want to report on the errors, but return as many of the positive items as possible)

I don't think returning an error and success for a scalar value would comply with the spec. For example, if you're going to return an error for a nullable field, the field should be null.

Having something like Graphql.Resolve_list_result.t that I proposed above would solve the problem you're describing with lists.

[andreas]

@dwwoelfel Thanks for following up. I've thought more about this since -- here's an attempt to summarise the situation right now:

The current design is that the type parameter 'src for a GraphQL type ('ctx, 'src) Schema.typ does not capture that the resolver can fail. For nullable types, 'src has the shape 'a option, but that only allows returning None, not providing an error. The ability to return an error is "regained" by using io_field rather than field, which allows a resolve function to return ('a, string) result io for a field with output type ('ctx, 'a) Schema.typ. However, this only allows to return an error for the entire field, not for an individual list element -- this is the crux of the problem, it seems. Note that this is only a problem with lists, since it's the only composite type.

I have two different ideas to address this:

• Introduce a new type modifier, say errorable_list, with type ('ctx, 'a) Schema.typ -> ('ctx, ('a, string) result list option) Schema.typ. This seems like a bit of a bandaid though.
• Instead of using the type ('ctx, 'src option) Schema.typ to signify a nullable type, we could use ('ctx, 'src nullable) Schema.typ, where:

  type 'a nullable = Ok of 'a | Null | Error of string

  In this case, I imagine the resolve function of an io_field would simply return an 'a io given an output type of ('ctx, 'a) Schema.typ. This begs the question how to signal errors in case of a non-nullable type -- maybe the answer is exceptions?

Lastly, solving the problem of returning errors for individual list elements does not solve the problem originally posed in this PR: changing the context value. It still seems like adding a type Graphql.Resolved.t could provide value, though I haven't thought through the details of how the two features interact.