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

Setting directives using ObjectType constructor (not IDL) #1343

Open
19majkel94 opened this issue May 8, 2018 · 21 comments

Comments

@19majkel94
Copy link

commented May 8, 2018

I am reviving #1262 as I have an use case that is related to that discussion:

Right now directives are purely a feature of the GraphQL language and IDL, so a schema not created using IDL definitionally won't contain directives. When building a schema in code, you have the full power of the host programming environment, so directives shouldn't be necessary. Directives are a tool for supplying additional intent to be later interpreted by the host programming environment.

You can include any arbitrary data on a GraphQLObjectType that you like - it's just a JS object after all. Directives are a language-level feature, not a structural property of a type.

I agree that "directives are a tool for supplying additional intent" and "you have the full power of the host programming environment". But since the GraphQL ecosystem has grown, I think we should think about interoperability with 3rd-party libraries. The examples that I am thinking about is Apollo cache control and Prisma, which are using directives to provide some metadata to make awesome underlaying features work.

As graphql-js doesn't allow for registering directives using imperative way (ObjectType constructor), to be able to use Prisma, we are forced to use IDL to create our types. But this way might be not available when you are using the imperative way as you build a library on top of graphql-js, as well as when you would have to refactor your whole app by migrating to IDL to add apollo-cache integration.

So I think that now directives are a much more widespread feature and we should allow for interoperability with this 3rd-party tools.

@19majkel94

This comment has been minimized.

Copy link
Author

commented May 11, 2018

After 3 days, ping @leebyron as an author of the quotes and ping @IvanGoncharov as an author of #746 that have added support for directives 😃

@IvanGoncharov

This comment has been minimized.

Copy link
Member

commented May 11, 2018

@19majkel94 My position is the same as I wrote in #1262

I think this question is connected to the topic discussed on the last WG: Exposing schema metadata. So I think we should decide what we expose through introspection(directives or something else) and then add the same thing to GraphQL*Type classes.

@nodkz

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

The new Apollo Server v2 introduce requirements for graphql-compose (and graphql-js) users to somehow provide directive via GraphQL*Type classes for providing an info for CDN cache. Without providing directives via GraphQL*Type we have no way to use Apollo Server v2.

Another use case was with graphql-cost-analysis which allows rejecting complex queries. It provides a comfortable way to add a complexity calculation logic without modifying resolvers.

It will be cool if Lee changes its position according to directives.

Repeat here a desired way for declaration directives from #1262:

const UserType = new GraphQLObjectType({
  name: 'User',
  fields: {
    name: {
      type: GraphQLString,
      description: '...',
      resolve: () => ...,
      directives: [
        { name: 'cost',  args: { useMultipliers: false, complexity: 2 } },
        { name: 'unique' },
      ],
      // or maybe simpler if graphql spec does not allow duplicated names
      directives: {
        cost: { useMultipliers: false, complexity: 2 },
        unique: true,
      },
    },
  },
});
@nodkz

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2018

@IvanGoncharov @taion maybe you take care about this feature in the upcoming WG meetup 2018-09-20?

My English does not alow to me to do it myself 😊

@taion

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2018

The cost analysis case example is interesting, because in the context of using the programmatic API, I think it's suboptimal to set up costing via directives. Instead of the above, a better way to express the same would be to add a getCost(childCost, variables) callback to the field, then do something like:

myConnection: {
  type: FooConnection,
  getCost: (childCost, { first }) => first * childCost,
},
@leebyron

This comment has been minimized.

Copy link
Collaborator

commented Sep 18, 2018

Directives are purely a feature of the GraphQL language and are not part of type definitions - this is because different tools will use and interpret directives in different ways.

I assume the Apollo tools translate these GraphQL language directives to some other kind of metadata on the underlying schema and type objects - it would be best to use whatever API the apollo tools expect directly.

@nodkz

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

@leebyron Now I understood what you mean. It can be explained in another way:

For the query:

query {
  authors {
    id
    name @uppercase
  }
}

needs to create such directive:

const UppercaseDirective = new GraphQLDirective({
  name: 'uppercase',
  description: 'Provides default value for input field.',
  locations: [DirectiveLocation.FIELD],
});
const schema = new GraphQLSchema({
  directives: [UppercaseDirective],
  ...
});

and such Author type with resolve method:

const UppercaseDirective = new GraphQLDirective({
  name: 'uppercase',
  description: 'Provides default value for input field.',
  locations: [DirectiveLocation.FIELD],
});

const AuthorType = new GraphQLObjectType({
  name: 'Author',
  fields: () => ({
    id: { type: GraphQLInt },
    name: {
      type: GraphQLString,
      resolve: (source, args, context, info) => {
        if (info.fieldNodes?.[0].directives?.[0]?.name?.value === 'uppercase') {
          return source.name.toUpperCase();
        }
        return source.name;
      },
    },
  }),
});

For now Directives are quite stupid and adds only some meta to infoAST argument. You need manually traverse fieldNodes in your resolve method for checking directive name by string. (BTW its very strange that info does not contain Directive instance for fast comparision which cheked on query validation step, eg info.fieldNodes?.[0].directives?.[0]?.instance === UppercaseDirective).

Anyway with current directive implementation it does not make sence adding directives to FieldConfig.

@nodkz

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

BUT what if can use directive as middleware?

For example, we create a new kind of directive:

const ChangeCaseDirective = new GraphQLMiddlewareDirective({
  name: 'changecase',
  // locations: [...], // no need, because this directive works only `DirectiveLocation.FIELD` level
  args: {
    fieldName: { type: new GraphQLNonNull(GraphQLString) },
    upper: { type: new GraphQLNonNull(GraphQLBoolean) },
    lower: { type: new GraphQLNonNull(GraphQLBoolean) },
  },
  resolve: async (directiveArgs, resolve, source, args, context, info) => {
     const { fieldName, upper,  lower} = directiveArgs;
     let value = await resolve(source, args, context, info) || source[fieldName];
     if (typeof value === 'string') {
        if (directiveArgs.upper) value = value.toUpperCase();
        else if (directiveArgs.lower) value = value.toLowerCase();
     }
     return value;
  },
});

const schema = new GraphQLSchema({
  directives: [ChangeCaseDirective],
  ...
});

This directive can be defined only on schema build step via SDL or ObjectType constructor. It cannot be defined in GraphQL query, otherwise

  • for such purposes can be used regular args
  • or it can be security vulnerabilities.

So with such directive we can custruct type in such way:

const AuthorType = new GraphQLObjectType({
  name: 'Author',
  fields: () => ({
    id: { type: GraphQLInt },
    name: { type: GraphQLString },
    lowerName: { 
      type: GraphQLString,
      directives: [
        [ChangeCaseDirective, { fieldName: 'name', lower: true} ]
      ],
    },
    upperName: { 
      type: GraphQLString,
      directives: [
        [ChangeCaseDirective, { fieldName: 'name', upper: true} ]
      ],
    }, 
  }),
});

With such case, it is almost similar to @maticzav aproach in graphql-middleware.

And maybe no reason to make such GraphQLMiddlewareDirective.
Cause with graphql-middleware it can be done in the more convenient way with better static analysis:

const AuthorType = new GraphQLObjectType({
  name: 'Author',
  fields: () => ({
    id: { type: GraphQLInt },
    name: { type: GraphQLString },
    lowerName: { 
      type: GraphQLString,
      middlewares: [
        changeCase({ fieldName: 'name', lower: true }),
      ],
    },
    upperName: { 
      type: GraphQLString,
      middlewares: [
        changeCase({ fieldName: 'name', upper: true }),
      ],
    }, 
  }),
});

@leebyron It will be great if graphql will have the ability to provide middlewares for resolve methods out of the box. Cause it helps to keep resolve methods thin and enabling code reuse and a clear separation of concerns (for query cost, permission rules, logging, error tracking and other useful methods).

@nodkz

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

With current implementation of resolve method applying several middlewares looks aweful:

const changeCaseMW = changeCase({ fieldName: 'name', lower: true });
const queryCostMW = queryCost({ ...someSettings });
 
const AuthorType = new GraphQLObjectType({
  name: 'Author',
  fields: () => ({
    id: { type: GraphQLInt },
    name: { type: GraphQLString },
    lowerName: { 
      type: GraphQLString,
      resolve: async (source, args, context, info) => {
        return queryCostMW( 
           () =>changeCaseMW(
               () => { return ...somehow resolved data... },
               source, args, context, info
            ),
            source, args, context, info
        );
      },
    },
  }),
});
@19majkel94

This comment has been minimized.

Copy link
Author

commented Sep 18, 2018

I assume the Apollo tools translate these GraphQL language directives to some other kind of metadata on the underlying schema and type objects - it would be best to use whatever API the apollo tools expect directly.

@leebyron For me it looks like they read the directives from AST directly:
https://github.com/apollographql/apollo-server/blob/ae9da10e625cf283568ba6d29cea8c3e69a7a03f/packages/apollo-cache-control/src/index.ts

So there's no way to pass them as an GraphQLObjectType constructor argument option, like a cacheControl field or something like that. graphql-query-complexity use this way of attaching metadata to types:

const Query = new GraphQLObjectType({
  name: 'Query',
  fields: () => ({
    posts: {
      type: new GraphQLList(Post),
      complexity: ({args, childComplexity}) => childComplexity * args.count,
      args: {
        count: {
          type: GraphQLInt,
          defaultValue: 10
        }
      }
    },
  }),
});

I get your point that when we are responsible for executing the schema, we don't need to add metadata by directives as we can just use kinda middlewares in resolvers.

But for integrating our schema with 3rd party tools, I believe that it's better to expose directives as the most developer friendly way, rather than forcing everybody to dig into the source code of the package.

@mattkrick

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

Apollo sticks it on the astNode directly, whereas the pattern in @nodkz's example puts it on the field.

The difference in implementation is pretty trivial, but enough to break it. For example, here's what a @rateLimiter directive looks like using the IDL:
https://github.com/withspectrum/spectrum/pull/2874/files#diff-2a03f207138e321cfd01f51e50800816R82

And here's how the same thing looks using JavaScript objects instead of the IDL:
ParabolInc/action@5bc77a3#diff-41b9790b1ca42a3ee29cfeb3ccc31713R16

Frankly, I think it's an error that directives are put directly on the astNode & that Apollo's parser should instead assign it to the field, but I'm often wrong 🤷‍♂

FWIW, over the last couple years I've implemented a handful of custom directives & each time I do, I regret it. The logic usually fits much better down the chain in a HOF around resolve or up the chain in the request's context.

For example:
resolve: rateLimit({perMinute: 10})(...} is just so much cleaner than directives: [{name: 'rateLimit', args: {perMinute: 10}}].

@19majkel94

This comment has been minimized.

Copy link
Author

commented Oct 13, 2018

FWIW, over the last couple years I've implemented a handful of custom directives & each time I do, I regret it. The logic usually fits much better down the chain in a HOF around resolve or up the chain in the request's context.

And nobody denies it. Support for registering directives is only for 3rd party libs like apollo-cache-control that use IDL and read directives from AST:
https://github.com/apollographql/apollo-server/blob/ae9da10e625cf283568ba6d29cea8c3e69a7a03f/packages/apollo-cache-control/src/index.ts

All custom code can be implemented by HOC, middlewares, and custom properties in ObjectType config.

@IvanGoncharov

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

The solution for this is discussed in #1527
@nodkz We planning to include extensions in 14.2.0 release.
Can you please review #1527 and comment on whenever it solve your issues?

@19majkel94

This comment has been minimized.

Copy link
Author

commented Jan 30, 2019

@IvanGoncharov
It's not a solution for this. It's only for decorators-like behavior for custom JS code, like placing the metadata and reading it in resolvers (middlewares).

We still need a way to register directives that are present in printSchema and available in astNode because some 3rd party packages read the directives from AST directly (apollo-cache-control). And for 19majkel94/type-graphql#217 it would be great to be able to print a schema with directives generated by graphql-js JS API.

@IvanGoncharov

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

because some 3rd party packages read the directives from AST directly (apollo-cache-control).

As an author of #746 that added astNode in the first place, I can say that initially I also wanted to expose appliedDirectives on every type, field, enum value, etc. So then Lee rejected this proposal with this comment:
#746 (comment)

Right now directives are purely a feature of the GraphQL language and IDL, so a schema not created using IDL definitionally won't contain directives. When building a schema in code, you have the full power of the host programming environment, so directives shouldn't be necessary. Directives are a tool for supplying additional intent to be later interpreted by the host programming environment.

As for exposing more information from introspection - that's a bit out of scope for this particular change, and that RFC may go through more changes. I see type metadata and IDL directives as likely two related but not identical concepts, and don't see a need to prematurely link the two for this particular change.

I didn't fully understood it but was happy that he would at least agree to add astNode that was better than nothing.
After working more with GraphQL and helping to maintain both spec and graphql-js I now fully agree with Lee point, but I feel like we never really explain it properly. It would be great to post here URL to some article or design document that explains it better, but I never saw any such document 😞 So I will try my best (it's late night ATM + I'm not a native speaker) to explain it here:

Directives were added as an extension point for creating new syntax, and they were never intended to be a part of the type system since attaching metadata is not the only use case for directives and in many cases directives are used to create completely new syntax, e.g.:
https://github.com/OlegIlyenko/graphql-gateway/blob/master/testSchema.graphql
Another example is @import discussed here: graphql/graphql-spec#410

I think we can all agree that such directives make sense only during schema building and shouldn't be present in result schema. It would be even more confusing if such directives would reappear when printing result schema back to SDL.

For a long time, I thought that the correct solution would be to whitelist directives that should be passed to the schema and ignore the rest, but after several years I understood that this workaround wouldn't solve the fundamental issue:
On the one hand, we want type system to be as strict as possible to make it easy to work with (in middlewares or in any other tool). On the other hand, we want directives to be as flexible as possible so 3rd-party developers can experiment with new syntax without forking GraphQL parser and creating new GraphQL dialect.

To explain it closer to code lets take my initial version of #746 and imagine it was merged as is and all schema object have appliedDirectives as the map with a directive name as key and directive args as value. It would be easy to work with and fully compatible with a spec at that time, but since then we discover a few new use cases for directives:

  1. graphql/graphql-spec#470 to implement directive chains like this:
type Query {
  users: [User]
    @httpGet(url: "...")
    @jsonPath(path: "$.results")
    @enrichData
    @jsonPath(path: "$.profile")
}
  1. graphql/graphql-spec#472 That allow to duplicate directives on the same location

Both these changes are fit original intention for directives so ATM they are on a fast track to be included in the next spec release. But they break my origin idea for appliedDirectives since we can't duplicate keys and we can't preserve key order.

@nodkz proposal of serializing directives as an array of objects with name and args properties address both issues but there is no guarantee that it will fit nicely with all future spec addition for directives, e.g., top-level directives (graphql/graphql-spec#410) that are different from schema directives so they can't be attached to schema or any other object inside schema.

There are many other potential problems, but I don't want to convert this comment into the blog post :)

@19majkel94 Not sure that above explanation clearly explains my reasoning so I will also try to use the analogy from your domain.
Please correct me if I'm wrong since I never used JS decorators myself but AFAIK they are functions executed during object creation, and you can't access their args after the object was created. But JS decorators are free to do any transformation including adding new properties.

Imagine how restrictive it would be if instead of calling a function JS decorators would add an item to decorators array defined on the appropriate object. Plus it would be very easy to lose such decorator:

class Test {
  @someDecorator
  test() {
  }
}

const modifiedTest = Test.prototype.test.bind(...)

Should modifiedTest retain decorators that were attached to the test method?

Similar to JS decorators that make sense only as syntax sugar during class creation, GraphQL directives are syntax sugar that should be applied only during SDL => GraphQLSchema phase.
And similar to JS decorators that can add additional properties, GraphQL directives can write to extensions.


Hope now you understand why I strongly oppose adding applied directives to GraphQL schema even though I was promoting this idea just a few years ago.

We still need a way to register directives that are present in printSchema and available in astNode because some 3rd party packages read the directives from AST directly (apollo-cache-control).

Ideally, 3rd party tools should use astNodes from user-provided schema (if it passed as GraphQLSchema) only for error reporting and nothing else. ATM schema transformations becoming increasingly more popular and it's even more concerning since during transformation schema is heavily modified, but astNode stays unchanged.

I think extensions (discussed in #1527) is a good long term solution.
Ideally, tools like apollo-cache-control should provide schema transformation that you would use on SDL => Schema phase to convert directives from astNodes into extensions.

This is a better solution since you can always write JS code that transforms any syntax sugar you invented (top level directives, directive chains, duplicated directives) into a single extensions property that has no restrictions on its value (with directives you always restricted to directive args).

Plus extensions are way easier to use programmatically since you can do queryType.extensions.apolloCacheControl instead of queryType.directives.find(dir => dir.name === 'apolloCacheControl').

And for 19majkel94/type-graphql#217 it would be great to be able to print a schema with directives generated by graphql-js JS API.

We can always add some mechanism (e.g., callback) to printSchema that will convert extensions into a set of directives on appropriate node.

P.S. I also working on spec RFC to add extensions into introspection since it next logical step for exposing user-defined metadata SDL => Schema => Introspection => ClientSchema. But it still at the early stage so I'm not ready to publish it ATM.


@nodkz If you have time read this super long comment it would be great to hear your opinion on the topic.

@nodkz

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

@IvanGoncharov extensions on fieldConfig level looks good for me and for graphql-compose 👍

BUT we should consider unification of directives (on field level at schema definition) and extensions (on field config level).

Consider using a new proposed extension with graphql-compose:

// add field via FieldConfig
UserTC = TypeComposer.create('User');
UserTC.addField('name', {
  type: 'String',
  extensions: {
    cost: { complexity: 2 },
  }
});

// create type via SDL
ArticleTC = TypeComposer.create(`
  type Article {
    title @cost(complexity: 2)
  }
`);

The question is: Can be schema directives and extensions unified somehow? If we not unify them, or not provide migration procedure from schema directives to extensions, then we split graphql world quite more than now. 🤢


Let's make one step back.

Clients

When clients need to tune resolver behaviour then may use args. So backender can read the second argument in resolve method and provide such behavior.

Backanders

For now become quite popular to use middlewares for resolvers. It helps to drastically reduce amount of resolvers code. But need to have a nice way to provide such "backenders" arguments from schema to the middlewares.

For now backenders cannot setup any args on schema level. In SDL they found a hack via diretives on schema construction. In field config way there are not such ability. And extensions will add such ability.

So we need to think how can be unified using of SDL directives and extensions for middleware authors. Eg. via a new property info.fieldExtensions or in some other way:

resolve(source, args, context, info) {
  info.fieldExtensions
}

Also we should not forget about modularity and schema stitching. Some part of schemas may be realized via graphql-tools way, other via graphql ConfigObjects.

It will be nice to have such implementation which helps to use backenders args in runtime. Current directive info.fieldNodes?.[0].directives?.[0]?.name?.value === 'uppercase' implementation provides too much runtime checks.

Anyway we need to take a fresh look on problem of backenders' arguments for resolve methods. And solution for this problems shows us a way how to implement extensions and unify them with current directives in SDL.

Thoughts?

@nodkz

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

@IvanGoncharov you may ping me via Telegram by nickname @nodkz or via Skype nodkz1985
and we can closely discuss this problem.

@taion

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

I'll also note that some of the concerns around metadata are specifically around communicating more information to clients. A canonical example here is augmenting type system information by informing the client of e.g. min and max values for numeric inputs.

@19majkel94

This comment has been minimized.

Copy link
Author

commented Feb 1, 2019

@IvanGoncharov
Great explanation! 👍

Now I get your point of view and agree with that - it makes no sense to add weird things just to match with 3rd party libs that are using workarounds. It's better to propose a well designed API/specification and change those libs to support that 😉

And I agree with @nodkz that we need some kind of unification of simple metadata-like directives with the extensions, so we can support both ways of registering additional data with simple way of handling the related logic (like cost and complexity).

@freiksenet

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

At Gatsby we've been using graphq-compose type/field extеnsions. We also use directives to allow setting extension metadata through that. I feel that having a distinct namespaces for directives (within or outside extensions), that is a list of directives with arguments applied to type/field/argument is the way to go here. This matches the behavior between AST and extensions (eg you can apply same directive multiple time). If extensions would also be exposed through introspection, this allows inspecting them too (again, exactly in the way they are defined in SDL).

type Foo @directive1(arg1: "bar") {
  id: ID! @id
  string: String @validation(regex: "/abc+/") @required
}

> Foo.extensions

{
  directives: [
    {
      name: "directive1",
      args: {
        arg1: "bar"
      }
    }
  ]
}

> Foo.getField('string').extensions

{
  directives: [
    {
      name: "validation",
      args: {
        regex: "/abc+/"
      }
    },
    {
      name: "required",
      args: {},
    }
  ]
}
@nodkz

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@freiksenet your approach definitely SHOULD land to the core of graphql-compose

Even more for easy type manipulation, I'll add helper methods, like

FooTC.getDirectiveByName('directive1'); // { arg1: "bar" }
FooTC.getDirectiveByName('directive2'); // undefined
FooTC.getDirectiveNames(); // ['directive1']

FooTC.getFieldDirectiveByName('string', 'validation'); // { regex: "/abc+/" }
FooTC.getFieldDirectiveByName('string', 'required'); // {}
FooTC.getFieldDirectiveNames('string'); // ['validation', 'required']

// For users who use multiple directives with same names
FooTC.getFieldDirectiveById('string', 0); // { name: "validation", args: { regex: "/abc+/" }}
FooTC.getFieldDirectiveById('string', 1); // { name: "required", args: {}}
FooTC.getFieldDirectiveById('string', 2); // undefined

These methods help to simplify the code of end users.

nodkz added a commit to graphql-compose/graphql-compose that referenced this issue Apr 5, 2019

nodkz added a commit to graphql-compose/graphql-compose that referenced this issue Apr 6, 2019

feat(TypeComposers): add getDirectives methods
Refactor EnumTypeComposer for adding directives for enum values.

Related graphql/graphql-js#1343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.