Skip to content

Comments

Bugfix: do not use default operation name types if not included in schema definition block#3088

Merged
dondonz merged 4 commits intomasterfrom
schema-block-check
Mar 15, 2023
Merged

Bugfix: do not use default operation name types if not included in schema definition block#3088
dondonz merged 4 commits intomasterfrom
schema-block-check

Conversation

@dondonz
Copy link
Member

@dondonz dondonz commented Feb 6, 2023 •
edited
Loading

This PR fixes a bug where the schema definition block schema { } is used. If there happened to be a type called Mutation or Subscription, but not included in the schema definition block, it would be mistakenly set as an operation.

This PR follows on from this proposed spec improvement graphql/graphql-spec#987 and corresponding fix in the JS reference implementation https://github.com/graphql/graphql-js/pull/3839/files.

Here's Benjie's virus schema example:

type Query {
  viruses: [Virus!]
}

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

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

schema {
  query: Query
}

The schema definition block/keyword defines query as the only operation in this schema. We have a type Mutation but that's referring to a virus, not the GraphQL operation. (Health warning: the word schema is terribly overloaded.)

Previous behaviour was

  • Operation type definitions only recognise query as an operation - this was correct
  • Generated GraphQLSchema sets the mutationType - this was not correct, and is fixed by this PR

@dondonz dondonz added this to the 20.1 milestone Feb 15, 2023
@dondonz dondonz changed the title WIP Schema definition ambiguity RFC Bugfix: default operation name types not to be used if not defined in schema definition block Mar 14, 2023
@dondonz dondonz changed the title Bugfix: default operation name types not to be used if not defined in schema definition block Bugfix: do not use default operation name types if not included in schema definition block Mar 14, 2023
dondonz
dondonz commented Mar 14, 2023
View reviewed changes
src/test/groovy/graphql/schema/idl/SchemaParserTest.groovy
def "correctly parses schema keyword block, include Query, does not include Mutation type"() {
// From RFC to clarify spec https://github.com/graphql/graphql-spec/pull/987
when:
def schema = """schema {
Copy link
Member Author

@dondonz dondonz Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funky indentation is required for schema printing comparison

Copy link
Member

@bbakerman bbakerman Mar 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comparing via strings will often result in funky indents - its the nature of the game

@dondonz dondonz requested review from andimarek and bbakerman March 14, 2023 05:14
@bbakerman
Copy link
Member

bbakerman commented Mar 14, 2023

Nice one

@dondonz dondonz added this pull request to the merge queue Mar 15, 2023
Merged via the queue into master with commit 09984d7 Mar 15, 2023
@dondonz dondonz deleted the schema-block-check branch March 15, 2023 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bbakerman bbakerman bbakerman approved these changes

@andimarek andimarek Awaiting requested review from andimarek

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

20.1

Development

Successfully merging this pull request may close these issues.

2 participants

@dondonz @bbakerman