Separate errors to report to user from unexpected exception (issue #209)

Author: ods

graphql/error/tests/test_base.py

@@ -5,6 +5,7 @@
 
 from promise import Promise
 
+from graphql.error import GraphQLError
 from graphql.execution import execute
 from graphql.language.parser import parse
 from graphql.type import GraphQLField, GraphQLObjectType, GraphQLSchema, GraphQLString
@@ -22,7 +23,7 @@ def test_raise():
 
     def resolver(context, *_):
         # type: (Optional[Any], *ResolveInfo) -> None
-        raise Exception("Failed")
+        raise GraphQLError("Failed")
 
     Type = GraphQLObjectType(
         "Type", {"a": GraphQLField(GraphQLString, resolver=resolver)}
@@ -38,14 +39,14 @@ def test_reraise():
 
     def resolver(context, *_):
         # type: (Optional[Any], *ResolveInfo) -> None
-        raise Exception("Failed")
+        raise GraphQLError("Failed")
 
     Type = GraphQLObjectType(
         "Type", {"a": GraphQLField(GraphQLString, resolver=resolver)}
     )
 
     result = execute(GraphQLSchema(Type), ast)
-    with pytest.raises(Exception) as exc_info:
+    with pytest.raises(GraphQLError) as exc_info:
         result.errors[0].reraise()
 
     extracted = traceback.extract_tb(exc_info.tb)
@@ -59,7 +60,7 @@ def resolver(context, *_):
             "return executor.execute(resolve_fn, source, info, **args)",
         ),
         ("execute", "return fn(*args, **kwargs)"),
-        ("resolver", 'raise Exception("Failed")'),
+        ("resolver", 'raise GraphQLError("Failed")'),
     ]
 
     assert str(exc_info.value) == "Failed"
@@ -71,7 +72,7 @@ def test_reraise_from_promise():
     ast = parse("query Example { a }")
 
     def fail():
-        raise Exception("Failed")
+        raise GraphQLError("Failed")
 
     def resolver(context, *_):
         # type: (Optional[Any], *ResolveInfo) -> None
@@ -95,7 +96,7 @@ def resolver(context, *_):
         ("test_reraise_from_promise", "result.errors[0].reraise()"),
         ("_resolve_from_executor", "executor(resolve, reject)"),
         ("<lambda>", "return Promise(lambda resolve, reject: resolve(fail()))"),
-        ("fail", 'raise Exception("Failed")'),
+        ("fail", 'raise GraphQLError("Failed")'),
     ]
 
     assert str(exc_info.value) == "Failed"

graphql/execution/executor.py

@@ -128,8 +128,10 @@ def promise_executor(v):
 
     def on_rejected(error):
         # type: (Exception) -> None
-        exe_context.errors.append(error)
-        return None
+        if isinstance(error, GraphQLError):
+            exe_context.errors.append(error)
+            return None
+        return Promise.rejected(error)
 
     def on_resolve(data):
         # type: (Union[None, Dict, Observable]) -> Union[ExecutionResult, Observable]
@@ -271,9 +273,6 @@ def subscribe_fields(
     # type: (...) -> Observable
     subscriber_exe_context = SubscriberExecutionContext(exe_context)
 
-    def on_error(error):
-        subscriber_exe_context.report_error(error)
-
     def map_result(data):
         # type: (Dict[str, Any]) -> ExecutionResult
         if subscriber_exe_context.errors:
@@ -482,14 +481,16 @@ def complete_value_catching_error(
 
             def handle_error(error):
                 # type: (Union[GraphQLError, GraphQLLocatedError]) -> Optional[Any]
+                if not isinstance(error, GraphQLError):
+                    return Promise.rejected(error)
                 traceback = completed._traceback  # type: ignore
                 exe_context.report_error(error, traceback)
                 return None
 
             return completed.catch(handle_error)
 
         return completed
-    except Exception as e:
+    except GraphQLError as e:
         traceback = sys.exc_info()[2]
         exe_context.report_error(e, traceback)
         return None
@@ -531,12 +532,15 @@ def complete_value(
             ),
             lambda error: Promise.rejected(
                 GraphQLLocatedError(field_asts, original_error=error, path=path)
+                if isinstance(error, GraphQLError) else error
             ),
         )
 
     # print return_type, type(result)
-    if isinstance(result, Exception):
+    if isinstance(result, GraphQLError):
         raise GraphQLLocatedError(field_asts, original_error=result, path=path)
+    if isinstance(result, Exception):
+        raise result
 
     if isinstance(return_type, GraphQLNonNull):
         return complete_nonnull_value(

graphql/execution/tests/test_executor.py

@@ -263,7 +263,7 @@ def ok(self):
 
         def error(self):
             # type: () -> NoReturn
-            raise Exception("Error getting error")
+            raise GraphQLError("Error getting error")
 
     doc_ast = parse(doc)
 
@@ -559,32 +559,58 @@ def test_fails_to_execute_a_query_containing_a_type_definition():
     )
 
 
-def test_exceptions_are_reraised_if_specified(mocker):
-    # type: (MockFixture) -> None
-
-    logger = mocker.patch("graphql.execution.executor.logger")
+def test_exceptions_are_reraised():
+    # type: () -> None
 
     query = parse(
         """
     { foo }
     """
     )
 
+    class Error(Exception):
+        pass
+
     def resolver(*_):
         # type: (*Any) -> NoReturn
-        raise Exception("UH OH!")
+        raise Error("UH OH!")
 
     schema = GraphQLSchema(
         GraphQLObjectType(
             name="Query", fields={"foo": GraphQLField(GraphQLString, resolver=resolver)}
         )
     )
 
-    execute(schema, query)
-    logger.exception.assert_called_with(
-        "An error occurred while resolving field Query.foo"
+    with raises(Error):
+        execute(schema, query)
+
+
+def test_exceptions_are_reraised_promise():
+    # type: () -> None
+
+    query = parse(
+        """
+    { foo }
+    """
     )
 
+    class Error(Exception):
+        pass
+
+    @Promise.promisify
+    def resolver(*_):
+        # type: (*Any) -> NoReturn
+        raise Error("UH OH!")
+
+    schema = GraphQLSchema(
+        GraphQLObjectType(
+            name="Query", fields={"foo": GraphQLField(GraphQLString, resolver=resolver)}
+        )
+    )
+
+    with raises(Error):
+        execute(schema, query)
+
 
 def test_executor_properly_propogates_path_data(mocker):
     # type: (MockFixture) -> None

graphql/execution/tests/test_executor_asyncio.py

@@ -8,7 +8,7 @@
 
 asyncio = pytest.importorskip("asyncio")
 
-from graphql.error import format_error
+from graphql.error import format_error, GraphQLError
 from graphql.execution import execute
 from graphql.language.parser import parse
 from graphql.type import GraphQLField, GraphQLObjectType, GraphQLSchema, GraphQLString
@@ -95,7 +95,7 @@ def resolver(context, *_):
     def resolver_2(context, *_):
         # type: (Optional[Any], *ResolveInfo) -> NoReturn
         asyncio.sleep(0.003)
-        raise Exception("resolver_2 failed!")
+        raise GraphQLError("resolver_2 failed!")
 
     Type = GraphQLObjectType(
         "Type",
@@ -117,6 +117,28 @@ def resolver_2(context, *_):
     assert result.data == {"a": "hey", "b": None}
 
 
+def test_asyncio_executor_exceptions_reraised():
+    # type: () -> None
+    ast = parse("query Example { a }")
+
+    class Error(Exception):
+        pass
+
+    def resolver(context, *_):
+        # type: (Optional[Any], *ResolveInfo) -> str
+        raise Error("UH OH!")
+
+    Type = GraphQLObjectType(
+        "Type",
+        {
+            "a": GraphQLField(GraphQLString, resolver=resolver),
+        },
+    )
+
+    with pytest.raises(Error):
+        execute(GraphQLSchema(Type), ast, executor=AsyncioExecutor())
+
+
 def test_evaluates_mutations_serially():
     # type: () -> None
     assert_evaluate_mutations_serially(executor=AsyncioExecutor())

graphql/execution/tests/test_executor_gevent.py

@@ -8,7 +8,7 @@
 
 gevent = pytest.importorskip("gevent")
 
-from graphql.error import format_error
+from graphql.error import format_error, GraphQLError
 from graphql.execution import execute
 from graphql.language.location import SourceLocation
 from graphql.language.parser import parse
@@ -54,7 +54,7 @@ def resolver(context, *_):
 
     def resolver_2(context, *_):
         gevent.sleep(0.003)
-        raise Exception("resolver_2 failed!")
+        raise GraphQLError("resolver_2 failed!")
 
     Type = GraphQLObjectType(
         "Type",

graphql/execution/tests/test_executor_thread.py

@@ -1,5 +1,5 @@
 # type: ignore
-from graphql.error import format_error
+from graphql.error import format_error, GraphQLError
 from graphql.execution import execute
 from graphql.language.parser import parse
 from graphql.type import (
@@ -181,19 +181,19 @@ def sync(self):
 
         def syncError(self):
             # type: () -> NoReturn
-            raise Exception("Error getting syncError")
+            raise GraphQLError("Error getting syncError")
 
         def syncReturnError(self):
             # type: () -> Exception
-            return Exception("Error getting syncReturnError")
+            return GraphQLError("Error getting syncReturnError")
 
         def syncReturnErrorList(self):
             # type: () -> List[Union[Exception, str]]
             return [
                 "sync0",
-                Exception("Error getting syncReturnErrorList1"),
+                GraphQLError("Error getting syncReturnErrorList1"),
                 "sync2",
-                Exception("Error getting syncReturnErrorList3"),
+                GraphQLError("Error getting syncReturnErrorList3"),
             ]
 
         def asyncBasic(self):
@@ -202,15 +202,15 @@ def asyncBasic(self):
 
         def asyncReject(self):
             # type: () -> Promise
-            return rejected(Exception("Error getting asyncReject"))
+            return rejected(GraphQLError("Error getting asyncReject"))
 
         def asyncEmptyReject(self):
             # type: () -> Promise
-            return rejected(Exception("An unknown error occurred."))
+            return rejected(GraphQLError("An unknown error occurred."))
 
         def asyncReturnError(self):
             # type: () -> Promise
-            return resolved(Exception("Error getting asyncReturnError"))
+            return resolved(GraphQLError("Error getting asyncReturnError"))
 
     schema = GraphQLSchema(
         query=GraphQLObjectType(

graphql/execution/tests/test_lists.py

@@ -1,7 +1,7 @@
 # type: ignore
 from collections import namedtuple
 
-from graphql.error import format_error
+from graphql.error import format_error, GraphQLError
 from graphql.execution import execute
 from graphql.language.parser import parse
 from graphql.type import (
@@ -66,7 +66,7 @@ class Test_ListOfT_Promise_Array_T:  # [T] Promise<Array<T>>
     )
     test_returns_null = check(resolved(None), {"data": {"nest": {"test": None}}})
     test_rejected = check(
-        lambda: rejected(Exception("bad")),
+        lambda: rejected(GraphQLError("bad")),
         {
             "data": {"nest": {"test": None}},
             "errors": [
@@ -91,7 +91,7 @@ class Test_ListOfT_Array_Promise_T:  # [T] Array<Promise<T>>
         {"data": {"nest": {"test": [1, None, 2]}}},
     )
     test_contains_reject = check(
-        lambda: [resolved(1), rejected(Exception("bad")), resolved(2)],
+        lambda: [resolved(1), rejected(GraphQLError("bad")), resolved(2)],
         {
             "data": {"nest": {"test": [1, None, 2]}},
             "errors": [
@@ -149,7 +149,7 @@ class Test_NotNullListOfT_Promise_Array_T:  # [T]! Promise<Array<T>>>
     )
 
     test_rejected = check(
-        lambda: rejected(Exception("bad")),
+        lambda: rejected(GraphQLError("bad")),
         {
             "data": {"nest": None},
             "errors": [
@@ -173,7 +173,7 @@ class Test_NotNullListOfT_Array_Promise_T:  # [T]! Promise<Array<T>>>
         {"data": {"nest": {"test": [1, None, 2]}}},
     )
     test_contains_reject = check(
-        lambda: [resolved(1), rejected(Exception("bad")), resolved(2)],
+        lambda: [resolved(1), rejected(GraphQLError("bad")), resolved(2)],
         {
             "data": {"nest": {"test": [1, None, 2]}},
             "errors": [
@@ -228,7 +228,7 @@ class TestListOfNotNullT_Promise_Array_T:  # [T!] Promise<Array<T>>
     test_returns_null = check(resolved(None), {"data": {"nest": {"test": None}}})
 
     test_rejected = check(
-        lambda: rejected(Exception("bad")),
+        lambda: rejected(GraphQLError("bad")),
         {
             "data": {"nest": {"test": None}},
             "errors": [
@@ -262,7 +262,7 @@ class TestListOfNotNullT_Array_Promise_T:  # [T!] Array<Promise<T>>
         },
     )
     test_contains_reject = check(
-        lambda: [resolved(1), rejected(Exception("bad")), resolved(2)],
+        lambda: [resolved(1), rejected(GraphQLError("bad")), resolved(2)],
         {
             "data": {"nest": {"test": None}},
             "errors": [
@@ -341,7 +341,7 @@ class TestNotNullListOfNotNullT_Promise_Array_T:  # [T!]! Promise<Array<T>>
     )
 
     test_rejected = check(
-        lambda: rejected(Exception("bad")),
+        lambda: rejected(GraphQLError("bad")),
         {
             "data": {"nest": None},
             "errors": [
@@ -375,7 +375,7 @@ class TestNotNullListOfNotNullT_Array_Promise_T:  # [T!]! Array<Promise<T>>
         },
     )
     test_contains_reject = check(
-        lambda: [resolved(1), rejected(Exception("bad")), resolved(2)],
+        lambda: [resolved(1), rejected(GraphQLError("bad")), resolved(2)],
         {
             "data": {"nest": None},
             "errors": [