From 21bfe5251391850bffd3202db5486d277d2c8dfe Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Wed, 14 Oct 2026 14:02:32 -0400 Subject: [PATCH 1/3] Pass each interceptor's modifications to the next in InterceptorChain InterceptorChain called every interceptor's modify_* hook with the original context and returned only the last interceptor's result. Any interceptor after one that modified the request, response, or output received the unmodified value, and returning it unchanged, as the base Interceptor does, discarded the earlier modification. For example, a user interceptor added through per-call plugins undid changes an operation's default interceptor made in modify_before_signing. Each interceptor now receives a context holding the value returned by the interceptor before it. modify_before_completion also called each interceptor's modify_before_attempt_completion hook instead of its modify_before_completion hook. --- ...gfix-068484dcc3df4c57abfa1cace59988d1.json | 4 + .../src/smithy_core/interceptors.py | 51 ++++++----- .../tests/unit/test_interceptors.py | 85 +++++++++++++++++++ 3 files changed, 119 insertions(+), 21 deletions(-) create mode 100644 packages/smithy-core/.changes/next-release/smithy-core-bugfix-068484dcc3df4c57abfa1cace59988d1.json create mode 100644 packages/smithy-core/tests/unit/test_interceptors.py diff --git a/packages/smithy-core/.changes/next-release/smithy-core-bugfix-068484dcc3df4c57abfa1cace59988d1.json b/packages/smithy-core/.changes/next-release/smithy-core-bugfix-068484dcc3df4c57abfa1cace59988d1.json new file mode 100644 index 000000000..07a364844 --- /dev/null +++ b/packages/smithy-core/.changes/next-release/smithy-core-bugfix-068484dcc3df4c57abfa1cace59988d1.json @@ -0,0 +1,4 @@ +{ + "type": "bugfix", + "description": "Fixed `InterceptorChain` discarding changes made by `modify_*` hooks. Each interceptor now receives the value returned by the interceptor before it, so an interceptor that returns its input unchanged no longer undoes earlier modifications. `modify_before_completion` also now calls each interceptor's `modify_before_completion` hook instead of `modify_before_attempt_completion`." +} \ No newline at end of file diff --git a/packages/smithy-core/src/smithy_core/interceptors.py b/packages/smithy-core/src/smithy_core/interceptors.py index aca9a4cfa..82217bff5 100644 --- a/packages/smithy-core/src/smithy_core/interceptors.py +++ b/packages/smithy-core/src/smithy_core/interceptors.py @@ -551,10 +551,11 @@ def read_before_execution(self, context: InputContext[Any]) -> None: interceptor.read_before_execution(context) def modify_before_serialization(self, context: InputContext[Any]) -> Any: - request = context.request for interceptor in self._chain: - request = interceptor.modify_before_serialization(context) - return request + context = replace( + context, request=interceptor.modify_before_serialization(context) + ) + return context.request def read_before_serialization(self, context: InputContext[Any]) -> None: for interceptor in self._chain: @@ -565,20 +566,23 @@ def read_after_serialization(self, context: RequestContext[Any, Any]) -> None: interceptor.read_after_serialization(context) def modify_before_retry_loop(self, context: RequestContext[Any, Any]) -> Any: - transport_request = context.transport_request for interceptor in self._chain: - transport_request = interceptor.modify_before_retry_loop(context) - return transport_request + context = replace( + context, + transport_request=interceptor.modify_before_retry_loop(context), + ) + return context.transport_request def read_before_attempt(self, context: RequestContext[Any, Any]) -> None: for interceptor in self._chain: interceptor.read_before_attempt(context) def modify_before_signing(self, context: RequestContext[Any, Any]) -> Any: - transport_request = context.transport_request for interceptor in self._chain: - transport_request = interceptor.modify_before_signing(context) - return transport_request + context = replace( + context, transport_request=interceptor.modify_before_signing(context) + ) + return context.transport_request def read_before_signing(self, context: RequestContext[Any, Any]) -> None: for interceptor in self._chain: @@ -589,10 +593,11 @@ def read_after_signing(self, context: RequestContext[Any, Any]) -> None: interceptor.read_after_signing(context) def modify_before_transmit(self, context: RequestContext[Any, Any]) -> Any: - transport_request = context.transport_request for interceptor in self._chain: - transport_request = interceptor.modify_before_transmit(context) - return transport_request + context = replace( + context, transport_request=interceptor.modify_before_transmit(context) + ) + return context.transport_request def read_before_transmit(self, context: RequestContext[Any, Any]) -> None: for interceptor in self._chain: @@ -605,10 +610,12 @@ def read_after_transmit(self, context: ResponseContext[Any, Any, Any]) -> None: def modify_before_deserialization( self, context: ResponseContext[Any, Any, Any] ) -> Any: - transport_response = context.transport_response for interceptor in self._chain: - transport_response = interceptor.modify_before_deserialization(context) - return transport_response + context = replace( + context, + transport_response=interceptor.modify_before_deserialization(context), + ) + return context.transport_response def read_before_deserialization( self, context: ResponseContext[Any, Any, Any] @@ -625,10 +632,11 @@ def read_after_deserialization( def modify_before_attempt_completion( self, context: OutputContext[Any, Any, Any, Any | None] ) -> Any | Exception: - response = context.response for interceptor in self._chain: - response = interceptor.modify_before_attempt_completion(context) - return response + context = replace( + context, response=interceptor.modify_before_attempt_completion(context) + ) + return context.response def read_after_attempt( self, context: OutputContext[Any, Any, Any, Any | None] @@ -639,10 +647,11 @@ def read_after_attempt( def modify_before_completion( self, context: OutputContext[Any, Any, Any | None, Any | None] ) -> Any | Exception: - response = context.response for interceptor in self._chain: - response = interceptor.modify_before_attempt_completion(context) - return response + context = replace( + context, response=interceptor.modify_before_completion(context) + ) + return context.response def read_after_execution( self, context: OutputContext[Any, Any, Any | None, Any | None] diff --git a/packages/smithy-core/tests/unit/test_interceptors.py b/packages/smithy-core/tests/unit/test_interceptors.py new file mode 100644 index 000000000..b9cdcd662 --- /dev/null +++ b/packages/smithy-core/tests/unit/test_interceptors.py @@ -0,0 +1,85 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +from typing import Any, cast + +import pytest +from smithy_core.interceptors import ( + InputContext, + Interceptor, + InterceptorChain, + OutputContext, + RequestContext, + ResponseContext, +) +from smithy_core.types import TypedProperties + + +def _context() -> OutputContext[Any, Any, Any, Any]: + return OutputContext( + request=cast(Any, ""), + transport_request="", + transport_response="", + response=cast(Any, ""), + properties=TypedProperties(), + ) + + +MODIFY_HOOKS = [ + "modify_before_serialization", + "modify_before_retry_loop", + "modify_before_signing", + "modify_before_transmit", + "modify_before_deserialization", + "modify_before_attempt_completion", + "modify_before_completion", +] + + +class _Appender(Interceptor[Any, Any, Any, Any]): + """Appends a suffix to the value each modify hook receives.""" + + def __init__(self, suffix: str) -> None: + self._suffix = suffix + + def modify_before_serialization(self, context: InputContext[Any]) -> Any: + return context.request + self._suffix + + def modify_before_retry_loop(self, context: RequestContext[Any, Any]) -> Any: + return context.transport_request + self._suffix + + def modify_before_signing(self, context: RequestContext[Any, Any]) -> Any: + return context.transport_request + self._suffix + + def modify_before_transmit(self, context: RequestContext[Any, Any]) -> Any: + return context.transport_request + self._suffix + + def modify_before_deserialization( + self, context: ResponseContext[Any, Any, Any] + ) -> Any: + return context.transport_response + self._suffix + + def modify_before_attempt_completion( + self, context: OutputContext[Any, Any, Any, Any] + ) -> Any: + return cast(str, context.response) + self._suffix + + def modify_before_completion( + self, context: OutputContext[Any, Any, Any, Any] + ) -> Any: + return cast(str, context.response) + self._suffix + + +@pytest.mark.parametrize("hook", MODIFY_HOOKS) +def test_chain_passes_each_modification_to_the_next_interceptor(hook: str) -> None: + chain = InterceptorChain([_Appender("a"), _Appender("b")]) + + assert getattr(chain, hook)(_context()) == "ab" + + +@pytest.mark.parametrize("hook", MODIFY_HOOKS) +def test_chain_keeps_modifications_from_earlier_interceptors(hook: str) -> None: + # The base interceptor returns what it was given, which must be the value the + # interceptor before it returned, not the original. + chain = InterceptorChain([_Appender("a"), Interceptor()]) + + assert getattr(chain, hook)(_context()) == "a" From a5550abb2124ef7a9079f8c1d5b3abd5c0d0a91f Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Wed, 14 Oct 2026 14:26:55 -0400 Subject: [PATCH 2/3] Run every interceptor's read hook before raising in InterceptorChain When an interceptor raised in a read_* hook, InterceptorChain let the exception propagate immediately, so the interceptors after it never saw that stage. Read hooks can't change the request or response, so one failing shouldn't keep the others from running. Each read hook now runs on every interceptor, then the last exception is raised and earlier ones are logged and dropped. read_after_execution already ran every interceptor and now logs the exceptions it drops too. modify_* hooks still stop at the first exception, since later interceptors would receive a value that was never produced. --- ...gfix-59eb8351a1d040ee910f2c25c758ca12.json | 4 ++ .../src/smithy_core/interceptors.py | 70 +++++++++++++------ .../tests/unit/test_interceptors.py | 55 +++++++++++++++ 3 files changed, 106 insertions(+), 23 deletions(-) create mode 100644 packages/smithy-core/.changes/next-release/smithy-core-bugfix-59eb8351a1d040ee910f2c25c758ca12.json diff --git a/packages/smithy-core/.changes/next-release/smithy-core-bugfix-59eb8351a1d040ee910f2c25c758ca12.json b/packages/smithy-core/.changes/next-release/smithy-core-bugfix-59eb8351a1d040ee910f2c25c758ca12.json new file mode 100644 index 000000000..61fa10ab9 --- /dev/null +++ b/packages/smithy-core/.changes/next-release/smithy-core-bugfix-59eb8351a1d040ee910f2c25c758ca12.json @@ -0,0 +1,4 @@ +{ + "type": "bugfix", + "description": "Fixed `InterceptorChain` skipping later interceptors when one raised in a `read_*` hook. Every interceptor's read hook now runs, then the last exception is raised and earlier ones are logged." +} \ No newline at end of file diff --git a/packages/smithy-core/src/smithy_core/interceptors.py b/packages/smithy-core/src/smithy_core/interceptors.py index 82217bff5..453ac076a 100644 --- a/packages/smithy-core/src/smithy_core/interceptors.py +++ b/packages/smithy-core/src/smithy_core/interceptors.py @@ -1,6 +1,7 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 -from collections.abc import Sequence +import logging +from collections.abc import Callable, Sequence from dataclasses import dataclass, replace from typing import Any @@ -8,6 +9,8 @@ from .interfaces import TypedProperties from .serializers import SerializeableShape +_LOGGER = logging.getLogger(__name__) + @dataclass(kw_only=True, frozen=True, slots=True) class InputContext[Request: SerializeableShape]: @@ -547,8 +550,7 @@ def __repr__(self) -> str: return f"InterceptorChain(chain={self._chain!r})" def read_before_execution(self, context: InputContext[Any]) -> None: - for interceptor in self._chain: - interceptor.read_before_execution(context) + self._invoke_all(lambda interceptor: interceptor.read_before_execution(context)) def modify_before_serialization(self, context: InputContext[Any]) -> Any: for interceptor in self._chain: @@ -558,12 +560,14 @@ def modify_before_serialization(self, context: InputContext[Any]) -> Any: return context.request def read_before_serialization(self, context: InputContext[Any]) -> None: - for interceptor in self._chain: - interceptor.read_before_serialization(context) + self._invoke_all( + lambda interceptor: interceptor.read_before_serialization(context) + ) def read_after_serialization(self, context: RequestContext[Any, Any]) -> None: - for interceptor in self._chain: - interceptor.read_after_serialization(context) + self._invoke_all( + lambda interceptor: interceptor.read_after_serialization(context) + ) def modify_before_retry_loop(self, context: RequestContext[Any, Any]) -> Any: for interceptor in self._chain: @@ -574,8 +578,7 @@ def modify_before_retry_loop(self, context: RequestContext[Any, Any]) -> Any: return context.transport_request def read_before_attempt(self, context: RequestContext[Any, Any]) -> None: - for interceptor in self._chain: - interceptor.read_before_attempt(context) + self._invoke_all(lambda interceptor: interceptor.read_before_attempt(context)) def modify_before_signing(self, context: RequestContext[Any, Any]) -> Any: for interceptor in self._chain: @@ -585,12 +588,10 @@ def modify_before_signing(self, context: RequestContext[Any, Any]) -> Any: return context.transport_request def read_before_signing(self, context: RequestContext[Any, Any]) -> None: - for interceptor in self._chain: - interceptor.read_before_signing(context) + self._invoke_all(lambda interceptor: interceptor.read_before_signing(context)) def read_after_signing(self, context: RequestContext[Any, Any]) -> None: - for interceptor in self._chain: - interceptor.read_after_signing(context) + self._invoke_all(lambda interceptor: interceptor.read_after_signing(context)) def modify_before_transmit(self, context: RequestContext[Any, Any]) -> Any: for interceptor in self._chain: @@ -600,12 +601,10 @@ def modify_before_transmit(self, context: RequestContext[Any, Any]) -> Any: return context.transport_request def read_before_transmit(self, context: RequestContext[Any, Any]) -> None: - for interceptor in self._chain: - interceptor.read_before_transmit(context) + self._invoke_all(lambda interceptor: interceptor.read_before_transmit(context)) def read_after_transmit(self, context: ResponseContext[Any, Any, Any]) -> None: - for interceptor in self._chain: - interceptor.read_after_transmit(context) + self._invoke_all(lambda interceptor: interceptor.read_after_transmit(context)) def modify_before_deserialization( self, context: ResponseContext[Any, Any, Any] @@ -620,14 +619,16 @@ def modify_before_deserialization( def read_before_deserialization( self, context: ResponseContext[Any, Any, Any] ) -> None: - for interceptor in self._chain: - interceptor.read_before_deserialization(context) + self._invoke_all( + lambda interceptor: interceptor.read_before_deserialization(context) + ) def read_after_deserialization( self, context: OutputContext[Any, Any, Any, Any] ) -> None: - for interceptor in self._chain: - interceptor.read_after_deserialization(context) + self._invoke_all( + lambda interceptor: interceptor.read_after_deserialization(context) + ) def modify_before_attempt_completion( self, context: OutputContext[Any, Any, Any, Any | None] @@ -641,8 +642,7 @@ def modify_before_attempt_completion( def read_after_attempt( self, context: OutputContext[Any, Any, Any, Any | None] ) -> None: - for interceptor in self._chain: - interceptor.read_after_attempt(context) + self._invoke_all(lambda interceptor: interceptor.read_after_attempt(context)) def modify_before_completion( self, context: OutputContext[Any, Any, Any | None, Any | None] @@ -662,7 +662,31 @@ def read_after_execution( try: interceptor.read_after_execution(context) except Exception as e: + if exception is not None: + _log_dropped(exception) context = replace(context, response=e) exception = e if exception is not None: raise exception + + def _invoke_all(self, hook: Callable[[AnyInterceptor], None]) -> None: + # A failing read hook mustn't keep later interceptors from running, so + # every interceptor is invoked before the last exception is raised. + exception: Exception | None = None + for interceptor in self._chain: + try: + hook(interceptor) + except Exception as e: + if exception is not None: + _log_dropped(exception) + exception = e + if exception is not None: + raise exception + + +def _log_dropped(exception: Exception) -> None: + _LOGGER.error( + "An interceptor raised an exception that was dropped because a later " + "interceptor raised one too.", + exc_info=exception, + ) diff --git a/packages/smithy-core/tests/unit/test_interceptors.py b/packages/smithy-core/tests/unit/test_interceptors.py index b9cdcd662..f14b03e88 100644 --- a/packages/smithy-core/tests/unit/test_interceptors.py +++ b/packages/smithy-core/tests/unit/test_interceptors.py @@ -1,6 +1,7 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 from typing import Any, cast +from unittest.mock import Mock import pytest from smithy_core.interceptors import ( @@ -24,6 +25,21 @@ def _context() -> OutputContext[Any, Any, Any, Any]: ) +READ_HOOKS = [ + "read_before_execution", + "read_before_serialization", + "read_after_serialization", + "read_before_attempt", + "read_before_signing", + "read_after_signing", + "read_before_transmit", + "read_after_transmit", + "read_before_deserialization", + "read_after_deserialization", + "read_after_attempt", + "read_after_execution", +] + MODIFY_HOOKS = [ "modify_before_serialization", "modify_before_retry_loop", @@ -83,3 +99,42 @@ def test_chain_keeps_modifications_from_earlier_interceptors(hook: str) -> None: chain = InterceptorChain([_Appender("a"), Interceptor()]) assert getattr(chain, hook)(_context()) == "a" + + +def _raising(name: str, calls: list[str]) -> Any: + """An interceptor whose every hook records its name and raises.""" + + def raise_error(context: Any) -> None: + calls.append(name) + raise RuntimeError(name) + + interceptor = Mock() + for hook in READ_HOOKS + MODIFY_HOOKS: + getattr(interceptor, hook).side_effect = raise_error + return interceptor + + +@pytest.mark.parametrize("hook", READ_HOOKS) +def test_read_hook_runs_every_interceptor_and_raises_the_last_error( + hook: str, caplog: pytest.LogCaptureFixture +) -> None: + calls: list[str] = [] + chain = InterceptorChain([_raising(name, calls) for name in ("1", "2", "3")]) + + with pytest.raises(RuntimeError, match="3"): + getattr(chain, hook)(_context()) + + assert calls == ["1", "2", "3"] + dropped = [str(record.exc_info[1]) for record in caplog.records if record.exc_info] + assert dropped == ["1", "2"] + + +@pytest.mark.parametrize("hook", MODIFY_HOOKS) +def test_modify_hook_stops_at_the_first_error(hook: str) -> None: + calls: list[str] = [] + chain = InterceptorChain([_raising(name, calls) for name in ("1", "2")]) + + with pytest.raises(RuntimeError, match="1"): + getattr(chain, hook)(_context()) + + assert calls == ["1"] From 26a690b327577ad9394620d122ce6981b7eb9cec Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Thu, 15 Oct 2026 11:03:03 -0400 Subject: [PATCH 3/3] Use the modified request in the pipeline and tighten InterceptorChain The pipeline serialized and resolved endpoints from call.input, so the request returned by modify_before_serialization never reached the wire. It now uses the modified request, including on the serialization error path. InterceptorChain now raises TypeError when a modify_* hook returns None, passes errors to later interceptors in read_after_attempt as read_after_execution does, logs dropped errors at debug, and reuses the context when a hook returns its input unchanged. Read-hook docstrings now describe running every interceptor before raising. --- ...gfix-21165dd714af453ea800bc3004248979.json | 4 + ...gfix-59eb8351a1d040ee910f2c25c758ca12.json | 2 +- .../smithy-core/src/smithy_core/aio/client.py | 13 +- .../src/smithy_core/interceptors.py | 146 ++++++++++++------ .../tests/unit/aio/_pipeline_harness.py | 6 + .../smithy-core/tests/unit/aio/test_client.py | 59 ++++++- .../tests/unit/test_interceptors.py | 102 +++++++++--- 7 files changed, 255 insertions(+), 77 deletions(-) create mode 100644 packages/smithy-core/.changes/next-release/smithy-core-bugfix-21165dd714af453ea800bc3004248979.json diff --git a/packages/smithy-core/.changes/next-release/smithy-core-bugfix-21165dd714af453ea800bc3004248979.json b/packages/smithy-core/.changes/next-release/smithy-core-bugfix-21165dd714af453ea800bc3004248979.json new file mode 100644 index 000000000..a5d7f3867 --- /dev/null +++ b/packages/smithy-core/.changes/next-release/smithy-core-bugfix-21165dd714af453ea800bc3004248979.json @@ -0,0 +1,4 @@ +{ + "type": "bugfix", + "description": "Fixed the request pipeline ignoring the request returned by `modify_before_serialization`. The modified request is now serialized, used for endpoint resolution, and passed to later hooks if serialization fails. `InterceptorChain` now also raises a `TypeError` when a `modify_*` hook returns `None`, instead of passing `None` to the next interceptor." +} \ No newline at end of file diff --git a/packages/smithy-core/.changes/next-release/smithy-core-bugfix-59eb8351a1d040ee910f2c25c758ca12.json b/packages/smithy-core/.changes/next-release/smithy-core-bugfix-59eb8351a1d040ee910f2c25c758ca12.json index 61fa10ab9..4b79caa1d 100644 --- a/packages/smithy-core/.changes/next-release/smithy-core-bugfix-59eb8351a1d040ee910f2c25c758ca12.json +++ b/packages/smithy-core/.changes/next-release/smithy-core-bugfix-59eb8351a1d040ee910f2c25c758ca12.json @@ -1,4 +1,4 @@ { "type": "bugfix", - "description": "Fixed `InterceptorChain` skipping later interceptors when one raised in a `read_*` hook. Every interceptor's read hook now runs, then the last exception is raised and earlier ones are logged." + "description": "Fixed `InterceptorChain` skipping later interceptors when one raised in a `read_*` hook. Every interceptor's read hook now runs, then the last exception is raised and earlier ones are logged. `read_after_attempt` also now passes each exception to later interceptors as the `response`, as `read_after_execution` already did." } \ No newline at end of file diff --git a/packages/smithy-core/src/smithy_core/aio/client.py b/packages/smithy-core/src/smithy_core/aio/client.py index c358a9c1b..c26435a4c 100644 --- a/packages/smithy-core/src/smithy_core/aio/client.py +++ b/packages/smithy-core/src/smithy_core/aio/client.py @@ -322,10 +322,9 @@ async def _handle_execution[I: SerializeableShape, O: DeserializeableShape]( call: ClientCall[I, O], request_future: Future[RequestContext[I, TRequest]] | None, ) -> OutputContext[I, O, TRequest | None, TResponse | None]: + interceptor = call.interceptor + input_context = InputContext(request=call.input, properties=call.context) try: - interceptor = call.interceptor - - input_context = InputContext(request=call.input, properties=call.context) interceptor.read_before_execution(input_context) input_context = replace( @@ -338,7 +337,7 @@ async def _handle_execution[I: SerializeableShape, O: DeserializeableShape]( transport_request = self.protocol.serialize_request( operation=call.operation, - input=call.input, + input=input_context.request, endpoint=_UNRESOLVED, context=input_context.properties, ) @@ -353,11 +352,11 @@ async def _handle_execution[I: SerializeableShape, O: DeserializeableShape]( ) except Exception as e: return OutputContext( - request=call.input, + request=input_context.request, response=e, transport_request=None, transport_response=None, - properties=call.context, + properties=input_context.properties, ) try: @@ -449,7 +448,7 @@ async def _handle_attempt[I: SerializeableShape, O: DeserializeableShape]( endpoint_params = EndpointResolverParams( operation=call.operation, - input=call.input, + input=request_context.request, context=request_context.properties, ) _LOGGER.debug("Calling endpoint resolver with params: %s", endpoint_params) diff --git a/packages/smithy-core/src/smithy_core/interceptors.py b/packages/smithy-core/src/smithy_core/interceptors.py index 453ac076a..381892f31 100644 --- a/packages/smithy-core/src/smithy_core/interceptors.py +++ b/packages/smithy-core/src/smithy_core/interceptors.py @@ -125,8 +125,11 @@ def read_before_serialization(self, context: InputContext[Request]) -> None: The `request` of the context will always be available. Other static properties will be None. - If exceptions are thrown by this hook, execution will jump to - `modify_before_completion` with the thrown exception as the `response`. + Exceptions thrown by this hook will be stored until all interceptors have had + their `read_before_serialization` invoked. Execution will then jump to + `modify_before_completion` with the thrown exception as the `response`. If + multiple `read_before_serialization` methods throw exceptions, the latest will + be used and earlier ones will be logged and dropped. """ def read_after_serialization( @@ -145,8 +148,11 @@ def read_after_serialization( The `request` and `transport_request` of the context will always be available. Other static properties will be None. - If exceptions are thrown by this hook, execution will jump to - `modify_before_completion` with the thrown exception as the `response`. + Exceptions thrown by this hook will be stored until all interceptors have had + their `read_after_serialization` invoked. Execution will then jump to + `modify_before_completion` with the thrown exception as the `response`. If + multiple `read_after_serialization` methods throw exceptions, the latest will be + used and earlier ones will be logged and dropped. """ def modify_before_retry_loop( @@ -238,8 +244,11 @@ def read_before_signing( not include changes made in previous attempts (e.g. by request signers or other interceptors). - If exceptions are thrown by this hook, execution will jump to + Exceptions thrown by this hook will be stored until all interceptors have had + their `read_before_signing` invoked. Execution will then jump to `modify_before_attempt_completion` with the thrown exception as the `response`. + If multiple `read_before_signing` methods throw exceptions, the latest will be + used and earlier ones will be logged and dropped. """ def read_after_signing( @@ -261,8 +270,11 @@ def read_after_signing( not include changes made in previous attempts (e.g. by request signers or other interceptors). - If exceptions are thrown by this hook, execution will jump to + Exceptions thrown by this hook will be stored until all interceptors have had + their `read_after_signing` invoked. Execution will then jump to `modify_before_attempt_completion` with the thrown exception as the `response`. + If multiple `read_after_signing` methods throw exceptions, the latest will be + used and earlier ones will be logged and dropped. """ def modify_before_transmit( @@ -311,8 +323,11 @@ def read_before_transmit( not include changes made in previous attempts (e.g. by request signers or other interceptors). - If exceptions are thrown by this hook, execution will jump to + Exceptions thrown by this hook will be stored until all interceptors have had + their `read_before_transmit` invoked. Execution will then jump to `modify_before_attempt_completion` with the thrown exception as the `response`. + If multiple `read_before_transmit` methods throw exceptions, the latest will be + used and earlier ones will be logged and dropped. """ def read_after_transmit( @@ -337,8 +352,11 @@ def read_after_transmit( retries, the context will not include changes made in previous attempts (e.g. by request signers or other interceptors). - If exceptions are thrown by this hook, execution will jump to + Exceptions thrown by this hook will be stored until all interceptors have had + their `read_after_transmit` invoked. Execution will then jump to `modify_before_attempt_completion` with the thrown exception as the `response`. + If multiple `read_after_transmit` methods throw exceptions, the latest will be + used and earlier ones will be logged and dropped. """ def modify_before_deserialization( @@ -392,8 +410,11 @@ def read_before_deserialization( retries, the context will not include changes made in previous attempts (e.g. by request signers or other interceptors). - If exceptions are thrown by this hook, execution will jump to + Exceptions thrown by this hook will be stored until all interceptors have had + their `read_before_deserialization` invoked. Execution will then jump to `modify_before_attempt_completion` with the thrown exception as the `response`. + If multiple `read_before_deserialization` methods throw exceptions, the latest + will be used and earlier ones will be logged and dropped. """ def read_after_deserialization( @@ -417,8 +438,11 @@ def read_after_deserialization( include changes made in previous attempts (e.g. by request signers or other interceptors). - If exceptions are thrown by this hook, execution will jump to + Exceptions thrown by this hook will be stored until all interceptors have had + their `read_after_deserialization` invoked. Execution will then jump to `modify_before_attempt_completion` with the thrown exception as the `response`. + If multiple `read_after_deserialization` methods throw exceptions, the latest + will be used and earlier ones will be logged and dropped. """ def modify_before_attempt_completion( @@ -472,9 +496,10 @@ def read_after_attempt( other interceptors). Exceptions thrown by this hook will be stored until all interceptors have had - their `read_after_attempt` invoked. If multiple `read_after_attempt` methods - throw exceptions, the latest will be used and earlier ones will be logged and - dropped. If the retry strategy determines that the execution is retryable, + their `read_after_attempt` invoked, and later interceptors will receive the + most recent exception as the `response`. If multiple `read_after_attempt` + methods throw exceptions, the latest will be used and earlier ones will be + logged and dropped. If the retry strategy determines that the execution is retryable, execution will then jump to `read_before_attempt`. Otherwise, execution will jump to `modify_before_completion` with the thrown exception as the `response`. """ @@ -524,8 +549,9 @@ def read_after_execution( proceeded far enough for them to be generated. Exceptions thrown by this hook will be stored until all interceptors have had - their `read_after_execution` invoked. The exception will then be treated as the - final response. If multiple `read_after_execution` methods throw exceptions, + their `read_after_execution` invoked, and later interceptors will receive the + most recent exception as the `response`. The exception will then be treated as + the final response. If multiple `read_after_execution` methods throw exceptions, the latest will be used and earlier ones will be logged and dropped. """ @@ -554,9 +580,11 @@ def read_before_execution(self, context: InputContext[Any]) -> None: def modify_before_serialization(self, context: InputContext[Any]) -> Any: for interceptor in self._chain: - context = replace( - context, request=interceptor.modify_before_serialization(context) - ) + request = interceptor.modify_before_serialization(context) + _check_returned(request, interceptor, "modify_before_serialization") + # Most hooks return what they were given, so only copy on a change. + if request is not context.request: + context = replace(context, request=request) return context.request def read_before_serialization(self, context: InputContext[Any]) -> None: @@ -571,10 +599,10 @@ def read_after_serialization(self, context: RequestContext[Any, Any]) -> None: def modify_before_retry_loop(self, context: RequestContext[Any, Any]) -> Any: for interceptor in self._chain: - context = replace( - context, - transport_request=interceptor.modify_before_retry_loop(context), - ) + transport_request = interceptor.modify_before_retry_loop(context) + _check_returned(transport_request, interceptor, "modify_before_retry_loop") + if transport_request is not context.transport_request: + context = replace(context, transport_request=transport_request) return context.transport_request def read_before_attempt(self, context: RequestContext[Any, Any]) -> None: @@ -582,9 +610,10 @@ def read_before_attempt(self, context: RequestContext[Any, Any]) -> None: def modify_before_signing(self, context: RequestContext[Any, Any]) -> Any: for interceptor in self._chain: - context = replace( - context, transport_request=interceptor.modify_before_signing(context) - ) + transport_request = interceptor.modify_before_signing(context) + _check_returned(transport_request, interceptor, "modify_before_signing") + if transport_request is not context.transport_request: + context = replace(context, transport_request=transport_request) return context.transport_request def read_before_signing(self, context: RequestContext[Any, Any]) -> None: @@ -595,9 +624,10 @@ def read_after_signing(self, context: RequestContext[Any, Any]) -> None: def modify_before_transmit(self, context: RequestContext[Any, Any]) -> Any: for interceptor in self._chain: - context = replace( - context, transport_request=interceptor.modify_before_transmit(context) - ) + transport_request = interceptor.modify_before_transmit(context) + _check_returned(transport_request, interceptor, "modify_before_transmit") + if transport_request is not context.transport_request: + context = replace(context, transport_request=transport_request) return context.transport_request def read_before_transmit(self, context: RequestContext[Any, Any]) -> None: @@ -610,10 +640,12 @@ def modify_before_deserialization( self, context: ResponseContext[Any, Any, Any] ) -> Any: for interceptor in self._chain: - context = replace( - context, - transport_response=interceptor.modify_before_deserialization(context), + transport_response = interceptor.modify_before_deserialization(context) + _check_returned( + transport_response, interceptor, "modify_before_deserialization" ) + if transport_response is not context.transport_response: + context = replace(context, transport_response=transport_response) return context.transport_response def read_before_deserialization( @@ -634,58 +666,80 @@ def modify_before_attempt_completion( self, context: OutputContext[Any, Any, Any, Any | None] ) -> Any | Exception: for interceptor in self._chain: - context = replace( - context, response=interceptor.modify_before_attempt_completion(context) - ) + response = interceptor.modify_before_attempt_completion(context) + _check_returned(response, interceptor, "modify_before_attempt_completion") + if response is not context.response: + context = replace(context, response=response) return context.response def read_after_attempt( self, context: OutputContext[Any, Any, Any, Any | None] ) -> None: - self._invoke_all(lambda interceptor: interceptor.read_after_attempt(context)) + self._invoke_all_passing_errors( + context, lambda interceptor, ctx: interceptor.read_after_attempt(ctx) + ) def modify_before_completion( self, context: OutputContext[Any, Any, Any | None, Any | None] ) -> Any | Exception: for interceptor in self._chain: - context = replace( - context, response=interceptor.modify_before_completion(context) - ) + response = interceptor.modify_before_completion(context) + _check_returned(response, interceptor, "modify_before_completion") + if response is not context.response: + context = replace(context, response=response) return context.response def read_after_execution( self, context: OutputContext[Any, Any, Any | None, Any | None] ) -> None: + self._invoke_all_passing_errors( + context, lambda interceptor, ctx: interceptor.read_after_execution(ctx) + ) + + def _invoke_all(self, hook: Callable[[AnyInterceptor], None]) -> None: + # A failing read hook mustn't keep later interceptors from running, so + # every interceptor is invoked before the last exception is raised. exception: Exception | None = None for interceptor in self._chain: - # Every one of these is supposed to be guaranteed to be called. try: - interceptor.read_after_execution(context) + hook(interceptor) except Exception as e: if exception is not None: _log_dropped(exception) - context = replace(context, response=e) exception = e if exception is not None: raise exception - def _invoke_all(self, hook: Callable[[AnyInterceptor], None]) -> None: - # A failing read hook mustn't keep later interceptors from running, so - # every interceptor is invoked before the last exception is raised. + def _invoke_all_passing_errors[C: OutputContext[Any, Any, Any, Any]]( + self, context: C, hook: Callable[[AnyInterceptor, C], None] + ) -> None: + # Like _invoke_all, but each exception also becomes the response that later + # interceptors see, since it has replaced the attempt's or execution's result. exception: Exception | None = None for interceptor in self._chain: try: - hook(interceptor) + hook(interceptor, context) except Exception as e: if exception is not None: _log_dropped(exception) + context = replace(context, response=e) exception = e if exception is not None: raise exception +def _check_returned(value: Any, interceptor: AnyInterceptor, hook: str) -> None: + # A modify hook that changes its value in place and forgets to return it would + # otherwise hand None to the next interceptor, which then fails confusingly. + if value is None: + raise TypeError( + f"{interceptor!r}.{hook} returned None. Modify hooks must return the " + "value to use, even if they changed it in place." + ) + + def _log_dropped(exception: Exception) -> None: - _LOGGER.error( + _LOGGER.debug( "An interceptor raised an exception that was dropped because a later " "interceptor raised one too.", exc_info=exception, diff --git a/packages/smithy-core/tests/unit/aio/_pipeline_harness.py b/packages/smithy-core/tests/unit/aio/_pipeline_harness.py index 52cb2712a..c4420181a 100644 --- a/packages/smithy-core/tests/unit/aio/_pipeline_harness.py +++ b/packages/smithy-core/tests/unit/aio/_pipeline_harness.py @@ -147,6 +147,7 @@ async def close(self) -> None: class StubProtocol: def __init__(self) -> None: self.serialize_request_calls = 0 + self.serialized_inputs: list[Any] = [] self.set_service_endpoint_calls = 0 self.deserialize_response_calls = 0 self.create_event_publisher_calls = 0 @@ -166,6 +167,7 @@ def id(self) -> ShapeID: def serialize_request(self, **kwargs: Any) -> StubRequest: self.serialize_request_calls += 1 + self.serialized_inputs.append(kwargs["input"]) return StubRequest() def set_service_endpoint(self, *, request: Any, endpoint: Any) -> Any: @@ -198,7 +200,11 @@ def __init__(self) -> None: class StubEndpointResolver: + def __init__(self) -> None: + self.resolved_inputs: list[Any] = [] + async def resolve_endpoint(self, params: EndpointResolverParams[Any]) -> Any: + self.resolved_inputs.append(params.input) return StubEndpoint() diff --git a/packages/smithy-core/tests/unit/aio/test_client.py b/packages/smithy-core/tests/unit/aio/test_client.py index f971d142b..1e3f3d37a 100644 --- a/packages/smithy-core/tests/unit/aio/test_client.py +++ b/packages/smithy-core/tests/unit/aio/test_client.py @@ -10,7 +10,13 @@ from smithy_core.aio.eventstream import DuplexEventStream, InputEventStream from smithy_core.aio.interfaces import ClientTransport from smithy_core.auth import AuthOption -from smithy_core.exceptions import CallError, UnsupportedTransportError +from smithy_core.exceptions import CallError, SmithyError, UnsupportedTransportError +from smithy_core.interceptors import ( + InputContext, + Interceptor, + InterceptorChain, + OutputContext, +) from smithy_core.response import EMPTY_RESPONSE_METADATA from smithy_core.shapes import ShapeID from smithy_core.types import TypedProperties @@ -18,8 +24,10 @@ from ._pipeline_harness import ( DuplexTransport, NonDuplexTransport, + StubEndpointResolver, StubEvent, StubEventReceiver, + StubInput, StubOutput, UndeclaredTransport, client_call, @@ -172,6 +180,55 @@ async def test_auth_option_signer_properties_override_scheme_defaults() -> None: } +class _ReplaceInput(Interceptor[Any, Any, Any, Any]): + """Replaces the input before serialization and records what later hooks see.""" + + def __init__(self, fail_after_modifying: bool = False) -> None: + self.replacement = StubInput() + self.fail_after_modifying = fail_after_modifying + self.completion_request: Any = None + + def modify_before_serialization(self, context: InputContext[Any]) -> Any: + return self.replacement + + def read_before_serialization(self, context: InputContext[Any]) -> None: + if self.fail_after_modifying: + raise RuntimeError("boom") + + def modify_before_completion( + self, context: OutputContext[Any, Any, Any, Any] + ) -> Any: + self.completion_request = context.request + return context.response + + +async def test_modified_input_is_serialized_and_used_to_resolve_endpoint() -> None: + harness = pipeline_harness(NonDuplexTransport()) + interceptor = _ReplaceInput() + resolver = StubEndpointResolver() + call = replace( + client_call(), + interceptor=InterceptorChain([interceptor]), + endpoint_resolver=resolver, + ) + + await harness.pipeline(call) + + assert harness.protocol.serialized_inputs == [interceptor.replacement] + assert resolver.resolved_inputs == [interceptor.replacement] + + +async def test_modified_input_reaches_completion_when_serialization_fails() -> None: + harness = pipeline_harness(NonDuplexTransport()) + interceptor = _ReplaceInput(fail_after_modifying=True) + call = replace(client_call(), interceptor=InterceptorChain([interceptor])) + + with pytest.raises(SmithyError): + await harness.pipeline(call) + + assert interceptor.completion_request is interceptor.replacement + + class _CountingTransport: def __init__(self) -> None: self.close_calls = 0 diff --git a/packages/smithy-core/tests/unit/test_interceptors.py b/packages/smithy-core/tests/unit/test_interceptors.py index f14b03e88..1f0cf3567 100644 --- a/packages/smithy-core/tests/unit/test_interceptors.py +++ b/packages/smithy-core/tests/unit/test_interceptors.py @@ -1,5 +1,6 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 +import logging from typing import Any, cast from unittest.mock import Mock @@ -16,11 +17,12 @@ def _context() -> OutputContext[Any, Any, Any, Any]: + # Each field holds its own name, so a hook that returns the wrong field shows up. return OutputContext( - request=cast(Any, ""), - transport_request="", - transport_response="", - response=cast(Any, ""), + request=cast(Any, "request"), + transport_request="transport_request", + transport_response="transport_response", + response=cast(Any, "response"), properties=TypedProperties(), ) @@ -40,15 +42,16 @@ def _context() -> OutputContext[Any, Any, Any, Any]: "read_after_execution", ] -MODIFY_HOOKS = [ - "modify_before_serialization", - "modify_before_retry_loop", - "modify_before_signing", - "modify_before_transmit", - "modify_before_deserialization", - "modify_before_attempt_completion", - "modify_before_completion", -] +# Each modify hook mapped to the context field it replaces. +MODIFY_HOOKS = { + "modify_before_serialization": "request", + "modify_before_retry_loop": "transport_request", + "modify_before_signing": "transport_request", + "modify_before_transmit": "transport_request", + "modify_before_deserialization": "transport_response", + "modify_before_attempt_completion": "response", + "modify_before_completion": "response", +} class _Appender(Interceptor[Any, Any, Any, Any]): @@ -85,20 +88,47 @@ def modify_before_completion( return cast(str, context.response) + self._suffix -@pytest.mark.parametrize("hook", MODIFY_HOOKS) -def test_chain_passes_each_modification_to_the_next_interceptor(hook: str) -> None: +@pytest.mark.parametrize("hook,field", MODIFY_HOOKS.items()) +def test_chain_passes_each_modification_to_the_next_interceptor( + hook: str, field: str +) -> None: + # This also covers a default interceptor undoing earlier changes, since the base + # hooks return the context's value just as _Appender reads it. chain = InterceptorChain([_Appender("a"), _Appender("b")]) - assert getattr(chain, hook)(_context()) == "ab" + assert getattr(chain, hook)(_context()) == f"{field}ab" + + +@pytest.mark.parametrize("hook", MODIFY_HOOKS) +def test_chain_reuses_the_context_when_nothing_changes(hook: str) -> None: + context = _context() + later = Mock(spec=Interceptor) + + getattr(InterceptorChain([Interceptor(), later]), hook)(context) + + assert getattr(later, hook).call_args.args[0] is context + + +@pytest.mark.parametrize("hook", READ_HOOKS + list(MODIFY_HOOKS)) +def test_chain_calls_only_the_matching_hook(hook: str) -> None: + interceptors = [Mock(spec=Interceptor), Mock(spec=Interceptor)] + + getattr(InterceptorChain(interceptors), hook)(_context()) + + for interceptor in interceptors: + assert [name for name, *_ in interceptor.method_calls] == [hook] @pytest.mark.parametrize("hook", MODIFY_HOOKS) -def test_chain_keeps_modifications_from_earlier_interceptors(hook: str) -> None: - # The base interceptor returns what it was given, which must be the value the - # interceptor before it returned, not the original. - chain = InterceptorChain([_Appender("a"), Interceptor()]) +def test_modify_hook_returning_none_raises(hook: str) -> None: + forgot_to_return = Mock(spec=Interceptor) + getattr(forgot_to_return, hook).return_value = None + after = Mock(spec=Interceptor) + + with pytest.raises(TypeError, match=hook): + getattr(InterceptorChain([forgot_to_return, after]), hook)(_context()) - assert getattr(chain, hook)(_context()) == "a" + assert not after.method_calls def _raising(name: str, calls: list[str]) -> Any: @@ -109,7 +139,7 @@ def raise_error(context: Any) -> None: raise RuntimeError(name) interceptor = Mock() - for hook in READ_HOOKS + MODIFY_HOOKS: + for hook in READ_HOOKS + list(MODIFY_HOOKS): getattr(interceptor, hook).side_effect = raise_error return interceptor @@ -118,6 +148,7 @@ def raise_error(context: Any) -> None: def test_read_hook_runs_every_interceptor_and_raises_the_last_error( hook: str, caplog: pytest.LogCaptureFixture ) -> None: + caplog.set_level(logging.DEBUG, logger="smithy_core.interceptors") calls: list[str] = [] chain = InterceptorChain([_raising(name, calls) for name in ("1", "2", "3")]) @@ -129,6 +160,33 @@ def test_read_hook_runs_every_interceptor_and_raises_the_last_error( assert dropped == ["1", "2"] +@pytest.mark.parametrize("hook", READ_HOOKS) +def test_read_hook_raises_when_a_later_interceptor_succeeds(hook: str) -> None: + calls: list[str] = [] + succeeds = Mock(spec=Interceptor) + chain = InterceptorChain([_raising("1", calls), succeeds]) + + with pytest.raises(RuntimeError, match="1"): + getattr(chain, hook)(_context()) + + assert [name for name, *_ in succeeds.method_calls] == [hook] + + +@pytest.mark.parametrize("hook", ["read_after_attempt", "read_after_execution"]) +def test_completion_read_hook_passes_the_error_to_later_interceptors( + hook: str, +) -> None: + later = Mock(spec=Interceptor) + chain = InterceptorChain([_raising("1", []), later]) + + with pytest.raises(RuntimeError): + getattr(chain, hook)(_context()) + + response = getattr(later, hook).call_args.args[0].response + assert isinstance(response, RuntimeError) + assert str(response) == "1" + + @pytest.mark.parametrize("hook", MODIFY_HOOKS) def test_modify_hook_stops_at_the_first_error(hook: str) -> None: calls: list[str] = []