Always return unformatted errors and serialize error locations properly

dittos · dittos · commit 5a5453049eef · 2015-09-27T15:35:33.000+09:00
diff --git a/graphql/core/error.py b/graphql/core/error.py
@@ -1,4 +1,3 @@
-from .defer import DeferredException
 from .language.location import get_location
 
 
@@ -39,10 +38,10 @@ def locations(self):
 
 
 def format_error(error):
-    if isinstance(error, DeferredException):
-        error = error.value
-
     return {
         'message': error.message,
-        'locations': error.locations,
+        'locations': [
+            {'line': loc.line, 'column': loc.column}
+            for loc in error.locations
+        ],
     }
diff --git a/graphql/core/execution/base.py b/graphql/core/execution/base.py
@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-
+from ..defer import DeferredException
 from ..error import GraphQLError
 from ..language import ast
 from ..type.definition import (
@@ -68,6 +68,11 @@ class ExecutionResult(object):
 
     def __init__(self, data=None, errors=None, invalid=False):
         self.data = data
+        if errors:
+            errors = [
+                error.value if isinstance(error, DeferredException) else error
+                for error in errors
+            ]
         self.errors = errors
         if invalid:
             assert data is None
diff --git a/graphql/core/execution/executor.py b/graphql/core/execution/executor.py
@@ -2,7 +2,7 @@
 import functools
 
 from ..defer import Deferred, DeferredDict, DeferredList, defer, succeed
-from ..error import GraphQLError, format_error
+from ..error import GraphQLError
 from ..language import ast
 from ..language.parser import parse
 from ..language.source import Source
@@ -71,7 +71,7 @@ def _execute_graphql_query(self, root, ast, operation_name, args, request_contex
             lambda error: ctx.errors.append(error)
         ) \
             .add_callback(
-            lambda data: ExecutionResult(data, list(map(format_error, ctx.errors))),
+            lambda data: ExecutionResult(data, ctx.errors),
         )
 
     def _execute_operation(self, ctx, root, operation, execute_serially):
diff --git a/graphql/core/language/error.py b/graphql/core/language/error.py
@@ -1,25 +1,23 @@
-from ..compat import native_str
-from ..error import Error
+from ..error import GraphQLError
 from .location import get_location
 
 __all__ = ['LanguageError']
 
 
-class LanguageError(Error):
+class LanguageError(GraphQLError):
     def __init__(self, source, position, description):
-        self.source = source
-        self.position = position
-        self.description = description
-
-    def __str__(self):
-        location = get_location(self.source, self.position)
-        return native_str(u'Syntax Error {} ({}:{}) {}\n\n{}'.format(
-            self.source.name,
-            location.line,
-            location.column,
-            self.description,
-            highlight_source_at_location(self.source, location),
-        ), errors='replace')
+        location = get_location(source, position)
+        super(LanguageError, self).__init__(
+            message=u'Syntax Error {} ({}:{}) {}\n\n{}'.format(
+                source.name,
+                location.line,
+                location.column,
+                description,
+                highlight_source_at_location(source, location),
+            ),
+            source=source,
+            positions=[position],
+        )
 
 
 def highlight_source_at_location(source, location):
diff --git a/tests/core_execution/test_concurrent_executor.py b/tests/core_execution/test_concurrent_executor.py
@@ -1,3 +1,4 @@
+from graphql.core.error import format_error
 from graphql.core.execution import Executor
 from graphql.core.execution.middlewares.sync import SynchronousExecutionMiddleware
 from graphql.core.defer import succeed, Deferred, fail
@@ -147,9 +148,10 @@ def notPromise(self):
     result = executor.execute(doc, Data(), operation_name='Example')
     assert not isinstance(result, Deferred)
     assert result.data == {"promise": None, 'notPromise': 'i should work'}
-    assert result.errors == [{'locations': [SourceLocation(line=3, column=9)],
-                              'message': 'You cannot return a Deferred from a resolver '
-                                         'when using SynchronousExecutionMiddleware'}]
+    formatted_errors = list(map(format_error, result.errors))
+    assert formatted_errors == [{'locations': [dict(line=3, column=9)],
+                                 'message': 'You cannot return a Deferred from a resolver '
+                                            'when using SynchronousExecutionMiddleware'}]
 
 
 def test_synchronous_executor_doesnt_support_defers():
@@ -176,9 +178,10 @@ def notPromise(self):
     result = executor.execute(doc, Data(), operation_name='Example')
     assert not isinstance(result, Deferred)
     assert result.data is None
-    assert result.errors == [{'locations': [SourceLocation(line=3, column=9)],
-                              'message': 'You cannot return a Deferred from a resolver '
-                                         'when using SynchronousExecutionMiddleware'}]
+    formatted_errors = list(map(format_error, result.errors))
+    assert formatted_errors == [{'locations': [dict(line=3, column=9)],
+                                 'message': 'You cannot return a Deferred from a resolver '
+                                            'when using SynchronousExecutionMiddleware'}]
 
 
 def test_executor_defer_failure():
@@ -206,8 +209,9 @@ def notPromise(self):
     assert result.called
     result = result.result
     assert result.data is None
-    assert result.errors == [{'locations': [SourceLocation(line=3, column=9)],
-                              'message': "Something bad happened! Sucks :("}]
+    formatted_errors = list(map(format_error, result.errors))
+    assert formatted_errors == [{'locations': [dict(line=3, column=9)],
+                                 'message': "Something bad happened! Sucks :("}]
 
 
 def test_synchronous_executor_will_synchronously_resolve():
diff --git a/tests/core_execution/test_executor.py b/tests/core_execution/test_executor.py
@@ -231,7 +231,7 @@ def error(self):
     result = execute(GraphQLSchema(Type), Data(), doc_ast)
     assert result.data == {'ok': 'ok', 'error': None}
     assert len(result.errors) == 1
-    assert result.errors[0]['message'] == 'Error getting error'
+    assert result.errors[0].message == 'Error getting error'
     # TODO: check error location
 
 
diff --git a/tests/core_execution/test_gevent.py b/tests/core_execution/test_gevent.py
@@ -1,5 +1,5 @@
 # flake8: noqa
-
+from graphql.core.error import format_error
 from graphql.core.execution import Executor
 from graphql.core.execution.middlewares.gevent import GeventExecutionMiddleware, run_in_greenlet
 from graphql.core.language.location import SourceLocation
@@ -57,5 +57,6 @@ def resolver_2(context, *_):
 
     executor = Executor(GraphQLSchema(Type), [GeventExecutionMiddleware()])
     result = executor.execute(doc)
-    assert result.errors == [{'locations': [SourceLocation(1, 20)], 'message': 'resolver_2 failed!'}]
+    formatted_errors = list(map(format_error, result.errors))
+    assert formatted_errors == [{'locations': [{'line': 1, 'column': 20}], 'message': 'resolver_2 failed!'}]
     assert result.data == {'a': 'hey', 'b': None}
diff --git a/tests/core_execution/test_lists.py b/tests/core_execution/test_lists.py
@@ -50,7 +50,7 @@ def test_non_nullable_list_of_nullables():
     # Returns null
     result = run(type, None)
     assert len(result.errors) == 1
-    assert result.errors[0]['message'] == 'Cannot return null for non-nullable field DataType.test.'
+    assert result.errors[0].message == 'Cannot return null for non-nullable field DataType.test.'
     # TODO: check error location
     assert result.data == {'nest': None}
 
@@ -62,7 +62,7 @@ def test_nullable_list_of_non_nullables():
     # Contains null
     result = run(type, [1, None, 2])
     assert len(result.errors) == 1
-    assert result.errors[0]['message'] == 'Cannot return null for non-nullable field DataType.test.'
+    assert result.errors[0].message == 'Cannot return null for non-nullable field DataType.test.'
     # TODO: check error location
     assert result.data == {'nest': {'test': None}}
     # Returns null
@@ -76,12 +76,12 @@ def test_non_nullable_list_of_non_nullables():
     # Contains null
     result = run(type, [1, None, 2])
     assert len(result.errors) == 1
-    assert result.errors[0]['message'] == 'Cannot return null for non-nullable field DataType.test.'
+    assert result.errors[0].message == 'Cannot return null for non-nullable field DataType.test.'
     # TODO: check error location
     assert result.data == {'nest': None}
     # Returns null
     result = run(type, None)
     assert len(result.errors) == 1
-    assert result.errors[0]['message'] == 'Cannot return null for non-nullable field DataType.test.'
+    assert result.errors[0].message == 'Cannot return null for non-nullable field DataType.test.'
     # TODO: check error location
     assert result.data == {'nest': None}
diff --git a/tests/core_execution/test_mutations.py b/tests/core_execution/test_mutations.py
@@ -128,5 +128,5 @@ def test_evaluates_mutations_correctly_in_the_presense_of_a_failed_mutation():
         }
     assert len(result.errors) == 2
     # TODO: check error location
-    assert result.errors[0]['message'] == 'Cannot change the number'
-    assert result.errors[1]['message'] == 'Cannot change the number'
+    assert result.errors[0].message == 'Cannot change the number'
+    assert result.errors[1].message == 'Cannot change the number'
diff --git a/tests/core_execution/test_nonnull.py b/tests/core_execution/test_nonnull.py
@@ -54,7 +54,7 @@ def test_nulls_a_nullable_field_that_throws_sync():
     result = execute(schema, ThrowingData(), ast, 'Q', {})
     assert len(result.errors) == 1
     # TODO: check error location
-    assert result.errors[0]['message'] == str(sync_error)
+    assert result.errors[0].message == str(sync_error)
     assert result.data == {
         'sync': None
     }
@@ -72,7 +72,7 @@ def test_nulls_a_sync_returned_object_that_contains_a_non_nullable_field_that_th
     result = execute(schema, ThrowingData(), ast, 'Q', {})
     assert len(result.errors) == 1
     # TODO: check error location
-    assert result.errors[0]['message'] == str(non_null_sync_error)
+    assert result.errors[0].message == str(non_null_sync_error)
     assert result.data == {
         'nest': None
     }
@@ -111,8 +111,8 @@ def test_nulls_a_complex_tree_of_nullable_fields_that_throw():
     result = execute(schema, ThrowingData(), ast, 'Q', {})
     assert len(result.errors) == 2
     # TODO: check error location
-    assert result.errors[0]['message'] == str(sync_error)
-    assert result.errors[1]['message'] == str(sync_error)
+    assert result.errors[0].message == str(sync_error)
+    assert result.errors[1].message == str(sync_error)
     assert result.data == {
         'nest': {
             'sync': None,
@@ -149,7 +149,7 @@ def test_nulls_a_sync_returned_object_that_contains_a_non_nullable_field_that_re
     result = execute(schema, NullingData(), ast, 'Q', {})
     assert len(result.errors) == 1
     # TODO: check error location
-    assert result.errors[0]['message'] == 'Cannot return null for non-nullable field DataType.nonNullSync.'
+    assert result.errors[0].message == 'Cannot return null for non-nullable field DataType.nonNullSync.'
     assert result.data == {
         'nest': None
     }
@@ -206,7 +206,7 @@ def test_nulls_the_top_level_if_sync_non_nullable_field_throws():
     assert result.data is None
     assert len(result.errors) == 1
     # TODO: check error location
-    assert result.errors[0]['message'] == str(non_null_sync_error)
+    assert result.errors[0].message == str(non_null_sync_error)
 
 
 def test_nulls_the_top_level_if_sync_non_nullable_field_returns_null():
@@ -218,4 +218,4 @@ def test_nulls_the_top_level_if_sync_non_nullable_field_returns_null():
     assert result.data is None
     assert len(result.errors) == 1
     # TODO: check error location
-    assert result.errors[0]['message'] == 'Cannot return null for non-nullable field DataType.nonNullSync.'
+    assert result.errors[0].message == 'Cannot return null for non-nullable field DataType.nonNullSync.'
diff --git a/tests/core_language/test_lexer.py b/tests/core_language/test_lexer.py
@@ -170,4 +170,4 @@ def test_lex_reports_useful_unknown_character_error():
 
     with raises(LanguageError) as excinfo:
         lex_one(u'\u203B')
-    assert r'Syntax Error GraphQL (1:1) Unexpected character "\u203b"' in str(excinfo.value)
+    assert r'Syntax Error GraphQL (1:1) Unexpected character "\u203b"' in excinfo.value.message
diff --git a/tests/core_starwars/test_query.py b/tests/core_starwars/test_query.py
@@ -1,5 +1,6 @@
 from starwars_schema import StarWarsSchema
 from graphql.core import graphql
+from graphql.core.error import format_error
 
 
 def test_hero_name_query():
@@ -343,3 +344,15 @@ def test_check_type_of_luke():
     result = graphql(StarWarsSchema, query)
     assert not result.errors
     assert result.data == expected
+
+
+def test_parse_error():
+    query = '''
+        qeury
+    '''
+    result = graphql(StarWarsSchema, query)
+    assert result.invalid
+    formatted_error = format_error(result.errors[0])
+    assert formatted_error['locations'] == [{'column': 9, 'line': 2}]
+    assert 'Syntax Error GraphQL request (2:9) Unexpected Name "qeury"' in formatted_error['message']
+    assert result.data is None
diff --git a/tests/core_type/test_introspection.py b/tests/core_type/test_introspection.py
@@ -718,7 +718,7 @@ def test_fails_as_expected_on_the_type_root_field_without_an_arg():
     }'''
     result = graphql(schema, request)
     expected_error = {'message': ProvidedNonNullArguments.missing_field_arg_message('__type', 'name', 'String!'),
-                      'locations': [SourceLocation(line=3, column=9)]}
+                      'locations': [dict(line=3, column=9)]}
     assert (expected_error in [format_error(error) for error in result.errors])
 
 
diff --git a/tests/core_validation/utils.py b/tests/core_validation/utils.py
@@ -179,6 +179,11 @@ def sort_lists(value):
 def expect_invalid(schema, rules, query, expected_errors, sort_list=True):
     errors = validate(schema, parse(query), rules)
     assert errors, 'Should not validate'
+    for error in expected_errors:
+        error['locations'] = [
+            {'line': loc.line, 'column': loc.column}
+            for loc in error['locations']
+        ]
     if sort_list:
         assert sort_lists(list(map(format_error, errors))) == sort_lists(expected_errors)
 
diff --git a/tests_py35/core_execution/test_asyncio_executor.py b/tests_py35/core_execution/test_asyncio_executor.py
@@ -2,9 +2,9 @@
 
 import asyncio
 import functools
+from graphql.core.error import format_error
 from graphql.core.execution import Executor
 from graphql.core.execution.middlewares.asyncio import AsyncioExecutionMiddleware
-from graphql.core.language.location import SourceLocation
 from graphql.core.type import (
     GraphQLSchema,
     GraphQLObjectType,
@@ -62,5 +62,6 @@ async def resolver_2(context, *_):
 
     executor = Executor(GraphQLSchema(Type), [AsyncioExecutionMiddleware()])
     result = await executor.execute(doc)
-    assert result.errors == [{'locations': [SourceLocation(1, 20)], 'message': 'resolver_2 failed!'}]
-    assert result.data == {'a': 'hey', 'b': None}
+    formatted_errors = list(map(format_error, result.errors))
+    assert formatted_errors == [{'locations': [{'line': 1, 'column': 20}], 'message': 'resolver_2 failed!'}]
+    assert result.data == {'a': 'hey', 'b': None}

Original file line number	Diff line number	Diff line change
`@@ -128,5 +128,5 @@ def test_evaluates_mutations_correctly_in_the_presense_of_a_failed_mutation():`
`128`	`128`	`}`
`129`	`129`	`assert len(result.errors) == 2`
`130`	`130`	`# TODO: check error location`
`131`		`- assert result.errors[0]['message'] == 'Cannot change the number'`
`132`		`- assert result.errors[1]['message'] == 'Cannot change the number'`
	`131`	`+ assert result.errors[0].message == 'Cannot change the number'`
	`132`	`+ assert result.errors[1].message == 'Cannot change the number'`