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

[RFC] Repeatable directives #472

Merged
merged 9 commits into from
Jan 10, 2020
Merged

Conversation

OlegIlyenko
Copy link
Contributor

@OlegIlyenko OlegIlyenko commented Jun 24, 2018

This proposal directly relates to a discussion in #429. As was discussed at the latest WG meeting, I'm creating several alternative proposals. This one implements proposed solution 4. Limit the validation to only these directives that are explicitly marked as "unique". This implies that we need to introduce a new option in the directive definition.

It limits the scope of "Directives Are Unique Per Location" to directives that are explicitly marked as unique.

This proposal is mutually exclusive with other alternative proposals:

@leebyron @IvanGoncharov @jjergus I would appreciate your reviews.

Closes #429
Closes #471

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 25, 2018

@OlegIlyenko I need a couple more days to think about #472 vs #471 vs keep everything as is.
But here are a couple points after quick review:

  1. I don't like unique in front of the directive: unique directive @example on OBJECT | INTERFACE
    How about directive @example uniquely on OBJECT | INTERFACE?
  2. I think directives should be unique by default since it what make sense in most of the cases. If you want to support some advanced usage of your directive (chaining, accumulation, etc.) you should explicitly allow this. Can't come up with better English word so I will use nonUniquely as temporary keyword: directive @example nonUniquely on OBJECT | INTERFACE.

@OlegIlyenko
Copy link
Contributor Author

@IvanGoncharov thanks a lot for your feedback and for taking your time to review the PR!

These are good points. I think the reason I used unique directive ... syntax is because it makes presence of an additional constraint on directives explicit. For me personally, it is more intuitive to assume that without any explicitly stated information, the entity is unconstrained. SQL DDL is one example of this approach. As well as the GraphQL itself, where you need to put an explicit ! to add extra not-null constraint on the field.

That said, I also think that your suggestion has a good argument (default that represents the prevalent use-case). So I think I don't have a strong preference between these two alternatives. Maybe you have some good examples of non-standard directives that are widely used and either need to be unique or non-unique? (would be interesting to compare both cases). Also, you be interesting to hear opinions on this aspect from other reviewers.

Regarding unique directive @example vs directive @example uniquely. The second variation look a bit odd to me, so I think I would prefer the first one, but it's more personal opinion. Although with the syntax of the second one it might be easier to add additional constraints in future. So I guess this is a thing to consider.

Also in my experience, directives are very rarely authored in the SDL. Most of the time I worked with and saw them used in the host language. From this perspective most of the time directive-definitions-in-sdl show up in the rendered output of actual schema (in this case I would prefer more explicit unique directive ...). But maybe it's just me, I'm not sure about this point.

@IvanGoncharov
Copy link
Member

Maybe you have some good examples of non-standard directives that are widely used and either need to be unique or non-unique? (would be interesting to compare both cases).

@OlegIlyenko In majority of cases directives are used to attach additional metadata to the SDL entity like: @scopes, @cacheControl, etc.
So if you makes such directive non unique by default it could be breaking change for some implementations.

A few examples of unique directives from my own tools would be: @fake from graphql-faker and @_ from graphql-lodash.

I think the reason I used unique directive ... syntax is because it makes presence of an additional constraint on directives explicit.

If we decide to add unique prefix to any other top-level keyword it will require non-trivial changes to parser and make parser more complex overall.

For me personally, it is more intuitive to assume that without any explicitly stated information, the entity is unconstrained. SQL DDL is one example of this approach. As well as the GraphQL itself, where you need to put an explicit ! to add extra not-null constraint on the field.

In case of describing constrains on data I totally agree with you that everything should be unconstrained by default.
But directives are intended as GraphQL language extensions and in language specification keywords/constructs are unique by default.

Also in my experience, directives are very rarely authored in the SDL.

Not sure about other language but in JS it looks like defining directives in SDL is quite popular approach:
https://www.apollographql.com/docs/graphql-tools/schema-directives.html#Uppercasing-strings
A few more examples from my own source code:
https://github.com/APIs-guru/graphql-faker/blob/master/src/fake_definition.graphql#L192
https://github.com/APIs-guru/graphql-lodash/blob/master/src/lodash_idl.ts#L101

@mjmahone
Copy link
Contributor

I think a near-ideal English word would be repeatable: basically, you are allowed to repeat the directive at the same location. I still don't think that's a perfect word, but I think it's better than nonUnique.

I also wonder if, instead of adding a language keyword, we should instead allow directives on directive definitions. Then you'd have something like:

directive @repeatable on DIRECTIVE
directive @my_skip(if: Boolean) @repeatable on FIELD

I agree with @IvanGoncharov that by default, most directives will be unique per location. A couple of potential directive examples:

  • @ignore_validation: basically, if validation on this fails, keep going. This one should probably not be repeatable.
  • @ignore_validation_rules(rules_to_ignore: [ValidationRuleName]): this could go either way, but I think most implementers of this would first make it non-repeatable/unique, as it's easier to implement if it's more rather than less strict. Then once the need for it to be repeatable is clear, it can be "upgraded".

As I think it'll be easier to implement unique directives, I suspect it's better if by default the implementer doesn't have to worry about merging them. I'm not totally sure we'll ever get to a set of 100% consistent directive-merging rules that work for all use cases. But if we did this, it would be easy if we decided, say, that @skip

@IvanGoncharov
Copy link
Member

@mjmahone I like repeatable 👍

directive @example repeatable on OBJECT | INTERFACE

I also wonder if, instead of adding a language keyword, we should instead allow directives on directive definitions

It creates one more source of recursion in GraphQL schema and requires to add additional validation rules.
I think to keep specification simple and straightforward we need to add such features only based on real-life use cases.

As for @repeatable in particular, I think it better suited for keyword because:

  1. it doesn't accept arguments
  2. in the position that you propose it will look strange for directives without arguments directive @test @repeatable on QUERY
  3. it only applies to directive definitions and that means @repetable became reserved for all other locations.

This why I think @deprecate make sense as a directive but it better to specify repeatable as a keyword.

@mjmahone
Copy link
Contributor

@IvanGoncharov those arguments are pretty convincing to me. It does end up potentially opening up a class of "things we want to add to directive definitions", and this argument basically means this entire class cannot be experimentally added via directives first, but instead must be added to the language. I can't immediately think of another example that lives in this class though, so maybe this is indeed the only thing in the entire class of "additions to directive definitions" we would want to add.

@OlegIlyenko
Copy link
Contributor Author

@mjmahone I also like repeatable, it looks and sounds good in SDL. I also don't have a strong opinion on the default. If we have a strong consensus on non-repeatable default, then let's do it. Also, if most of that directives that people are using in practice are non-repeatable, then I think it is indeed a very good idea to represent it as the SDL default.

I also wonder if, instead of adding a language keyword, we should instead allow directives on directive definitions

I thought about this as well, but I think I would agree with @IvanGoncharov's arguments. I feel that the syntax will get quite confusing in SDL. ATM I also can't think of other use-cases for directives-on-directives.

@leebyron leebyron added 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) and removed RFC labels Oct 2, 2018
@leebyron
Copy link
Collaborator

leebyron commented Oct 2, 2018

To follow up from the last WG session: a repeatable rather than unique new keyword, where the absence of repeatable means "unique" is the way to go because that leaves all existing directive definitions with unchanged semantics, which makes this a non-breaking change.

Next steps are to revise the spec change and create a GraphQL.js PR to ensure there are no hidden issues with implementation!

@OlegIlyenko
Copy link
Contributor Author

@leebyron I updated the spec text with repeatable instead of unique syntax and the default: 1ba4e19

I'll try to followup with a graphql-js PR soon.

@OlegIlyenko
Copy link
Contributor Author

OlegIlyenko commented Oct 3, 2018

FYI: I just implemented it for the reference implementation: graphql/graphql-js#1541

@@ -417,3 +418,4 @@ Fields
locations this directive may be placed.
* `args` returns a List of `__InputValue` representing the arguments this
directive accepts.
* `repeatable` must return a Boolean which permits using the directive multiple times at the same location.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already have isDeprecated as boolean predicate I think we should follow this naming convention and name it isRepeatable.
As a bonus, it would be immediately clear that it's boolean value and not a something else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with both alternatives. Though, I also find consistency argument quite compelling.

I renamed it for now 5c8c4bd, but I can chnage it later if somebody raises a concern.

I wonder what others think about it?

```graphql example
directive @example repeatable on OBJECT | INTERFACE
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to include a more complex example that illustrates how repeatable is useful or at least add a note that directive order is significant and you can use it to create chains of directives.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extended the example 218e0da and the description 477919e.

I struggled with it a bit since I find it hard to strike the right balance between completeness of the example including its description and succinct focused content required from a perspective of the specification. For this reason I also hesitated mentioning the order significance since it has its own dedication section with a separate example.

WDYT?

@OlegIlyenko
Copy link
Contributor Author

BTW, I tried repeatable directives in one of my projects:

https://github.com/OlegIlyenko/graphql-gateway/blob/master/testSchema.graphql#L6-L15

It looks so much better now <3 In comparison to the previous version

https://github.com/OlegIlyenko/graphql-gateway/blob/4f0b3e1218f5a8fb706854ac2acfcfcc7147c861/testSchema.graphql#L6-L25

Now I also can distribute @incluideSchema and @includeFields between type/schema and type/schema extensions. It works quite well.

@michaelstaib
Copy link
Member

Directive may be defined as repeatable per location with the repeatable

does that mean a directive can be repeatable on an object but must be unique on a field?

@OlegIlyenko
Copy link
Contributor Author

OlegIlyenko commented Oct 8, 2018

@michaelstaib no, in a current form "repeatable" is a property of a directive regardless of the location. For example:

directive @example repeatable on OBJECT | FIELD_DEFINITION

@example directive is allowed be repeated on both object type definitions as well as field definitions.

@michaelstaib
Copy link
Member

But than the text should be changed because may be defined as repeatable per location with the sounds like you could do something like the following:

directive @example repeatable on OBJECT | unique on INTERFACE

I know that this example is non-sense but it sounded to me like that was implied.

@@ -1660,6 +1660,21 @@ fragment SomeFragment on SomeType {
}
```

A directive may be defined as repeatable at any permitted location with the `repeatable`
keyword. Repeatable directives often useful when the same directive should be used with
different arguments at the one location, especially in cases where additional information
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
different arguments at the one location, especially in cases where additional information
different arguments at a single location, especially in cases where additional information

A directive may be defined as repeatable at any permitted location with the `repeatable`
keyword. Repeatable directives often useful when the same directive should be used with
different arguments at the one location, especially in cases where additional information
need to be provided in a form of directive via a type or a schema extension:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
need to be provided in a form of directive via a type or a schema extension:
needs to be provided to a type or schema extension through a directive:

@@ -417,3 +418,4 @@ Fields
locations this directive may be placed.
* `args` returns a List of `__InputValue` representing the arguments this
directive accepts.
* `isRepeatable` must return a Boolean which permits using the directive multiple times at the same location.
Copy link
Member

@spawnia spawnia Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `isRepeatable` must return a Boolean which permits using the directive multiple times at the same location.
* `isRepeatable` must return a Boolean that indicates if a directive may be used repeatedly at a single location.

@@ -1440,7 +1440,7 @@ query @skip(if: $foo) {
**Formal Specification**

* For every {location} in the document for which Directives can apply:
* Let {directives} be the set of Directives which apply to {location}.
* Let {directives} be the set of Directives which apply to {location} and not marked as `repeatable`.
Copy link
Member

@spawnia spawnia Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Let {directives} be the set of Directives which apply to {location} and not marked as `repeatable`.
* Let {directives} be the set of Directives which apply to {location} and are not `repeatable`.

@spawnia
Copy link
Member

spawnia commented Oct 11, 2019

@leebyron @IvanGoncharov the title of this PR has become misleading as the change evolved to repeatable directives. Can you change the title or should we make a new PR that sums it up?

@leebyron leebyron changed the title [RFC] Limit directive uniqueness to explicitly marked directives [RFC] Repeatable directives Jan 10, 2020
Co-authored-by: Benedikt Franke <benedikt@franke.tech>
@leebyron leebyron added 🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md) and removed 📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md) labels Jan 10, 2020
@leebyron leebyron merged commit be33a64 into graphql:master Jan 10, 2020
@leebyron leebyron added this to the May2021 milestone Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limiting the scope of "Directives Are Unique Per Location" validation
7 participants