[RFC] Added support for repeatable directives #1541

OlegIlyenko · 2018-10-02T22:42:43Z

This in an implementation for a spec proposal:

Spec proposal: [RFC] Limit directive uniqueness to explicitly marked directives
Original issue: Limiting the scope of "Directives Are Unique Per Location" validation

I think I have covered all places:

SDL AST & parsing
Schema printer (based on AST as well as based GraphQL* types)
Introspection
buildASTSchema & buildClientSchema
The actual validation rule

Please let me know if I forgot something.

# Conflicts: # src/language/__tests__/schema-kitchen-sink.graphql # src/language/__tests__/schema-printer-test.js # src/language/ast.js # src/language/printer.js # src/type/directives.js # src/type/introspection.js # src/utilities/__tests__/schemaPrinter-test.js # src/validation/rules/KnownDirectives.js # src/validation/validate.js

IvanGoncharov · 2018-10-03T21:01:03Z

src/validation/rules/UniqueDirectivesPerLocation.js

@@ -48,7 +70,11 @@ export function UniqueDirectivesPerLocation(
              ]),
            );
          } else {
-            knownDirectives[directiveName] = directive;
+            const repeatable = repeatableMap[directiveName];


Can you just do:

for (const directive of directives) { const directiveName = directive.name.value; if (repeatableMap[directiveName]) { continue; } // ... }

I guess I generally trying to avoid usage of break and continue, but I can update the code and use it here.

@OlegIlyenko Thinking about this can you change repeatableMap to uniqueDirectiveMap with reverse value and do:

for (const directive of directives) { const directiveName = directive.name.value; if (uniqueDirectiveMap[directiveName]) { // ... } }

Plus uniqueDirectiveMap match nicely with UniqueDirectivesPerLocation rule name.

good idea, done ae9e7f9

IvanGoncharov · 2018-10-03T21:29:28Z

src/type/directives.js


  constructor(config: GraphQLDirectiveConfig): void {
    this.name = config.name;
    this.description = config.description;
    this.locations = config.locations;
    this.astNode = config.astNode;
+    this.repeatable =
+      config.repeatable === undefined || config.repeatable === null


In JS you can do that with a single config.repeatable == null and we already using it in couple places.

I believe config.repeatable === undefined is still necessary since I defined the flow type like this:

repeatable?: ?boolean,

If I don't do the check, I will get following flow error:

Cannot assign `config.repeatable === null ? false : config.repeatable` to `this.repeatable` because: - null or undefined [1] is incompatible with boolean [2]. - undefined [1] is incompatible with boolean [2].

@OlegIlyenko Note == null (two equal signs) compares both to null and undefined and Flow should handle it without problems.

If I don't do the check, I will get following flow error:

It's because you use === instead of ==.
It's safe and very common pattern in JS to do == null:
https://eslint.org/docs/rules/eqeqeq#smart

Ah, I see. Haven't noticed this, thanks for the clarification.

I got an impression that the use of == is discouraged in javascript community since it relies of a set of coercion rules that many people find error-prone. This is why I tried to avoid using it. I was not aware of the exceptions described in the eslint documentation though. I will update the code and use ==.

IvanGoncharov · 2018-10-03T21:36:36Z

Please let me know if I forgot something.

@OlegIlyenko You also need to handle repeatable in extendSchema:

graphql-js/src/utilities/extendSchema.js

Lines 317 to 323 in 94b3844

    
           return new GraphQLDirective({ 
        
             name: directive.name, 
        
             description: directive.description, 
        
             locations: directive.locations, 
        
             args: extendArgs(directive.args), 
        
             astNode: directive.astNode, 
        
           });

Also remove of repeatable is breaking change so would be great to have it here:

graphql-js/src/utilities/findBreakingChanges.js

Line 96 in 94b3844

...findRemovedDirectiveLocations(oldSchema, newSchema),

IvanGoncharov · 2018-10-03T21:44:03Z

src/validation/rules/UniqueDirectivesPerLocation.js

-            knownDirectives[directiveName] = directive;
+            const repeatable = repeatableMap[directiveName];
+
+            if (repeatable === undefined || !repeatable) {


Up to discussion: I don't think unknown directives should be treated as non-repeatable. Unknown directives should trigger KnownDirectives and other rules shouldn't make any assumption about it.
Previously it was safe since all directives were non-repeatable.

It is a good point. After thinking about it, I think I would agree with you. In both scenations: directive is not known and it is known but repeatable, it should be excluded from this validation entirely. I will adjust the logic.

…ents

OlegIlyenko · 2018-10-04T12:32:04Z

@IvanGoncharov Thanks for pointing out 2 remaining places! I believe I addressed all review comments now.

IvanGoncharov · 2018-10-04T13:03:44Z

src/utilities/introspectionQuery.js

@@ -33,6 +33,7 @@ export function getIntrospectionQuery(options?: IntrospectionOptions): string {
          args {
            ...InputValue
          }
+          repeatable


@OlegIlyenko As discussed on the working group we can't assume that legacy servers would support repeatable. So this change will break GraphiQL and many other tools.

I would suggest making option directiveRepeatable similar to this one:

graphql-js/src/utilities/introspectionQuery.js

Lines 12 to 16 in f8378d0

export type IntrospectionOptions = {|

// Whether to include descriptions in the introspection result.

// Default: true

descriptions: boolean,

|};

Long term solution would be to implement two-stage introspection as suggested by Lee.

It's a good point, I added an option for it 3b5bf03 I also made if disabled by defult in order to maintain backwards compatibility.

Also this one: 3a7fafa I forgot to update the introspection flow type

based on 'expectOptionalKeyword' from graphql#1541

IvanGoncharov · 2018-10-04T13:15:49Z

src/utilities/introspectionQuery.js

@@ -273,4 +274,5 @@ export type IntrospectionDirective = {|
  +description?: ?string,
  +locations: $ReadOnlyArray<DirectiveLocationEnum>,
  +args: $ReadOnlyArray<IntrospectionInputValue>,
+  +repeatable: boolean,


@OlegIlyenko Can you please address this comment in spec RFC:
https://github.com/facebook/graphql/pull/472/files/1ba4e196aa710a10eceaa346d02a970dfd0b2d3a#diff-35fb422eded6e8f4df638c0d159008f6

It blocks this PR but I think we should discuss it in the scope of spec RFC.

Could you please describe in more detail what kind of information you would like to add? I'm not 100% sure I understood your comment.

@OlegIlyenko I mean if merge this PR that will expose repeatable through introspection and it would be significantly harder to rename it later.
As I wrote in my comment to the spec RFC I think this field should be named isRepeatable for consistency with isDeprecated.

Everything else in this PR looks good so would be great to discuss this last point but since this PR is just an implementation of spec RFC I think we should discuss it in spec PR.

I see, thanks for the clarification. Sure thing, I will open a discussion on this topic in the RFC PR.

@OlegIlyenko I feel very stupid because in the first comment of this thread I linked review comment I forget to submit (review comments UI is very weird).
I already submitted it in.

👍 i see, thanks! now it's clear, i was confused about wrong link

I did the renaming 56fee7d I will adjust the spec text accordingly

Oh I just saw this comment. Sorry for the late comment: I guess keeping consistent with isDeprecated makes sense, though it still feels a little weird. Oh well.

IvanGoncharov · 2018-10-04T21:07:53Z

@OlegIlyenko I reviewed it one more time and found only one small issue.
Rule description should be adjusted here:

graphql-js/src/validation/rules/UniqueDirectivesPerLocation.js

Lines 27 to 32 in 56fee7d

    
           /** 
        
            * Unique directive names per location 
        
            * 
        
            * A GraphQL document is only valid if all directives at a given location 
        
            * are uniquely named. 
        
            */

OlegIlyenko · 2018-10-04T21:20:11Z

@IvanGoncharov I updated the comment 0dff21f Thanks a lot for a fast and careful review! 🙇‍♂️

mjmahone · 2018-10-04T22:00:37Z

src/language/ast.js

@@ -508,6 +508,7 @@ export type DirectiveDefinitionNode = {
  +description?: StringValueNode,
  +name: NameNode,
  +arguments?: $ReadOnlyArray<InputValueDefinitionNode>,
+  +isRepeatable: boolean,


I wonder if this should be:

+repeatable?: boolean,

If someone never uses repeatable directives, this adds no extra weight to the node.

I also don't think we need the is prefix: the fact that it's a boolean is enough information, and the question repeatable? false is close-to-valid English as-is (just as understandable as is repeatable? false).

The is prefix primarily added for consistency with isDeprecated, as @IvanGoncharov pointed out https://github.com/facebook/graphql/pull/472/files/1ba4e196aa710a10eceaa346d02a970dfd0b2d3a#diff-35fb422eded6e8f4df638c0d159008f6

I think I would prefer it keep it mandatory. This makes it easier to work with and i feel that the performance overhead is negligible. But I can also make it optional.

WDYT?

@OlegIlyenko In the context of AST, I'm more in favor of @mjmahone proposal since we already have block with similar prototype:

graphql-js/src/language/ast.js

Line 320 in 0dff21f

+block?: boolean,

Plus it looks like JS AST doesn't use is prefix, e.g. async:
https://github.com/estree/estree/blob/df2290b23a652e1ec354b6775459a3b42e22f108/es2017.md#function

But outside of AST I strongly believe it should be named isRepeatable for the consistency with isDeprecated.

This makes it easier to work with and i feel that the performance overhead is negligible.

I'm with you on that point especially since JS parser always preserve boolean flags and they typically work with way bigger files than GraphQL.
Also defacto (in our parser) we always provide values for all keys so why should Flow typings mark that as optional.

That said I think it's outside of scope of this PR and should be disscussed for AST as a whole and not for individual flags.
So for now I think it should be optional.

I think both alternatives should be fine. I also considered keep repeatable name on AST nodes, but then renamed it with is prefix across the whole codebase for the sake of the naming consistency.

But I think it should totally fine to have repeatable field on AST node and then use isRepeatable everywhere else. @mjmahone What do you think?

I renamed isRepeatable back to repeatable in all places except introspection: 89f5780

mjmahone · 2018-10-04T22:08:05Z

src/validation/rules/UniqueDirectivesPerLocation.js

 * are uniquely named.
 */
 export function UniqueDirectivesPerLocation(
-  context: ASTValidationContext,
+  context: ValidationContext | SDLValidationContext,


This is technically a breaking change. It's probably OK as I doubt anyone is using this validation explicitly yet, but definitely need to note that in our release notes.

But I think there should be a way, today, to re-order our validation contexts to provide a Schema SDL if possible. I will probably end up forking all of these validation rules to run off an AST-only schema object, because building (and extending) the current GraphQLSchema object is too slow. I won't block this on not using the GraphQLSchema, but I'd like for us to move in that direction.

Do understand it correctly: you mean expressing all existing validations purely in terms of SDL context? Does this also imply that in case of normal query validation you would like to transform existing schema in SDL and then validate against it?

@mjmahone It's not a breaking change since ASTValidationContext is not a part of public API. As we agree during 14.0.0 SDL validation is an experimental feature so I didn't expose this and other new class as public API.
ATM only ValidationContext is exposed so if library client use only public API he shouldn't be affected by this change in any way.

mjmahone · 2018-10-04T22:14:04Z

src/utilities/introspectionQuery.js

@@ -13,10 +13,15 @@ export type IntrospectionOptions = {|
  // Whether to include descriptions in the introspection result.
  // Default: true
  descriptions: boolean,
+  // Whether to include `isRepeatable` flag on directives.
+  // Default: false


Why would this default to false?

For example, if it would be true, then after an update without any adjustments third-party libraries (like GraphiQL) would start to send queries with isRepeatable flag to the servers that potentially not updated yet. So default is primarily defined like this to maintain backwards compatibility.

Would you like to change the default and require all downstream libraries to explicitly set this flag? (at least at the initial stage where servers are not yet updated)

@mjmahone Do you remember we discussed it on the last WG in the scope of my
"schema description" RFC. And the consensus was to keep it backward compatible for now but design two stage introspection as a long-term solution.

# Conflicts: # src/language/printer.js

…introspection

mjmahone

This looks really good now! Thanks for pushing this PR forward @OlegIlyenko, this idea is in much better shape thanks to all the work you've put into it, and I am glad it's turned out to be a fairly surgically-precise PR to enable it.

The backwards-compatibility problem with schema introspection makes me think schema introspection isn't that useful: we probably can migrate the introspection query to actually just return a string of the schema text. But that's a question we can work towards fixing later.

a-type · 2019-05-20T14:50:03Z

Any movement on this? While it's not a blocker, it would improve syntax for a project I'm experimenting with.

IvanGoncharov · 2019-05-20T15:36:13Z

@a-type It's blocked by introspection changes that I plan to do this week:
https://docs.google.com/presentation/d/1ISS5QhF6D2ncD8ZGChoCv1LnOWj9jryZrJ2dG11i4Mc/edit?usp=sharing

Code is based on graphql#1541 but without introspection changes and without breaking change detection

Code is based on #1541 but without introspection changes and without breaking change detection

IvanGoncharov · 2019-06-09T15:51:37Z

@a-type I decided that we shouldn't fully block repeatable directives because of required introspection changes so I merged everything except blocked parts as #1965.

Code is taken from graphql#1541

Code is taken from #1541

Code is taken from graphql#1541

Code is taken from #1541

spawnia · 2020-04-13T19:21:11Z

@IvanGoncharov Is there anything left in this PR that you have not merged elsewhere? Asking because i am working on the PHP implementation just now.

IvanGoncharov · 2020-04-14T14:52:31Z

@spawnia Thanks for the ping 👍
Everything is already implemented in 15.0.0 release.
Closing.

[RFC] Added support for repeatable directives

cab37c7

facebook-github-bot added the CLA Signed label Oct 2, 2018

OlegIlyenko added 3 commits October 3, 2018 01:03

Updated with the most recent changes

7eb1fd7

Adjusted json formatting

8cd0e52

OlegIlyenko mentioned this pull request Oct 3, 2018

[RFC] Repeatable directives graphql/graphql-spec#472

Merged

IvanGoncharov added this to the v14.1.0 milestone Oct 3, 2018

IvanGoncharov reviewed Oct 3, 2018

View reviewed changes

OlegIlyenko added 4 commits October 4, 2018 13:51

Skip unknown directives + other improvements based on the review comm…

77f85b3

…ents

Test for unknown directives

20deb26

Added support for repeatable directives in extendSchema

1180d92

Added DIRECTIVE_REPEATABLE_REMOVED breaking change

f8378d0

IvanGoncharov reviewed Oct 4, 2018

View reviewed changes

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Oct 4, 2018

Parser: Add 'skipKeyword' function

59792e5

based on 'expectOptionalKeyword' from graphql#1541

IvanGoncharov mentioned this pull request Oct 4, 2018

Parser: Add 'skipKeyword' function #1545

Merged

IvanGoncharov reviewed Oct 4, 2018

View reviewed changes

OlegIlyenko added 2 commits October 4, 2018 17:50

Skip repeatable flag by default in the introspection.

3b5bf03

repeatable flag in the introspection results is now optional

3a7fafa

IvanGoncharov requested a review from mjmahone October 4, 2018 17:19

OlegIlyenko added 2 commits October 4, 2018 22:37

Use uniqueDirectiveMap instead of repeatableMap

ae9e7f9

Rename repeatable → isRepeatable

56fee7d

IvanGoncharov approved these changes Oct 4, 2018

View reviewed changes

Updated UniqueDirectivesPerLocation validation docs

0dff21f

mjmahone reviewed Oct 4, 2018

View reviewed changes

This was referenced Oct 6, 2018

Support for repeatable directives sangria-graphql/sangria#409

Closed

Support for repeatable directives sangria-graphql/sangria#412

Closed

OlegIlyenko added 2 commits November 6, 2018 17:04

Merge branch 'master' into repeatable-directives

1a07bd9

# Conflicts: # src/language/printer.js

Use name repeatable instead of isRepeatable in all places except …

89f5780

…introspection

mjmahone approved these changes Nov 7, 2018

View reviewed changes

IvanGoncharov removed this from the v14.1.0 milestone Jan 16, 2019

ravangen mentioned this pull request Jan 18, 2019

Do I have any ways to set two rules at once ? teamplanes/graphql-rate-limit#18

Closed

IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label Feb 5, 2019

jbaxleyiii mentioned this pull request Jun 7, 2019

[Federation] GraphQL spec compliance vs multiple keys apollographql/apollo-server#2793

Closed

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jun 9, 2019

Added partial support for repeatable directives

445439e

Code is based on graphql#1541 but without introspection changes and without breaking change detection

IvanGoncharov mentioned this pull request Jun 9, 2019

Added partial support for repeatable directives #1965

Merged

IvanGoncharov added a commit that referenced this pull request Jun 9, 2019

Added partial support for repeatable directives (#1965)

2c7224c

Code is based on #1541 but without introspection changes and without breaking change detection

IvanGoncharov force-pushed the master branch from e7509a6 to 16f5ca0 Compare June 26, 2019 17:05

yanns mentioned this pull request Nov 5, 2019

Support for repeatable directives sangria-graphql-org/sangria#5

Open

jdolle mentioned this pull request Nov 26, 2019

Add assumeValidSDL config option dotansimha/graphql-code-generator#3014

Closed

spawnia mentioned this pull request Jan 11, 2020

Support repeatable directives webonyx/graphql-php#608

Closed

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jan 30, 2020

findBreakingChanges: add check for removing repeatable from directive

0ad4c06

Code is taken from graphql#1541

IvanGoncharov mentioned this pull request Jan 30, 2020

findBreakingChanges: add check for removing repeatable from directive #2415

Merged

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jan 30, 2020

findBreakingChanges: add check for removing repeatable from directive

d28529e

Code is taken from graphql#1541

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jan 30, 2020

findBreakingChanges: add check for removing repeatable from directive

f506aec

Code is taken from graphql#1541

IvanGoncharov added a commit that referenced this pull request Jan 30, 2020

findBreakingChanges: add check for removing repeatable from dir… (#2415)

69c4a09

Code is taken from #1541

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jan 30, 2020

introspection: Add support for repeatable directives

4ee39b8

Code is taken from graphql#1541

IvanGoncharov mentioned this pull request Jan 30, 2020

introspection: Add support for repeatable directives #2416

Merged

IvanGoncharov added a commit that referenced this pull request Jan 30, 2020

introspection: Add support for repeatable directives (#2416)

cc4c363

Code is taken from #1541

IvanGoncharov closed this Apr 14, 2020

	export type IntrospectionOptions = {\|
	// Whether to include descriptions in the introspection result.
	// Default: true
	descriptions: boolean,
	\|};

[RFC] Added support for repeatable directives #1541

[RFC] Added support for repeatable directives #1541

Conversation

OlegIlyenko commented Oct 2, 2018

IvanGoncharov Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegIlyenko commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegIlyenko Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Oct 4, 2018

OlegIlyenko commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegIlyenko Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjmahone left a comment

Choose a reason for hiding this comment

a-type commented May 20, 2019

IvanGoncharov commented May 20, 2019

IvanGoncharov commented Jun 9, 2019

spawnia commented Apr 13, 2020 • edited Loading

IvanGoncharov commented Apr 14, 2020

IvanGoncharov Oct 3, 2018 •

edited

Loading

OlegIlyenko Oct 4, 2018 •

edited

Loading

OlegIlyenko Oct 4, 2018 •

edited

Loading

spawnia commented Apr 13, 2020 •

edited

Loading