Fix TypeError while sorting errors in build_response

[tebanep]

As GraphQLError objects might have 'None' as value for their 'locations' and 'path' attributes, a check must be placed in order to avoid raising a TypeError inside the errors list's 'sort' method. This commit changes None by an empty list to enable proper pairwise comparisons.

[tebanep]

First of all, thank you very much for this project! It is very fortunate for the Python community to count with such a complete implementation of the GraphQL specification.

Regarding this Pull request, I want to clarify that I couldn't manage to create a test to reproduce exactly what is happening to me in my production environment. However, as you can see, the proposed change doesn't invalidate any of the current tests. What I'm experiencing, albeit randomly, is the following unhandled error:

TypeError: '<' not supported between instances of 'NoneType' and 'list'

To understand what was happening, I put the following code inside the ExecutionContext.build_response method just before the errors sorting:

    def build_response(
        self, data: AwaitableOrValue[Optional[Dict[str, Any]]]
    ) -> AwaitableOrValue[ExecutionResult]:
        
        ...

        for i, error in enumerate(errors):  # Display Error Attributes
            print(f'<{i}> L: {error.locations} - P: {error.path} - M: {error.message}')

        errors.sort(key=lambda error: (error.locations, error.path, error.message))
        return ExecutionResult(data, errors)

And what I'm getting in the console is the following:

<0> L: [SourceLocation(line=4, column=7)] - P: ['getOccurrences', 'occurrences', 0, 'site'] - M: 
<1> L: [SourceLocation(line=4, column=7)] - P: ['getOccurrences', 'occurrences', 2, 'site'] - M: 
<2> L: [SourceLocation(line=4, column=7)] - P: ['getOccurrences', 'occurrences', 1, 'site'] - M: 
<3> L: [SourceLocation(line=3, column=5)] - P: ['getOccurrences', 'occurrences', 0] - M: 
<4> L: [SourceLocation(line=3, column=5)] - P: ['getOccurrences', 'occurrences', 2] - M: 
<5> L: [SourceLocation(line=3, column=5)] - P: ['getOccurrences', 'occurrences', 1] - M: 
<6> L: [SourceLocation(line=3, column=5)] - P: ['getOccurrences', 'occurrences'] - M: 
<7> L: [SourceLocation(line=2, column=3)] - P: ['getOccurrences'] - M: 
<8> L: None - P: None - M:
Error handling request
Traceback (most recent call last):
  File "/home/eecheverry/.virtualenvs/integrark/lib/python3.6/site-packages/aiohttp/web_protocol.py", line 418, in start
    resp = await task
  File "/home/eecheverry/.virtualenvs/integrark/lib/python3.6/site-packages/aiohttp/web_app.py", line 458, in _handle
    resp = await handler(request)
  File "/home/eecheverry/.virtualenvs/integrark/lib/python3.6/site-packages/aiohttp/web_middlewares.py", line 119, in impl
    return await handler(request)
  File "/home/eecheverry/Workspace/dev/github.com/knowark/integrark/integrark/infrastructure/web/middleware.py", line 25, in user_middleware
    response = await handler(request)
  File "/home/eecheverry/Workspace/dev/github.com/knowark/integrark/integrark/infrastructure/web/resources/graphql.py", line 34, in post
    result = await self.execution_coordinator.execute(query, context)
  File "/home/eecheverry/Workspace/dev/github.com/knowark/integrark/integrark/application/coordinators/execution_coordinator.py", line 12, in execute
    result = await self.query_service.run(query, context)
  File "/home/eecheverry/Workspace/dev/github.com/knowark/integrark/integrark/infrastructure/query/graphql/graphql_query_service.py", line 26, in run
    graphql_result = await graphql(self.schema, query, **graphql_kwargs)
  File "/home/eecheverry/.virtualenvs/integrark/lib/python3.6/site-packages/graphql/graphql.py", line 88, in graphql
    return await cast(Awaitable[ExecutionResult], result)
  File "/home/eecheverry/.virtualenvs/integrark/lib/python3.6/site-packages/graphql/execution/execute.py", line 309, in build_response_async
    return self.build_response(await data)  # type: ignore
  File "/home/eecheverry/.virtualenvs/integrark/lib/python3.6/site-packages/graphql/execution/execute.py", line 322, in build_response
    errors.sort(key=lambda error: (error.locations, error.path, error.message))
TypeError: '<' not supported between instances of 'NoneType' and 'list'

As you can see in line:
<8> L: None - P: None - M:
an error without locations or path is reaching the sort() method call, raising a:
TypeError: '<' not supported between instances of 'NoneType' and 'list'

By adding the "or []" check, we prevent such eventual unhandled errors.

[Cito]

Thank you for reporting this and the suggested fix, @tebanep.

I'd like to add a regression test, but currently cannot come up with a test case where located and non-located errors are output simultaneously. Can you find which kind of error was output as <8>? I also wonder why "M" is always empty in your output. There should actually be a message, and it should give us a clue how we could create a test case.

Also, theoretically even with the fix there could still be a problem since "path" contains mixed int and str values. Though it's even harder for me to think of an example where the paths are incompatible but the locations are equal. Anyway, maybe it's better to remove the path from the key, and just add id(error) as the last element in the key to make it deterministic?

[tebanep]

Hello @Cito. Thanks for your fast response.
The problem I'm experiencing is showing up while I'm working in Integrark (https://github.com/knowark/integrark) which is a server that acts such as Ariadne, but by dynamically importing an external set of definitions and resolvers.

As I mentioned it, I have not been able to reproduce the problem with a test, for that I think I'll need a deeper undestanding of this project. However, while working with the graphql playground and repeatedly pressing the "play" button, I randomly get the error at issue. I'll use a debugger to dig deeper in the code and find the possible cause of the problem. Nevertheless, don't you think some kind of validation should be placed in the errors sorting key since some of their attributes are declared as "optional" and might eventually have "None" values?

By the way, I also think that a simpler key to sort the errors would be preferable.

[tebanep]

I will also check why the messages are not being assigned to the created error instances. As you said, that might give us a clue.

[Cito]

Sure, that definitely needs to be fixed before the final release and I'm glad you brought it up. I just wanted to create a test case that produces both located and non-located errors, so if you find how you managed to get these, let me know :)

[Cito]

I thought about this a bit more. Actually, the fact that path is mixed int and str is not problematic. While it can happen that int and str appear at the same position in two paths, it cannot happen that the first elements of two paths are the same and the next one has a different type, which is the only problematic case for sorting. So path or [] should be safe as sort key. Also, my idea to add id(error) seems not to make sense any more on second thought, since the id depends on the order in which the errors are generated, which is the very thing we want to fix by sorting. So in short, I think your original suggestion seems to be the best fix to me.

If you find a way to reproduce the error, let me know, so that we can add a regression test.

[tebanep]

Sure, that definitely needs to be fixed before the final release and I'm glad you brought it up. I just wanted to create a test case that produces both located and non-located errors, so if you find how you managed to get these, let me know :)

Thanks a lot for integrating this fix, @Cito! As I told you, I will try to find the source of the problem to build a regression test. Let's see how it goes. Have a great day!