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

Support resolve on Input types #747

Closed
wmertens opened this issue Mar 8, 2017 · 18 comments
Closed

Support resolve on Input types #747

wmertens opened this issue Mar 8, 2017 · 18 comments

Comments

@wmertens
Copy link

wmertens commented Mar 8, 2017

Update: see #747 (comment)

Creating an API for reading data is easy now, thanks in part to per-field resolvers that allow you to map from server-side domains to the client domain in a piecewise manner. However, there is no corresponding function to go from client to server domain.

How about adding unresolve(root, args, context, info) to the field definition?

I think it would make sense if it was allowed to mutate root, and it would return the server-domain value of its field. So this would enable:

  1. re-packing/mapping fields:
const Foo = new GraphQLObjectType({
  name: 'Foo',
  fields: {
  	items: {
  		type: new GraphQLList(GraphQLString),
  		resolve: ({items}) => items.split(','),
  		unresolve: (_, items) => items.join(','),
  	},
       },
   })
  1. renaming fields:
const Foo = new GraphQLObjectType({
  name: 'Foo',
  fields: {
  	niceName: {
  		type: GraphQLString,
  		resolve: ({badName}) => badName,
  		unresolve: (root, value) => {
                        root.badName = value
                        delete root.niceName
                       }
                   }
  	},
     },
   })

Right now, to do this, you need to manually traverse the input arguments to apply the transformations where needed.

@leebyron
Copy link
Contributor

From what context would this unresolve function be called?

There should be nothing stopping you from adding these functions to your schema, but I'm not understanding what code should be calling them.

@wmertens
Copy link
Author

wmertens commented Apr 25, 2017

Hmm, in my example I was envisioning a world without input objects it seems :) pretend that the above types are InputObjects with unresolve and the resolvers are what is used on the corresponding output types.

The idea is that unresolve is called depth-first on the inputs of a mutation, so that the mutation resolver gets the arguments as already-mapped-to-server-domain arguments.

Graphql knows the schema much better than the dev and if you are using complex inputs it gets easy to forget to reverse the client mapping. Plus, if you manually call the unresolvers, you also have to do it depth-first, which gets hairy.

@leebyron
Copy link
Contributor

The leaf nodes on input types coerce input into the correct shape - does that help? Or you want to have the ability to have, for example, an input type which is an array of values, but provide it to server code as a comma-separated string?

@wmertens
Copy link
Author

wmertens commented Apr 25, 2017 via email

@wmertens
Copy link
Author

wmertens commented May 5, 2017

Another example is where your database expects a normalized structure but the graphql mutation interface is the joined structure. Without a per-type unresolve, you need to implement the normalization in every mutation call that includes the type somewhere.

Basically, resolve maps from server representation to graphql representation, and unresolve would do the reverse.

@wincent
Copy link
Contributor

wincent commented May 5, 2017

I get the use case, conceptually, but it's still not quite clear to me when/how these unresolve methods would be called.

@wmertens
Copy link
Author

wmertens commented May 6, 2017

I would expect these unresolve()rs to be called before the args object is given to the mutation's resolve(root, args, context, info). That way, the server code works with an inbound server-domain object, just like it returns server-domain objects.

So suppose an inbound mutation has a ImageInput argument, an object like {id, title, alt, tags[]}, and on the server that is stored as {uploadId, meta: {title, alt, tags-as-csv}}, then the ImageInput type would have an e.g. unresolve(inputvalue) function that returns the converted value. Graphql would walk then input args schema, calling unresolvers depth-first.

@wmertens
Copy link
Author

wmertens commented Jul 15, 2018

How about just supporting resolve on the fields of input objects? Conceptually simple, right?

const Foo = new GraphQLObjectType({
  name: 'Foo',
  fields: {
  	items: {
  		type: new GraphQLList(GraphQLString),
  		resolve: ({items}) => items.split(','),
  	},
  },
})

const FooInput = new GraphQLInputObjectType({
  name: 'FooInput',
  fields: {
  	items: {
  		type: new GraphQLList(GraphQLString),
  		resolve: (_, items) => items.join(','),
  	},
  },
})

The resolvers would get called right after the value is parsed, so depth-first.

Signature could be resolve(parsingObject, parsedField, context, info). Then the resolver can also mutate the given parsingObject, so that you can rename fields. If the resolver returns undefined, the value isn't added to the parsingObject.

const Foo = new GraphQLObjectType({
  name: 'Foo',
  fields: {
    niceName: {
      type: GraphQLString,
      resolve: ({badName}) => badName,
  	},
  },
})

const FooInput = new GraphQLInputObjectType({
  name: 'FooInput',
  fields: {
    niceName: {
      type: GraphQLString,
      resolve: (root, value) => {
        root.badName = value
      }
  	},
  },
})

@wmertens wmertens changed the title Allow specifying un-resolvers in fields Support resolve on Input types Jul 15, 2018
@IvanGoncharov
Copy link
Member

@wmertens I think this issue partly overlaps with #361 (comment)
I agree with @leebyron comment in #361 that input validation should be done before calling resolvers.

But for argument transformation, I think it should be done inside resolvers. The resolver should receive arguments that match field definition especially since it's documented in the specification: http://facebook.github.io/graphql/June2018/#sec-Value-Resolution

@wmertens
Copy link
Author

@IvanGoncharov there is no overlap because that issue is about output types and there is even a workaround possible by decorating the schema.

However, input types have no transformation possibilities besides creating a scalar type per resolver, which would even cause the schema to change.

I also disagree that this is precluded by the spec, since the spec is purely about the schema and this is implementation-specific.

I really think that adding this simple feature will lead to less mutation resolver code with better reuse and thus higher quality.

@IvanGoncharov
Copy link
Member

@wmertens If we allow changing types of arguments than resolvers will stop being type-safe. For example, you lose the ability to generate typings for your arguments, like this: https://github.com/nknapp/graphql-typewriter/blob/master/test/schemas/arguments.ts#L8

It also a problem for any GraphQL middleware that will try to wrap resolve functions since it can't expect that args value will match args definitions.

Scalars are the only place where graphql-js allow you to change types and I don't think we should add the second place especially since #1419 will provide good enough alternative.

@wmertens
Copy link
Author

  • not everybody uses typescript
  • the type of the InputType will be whatever the resolver returns, or if there is no resolver, the same graphql type as before

Types are nice, but now you're using them as an argument against something that would improve code quality?

@wmertens
Copy link
Author

Actually I'm not sure that we're on the same page. I am only requesting a nicer way to map graphql inputs to their server-side equivalents, just like output resolvers are a nice way to map server-side data to graphql.

@WestleyArgentum
Copy link

Fwiw, I would also very much like to see this.

I think it also comes up when you're working with an existing database that has snake_case columns and you want to provide a camelCase graphql interface. In the output types it's trivial to throw in resolvers like

createdAt: {
      type: GraphQLString,
      resolve: (root) => root.created_at
    },

But for input types you're kinda hosed, you just have to find somewhere you can slip some parsing / formatting code in without causing too much trouble.

@IvanGoncharov
Copy link
Member

This is a pretty big proposal so I think it should first be implemented outside of graphql-js and if it proves to be popular and battle-tested it can be merged in this lib.

Currently, we focusing on adding functionality to allow implementing such feature outside of graphql-js.
Namely support for assigning additional properties to fields (#1527) and support for resolver middleware (#1516).

@wmertens
Copy link
Author

wmertens commented Jan 30, 2019

@IvanGoncharov but these inputtype field resolvers have to run during the parsing of the input data given to graphql-js, otherwise they can't do depth-first.

Mutation resolvers run after that so then it is too late.

Would it maybe be possible to add a callback function that runs after each input parsing, for prototyping this?

@IvanGoncharov
Copy link
Member

Would it maybe be possible to add a callback function that runs after each input parsing, for prototyping this?

@wmertens Middleware 100% would support pre resolver callback in which you can replace args and also you will have access to argument types. So you can run extensions.resolve for all input objects inside args and pass the result to the resolver.

@gajus
Copy link

gajus commented Jun 26, 2019

This is a pretty big proposal so I think it should first be implemented outside of graphql-js and if it proves to be popular and battle-tested it can be merged in this lib.

– @IvanGoncharov, #747 (comment)

Yeah, unfortunately right now there is no way to process input types before they get passed to the resolver. That would probably be out of scope for being built into this library until it's added to the upstream graphql-js implementation this is based on.

– @stubailo, ardatan/graphql-tools#652 (comment)

This issue feels a bit like a hot potato at the moment.

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

No branches or pull requests

6 participants