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

[OlegIlyenko]

According to the spec, "Directives Are Unique Per Location" validation is defined like this:

Directives are used to describe some metadata or behavioral change on the definition they apply to. When more than one directive of the same name is used, the expected metadata or behavior becomes ambiguous, therefore only one of each directive is allowed per location.

In the light of recent GraphQL spec changes (extensions on all types and the schema itself) and other developments in the community, I would like to suggest to reconsider the inclusions of this validation rule or limiting it's scope to a subset of directives.

I would like to show 2 cases where it might be beneficial to allow non-unique directives.

Example 1: adding more information

In this scenario, directives might add more information. The order of directives is insignificant.

Considering the schema delegation case where multiple GraphQL schemas are merged into one. I can define the config like this (with the current limitation):

schema
  @includeGraphQL(schemas: [{
    name: "schema1"
    url: "http://some.server/graphql"
  }, {
    name: "schema2"
    url: "http://another.server/graphql"
  }]) {

  query: Query
}

If the limitation is lifted, I can potentially split the schema in 2 files like this:

# file 1
schema
  @includeGraphQL(
    name: "schema1", 
    url: "http://some.server/graphql") {
  query: Query
}

# file 2
extend schema
  @includeGraphQL(
    name: "schema2", 
    url: "http://another.server/graphql")

This provides more modularity (and, I would argue, simplicity), even in absence of an extension:

schema
  @includeGraphQL(
    name: "schema1", 
    url: "http://some.server/graphql") 
  @includeGraphQL(
    name: "schema2", 
    url: "http://another.server/graphql") {
  query: Query
}

Example 2: chaining directives/data transformation

In this scenario, I would like to transform the data in a chain. A small subset of directives might represent reusable logic for a data retrieval and transformation. In this case the ordering the directives is significant.

For example, if I have a set of directives to retrieve data from an external HTTP service, I can define a field like this:

type Query {
  users: [User]
    @httpGet(url: "...")
    @jsonPath(path: "$.results")
    @enrichData
    @jsonPath(path: "$.profile")
}

Possible solutions

We can tackle this in a number of ways:

1. Remove the validation altogether. In this case, we need to reconsider what to do with ambiguous @skip, @include and potentially other custom directives
2. Limit the validation to only @skip and @include directives
3. 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.
4. Leave "Directives Are Unique Per Location" validation as-is

I would really appreciate your opinions and comments on this suggestion.

[xuorig]

Awesome! We've talked about this before @OlegIlyenko and this is something we've been bit by recently too.

Although it's often possible to work around by having plural arguments (like in your example) i definitely think its more elegant to sometimes used multiple calls to a same directives like in your example:

schema
  @includeGraphQL(
    name: "schema1", 
    url: "http://some.server/graphql") 
  @includeGraphQL(
    name: "schema2", 
    url: "http://another.server/graphql") {
  query: Query
}

Should we have a limitation as far as argument values go? Would using the same directive more than once with the same argument be valid?

As far as @skip and @include go, its definitely tricky.

Note: Neither @Skip nor @include has precedence over the other. In the case that both the @Skip and @include directives are provided in on the same the field or fragment, it must be queried only if the @Skip condition is false and the @include condition is true. Stated conversely, the field or fragment must not be queried if either the @Skip condition is true or the @include condition is false.

Could we maybe use the same kind of logic for repeated skips and includes? If there are multiple skips then it would be evaluated as skip1 && skip2 && skip3, food for 💭.

Thanks for opening this!

Edit:

As an opposite point of view, I also kind of agree with @stubailo's comment and @leebyron's answer here: #229 (comment)

I do like how currently there are less chances of "misusing" directives since the schema lets you know what's possible.

[dschafer]

Wasn't able to make the WG meeting yesterday, but two question I had around this had to do with how this would interact with field collection from fragments; in particular, when that creates trickiness around duplication and ordering. For example:

# Assume Page and User both implement Profile
fragment F on Profile {
  name @A
  ... on User {
    name @B
  }
  ...H
  name @C
  ...G
  ... on Page {
    name @D
  }
}

fragment G on User {
  ...H
  name @E
}

fragment H on Profile {
  name @F
}

demonstrates some of the weird cases. If we apply this to a Page, then it would "collect" to { name @A @C @F @D } in that order I suppose... but that's certainly not obvious from looking at it. Even trickier is the case where it's applied to a User, since we bring in H twice, so we could:

• take both - {name @A @B @F @C @F @E }
• take the first - {name @A @B @F @C @E }
• take the last - {name @A @B @C @F @E }

which could potentially make the behavior of "ordering-significant" directives surprising.

[OlegIlyenko]

We discussed this issue at GraphQL WG Meeting #5. I saw a strong consensus on moving forward and continuing the work on this issue.

I would like to outline the list of suggested options with some updates:

1. Remove the validation altogether. In this case, we need to reconsider what to do with ambiguous @skip, @include and potentially other custom directives
2. Remove the validation, but still disallow exact duplicates at the same location. For example, {name @skip(if: true) @skip(if: false)} passes the validation, but {name @skip(if: true) @skip(if: true)} does not.
3. Limit the validation to only @skip and @include directives
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.
5. Limit the validation to only specific directive locations. For example, we can keep the validation for the query-side directives (like @skip and @include), but disable it for the SDL-side directives.
6. Leave "Directives Are Unique Per Location" validation as-is

As @xuorig pointed out, the validation was introduced in this PR: #229.

One concern is that whatever we decide, we need to make sure that the directive behavior is easy to understand and consistent across different parts of the GraphQL spec.

Syntactic location vs semantic interpretation

Even if we decide on the last option and don't change the validation, I believe that we still have an issue. For example, if we consider that type Book represents a specific unique domain concept, then I believe that on a semantic level following 2 alternatives should be considered equivalents:

# 1
type Book @foo(id: 1) @foo(id: 2) {
  title: String!
  pages: Int!
}

and

# 2
type Book @foo(id: 1)  {
  title: String!
}

extend type Book @foo(id: 2) {
  pages: Int!
}

As validation is defined right now, the alternative 1 fails, but the alternative 2 succeeds. In my opinion, this behavior should be more consistent and consider the semantic interpretation of the type and schema extensions.

This also extends to the query-side directives. @dschafer Thanks a lot for pointing this out! Initially, I haven't considered it in this context, but I believe that this is something that needs to be addressed as well. Even this simple query passes the validation:

{
  hello @skip(if: true)
  hello @skip(if: false)
}

After testing it against the reference implementation, the field is present in the response (so the @skip directives are interpreted with a logical or). As far as I can tell, avoiding this kind of unclear interaction between directives was the original motivation for introducing this validation.

Potentially breaking change

@leebyron pointed out that this is potentially breaking change since GraphQL libraries might rely on the fact that directive name is unique at a specific AST node. For example, a GraphQL library might internally represent the list of directives as a hash map.

This also raises another question: is the ordering of directives significant? I'm not 100% sure, but I believe that at the spec does not explicitly defines this. On an AST level directives are written in a specific order, but is it ok for a specific GraphQL implementation or library to rely on particular ordering of the directives? I think parallels can be made with a JSON object field ordering. JSON spec defines the object as:

An object is an unordered set of name/value pairs.

Still, GraphQL spec explicitly defines the semantic interpretation of the JSON object fields in the response (it reflects the field ordering in the GraphQL query).

@jjergus you were involved in the introduction on the validation rule. I would really appreciate your feedback and opinion on this issue.

[OlegIlyenko]

Just to express my personal preference: I think I would prefer the the first option where we remove the validation altogether. I think it also might be a good option to define a new validation specific to the interaction between @skip and @include directives. This validation might not only disallow duplicates, but it can also make @skip and @include mutually exclusive.

I see the directive semantics as a domain concern. So the interpretation of the directive semantics, uniqueness at a particular location, ordering, etc. should be done by the application that defines these directives. @skip and @include directives are defined by the GraphQL spec itself, so I think it is ok to define a specific validation rule for interaction between these 2 directives that ensures the domain constraint. But I don't think that we should generalize this specific semantics and apply it an all other possible user-defined directives.

[jjergus]

I agree, I like option 1 or 4.

In this case, we need to reconsider what to do with ambiguous @skip, @include

I would suggest that we define the behavior of multiple @skip/@include directives:

some_field @skip(if: $x) @skip(if: $y) @include(if: $z)

to be the same as if the field was queried multiple times with one directive each time:

some_field @skip(if: $x)
some_field @skip(if: $y)
some_field @include(if: $z)

This already has well defined behavior per the GraphQL spec (we include the field in the response if at leasts one of the instances would have us do so).

and potentially other custom directives.

Yeah, this part is kinda sad. This means that everytime anyone declares any directive in the future, they will have to specify the correct behavior for multiple instances. Inevitably, someone somewhere will forget to do this for some new directive, resulting in ambiguity.

Because of this, I also like your option 4 -- where directives would be (non-)unique unless explicitly marked as the opposite (I'm not sure what a better default would be -- probably unique by default?).

@jjergus you were involved in the introduction on the validation rule. I would really appreciate your feedback and opinion on this issue.

I suggested this validation rule because it was the simplest way to resolve the ambiguity that existed at the time -- not because I had a strong preference towards that solution -- so I am totally fine with changing it (as long as the spec is updated such that there is no ambiguity).

For example, a GraphQL library might internally represent the list of directives as a hash map.

True story.

This also raises another question: is the ordering of directives significant?

Same question already exists for field arguments, or for multiple non-identical directives.

[OlegIlyenko]

@jjergus thanks a lot for a quick reply and your input!

I like your solution as well (1 without an extra validation). I think I personally also fine with 4. Though in this case I think we need to address the question of uniqueness semantics (whether type & schema extensions as well as things like fragments should be considered).

Regarding the hashmap directive representation. If the first option would be picked, do you think it would a big issue for the applications your are working on? If you are relying on the uniqueness of the directive at the specific location, do you think it would be possible/reasonable to re-introduce this constraint as an application/company-level convention or validation?

[IvanGoncharov]

As validation is defined right now, the alternative 1 fails, but the alternative 2 succeeds. In my opinion, this behavior should be more consistent and consider the semantic interpretation of the type and schema extensions.

@OlegIlyenko According to current spec this should fail:

# 2
type Book @foo(id: 1)  {
  title: String!
}

extend type Book @foo(id: 2) {
  pages: Int!
}

Since directive location for both defintions is OBJECT.
Moreover, it's duplicated here:
http://facebook.github.io/graphql/draft/#example-605bd

Any directives provided must not already apply to the original Object type.

I actually think that 1-5 creates a problem in this example since the reader doesn't know if @foo is overridden or accumulated.

[jjergus]

Regarding the hashmap directive representation. If the first option would be picked, do you think it would a big issue for the applications your are working on? If you are relying on the uniqueness of the directive at the specific location, do you think it would be possible/reasonable to re-introduce this constraint as an application/company-level convention or validation?

Shouldn't be a problem. We could easily implement a solution that doesn't use a hashmap, or if that results in a performance regression, then add it back as a local validation rule specific to our codebases (we already have a few of those, as well as custom directives, client-side transforms, etc.)

[OlegIlyenko]

@IvanGoncharov you are totally right! I missed this part in the spec. Though I checked it against makeExecutableSchema from apollo-tools and buildSchema (just double-check these) and it does not look like they implement this validation. I also just tested it against sangria and it is indeed fails in the first scenario but successful in the second.

I guess this is something to implement :) I'm glad that spec considers this aspect for the type extensions, thanks for pointing this out 👍

I also checked the extend schema and it does not look like this constraint (Any directives provided must not already apply to the original *.) is defined for it. I guess this is something to consider depending on the outcome of the discussion on this issue.

I actually think that 1-5 creates a problem in this example since the reader doesn't know if @foo is overridden or accumulated.

Indeed. I think the main idea is that the directive itself defines the semantics. This flexibility has a danger where one directive overrides another one with the same name. But it also enables scenarios like the one I have shown in the "Example 1: adding more information" especially if extensions are spread in different files (it not only limited to schema definitions, are also have use-cases where this is useful for the type definitions).

That said, I also wonder whether the name of the directive is a sufficient criteria for ensuring the domain constraint. @skip and @include both override each others behaviour even though they have different names. I also think this is quite common when it comes to the interaction between various user-defined directives.

[IvanGoncharov]

Wasn't able to make the WG meeting yesterday

@dschafer We would be discussing it on WG tomorrow. If you have a time would be great if you join in.

but two question I had around this had to do with how this would interact with field collection from fragments; in particular, when that creates trickiness around duplication and ordering. For example:

I reviewed your example more closely and I think it presents a real problem but it's unrelated to Directives Are Unique Per Location rule.
Specification explicitly addresses this here:
http://facebook.github.io/graphql/June2018/#example-c5ee9

However the following example is valid because @skip has been used only once per location, despite being used twice in the query and on the same named field:

query ($foo: Boolean = true, $bar: Boolean = false) {
  field @skip(if: $foo) {
    subfieldA
  }
  field @skip(if: $bar) {
    subfieldB
  }
}

The same applies to your examples with fragments and spreads so it's also valid according to Directives Are Unique Per Location rule and according to the current version of the spec.
I'm not sure how to correctly fix this particular use case but I think it should be solved in the scope of Field Selection Merging rule:
http://facebook.github.io/graphql/June2018/#sec-Field-Selection-Merging

[IvanGoncharov]

@OlegIlyenko I think this issue uncovered some obscurity in the current version of the spec:

What is the directive location?

Is scalar Test and extend scalar Test the same location or not?
Is location just a synonym for AST Node or it's resulting entity (scalar type in this example)?

Are order of directives significant or not?

Argument are unordered. Input object fields are unordered.
Should directives also be unordered?

If we decide that directive are ordered how should we map it to type extensions? For example:

extend scalar Test @foo
scalar Test @bar

Is it equivalent to scalar Test @foo @bar or to scalar Test @bar @foo?

---

I think we need to open separate issues for this two questions.
@OlegIlyenko What do you think?