Skip to content

AST node type annotations vs. GraphQL spec #98

Closed
@LWprogramming

Description

@LWprogramming

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?

Activity

Cito

Cito commented on Jun 25, 2020

@Cito
Member

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

Cito commented on Jul 5, 2020

@Cito
Member

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.
added a commit that references this issue on Jul 8, 2020
added a commit that references this issue on Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      AST node type annotations vs. GraphQL spec · Issue #98 · graphql-python/graphql-core