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

AST node type annotations vs. GraphQL spec #98

Closed
LWprogramming opened this issue Jun 25, 2020 · 2 comments
Closed

AST node type annotations vs. GraphQL spec #98

LWprogramming opened this issue Jun 25, 2020 · 2 comments

Comments

@LWprogramming
Copy link

Some AST node types, such as the UnionTypeDefinitionNode and the EnumTypeDefinitionNode, have annotations that say their fields/values are optional. For instance, UnionTypeDefinitionNode has an optional frozenlist for the types that the union contains. However, according to the GraphQL spec, unions have to contain at least one unique member type, so it doesn't seem like the list would ever be None. Is there a case that I've missed?

@Cito
Copy link
Member

Cito commented Jun 25, 2020

Good question, @LWprogramming. The simple answer is that GraphQL-core is just a port of GraphQL.js where these are defined as optional in Flow and TypeScript. But the reason for that is not obvious to me either.

Generally, the Node types are more relaxed because you want to be able to parse invalid documents, and then let the validator complain about violation of the specs. So the parser would parse union x as a Node with an empty types list.

However, I think you're right in that the types should never be None. Also, it looks like the GraphQL spec validation rules are not enforced except that GraphQL-Core (but not GraphQL.js) checks that the member types of the union have the right types when building the GraphQLUnion (similar for the value of Enums). But uniqueness and not-emptyness are not checked.

I have passed on these questions to the GraphQL.js issue tracker now.

@Cito
Copy link
Member

Cito commented Jul 5, 2020

After discussion there, the questions above can be answered as follows:

  • Uniqueness and not-emptyness is in fact checked in type.validate_schema(), but this is only happens when a schema is executed via graphql() or graphql_sync(). This will probably changed in future versions of GraphQL.js, separating schema validation from query execution.
  • The arrays have been made optional in GraphQL.js to make the AST nodes forward-compatible and to allow simpler creation of AST nodes as plain JavaScript objects. Since we use well-defined classes in Python anyway, I think it makes sense to make them non-null, so I have changed this now. This will probably also be changed in later versions of GraphQL.js. So now, in an unfinished state these lists can be empty, but they will never be None.

@Cito Cito closed this as completed Jul 5, 2020
LWprogramming added a commit to kensho-technologies/graphql-compiler that referenced this issue Jul 8, 2020
Issue graphql-python/graphql-core#98 fixed in
graphql-core 3.1.2
obi1kenobi pushed a commit to kensho-technologies/graphql-compiler that referenced this issue Jul 14, 2020
* Implement type/interface/enum/union suppression

* Remove out-of-place InputSchemaStrings class

* Fix type annotation

* Implement error checking to prevent unsupported suppressions

* Remove outdated comment

* Raise error onuppressing interface implementation

* Nits

* lint

* Reorder functions

* Update CascadingSuppressionError message

* start lint

* Fix lint

* typo

* Fix mypy errors around GraphQL-core's type-hint

* mypy attempted fix with REMOVE set to Ellipsis

* Formatting

* fix cascading init header

* Replace return None with return IDLE

* Add missing words

* Refactor helper function to return IDLE value when appropriate

* Fix comments

* Add module docstring

* Fix bug with union cascading check

* Nits

* Update renamings argument description

* add missing whitespace

* satisfy mypy

* Fix docstring paste mistake

* Fix wording for cascading error explanation

* Correct docstrings for cascading visitor

* Add note about superclass error

* Clarify file docstring

* Fix rename_schema docstring

* Remove unnecessary comment

* Improve cascading error message

* fix comments

* typo

* Remove unnecessary arg

* Refactor to clarify object name

* address nits

* Update Pipfile to use graphql-core >= 3.1.2

* Narrow type hints with graphql-core update

* Remove null check

Issue graphql-python/graphql-core#98 fixed in
graphql-core 3.1.2

* Update scalar renaming documentation

* Indicate suppression along with renaming in comments

* Use bullet points in CascadingSuppressionError

* Add class docstrings describing error-checking visitor usage

* lint

* Address comments
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