Adhere to specification for schema extension

[niodice]

Looking into the GraphQL specification, I believe that schemas for federated graphs are being rendered in a slightly incorrect way. The specification states

A schema defines the initial root operation type for each kind of operation it supports: query, mutation, and subscription; this determines the place in the type system where those operations begin.

The query root operation type must be provided and must be an Object type.

What is important is that the final federated GraphQL schema obeys ☝️. However, subgraphs need not implement the full schema specification. Subgraphs which do not meet this criteria should use the extend keyword

Schema extensions are used to represent a schema which has been extended from an original schema. For example, this might be used by a GraphQL service which adds additional operation types, or additional directives to an existing schema.

I believe that this change more closely adheres to the correct specification.

[niodice]

@ghostdogpr hoping for some feedback here. Somebody more knowledgeable about the GQL specification noted that there are actually 2 conditions that we should check before considering the schema an extended schema:

1. !hasTypes && directives.nonEmpty
2. query.isEmpty && (mutation.nonEmpty || subscription.nonEmpty)
   • This is a valid check if the subgraph is a federated schema, but if it's just a standalone GQL schema, then I believe the schema is technically invalid, even if Caliban will generate a schema for the graph.

We can implement those rules, but developers may rely on the fact that caliban produces a non-extended schema for case (2) above. In either case, this change may be breaking.

I'm going to update the logic here to reflect the above checks, you can see my first attempt by looking at the commit history if you'd like

[ghostdogpr]

Pinging our expert in subgraphs @paulpdaniels to confirm

[kyri-petrou]

Based on your comment, shouldn't this be:

Suggested change

	(!hasTypes && directives.nonEmpty) || query.isEmpty && (mutation.nonEmpty || subscription.nonEmpty)
	(!hasTypes && directives.nonEmpty) || (query.isEmpty && (mutation.nonEmpty || subscription.nonEmpty))

[niodice]

whoops, I thought I had done it and the formatter stripped it, but I must have made a mistake. Fixed

[kyri-petrou]

I'm not sure I fully understand the reasoning behind (2) 🤔. Can you provide an example of a valid schema extension (in .graphql format) that would be considered an extension under the new rules but wouldn't before?

[paulpdaniels]

Is checking for a present Query part of our current validation step? If not then we may already be allowing invalid graphs to be generated.

[niodice]

I'm not sure I fully understand the reasoning behind (2) 🤔. Can you provide an example of a valid schema extension (in .graphql format) that would be considered an extension under the new rules but wouldn't before?

yes - this would be essentially any subgraph that has (1) only a mutation, (2) only a subscription, or (3) a mutation + subscription but no query. E.g:

"Directive which holds a freeform tag applied to an element."
directive @tag("The name of the tag to apply."
name: String!) repeatable on SCALAR | OBJECT | FIELD_DEFINITION | ARGUMENT_DEFINITION | INTERFACE | UNION | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION

extend schema @link(url: "https://specs.apollo.dev/federation/v2.6", import: ["@key", "@requires", "@provides", "@external", "@shareable", "@tag", "@inaccessible", "@override", "@extends", "@composeDirective", "@interfaceObject", "@authenticated", "@requiresScopes", "@policy"]) {
  mutation: Mutations
}

type Mutations {
  mutateFoo(fooId: String!): String @tag(name: "fooTag")
}

[niodice]

Wanted to circle back here to see if anyone can take a look at this change?

[ghostdogpr]

@kyri-petrou @paulpdaniels you guys can take another look?

[paulpdaniels]

Technically speaking I don't have any issues with this. My question above is, are we treating rendered vs interpreted schemas different? and are we ok with that?

For instance if we called interpreter instead of render are we returning a validation error in that case?

[ghostdogpr]

We have a validation rule validateRootQuery that checks that the root query is defined and fail otherwise. Federation seems to always have it because there is a Query defined with a _service field. So the case mentioned above with no Query would never happen?

[kyri-petrou]

I guess I'm OK with this change. I would have preferred if we were able to determine whether this is a schema extension a bit more explicitly (rather than basing it on the absence of the query field in the schema), but if this is the best that we can do then that's fine with me.