Skip to content

Forbid duplicate type definitions #744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 3, 2017

Conversation

turadg
Copy link
Contributor

@turadg turadg commented Mar 7, 2017

A GraphQL schema can't have more than one definition of the same type.

Facebook runs an integration test to prevent this:
#223 (comment)

With this PR the buildASTSchema step throws, causing any tests that load the schema to fail.

A GraphQL schema can't have more than one definition of the same type.

Facebook runs an integration test to prevent this:
graphql#223 (comment)

With this PR the `buildASTSchema` step throws, causing any tests that load the schema to fail.
@turadg
Copy link
Contributor Author

turadg commented Apr 17, 2017

@leebyron this is the bug we discussed at React Conf.

@leebyron
Copy link
Contributor

Constructing a GraphQLSchema should throw this error, correct?

Also, in the context of an AST document, it would be great to throw GraphQLError with reference to the problem AST nodes.

@wincent
Copy link
Contributor

wincent commented May 3, 2017

Thanks @turadg. This looks reasonable to me and in line with the spec which says:

All types within a GraphQL schema must have unique names. No two provided types may have the same name.

And schema construction is obviously the right time to throw the error too. So I'll merge this. Would you like to follow up and make @leebyron's suggested change of throwing a GraphQLError instead? Some tools may appreciate the metadata.

@wincent wincent merged commit 7ae9950 into graphql:master May 3, 2017
@turadg
Copy link
Contributor Author

turadg commented May 3, 2017

Thanks @wincent . I haven't encountered that case but if I do I bet I'll be back with another PR. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants