graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.13k forks source link

add __directive meta field parallel to __type #1114

Open yaacovCR opened 2 months ago

yaacovCR commented 2 months ago

Use case: inquiring about specific directives such as @defer and @stream

netlify[bot] commented 2 months ago

Deploy Preview for graphql-spec-draft ready!

Name Link
Latest commit 2196942b3015f819d858ecff77ababda2fd7748a
Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/66f401a944072500083a7389
Deploy Preview https://deploy-preview-1114--graphql-spec-draft.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

yaacovCR commented 2 months ago

Implemented by https://github.com/graphql/graphql-js/pull/4203

benjie commented 2 weeks ago

I've not reviewed if these spec changes are sufficient, but I'm supportive of the aim :+1:

yaacovCR commented 2 weeks ago

This was discussed at the November 2024 WG => this small change prompted an interesting discussion about "The Future of Introspection."

To my understanding, @leebyron 's feedback:

Perhaps __type should have always been nested as a type argument under __schema, so that similarly __directive should be nested as directive under __schema.

This general change might unlock general __schema arguments like:

  __schema(includeDeprecated: true) { ... }

that would apply to all nested fields.

As this change is not a "must-have" it will probably be paused to make sure it fits into whatever introspection future is planned. We do not have to actually implement that entire new future to move forward with this small feature change, but we have to make sure that this smaller change aligns with the plan.

benjie commented 4 days ago

I'm generally in favour of Lee's proposal; but I do have one concern that I wanted to think about: unless we're careful the way in which we execute this may set a bad precedent of passing down implicit values.

I've written about the problem of referencing ancestors before, but essentially if you can think of anything under __Schema as being an identifiable node in a graph then implicitly passing down settings like isDeprecated through the tree would break normalized caching. Take the following query for example:

{
  a: __schema(includeDeprecated: true) {
    type(name: "MyType") { name fields { name } }
  }
  b: __schema(includeDeprecated: false) {
    type(name: "MyType") { name fields { name } }
  }
}

here the objects a.type and b.type represent the same type (MyType):

flowchart TD
  Query --> a["__schema(includeDeprecated: true)"]
  Query --> b["__schema(includeDeprecated: false)"]
  a --> MyType
  b --> MyType

So when we access fields (with no arguments) we'd expect the same results:

flowchart TD
  Query --> a["__schema(includeDeprecated: true)"]
  Query --> b["__schema(includeDeprecated: false)"]
  a --> MyType
  b --> MyType
  MyType --> fields
  classDef hide fill:#fff,stroke:#aaa,color:#aaa;
  class Query,a,b,MyType hide;

Contradiction: but a.type.fields and b.type.fields are desired to be different if some fields are deprecated!


Despite raising this, I'd argue that this is in fact not an issue for the proposed changes, since none of the entries below __schema are identifiable nodes in the graph - requesting __schema(includeDeprecated: false) would actually yield a different schema with deprecated entities omitted (let's call it __Schema*), so the types returned from a.type and b.type are concretely different types - let's call them MyType and MyType*:

flowchart TD
  Query --> a["__Schema"]
  Query --> b["__Schema*"]
  a --> MyTypeA["MyType"]
  b --> MyTypeB["MyType*"]
  MyTypeA --> fieldsA["fields"]
  MyTypeB --> fieldsB["fields*"]

Importantly __Type does not have a reliably identifier. You might think __Type.name is an identifier, but it does not identify wrapper types (list/non-null have null name) so it isn't. Similarly no other nodes under __Schema have reliable globally unique identifiers (__Field has name but it is not unique - multiple types may define a field with the same name; etc). Therefore normalized caches are safe.

The caveat here is that we must be willing to commit to this always being true before we adopt this change.

martinbonnin commented 2 days ago

Rereading the wg notes:

Right now it’s possible to have an inconsistent schema by passing different includeDeprecated values

Trying to list the cases where this may happen, the only place I found is if an input value has a default value that is a deprecated enum value that was not fetched. Are there others?

This has the potential to become much more of an issue with deprecated object types but as it is now it's okay-ish?

All in all, I agree with @benjie's point that this is setting a precedent about accessing ancestors that might not be worth the tradeoff.

Personnal anecdote: I have been tempted to do something similar to https://github.com/graphql/graphql-spec/issues/144 in the Confetti API. The idea was to have a top-level conference(conferenceId) field that would filter out the content per-conference and that would effectively break normalized caching as Benjie described in his ancestors article. In retrospect, I'm glad I didn't do it. But were this pattern used in more places, I might have bought into it without really realizing all the consequences...

Another question (that deviates quite a bunch from the original issue): do we really need includeDeprecated? Is saving a few bytes really worth the complexity here? Put it otherwise: could we deprecated includeDeprecated πŸ€ͺ ?