diff --git a/src/graphql/error/graphql_error.py b/src/graphql/error/graphql_error.py index 2161c5e4..be88acc4 100644 --- a/src/graphql/error/graphql_error.py +++ b/src/graphql/error/graphql_error.py @@ -2,7 +2,6 @@ from __future__ import annotations -from sys import exc_info from typing import TYPE_CHECKING, Any, TypeAlias, TypedDict from ..pyutils import Undefined @@ -316,8 +315,6 @@ def __init__( if isinstance(original_extensions, dict): extensions = original_extensions self.extensions = extensions or {} - if not self.__traceback__: - self.__traceback__ = exc_info()[2] def __str__(self) -> str: """Get this error as a human-readable message with source locations. diff --git a/src/graphql/execution/executor.py b/src/graphql/execution/executor.py index 1cb594b8..ed887831 100644 --- a/src/graphql/execution/executor.py +++ b/src/graphql/execution/executor.py @@ -1211,9 +1211,14 @@ def abort_error(self) -> Exception: :meta private: """ - reason = self.abort_signal.reason # type: ignore[union-attr] + abort_signal = self.abort_signal + reason = abort_signal.reason # type: ignore[union-attr] if isinstance(reason, Exception): - return reason + # Restore the traceback the reason had when the signal was aborted, + # since raising the same reason again and again would extend it. + return reason.with_traceback( + getattr(abort_signal, "_reason_traceback", reason.__traceback__) + ) msg = f"Unexpected error value: {inspect(reason)}" return TypeError(msg) diff --git a/src/graphql/execution/incremental/computation.py b/src/graphql/execution/incremental/computation.py index 74a51d1d..0b8fd844 100644 --- a/src/graphql/execution/incremental/computation.py +++ b/src/graphql/execution/incremental/computation.py @@ -10,6 +10,7 @@ if TYPE_CHECKING: from asyncio import Future from collections.abc import Callable + from types import TracebackType from ...pyutils import AwaitableOrValue @@ -40,9 +41,10 @@ class Computation(Generic[T]): For internal use only. """ - __slots__ = "_fn", "_on_abort", "_status", "_value" + __slots__ = "_fn", "_on_abort", "_status", "_traceback", "_value" _status: str | None + _traceback: TracebackType | None _value: Any def __init__( @@ -55,6 +57,7 @@ def __init__( self._fn = fn self._on_abort = on_abort self._status = None + self._traceback = None self._value = None def prime(self) -> None: @@ -64,8 +67,7 @@ def prime(self) -> None: try: result = self._fn() except Exception as reason: - self._status = _REJECTED - self._value = reason + self._reject(reason) return if is_awaitable(result): future = ensure_future(result) @@ -95,7 +97,9 @@ def result(self) -> AwaitableOrValue[T]: reason = self._value if reason is None: raise CancelledError - raise reason + # Restore the original traceback, which would grow otherwise, + # since the same reason is raised every time the result is requested. + raise reason.with_traceback(self._traceback) return self._value # the pending future def abort(self, reason: BaseException | None = None) -> AwaitableOrValue[None]: @@ -106,12 +110,10 @@ def abort(self, reason: BaseException | None = None) -> AwaitableOrValue[None]: """ status = self._status if status is None: - self._status = _REJECTED - self._value = reason + self._reject(reason) elif status is _PENDING: future = self._value - self._status = _REJECTED - self._value = reason + self._reject(reason) future.cancel() on_abort = self._on_abort if on_abort is not None: @@ -123,15 +125,19 @@ def _settle(self, future: Future[T]) -> None: if future.cancelled(): if self._status is _PENDING: # cancelled externally, not via abort - self._status = _REJECTED - self._value = CancelledError() + self._reject(CancelledError()) return reason = future.exception() # always retrieve, avoiding warnings if self._status is not _PENDING: return # aborted after the future had already completed if reason is not None: - self._status = _REJECTED - self._value = reason + self._reject(reason) else: self._status = _FULFILLED self._value = future.result() + + def _reject(self, reason: BaseException | None) -> None: + """Memoize the reason why the computation failed or was aborted.""" + self._status = _REJECTED + self._value = reason + self._traceback = None if reason is None else reason.__traceback__ diff --git a/src/graphql/pyutils/abort_signal.py b/src/graphql/pyutils/abort_signal.py index b2a541ef..91a51f2a 100644 --- a/src/graphql/pyutils/abort_signal.py +++ b/src/graphql/pyutils/abort_signal.py @@ -3,7 +3,10 @@ from __future__ import annotations from asyncio import Event -from typing import Any +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from types import TracebackType __all__ = ["AbortController", "AbortError", "AbortSignal"] @@ -42,6 +45,7 @@ class AbortSignal: def __init__(self) -> None: self.aborted = False self.reason = None + self._reason_traceback: TracebackType | None = None self._event = Event() async def wait(self) -> Any: @@ -110,4 +114,6 @@ def abort(self, reason: Any = None) -> None: signal.reason = ( AbortError("This operation was aborted") if reason is None else reason ) + if isinstance(reason, BaseException): + signal._reason_traceback = reason.__traceback__ # noqa: SLF001 signal._event.set() # noqa: SLF001 diff --git a/src/graphql/validation/validate.py b/src/graphql/validation/validate.py index 4d183510..d452d5f8 100644 --- a/src/graphql/validation/validate.py +++ b/src/graphql/validation/validate.py @@ -30,11 +30,6 @@ class ValidationAbortedError(GraphQLError): """Error when a validation has been aborted (error limit reached).""" -validation_aborted_error = ValidationAbortedError( - "Too many validation errors, error limit reached. Validation aborted." -) - - # Per the specification, descriptions must not affect validation. # See https://spec.graphql.org/draft/#sec-Descriptions query_document_keys_to_validate: dict[str, tuple[str, ...]] = { @@ -113,7 +108,10 @@ def validate( def on_error(error: GraphQLError) -> None: if len(errors) >= max_errors: - raise validation_aborted_error + # Unlike GraphQL.js, create a new error each time, because raising a + # shared error would add the frames of every validation to its traceback. + msg = "Too many validation errors, error limit reached. Validation aborted." + raise ValidationAbortedError(msg) errors.append(error) context = ValidationContext( @@ -131,8 +129,11 @@ def on_error(error: GraphQLError) -> None: TypeInfoVisitor(type_info, ParallelVisitor(visitors)), query_document_keys_to_validate, ) - except ValidationAbortedError: - errors.append(validation_aborted_error) + except ValidationAbortedError as error: + # Do not keep the frames of the validation alive with the returned error, + # also not via the context of an error that was handled while aborting. + error.__context__ = None + errors.append(error.with_traceback(None)) return errors diff --git a/tests/error/test_graphql_error.py b/tests/error/test_graphql_error.py index 191b604f..7ff14ff6 100644 --- a/tests/error/test_graphql_error.py +++ b/tests/error/test_graphql_error.py @@ -2,6 +2,8 @@ from typing import cast +import pytest + from graphql.error import GraphQLError from graphql.language import ( NameNode, @@ -32,6 +34,18 @@ assert field_node +def assert_raised_with_new_traceback(error: GraphQLError, handled_error: Exception): + # In Python, an error gets its traceback when it is raised, and an error that + # is being handled at this time becomes its context, not part of its traceback. + with pytest.raises(GraphQLError) as exc_info: + raise error + assert exc_info.value is error + assert error.__traceback__ is not None + assert error.__traceback__ is not handled_error.__traceback__ + assert error.__traceback__.tb_next is None + assert error.__context__ is handled_error + + def describe_graphql_error(): def is_a_class_and_is_a_subclass_of_exception(): assert type(GraphQLError) is type @@ -117,14 +131,14 @@ def passes_the_cause_of_an_original_error(): def creates_new_stack_if_original_error_has_no_stack(): try: raise RuntimeError - except RuntimeError as original_with_traceback: - original_traceback = original_with_traceback.__traceback__ + except RuntimeError as handled_error: original = RuntimeError("original") e = GraphQLError("msg", original_error=original) + # the traceback of the handled error is not adopted + assert e.__traceback__ is None + assert_raised_with_new_traceback(e, handled_error) assert e.__class__.__name__ == "GraphQLError" assert original.__traceback__ is None - assert original_traceback is not None - assert e.__traceback__ is original_traceback assert e.message == "msg" assert e.original_error is original assert str(e.original_error) == "original" @@ -179,13 +193,13 @@ def prefers_cause_and_original_error_separately(): def creates_new_stack_if_cause_has_no_stack(): try: raise RuntimeError - except RuntimeError as runtime_error: - current_traceback = runtime_error.__traceback__ + except RuntimeError as handled_error: cause = RuntimeError("cause") e = GraphQLError("msg", cause=cause) + # the traceback of the handled error is not adopted + assert e.__traceback__ is None + assert_raised_with_new_traceback(e, handled_error) assert cause.__traceback__ is None - assert current_traceback is not None - assert e.__traceback__ is current_traceback assert e.message == "msg" assert e.cause is cause assert e.original_error is cause diff --git a/tests/execution/incremental/test_computation.py b/tests/execution/incremental/test_computation.py index c344f27f..b49823fa 100644 --- a/tests/execution/incremental/test_computation.py +++ b/tests/execution/incremental/test_computation.py @@ -9,6 +9,7 @@ if TYPE_CHECKING: from collections.abc import Callable + from types import TracebackType pytestmark = pytest.mark.anyio @@ -25,6 +26,15 @@ def __call__(self, *args: Any, **kwargs: Any) -> Any: return self.fn(*args, **kwargs) +def traceback_entries(error: BaseException) -> list[TracebackType]: + entries = [] + traceback = error.__traceback__ + while traceback: + entries.append(traceback) + traceback = traceback.tb_next + return entries + + def describe_computation(): def can_return_a_result(): computation = Computation(lambda: {"value": 123}) @@ -95,6 +105,56 @@ async def run() -> None: computation.result() assert run_spy.call_count == 1 + def raises_a_sync_error_again_with_its_original_traceback(): + def run() -> None: + raise RuntimeError("failure") + + computation = Computation(run) + + lengths = [] + for _ in range(3): + with pytest.raises(RuntimeError, match="failure") as exc_info: + computation.result() + lengths.append(len(traceback_entries(exc_info.value))) + assert lengths[0] > 1 + assert lengths[1] == lengths[2] == lengths[0] + + async def raises_an_async_error_again_with_its_original_traceback(): + async def run() -> None: + await sleep(0) + raise RuntimeError("failure") + + computation = Computation(Spy(run)) + + computation.prime() + with pytest.raises(RuntimeError, match="failure"): + await computation.result() + lengths = [] + for _ in range(3): + with pytest.raises(RuntimeError, match="failure") as exc_info: + computation.result() + lengths.append(len(traceback_entries(exc_info.value))) + assert lengths[1] == lengths[2] == lengths[0] + + def raises_an_abort_reason_again_with_its_original_traceback(): + try: + raise RuntimeError("aborted") + except RuntimeError as error: + abort_reason = error + original_traceback = abort_reason.__traceback__ + computation = Computation(lambda: {"value": 123}) + + computation.abort(abort_reason) + lengths = [] + for _ in range(3): + with pytest.raises(RuntimeError, match="aborted") as exc_info: + computation.result() + assert exc_info.value is abort_reason + entries = traceback_entries(abort_reason) + assert entries[-1] is original_traceback + lengths.append(len(entries)) + assert lengths[1] == lengths[2] == lengths[0] + def can_be_aborted_before_running(): on_abort_spy = Spy(lambda _reason: None) run_spy = Spy(lambda: {"value": 123}) diff --git a/tests/execution/test_cancellation.py b/tests/execution/test_cancellation.py index af625aca..bccb488d 100644 --- a/tests/execution/test_cancellation.py +++ b/tests/execution/test_cancellation.py @@ -4,6 +4,7 @@ from asyncio import Event, Future, ensure_future, sleep from collections.abc import AsyncIterator, Awaitable +from typing import TYPE_CHECKING import pytest @@ -26,6 +27,9 @@ from graphql.language import parse from graphql.pyutils import AbortController, AbortError, is_awaitable +if TYPE_CHECKING: + from types import TracebackType + pytestmark = [ pytest.mark.anyio, pytest.mark.filterwarnings("ignore:coroutine .* was never awaited:RuntimeWarning"), @@ -65,6 +69,15 @@ ) +def traceback_entries(error: BaseException) -> list[TracebackType]: + entries = [] + traceback = error.__traceback__ + while traceback: + entries.append(traceback) + traceback = traceback.tb_next + return entries + + def must_not_be_called(*_args, **_kwargs): pytest.fail("Should not be called") # pragma: no cover @@ -1236,3 +1249,40 @@ async def aclose(self): # wait for the partial result so that the cleanup has settled await exc_info.value.aborted_result assert return_called is True + + +def describe_abort_reason_traceback(): + def does_not_extend_the_traceback_of_the_default_reason(): + abort_controller = AbortController() + abort_controller.abort() + reason = abort_controller.signal.reason + document = parse("{ todo { id } }") + + lengths = [] + for _ in range(3): + with pytest.raises(AbortError) as exc_info: + execute(schema, document, abort_signal=abort_controller.signal) + assert exc_info.value is reason + lengths.append(len(traceback_entries(reason))) + assert lengths[0] > 0 + assert lengths[1] == lengths[2] == lengths[0] + + def does_not_extend_the_traceback_of_a_provided_reason(): + try: + raise RuntimeError("Custom abort error") + except RuntimeError as error: + reason = error + original_traceback = reason.__traceback__ + abort_controller = AbortController() + abort_controller.abort(reason) + document = parse("{ todo { id } }") + + lengths = [] + for _ in range(3): + with pytest.raises(RuntimeError, match=r"^Custom abort error$") as exc_info: + execute(schema, document, abort_signal=abort_controller.signal) + assert exc_info.value is reason + entries = traceback_entries(reason) + assert entries[-1] is original_traceback + lengths.append(len(entries)) + assert lengths[1] == lengths[2] == lengths[0] diff --git a/tests/validation/test_overlapping_fields_can_be_merged.py b/tests/validation/test_overlapping_fields_can_be_merged.py index c492da06..dcd90215 100644 --- a/tests/validation/test_overlapping_fields_can_be_merged.py +++ b/tests/validation/test_overlapping_fields_can_be_merged.py @@ -1,11 +1,17 @@ from contextlib import contextmanager from functools import partial +from graphql.language import parse from graphql.utilities import build_schema -from graphql.validation import OverlappingFieldsCanBeMergedRule +from graphql.validation import ( + FieldsOnCorrectTypeRule, + OverlappingFieldsCanBeMergedRule, + validate, +) from graphql.validation.rules import ( overlapping_fields_can_be_merged as overlapping_module, ) +from graphql.validation.validate import ValidationAbortedError from .harness import assert_validation_errors @@ -1666,6 +1672,46 @@ def aborts_validation_when_the_comparison_budget_is_exceeded(): schema=budget_schema, ) + def error_does_not_keep_the_exceeded_budget_alive(): + # The error is created while the internal budget signal is handled, + # but must not adopt its traceback, which keeps the comparison alive. + query = "{ field { " + " ".join(["... on Node { x }"] * 10) + " } }" + with budget_of(5): + errors = validate( + budget_schema, + parse(query, no_location=True), + [OverlappingFieldsCanBeMergedRule], + ) + assert len(errors) == 1 + error = errors[0] + assert error.message.startswith("Query is too complex to validate") + assert error.__traceback__ is None + assert error.__context__ is None + + def aborted_error_does_not_keep_the_exceeded_budget_alive(): + # The unknown field reaches the error limit, so reporting the exceeded + # budget aborts the validation. This happens while the internal budget + # signal is handled, which must not stay attached to the aborted error. + query = "{ unknown field { " + " ".join(["... on Node { x }"] * 10) + " } }" + with budget_of(5): + errors = validate( + budget_schema, + parse(query, no_location=True), + [FieldsOnCorrectTypeRule, OverlappingFieldsCanBeMergedRule], + max_errors=1, + ) + assert errors == [ + {"message": "Cannot query field 'unknown' on type 'Query'."}, + { + "message": "Too many validation errors, error limit reached." + " Validation aborted." + }, + ] + error = errors[-1] + assert isinstance(error, ValidationAbortedError) + assert error.__traceback__ is None + assert error.__context__ is None + def deduplication_alone_would_not_bound_this_query(): # Each `f` selection shares the response name `f` but wraps a distinct # sub-selection (`a0: x`, `a1: x`, ...), so every field has a distinct diff --git a/tests/validation/test_validation.py b/tests/validation/test_validation.py index bd01a7b3..bfd97652 100644 --- a/tests/validation/test_validation.py +++ b/tests/validation/test_validation.py @@ -1,9 +1,13 @@ +import gc +import weakref + import pytest from graphql.error import GraphQLError from graphql.language import parse from graphql.utilities import build_schema -from graphql.validation import ValidationRule, validate +from graphql.validation import FieldsOnCorrectTypeRule, ValidationRule, validate +from graphql.validation.validate import ValidationAbortedError from .harness import test_schema @@ -128,6 +132,84 @@ def limits_to_100_when_max_errors_is_not_passed(): " Validation aborted." } + class ReportWhileHandlingErrorRule(ValidationRule): + def enter_field(self, node, *_args): + try: + raise RuntimeError("Handled error from custom rule!") + except RuntimeError: + self.report_error(GraphQLError("Error from custom rule!", node)) + + def _validate_and_track(rules, count=3): + # Run aborted validations with fresh documents, keep the aborted errors, + # and track the documents and contexts with weak references. The other + # errors are dropped, since errors created while handling an exception + # keep its traceback on purpose. + aborted_errors, refs = [], [] + + class TrackContextRule(ValidationRule): + def __init__(self, context): + super().__init__(context) + refs.append(weakref.ref(context)) + + for _ in range(count): + document = parse(query, no_location=True) + refs.append(weakref.ref(document)) + errors = validate( + test_schema, document, [TrackContextRule, *rules], max_errors=1 + ) + assert len(errors) == 2 + aborted_errors.append(errors[-1]) + del document, errors + gc.collect() + return aborted_errors, refs + + def creates_a_new_error_for_every_aborted_validation(): + first_error = _validate_document(max_errors=1)[-1] + second_error = _validate_document(max_errors=1)[-1] + assert type(first_error) is ValidationAbortedError + assert type(second_error) is ValidationAbortedError + assert first_error == second_error + assert first_error is not second_error + + def returns_the_aborted_error_without_traceback_and_context(): + error = _validate_document(max_errors=1)[-1] + assert isinstance(error, ValidationAbortedError) + assert error.__traceback__ is None + assert error.__context__ is None + assert error.__cause__ is None + + def returns_the_aborted_error_without_context_of_a_handled_error(): + errors = validate( + test_schema, doc, [ReportWhileHandlingErrorRule], max_errors=1 + ) + assert len(errors) == 2 + error = errors[-1] + assert isinstance(error, ValidationAbortedError) + assert error.__traceback__ is None + assert error.__context__ is None + assert error.__cause__ is None + + def does_not_keep_aborted_validations_alive(): + aborted_errors, refs = _validate_and_track([FieldsOnCorrectTypeRule]) + assert all(isinstance(e, ValidationAbortedError) for e in aborted_errors) + assert len(refs) == 6 + assert all(ref() is None for ref in refs) + + def does_not_keep_validations_alive_that_were_aborted_while_handling_errors(): + aborted_errors, refs = _validate_and_track([ReportWhileHandlingErrorRule]) + assert all(isinstance(e, ValidationAbortedError) for e in aborted_errors) + assert len(refs) == 6 + assert all(ref() is None for ref in refs) + + def aborted_error_accepts_the_arguments_of_graphql_error(): + node = doc.definitions[0] + error = ValidationAbortedError( + "Validation aborted.", node, extensions={"code": "ABORTED"} + ) + assert error.message == "Validation aborted." + assert error.nodes == [node] + assert error.extensions == {"code": "ABORTED"} + def pass_through_exceptions_from_rules(): class CustomRule(ValidationRule): def enter_field(self, *_args):