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

Validate @defer directive in schema matches GraphQL spec definition #3287

Open
Tracked by #3093
calvincestari opened this issue Nov 17, 2023 · 3 comments
Open
Tracked by #3093
Assignees
Labels
codegen Issues related to or arising from code generation

Comments

@calvincestari
Copy link
Member

calvincestari commented Nov 17, 2023

This is in reference to an interesting bug reported on Discord - https://discord.com/channels/1022972389463687228/1174776645358465114/1174776645358465114.

tl;dr - AppSync appears to support a version of @defer and it's in schemas downloaded from AppSync. Our JS frontend manually adds the @defer directive to schemas so it can be used in operations. The directive from AppSync differs in that it allows it to be used on FIELD locations only.

The work here is to make sure that we're not adding a duplicate directive which would cause schema validation to fail. We also need to make sure that if one exists it's on the same locations and supports the same arguments otherwise we should fail our validation.

@harryblam
Copy link

harryblam commented Nov 27, 2023

For visibility, I'm currently getting the below error when generating my graph in version 1.7.1, (whereas in 1.7.0 it was working fine). It sounds like this PR may fix that.

Error: JavaScriptError: GraphQLSchemaValidationError-There can be only one directive named "@defer".-GraphQLSchemaValidationError@

@calvincestari
Copy link
Member Author

calvincestari commented Nov 27, 2023

For visibility, I'm currently getting the below error when generating my graph in version 1.7.1, (whereas in 1.7.0 it was working fine).

@harryblam that's correct because some of the defer work has been merged into main.

It sounds like this PR may fix that.

There isn't a PR to change this yet, this is just an issue to track the problem.

Are you also using AppSync though? If you are this error will remain even after it stops adding a duplicate directive. The reason for that is the check I'd implement would be to ensure that any pre-existing @defer directive must be on the same locations (FRAGMENT_SPREAD and INLINE_FRAGMENT) and support the same arguments (if and label). If the pre-existing directive differs from those codegen will abort because our codegen engine is not built to support defer in any other way.

Once the directive is merged into the GraphQL spec, AppSync is going to need to change in order to be spec compliant.

@calvincestari
Copy link
Member Author

I recently made a change to not duplicate the directive, which may be correct for the long-term too, but I still feel we need a check to ensure that any existing defer directive must comply with the RFCs specification of the directive.

@calvincestari calvincestari changed the title Don't add duplicate @defer directive to the schema Validate @defer directive in schema matches GraphQL spec definition Mar 20, 2024
@calvincestari calvincestari self-assigned this Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codegen Issues related to or arising from code generation
Projects
None yet
Development

No branches or pull requests

2 participants