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

GraphQL directive ordering not respected #6176

Closed
half2me opened this issue Dec 14, 2020 · 6 comments
Closed

GraphQL directive ordering not respected #6176

half2me opened this issue Dec 14, 2020 · 6 comments
Labels
graphql-transformer-v1 Issue related to GraphQL Transformer v1 question General question

Comments

@half2me
Copy link

half2me commented Dec 14, 2020

Note: If your issue/bug is regarding the AWS Amplify Console service, please log it in the
Amplify Console GitHub Issue Tracker

Describe the bug
The order of directives on a graphql schema is not always respected.

Amplify CLI Version
4.39.0

To Reproduce
Take the following schema for example:

type Mutation {
  doSomething: Foo @my_directive @another_directive
}

In this case, the order the directives are applied is:

  1. @another_directive
  2. @my_directive

If we swap the directives, like so:

type Mutation {
  doSomething: Foo @another_directive @my_directive
}

The order does not get reversed as expected. The ordering instead depends on the order that the transformers were defined in.

However using the same directive twice, say with different arguments, the order is properly kept.

type Mutation {
  doSomething: Foo @my_directive(option: "A") @my_directive(option: "B")
}

Results in:

  1. @my_directive(option: "A")
  2. @my_directive(option: "B")

And reversing them like so:

type Mutation {
  doSomething: Foo @my_directive(option: "B") @my_directive(option: "A")
}

Produces:

  1. @my_directive(option: "B")
  2. @my_directive(option: "A")

Expected behavior
Order of directives should be respected.

Desktop (please complete the following information):

  • OS: Mac
  • Node Version: v12.20.0

Additional context
Tested by running amplify mock
The directives were custom transformers that I wrote, and they are always executed in the order that they are added to the transform.conf.json file, instead of the order of the directives.

@half2me
Copy link
Author

half2me commented Dec 14, 2020

This issue is important when chaining functions inside a pipeline resolver with directives, like I have done here:
https://github.com/half2me/amplify-transformers#pipeline

@yuth yuth added graphql-transformer-v1 Issue related to GraphQL Transformer v1 question General question labels Dec 16, 2020
@yuth
Copy link
Contributor

yuth commented Dec 16, 2020

@half2me this is by design as we don't want to change the behavior of the transformer based on the order in which the transformer is executed based on the input document. For instance lets say if some one defines a schema like below

type Post
  @auth(rules: [{ allow: owner }])
  @key(name: "postByTitle", fields: ["title"])
  @model {
  id: ID!
  title: String!
  content: String!
}

the transformer wont be in a position to write the @auth or @key directives as the operate on the directives created by @model directive. The resolvers are appended or prepended with code generated by each directive.

We have a Transformer redesign in progress which is hidden behind Feature Flag graphQLTransformer.useExperimentalPipelinedTransformer, which uses phased approaches to execution of the transformer which every transformer plugin will be executed in each phase instead of single pass execution that we have in the current version of transformer. The generated resolvers also have multiple slots where the transformer plugins can add/contribute resolver code, which should help mitigate issue of plugin execution order

@half2me
Copy link
Author

half2me commented Dec 16, 2020

@yuth thanks for clarifying this. I did think this was done by design, but I think it could be very useful to consider the order of the directives when applying transformers, as it allows for more powerful manipulation. With the current design, you have no way of controlling the order. Having a requirement of defining @model before @key shouldn't be a difficult change for anyone, although it would be a breaking change.

This feature flag sounds nice, I'll give it a try. Is there an up-to-date list of all the feature flags available?
Also, where can I get more information about the useexperimentalpipelinedtransformer? Is there someone on the Discord channel I can talk with about it? I'm happy to beta test it, or even contribute to its development, as it's a requirement for some of my custom transformers' logic. I've tried enabling in, but amplify mock fails with a lot of "Unknown directive" errors.

@yuth
Copy link
Contributor

yuth commented Dec 17, 2020

Having a requirement of defining @model before @key shouldn't be a difficult change for anyone, although it would be a breaking change.
It is not a good developer experience to mandate developers to follow a certain order when adding directives as the GraphQL language itself does not have any notion for ordering of directives

This feature flag sounds nice, I'll give it a try. Is there an up-to-date list of all the feature flags available?
Also, where can I get more information about the useexperimentalpipelinedtransformer?

All the feature flags should be documented here, but unfortunately its not up to date. More upto date list can be found here in the code

private registerFlags = (): void => {
// Examples:
// this.registerFlag('keyTransformer', [
// {
// name: 'defaultQuery',
// type: 'boolean',
// defaultValueForExistingProjects: false,
// defaultValueForNewProjects: true,
// },
// ]);
this.registerFlag('graphQLTransformer', [
{
name: 'addMissingOwnerFields',
type: 'boolean',
defaultValueForExistingProjects: false,
defaultValueForNewProjects: true,
},
{
name: 'validateTypeNameReservedWords',
type: 'boolean',
defaultValueForExistingProjects: true,
defaultValueForNewProjects: true,
},
{
name: 'useExperimentalPipelinedTransformer',
type: 'boolean',
defaultValueForExistingProjects: false,
defaultValueForNewProjects: false,
},
{
name: 'enableIterativeGSIUpdates',
type: 'boolean',
defaultValueForExistingProjects: false,
defaultValueForNewProjects: false,
},
]);
this.registerFlag('frontend-ios', [
{
name: 'enableXcodeIntegration',
type: 'boolean',
defaultValueForExistingProjects: false,
defaultValueForNewProjects: true,
},
]);
this.registerFlag('auth', [
{
name: 'enableCaseInsensitivity',
type: 'boolean',
defaultValueForExistingProjects: false,
defaultValueForNewProjects: true,
},
]);
this.registerFlag('codegen', [
{
name: 'useAppSyncModelgenPlugin',
type: 'boolean',
defaultValueForExistingProjects: false,
defaultValueForNewProjects: true,
},
]);

@SwaySway
Copy link
Contributor

We do have an RFC open for this at the moment. Closing in favor of the RFC. The only available directives would be @model as of this time.
#6217

@github-actions
Copy link

This issue has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
graphql-transformer-v1 Issue related to GraphQL Transformer v1 question General question
Projects
None yet
Development

No branches or pull requests

3 participants