Fix ambiguity around when schema definition may be omitted

[benjie]

@rivantsov Pointed out in #978 that there's some ambiguity around when the schema keyword can be omitted from the SDL. Upon careful reading I've noticed that there is additional ambiguity around this topic.

While any type can be the root operation type for a GraphQL operation, the type
system definition language can omit the schema definition when the {query},
{mutation}, and {subscription} root types are named {"Query"}, {"Mutation"},
and {"Subscription"} respectively.

This seems to imply that all the root types are required in order to omit the schema definition. I've modified the text to indicate that the names only need to match for the root types that are actually present.

Likewise, when representing a GraphQL schema using the type system definition
language, a schema definition should be omitted if it only uses the default root
operation type names.

Imagine we're doing biological research, tracking mutations in a virus. We might have a schema like:

type Query {
  viruses: [Virus!]
}
type Virus {
  name: String!
  knownMutations: [Mutation!]!
}
type Mutation {
  name: String!
  geneSequence: String!
}
schema {
  query: Query
}

In this case we must not omit the schema definition when representing the schema using the SDL, because doing so would make it seem that the Mutation type was the root type for the mutation operation, when in fact the schema does not support a mutation operation.

I've clarified the wording to deal with this possibility too.

[netlify]

✅ Deploy Preview for graphql-spec-draft ready!

Name	Link
🔨 Latest commit	058b4ec
🔍 Latest deploy log	https://app.netlify.com/sites/graphql-spec-draft/deploys/63e5482961e4c30008f85266
😎 Deploy Preview	https://deploy-preview-987--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[benjie]

I've submitted a larger edit that I think improves accuracy and makes reading (and referencing) easier by using a definition.

[benjie]

@leebyron "default root type name" read better in context than "default operation type name" so I've gone with that. If you approve, I think this is ready to merge 👍

[benjie]

@graphql/tsc I'm going to add this to Thursday's WG, a review/merge before then would make the discussion shorter 😉

[leebyron]

Marking this as a Proposal since this is a meaningful change. Two changes here:

1. The clarity around default type names and whether the schema definition needs to exist is great, that's an editorial change.
2. The insight that we must not omit the schema definition if a default type name exists in a non-root type is a new change and we need to ensure it's built into the reference impl.

The test case we need to see is your description above: when putting SDL in to in-memory, then back to SDL, does a non-root type named "Mutation" end up in the right place?

[benjie]

@graphql/implementers Please could you add the following to your test suites, and report back if there are any issues:

1. Create a schema from the following SDL (or, if you don't support SDL, construct an equivalent schema)
2. Assert that the schema does not support mutations
3. Print that schema
4. Assert that the result matches the schema, namely that the schema has a query operation named Query but does not have a mutation operation (the Mutation type being just a regular type)

type Query {
  viruses: [Virus!]
}

type Virus {
  name: String!
  knownMutations: [Mutation!]!
}

type Mutation {
  name: String!
  geneSequence: String!
}

schema {
  query: Query
}

I've raised a PR against GraphQL.js that you can use for inspiration: graphql/graphql-js#3839

[spawnia]

@benjie I have added the described test case in webonyx/graphql-php#1303.

Constructing the schema from your example SDL worked fine, it did correctly not support mutations. I had to make the same fix as you did in graphql-js to correct the schema printer output.

[leebyron]

Necessary code changes looks great, key decisions look correct, good feedback from implementers - this is now RFC 2

[leebyron]

Would love feedback on my edits @benjie

• I thought it would be more clear if we made root operation type a definition as well to see how these two relate.
• I made some minor copy edits, please let me know if this improved clarity by your eye
• I added a simplified version of the test case above

[benjie]

This reads awkwardly, I think simply removing While would fix it.

[leebyron]

reworded

[benjie]

Much better; spot on 👍

[benjie]

@leebyron Looks good to me! While we're at it... do you think we should rename "root operation type" to simply "root type"? That would fit better with "default root type name", and I don't think it loses clarity.

[leebyron]

While we're at it...

Yes this would be clarifying but to keep this one from expanding further I'll do a follow up