search

graphql-python / graphql-core

A Python 3.6+ port of the GraphQL.js reference implementation of GraphQL.
MIT License
510 stars 135 forks source link

Validation/coercion of input variables violates the GraphQL spec #223

Open dkbarn opened 2 months ago

dkbarn commented 2 months ago

When callling graphql-core's execute() function with variable_values which do not pass validation -- for example, including an unexpected key in the variable_values dictionary -- the current behavior is that an ExecutionResult object is returned from the function, with the associated GraphQLError present inside it. Instead, this should be treated as a Request error, according to the spec, meaning that a GraphQLError should be raised from execute().

The GraphQL spec states:

Request errors Request errors are raised before execution begins. This may occur due to a parse grammar or validation error in the requested document, an inability to determine which operation to execute, or invalid input values for variables.

This means that it is incorrect for the coerce_variable_values function to be returning a GraphQLError inside an ExecutionResult: https://github.com/graphql-python/graphql-core/blob/9dcf25e66f6ed36b77de788621cf50bab600d1d3/src/graphql/execution/values.py#L93-L99

doing so means that a response payload is returned containing both an "errors" key and a null "data" key. Again, this a violation of the spec:

If a request error is raised, execution does not begin and the data entry in the response must not be present. The errors entry must include the error.

Cito commented 2 months ago

Hi @dkbarn, thanks for bringing this up.

Actually I believe this is ok in view of the comment directly in front of the lines you linked to. The error in the variable values should be caught during validation already and never happen when execute() is called via the high-level graphql() function that includes validation. Conversion of the ExecutionResult to a JSON response payload and removal of the data property with a null value for a request error should happen at the server level.

If you still believe the behavior of the values module should be changed, please report upstream at the GraphQL.js project since the goal of GraphQL-core is to implement the exact same behavior as this reference implementation. See the corresponding code here.