Skip to content
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

Throw error if no possible concrete types found #1277

Closed
mattkrick opened this issue Mar 7, 2018 · 3 comments
Closed

Throw error if no possible concrete types found #1277

mattkrick opened this issue Mar 7, 2018 · 3 comments

Comments

@mattkrick
Copy link
Contributor

While using relay modern v1.5.0, I got the following when running relay-compiler (I promise this is a graphql issue):

ERROR:
Cannot read property 'some' of undefined
error An unexpected error occurred: "Command failed.
Exit code: 100

Note that the error has no stack trace!

The some that it mentioned was:

	function hasConcreteTypeThatImplements(schema, type, interfaceName) {
	  return isAbstractType(type) && getConcreteTypes(schema, type).some(function (concreteType) {
	    return implementsInterface(concreteType, interfaceName);
	  });
	}

Ultimately, this was caused because when graphql built my schema, it crawled & found the GraphQLInterfaceType, but it didn't find the concrete types that implemented the interface.
This didn't throw an error in GraphQL; however, calling schema.getPossibleTypes returned undefined, which borked it in the compiler.

While it'd be nice for relay-compiler to catch errors & include a stack trace, I think the real issue is that GraphQL allows an abstract type to exist without any concrete types!

I propose closing this hole by calling schema.getPossibleTypes for each abstract type when building the schema. if the result is undefined then throw a friendly error telling the user to add that concrete type to their rootSchema.types.

Happy to PR if this sounds like a reasonable plan!

@IvanGoncharov
Copy link
Member

@mattkrick Sounds very reasonable to me 👍
I think it should be fixed in 3 places:

  1. getPossibleTypes should always return an array even if empty. It's required since schema validation is optional since Move schema validation into separate step (type constructors) #1132.
  2. Add new validation here: https://github.com/graphql/graphql-js/blob/master/src/type/validate.js#L259
  3. Add new validation to the specification, here: https://github.com/facebook/graphql/blame/master/spec/Section%203%20--%20Type%20System.md#L973

mattkrick added a commit to mattkrick/graphql-js that referenced this issue Mar 8, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@mattkrick
Copy link
Contributor Author

Wow, thank you @IvanGoncharov that made the PR super easy!
After reviewing the spec, I saw it already mentioned this issue, so there was no need to update it:

Spec:

An Interface type must define one or more fields

@IvanGoncharov
Copy link
Member

An Interface type must define one or more fields

@mattkrick Prevents you from writing without fields:

interface NoFields {
}

So it's different check.

@IvanGoncharov IvanGoncharov mentioned this issue May 16, 2018
mjmahone added a commit that referenced this issue Jun 8, 2018
Reverts #1277, and implements spec removal from #459. While it's an open question whether this validation is good, we need to provide an upgrade path for schemas that currently do not satisfy this validation rule.
mjmahone added a commit that referenced this issue Jun 8, 2018
* Allow interfaces to have no implementors
Reverts #1277, and implements spec removal from #459. While it's an open question whether this validation is good, we need to provide an upgrade path for schemas that currently do not satisfy this validation rule.

* Update test to accept unimplemented interface
mjmahone added a commit that referenced this issue Jun 8, 2018
It is unclear whether the spec validation from graphql/graphql-spec#424 as implemented by #1277 is what we want to have for the GraphQL Schema long term. This RFC lets us explore that option more, to see whether the breaking schema change is the right path forward.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants