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

Shouldn't type builders validate names according to the spec? #227

Closed
metacosm opened this issue Oct 18, 2016 · 6 comments
Closed

Shouldn't type builders validate names according to the spec? #227

metacosm opened this issue Oct 18, 2016 · 6 comments

Comments

@metacosm
Copy link

The spec states that valid names must conform to the /[_A-Za-z][_0-9A-Za-z]*/ regex (http://facebook.github.io/graphql/#sec-Names) but it doesn't seem like this validation is applied by this implementation.

@dminkovsky
Copy link
Contributor

That section refers to query documents (the spec does not deal very much with schema definition, if at all). We parse query documents with ANTLR, and names are validated per the spec.

Please see (2) in my response on our previous issue. It's important to differentiate between the query document and schema definition.

@dminkovsky
Copy link
Contributor

That said, the builders are kind of lacking IMO. For example, unlike the JS implementation, our builders do not validate that objects implement interfaces correctly. It would be nice if this were improved. Names might also be validated in the builders.

@dminkovsky
Copy link
Contributor

the spec does not deal very much with schema definition

In terms of how that definition system is implemented.

@metacosm
Copy link
Author

I agree that the spec for names addresses only query documents. However, if the schema doesn't conform to this naming scheme, you won't be able to query your data. As an example, if you attempt to use a schema that doesn't follow this naming format, GraphiQL will throw an error. A possible spec oversight but shouldn't the schema produce names that can be queried, regardless?

@dminkovsky
Copy link
Contributor

Yeah, validation (name and other properties) is definitely a deficiency. In addition to this deficiency, there are several other problems that have been identified with the schema definition system:

I've explored these and discovered that these are all related in one way or another.

I'm not entirely sure how best to resolve them at the moment. On one hand, it's easy to iterate on the simpler problems like validating names and interfaces. On the other hand, it's not as simple to work through the issues surrounding type references and hashCode()/equals(). But anyway, while all these things are issues, I wouldn't call them blocking issues and have been thinking about how to best address them while working more with the library in my applications.

Please feel free to submit PRs if you would like to iterate on these.

Thanks.

@okorz001
Copy link
Contributor

okorz001 commented Nov 8, 2016

+1 for detecting illegal fields at schema creation time instead of query execution time. There is no value in declaring a field that cannot be queried.

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

4 participants