[Federation] GraphQL spec compliance vs multiple keys

[ivome]

I have been playing with Apollo Federation and it seems like a really great architecture for developing complex applications!
I am a little hesitant to add this to my GraphQL servers as it is not fully GraphQL Spec compliant:

The GraphQL specification is pretty clear about the requirement that directives have to be unique per location:

https://graphql.github.io/graphql-spec/draft/#sec-Directives-Are-Unique-Per-Location

When adding multiple keys to a type, this seems to violate this requirement:

type Product @key(fields: "upc") @key(fields: "sku") {
  upc: UPC!
  sku: SKU!
  name: String
}

As this is explicitly mentioned in the docs, I assume this is intentional and there is some reasoning behind this?
Why not make it spec compliant with a directive definition like this:

directive @key(fields: [_FieldSet!]!) on OBJECT

I am concerned this might otherwise clash with other GraphQL tooling further down the line that follows the GraphQL specification.

[jbaxleyiii]

Hey @ivome! Thanks for the feedback!

To kick things off, repeatable directives is in the process of adoption in the spec (graphql/graphql-js#1541) and we plan on picking up the championing of it!

We went with two directives because the current fields argument isn't the only long term feature we see adding to keys. In having an array of fields, we limit ourselves to the ability to iterate and add additional value to each primary key. I personally also think the array is less clear about the delineation between primary key values vs the multiple directives

Hope that helps!

[ivome]

@jbaxleyiii thanks for your reply! This is great news, I wasn't aware of the change in the spec.

Do you know if this is a thing that is definitely going to be added to the spec or is this still up for debate? I've been wanting to use the same directive multiple times at the same location for other purposes, so thanks for championing the issue!

I agree with you on the other points... I was just too lazy to type another input type in the example 😄 The solution with multiple directives is definitely nicer!

[jbaxleyiii]

@ivome it should end up in the final spec since the reference implementation now supports it!