Empty error list returned when there are none

[tcsavage]

The GraphQL spec specifies that the errors entry in the result be non-empty if errors occurred, and should be absent entirely if no errors occurred. The graphql-core implementation returns an empty list if no errors occurred. This is an issue for compatibility with some GraphQL client libraries.

In my particular case, I have a GraphQL server using graphql-core, and a client using the Apollo JS library. There are times that Apollo thinks an error occurred when it did not, there was just an empty list of errors, when it expected an absent errors entry, null or similar. While this could certainly be fixed in Apollo, it wouldn't be a problem if graphql-core implemented the spec correctly.

I think graphql-core should return None instead of an empty list. While it's still not 100% true to the spec, it's certainly closer and would help solve issues like the above.

I have a patch which fixes this and I'd be happy to submit it if this is agreed to be a problem.

---

Minimal case:
Using graphql-core 1.0.1

from graphql import (
    graphql,
    GraphQLSchema,
    GraphQLObjectType,
    GraphQLField,
    GraphQLString
)

schema = GraphQLSchema(
  query= GraphQLObjectType(
    name='RootQueryType',
    fields={
      'hello': GraphQLField(
        type= GraphQLString,
        resolver=lambda *_: 'world'
      )
    }
  )
)

query = '{ hello }'

result = graphql(schema, query)

print(result.errors)

Prints []
Expected None

[syrusakbary]

Hi @tcsavage,

Even if returns None will still be non-compliant with the spec (as it requires the absence of the attribute, raising AttributeError should be the correct behavior), nullability is still different than undefinability.

However, there are different reasons this is developed the current way:

• Ease of appending errors in the ExecutionResult.
• bool(execution_result.errors) will still be False for an empty array.
• The clients (flask-graphql, graphene-django, sanic-graphql, ...) handle this automatically by adding a errors field only when there are errors in the execution result.

From the issue description, it seems none of this server packages are being used, which are required if you want to expose a "spec-compliant" GraphQL endpoint and the ones that will handle this special case for you.

[tcsavage]

You are correct that it will still not be fully compliant with the spec but I would argue that it would be closer to the intended semantics.

In any case, the docstring for ExecutionResult class says "errors is null if no errors occurred". So from my perspective the implementation should still be changed.

I was not aware that a support library was required to make it fully spec-compliant. In my case I'm actually using graphene rather than graphql-core directly and it does not reformat its result in a way compliant with the spec either. IMHO I don't think it's unreasonable to expect the underlying implementation should be spec compliant without any additional support libraries.

I will certainly move to using sanic-graphql regardless.

[syrusakbary]

You're right. I will improve it so ExecutionResult.errors is None if there are no errors.

Thanks for the feedback! :)