Skip to content

Commit 4765ebf

Browse files
CitoCito
committed
Sort error list to make it deterministic
Because fields are gathered in parallel, the errors may otherwise appear in an non-deterministic order.
1 parent efa9e82 commit 4765ebf

File tree

5 files changed

+270
-304
lines changed

5 files changed

+270
-304
lines changed

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ a query language for APIs created by Facebook.
1212
[![Python 3 Status](https://pyup.io/repos/github/graphql-python/graphql-core-next/python-3-shield.svg)](https://pyup.io/repos/github/graphql-python/graphql-core-next/)
1313

1414
The current version 1.0.1 of GraphQL-core-next is up-to-date with GraphQL.js version
15-
14.0.2. All parts of the API are covered by an extensive test suite of currently 1615
15+
14.0.2. All parts of the API are covered by an extensive test suite of currently 1616
1616
unit tests.
1717

1818

‎graphql/execution/execute.py‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,13 @@ async def build_response_async():
310310

311311
return build_response_async()
312312
data = cast(Optional[Dict[str, Any]], data)
313-
return ExecutionResult(data=data, errors=self.errors or None)
313+
errors = self.errors
314+
if not errors:
315+
return ExecutionResult(data, None)
316+
# sort the error list in order to make it deterministic,
317+
# since we might have been using parallel execution
318+
errors.sort(key=lambda error: (error.locations, error.path, error.message))
319+
return ExecutionResult(data, errors)
314320

315321
def execute_operation(
316322
self, operation: OperationDefinitionNode, root_value: Any
@@ -418,37 +424,34 @@ def execute_fields(
418424
Implements the "Evaluating selection sets" section of the spec
419425
for "read" mode.
420426
"""
421-
is_async = False
422-
423427
results = {}
428+
awaitable_fields = []
424429
for response_name, field_nodes in fields.items():
425430
field_path = add_path(path, response_name)
426431
result = self.resolve_field(
427432
parent_type, source_value, field_nodes, field_path
428433
)
429434
if result is not INVALID:
430435
results[response_name] = result
431-
if not is_async and isawaitable(result):
432-
is_async = True
436+
if isawaitable(result):
437+
awaitable_fields.append(response_name)
433438

434439
# If there are no coroutines, we can just return the object
435-
if not is_async:
440+
if not awaitable_fields:
436441
return results
437442

438-
# Otherwise, results is a map from field name to the result of
439-
# resolving that field, which is possibly a coroutine object.
440-
# Return a coroutine object that will yield this same map, but with
441-
# any coroutines awaited in parallel and replaced with the values they
442-
# yielded.
443+
# Otherwise, results is a map from field name to the result of resolving that
444+
# field, which is possibly a coroutine object. Return a coroutine object that
445+
# will yield this same map, but with any coroutines awaited in parallel and
446+
# replaced with the values they yielded.
443447
async def get_results():
444-
async def await_kv(key, value):
445-
return key, await value if isawaitable(value) else value
446-
447-
pairs = await gather(
448-
*(await_kv(key, value) for key, value in results.items())
448+
results.update(
449+
zip(
450+
awaitable_fields,
451+
await gather(*(results[field] for field in awaitable_fields)),
452+
)
449453
)
450-
451-
return dict(pairs)
454+
return results
452455

453456
return get_results()
454457

‎tests/execution/test_executor.py‎

Lines changed: 75 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
GraphQLResolveInfo,
2121
ResponsePath,
2222
)
23-
from .util import compare_query_results_unordered
2423

2524

2625
def describe_execute_handles_basic_execution_tasks():
@@ -418,70 +417,67 @@ async def asyncReturnErrorWithExtensions(self, _info):
418417
)
419418
)
420419

421-
compare_query_results_unordered(
422-
await execute(schema, ast, Data()),
423-
(
420+
assert await execute(schema, ast, Data()) == (
421+
{
422+
"syncOk": "sync ok",
423+
"syncError": None,
424+
"syncRawError": None,
425+
"syncReturnError": None,
426+
"syncReturnErrorList": ["sync0", None, "sync2", None],
427+
"asyncOk": "async ok",
428+
"asyncError": None,
429+
"asyncRawError": None,
430+
"asyncReturnError": None,
431+
"asyncReturnErrorWithExtensions": None,
432+
},
433+
[
434+
{
435+
"message": "Error getting syncError",
436+
"locations": [(3, 15)],
437+
"path": ["syncError"],
438+
},
439+
{
440+
"message": "Error getting syncRawError",
441+
"locations": [(4, 15)],
442+
"path": ["syncRawError"],
443+
},
444+
{
445+
"message": "Error getting syncReturnError",
446+
"locations": [(5, 15)],
447+
"path": ["syncReturnError"],
448+
},
449+
{
450+
"message": "Error getting syncReturnErrorList1",
451+
"locations": [(6, 15)],
452+
"path": ["syncReturnErrorList", 1],
453+
},
454+
{
455+
"message": "Error getting syncReturnErrorList3",
456+
"locations": [(6, 15)],
457+
"path": ["syncReturnErrorList", 3],
458+
},
459+
{
460+
"message": "Error getting asyncError",
461+
"locations": [(8, 15)],
462+
"path": ["asyncError"],
463+
},
464+
{
465+
"message": "Error getting asyncRawError",
466+
"locations": [(9, 15)],
467+
"path": ["asyncRawError"],
468+
},
469+
{
470+
"message": "Error getting asyncReturnError",
471+
"locations": [(10, 15)],
472+
"path": ["asyncReturnError"],
473+
},
424474
{
425-
"syncOk": "sync ok",
426-
"syncError": None,
427-
"syncRawError": None,
428-
"syncReturnError": None,
429-
"syncReturnErrorList": ["sync0", None, "sync2", None],
430-
"asyncOk": "async ok",
431-
"asyncError": None,
432-
"asyncRawError": None,
433-
"asyncReturnError": None,
434-
"asyncReturnErrorWithExtensions": None,
475+
"message": "Error getting asyncReturnErrorWithExtensions",
476+
"locations": [(11, 15)],
477+
"path": ["asyncReturnErrorWithExtensions"],
478+
"extensions": {"foo": "bar"},
435479
},
436-
[
437-
{
438-
"message": "Error getting syncError",
439-
"locations": [(3, 15)],
440-
"path": ["syncError"],
441-
},
442-
{
443-
"message": "Error getting syncRawError",
444-
"locations": [(4, 15)],
445-
"path": ["syncRawError"],
446-
},
447-
{
448-
"message": "Error getting syncReturnError",
449-
"locations": [(5, 15)],
450-
"path": ["syncReturnError"],
451-
},
452-
{
453-
"message": "Error getting syncReturnErrorList1",
454-
"locations": [(6, 15)],
455-
"path": ["syncReturnErrorList", 1],
456-
},
457-
{
458-
"message": "Error getting syncReturnErrorList3",
459-
"locations": [(6, 15)],
460-
"path": ["syncReturnErrorList", 3],
461-
},
462-
{
463-
"message": "Error getting asyncError",
464-
"locations": [(8, 15)],
465-
"path": ["asyncError"],
466-
},
467-
{
468-
"message": "Error getting asyncRawError",
469-
"locations": [(9, 15)],
470-
"path": ["asyncRawError"],
471-
},
472-
{
473-
"message": "Error getting asyncReturnError",
474-
"locations": [(10, 15)],
475-
"path": ["asyncReturnError"],
476-
},
477-
{
478-
"message": "Error getting asyncReturnErrorWithExtensions",
479-
"locations": [(11, 15)],
480-
"path": ["asyncReturnErrorWithExtensions"],
481-
"extensions": {"foo": "bar"},
482-
},
483-
],
484-
),
480+
],
485481
)
486482

487483
def full_response_path_is_included_for_non_nullable_fields():
@@ -873,39 +869,35 @@ def resolve_field(self, parent_type, source, field_nodes, path):
873869

874870
@mark.asyncio
875871
async def resolve_fields_in_parallel():
876-
class Barrier(object):
877-
# Makes progress only if at least `count` callers are `wait()`ing.
878-
def __init__(self, count):
879-
self.ev = asyncio.Event()
880-
self.count = count
872+
class Barrier:
873+
"""Barrier that makes progress only after a certain number of waits."""
881874

882-
async def wait(self):
883-
self.count -= 1
884-
if self.count == 0:
885-
self.ev.set()
875+
def __init__(self, number):
876+
self.event = asyncio.Event()
877+
self.number = number
886878

887-
return await self.ev.wait()
879+
async def wait(self):
880+
self.number -= 1
881+
if not self.number:
882+
self.event.set()
883+
return await self.event.wait()
888884

889885
barrier = Barrier(2)
890886

891-
async def f(*args):
887+
async def resolve(*_args):
892888
return await barrier.wait()
893889

894890
schema = GraphQLSchema(
895891
GraphQLObjectType(
896892
"Object",
897893
{
898-
"foo": GraphQLField(GraphQLBoolean, resolve=f),
899-
"bar": GraphQLField(GraphQLBoolean, resolve=f),
894+
"foo": GraphQLField(GraphQLBoolean, resolve=resolve),
895+
"bar": GraphQLField(GraphQLBoolean, resolve=resolve),
900896
},
901897
)
902898
)
903899

904-
query = "{foo, bar}"
905-
ast = parse(query)
906-
907-
res = await asyncio.wait_for(
908-
execute(schema, ast), 1.0 # don't wait forever for the test to fail
909-
)
900+
ast = parse("{foo, bar}")
901+
result = await asyncio.wait_for(execute(schema, ast), 1.0)
910902

911-
assert res == ({"foo": True, "bar": True}, None)
903+
assert result == ({"foo": True, "bar": True}, None)

0 commit comments

Comments
 (0)