From 625fa86f58abca7301ee53730114fa9ce6b8c9a7 Mon Sep 17 00:00:00 2001 From: JordonPhillips Date: Wed, 16 Apr 2025 17:07:52 +0200 Subject: [PATCH 1/3] Update exception hierarchy and add retry info This updates exceptions to embed necessary retry info. This will allow any layer that has access to the exception to set or utilize it without having to have additional interfaces and hooks. This does not modify the retry strategy interface or implementation, that will come in a follow-up. --- .../python/codegen/ClientGenerator.java | 84 ++++++++++--------- .../smithy/python/codegen/CodegenUtils.java | 42 +--------- .../python/codegen/PythonSymbolProvider.java | 12 ++- .../generators/ServiceErrorGenerator.java | 38 ++------- .../codegen/generators/SetupGenerator.java | 25 +++--- .../generators/StructureGenerator.java | 21 ++--- .../HttpProtocolGeneratorUtils.java | 10 +-- .../writer/MarkdownToRstDocConverter.java | 8 +- .../codegen/reserved-error-member-names.txt | 4 + .../smithy-core/src/smithy_core/exceptions.py | 48 +++++++++++ 10 files changed, 143 insertions(+), 149 deletions(-) create mode 100644 codegen/core/src/main/resources/software/amazon/smithy/python/codegen/reserved-error-member-names.txt diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java index 8e8b4fb49..1c28e0f53 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/ClientGenerator.java @@ -127,7 +127,6 @@ private void generateOperationExecutor(PythonWriter writer) { var transportRequest = context.applicationProtocol().requestType(); var transportResponse = context.applicationProtocol().responseType(); - var errorSymbol = CodegenUtils.getServiceError(context.settings()); var pluginSymbol = CodegenUtils.getPluginSymbol(context.settings()); var configSymbol = CodegenUtils.getConfigSymbol(context.settings()); @@ -302,18 +301,24 @@ def _classify_error( } writer.addStdlibImport("typing", "Any"); writer.addStdlibImport("asyncio", "iscoroutine"); + writer.addImports("smithy_core.exceptions", Set.of("SmithyException", "CallException")); + writer.pushState(); + writer.putContext("request", transportRequest); + writer.putContext("response", transportResponse); + writer.putContext("plugin", pluginSymbol); + writer.putContext("config", configSymbol); writer.write( """ async def _execute_operation[Input: SerializeableShape, Output: DeserializeableShape]( self, input: Input, - plugins: list[$1T], - serialize: Callable[[Input, $5T], Awaitable[$2T]], - deserialize: Callable[[$3T, $5T], Awaitable[Output]], - config: $5T, + plugins: list[${plugin:T}], + serialize: Callable[[Input, ${config:T}], Awaitable[${request:T}]], + deserialize: Callable[[${response:T}, ${config:T}], Awaitable[Output]], + config: ${config:T}, operation: APIOperation[Input, Output], - request_future: Future[RequestContext[Any, $2T]] | None = None, - response_future: Future[$3T] | None = None, + request_future: Future[RequestContext[Any, ${request:T}]] | None = None, + response_future: Future[${response:T}] | None = None, ) -> Output: try: return await self._handle_execution( @@ -321,27 +326,29 @@ def _classify_error( request_future, response_future, ) except Exception as e: + # Make sure every exception that we throw is an instance of SmithyException so + # customers can reliably catch everything we throw. + if not isinstance(e, SmithyException): + wrapped = CallException(str(e)) + wrapped.__cause__ = e + e = wrapped + if request_future is not None and not request_future.done(): - request_future.set_exception($4T(e)) + request_future.set_exception(e) if response_future is not None and not response_future.done(): - response_future.set_exception($4T(e)) - - # Make sure every exception that we throw is an instance of $4T so - # customers can reliably catch everything we throw. - if not isinstance(e, $4T): - raise $4T(e) from e + response_future.set_exception(e) raise async def _handle_execution[Input: SerializeableShape, Output: DeserializeableShape]( self, input: Input, - plugins: list[$1T], - serialize: Callable[[Input, $5T], Awaitable[$2T]], - deserialize: Callable[[$3T, $5T], Awaitable[Output]], - config: $5T, + plugins: list[${plugin:T}], + serialize: Callable[[Input, ${config:T}], Awaitable[${request:T}]], + deserialize: Callable[[${response:T}, ${config:T}], Awaitable[Output]], + config: ${config:T}, operation: APIOperation[Input, Output], - request_future: Future[RequestContext[Any, $2T]] | None, - response_future: Future[$3T] | None, + request_future: Future[RequestContext[Any, ${request:T}]] | None, + response_future: Future[${response:T}] | None, ) -> Output: operation_name = operation.schema.id.name logger.debug('Making request for operation "%s" with parameters: %s', operation_name, input) @@ -350,11 +357,16 @@ def _classify_error( plugin(config) input_context = InputContext(request=input, properties=TypedProperties({"config": config})) - transport_request: $2T | None = None - output_context: OutputContext[Input, Output, $2T | None, $3T | None] | None = None + transport_request: ${request:T} | None = None + output_context: OutputContext[ + Input, + Output, + ${request:T} | None, + ${response:T} | None + ] | None = None client_interceptors = cast( - list[Interceptor[Input, Output, $2T, $3T]], list(config.interceptors) + list[Interceptor[Input, Output, ${request:T}, ${response:T}]], list(config.interceptors) ) interceptor_chain = InterceptorChain(client_interceptors) @@ -455,24 +467,20 @@ await sleep(retry_token.retry_delay) async def _handle_attempt[Input: SerializeableShape, Output: DeserializeableShape]( self, - deserialize: Callable[[$3T, $5T], Awaitable[Output]], - interceptor: Interceptor[Input, Output, $2T, $3T], - context: RequestContext[Input, $2T], - config: $5T, + deserialize: Callable[[${response:T}, ${config:T}], Awaitable[Output]], + interceptor: Interceptor[Input, Output, ${request:T}, ${response:T}], + context: RequestContext[Input, ${request:T}], + config: ${config:T}, operation: APIOperation[Input, Output], - request_future: Future[RequestContext[Input, $2T]] | None, - ) -> OutputContext[Input, Output, $2T, $3T | None]: - transport_response: $3T | None = None + request_future: Future[RequestContext[Input, ${request:T}]] | None, + ) -> OutputContext[Input, Output, ${request:T}, ${response:T} | None]: + transport_response: ${response:T} | None = None try: # Step 7a: Invoke read_before_attempt interceptor.read_before_attempt(context) - """, - pluginSymbol, - transportRequest, - transportResponse, - errorSymbol, - configSymbol); + """); + writer.popState(); boolean supportsAuth = !ServiceIndex.of(model).getAuthSchemes(service).isEmpty(); writer.pushState(new ResolveIdentitySection()); @@ -873,8 +881,8 @@ private void writeSharedOperationInit(PythonWriter writer, OperationShape operat .orElse("The operation's input."); writer.write(""" - $L - """,docs); + $L + """, docs); writer.write(""); writer.write(":param input: $L", inputDocs); writer.write(""); diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/CodegenUtils.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/CodegenUtils.java index 734da83a5..47ed4d70d 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/CodegenUtils.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/CodegenUtils.java @@ -89,10 +89,10 @@ public static Symbol getPluginSymbol(PythonSettings settings) { /** * Gets the service error symbol. * - *

This error is the top-level error for the client. Every error surfaced by - * the client MUST be a subclass of this so that customers can reliably catch all - * exceptions it raises. The client implementation will wrap any errors that aren't - * already subclasses. + *

This error is the top-level error for the client. Errors surfaced by the client + * MUST be a subclass of this or SmithyException so that customers can reliably catch + * all the exceptions a client throws. The request pipeline will wrap exceptions of + * other types. * * @param settings The client settings, used to account for module configuration. * @return Returns the symbol for the client's error class. @@ -105,40 +105,6 @@ public static Symbol getServiceError(PythonSettings settings) { .build(); } - /** - * Gets the service API error symbol. - * - *

This error is the parent class for all errors returned over the wire by the - * service, including unknown errors. - * - * @param settings The client settings, used to account for module configuration. - * @return Returns the symbol for the client's API error class. - */ - public static Symbol getApiError(PythonSettings settings) { - return Symbol.builder() - .name("ApiError") - .namespace(String.format("%s.models", settings.moduleName()), ".") - .definitionFile(String.format("./src/%s/models.py", settings.moduleName())) - .build(); - } - - /** - * Gets the unknown API error symbol. - * - *

This error is the parent class for all errors returned over the wire by - * the service which aren't in the model. - * - * @param settings The client settings, used to account for module configuration. - * @return Returns the symbol for unknown API errors. - */ - public static Symbol getUnknownApiError(PythonSettings settings) { - return Symbol.builder() - .name("UnknownApiError") - .namespace(String.format("%s.models", settings.moduleName()), ".") - .definitionFile(String.format("./src/%s/models.py", settings.moduleName())) - .build(); - } - /** * Gets the symbol for the http auth params. * diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/PythonSymbolProvider.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/PythonSymbolProvider.java index 4c7907983..a44f7cb91 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/PythonSymbolProvider.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/PythonSymbolProvider.java @@ -9,7 +9,6 @@ import java.util.Locale; import java.util.logging.Logger; import software.amazon.smithy.codegen.core.ReservedWordSymbolProvider; -import software.amazon.smithy.codegen.core.ReservedWords; import software.amazon.smithy.codegen.core.ReservedWordsBuilder; import software.amazon.smithy.codegen.core.Symbol; import software.amazon.smithy.codegen.core.SymbolProvider; @@ -84,6 +83,10 @@ public PythonSymbolProvider(Model model, PythonSettings settings) { var reservedMemberNamesBuilder = new ReservedWordsBuilder() .loadWords(PythonSymbolProvider.class.getResource("reserved-member-names.txt"), this::escapeWord); + // Reserved words that only apply to error members. + var reservedErrorMembers = new ReservedWordsBuilder() + .loadWords(PythonSymbolProvider.class.getResource("reserved-error-member-names.txt"), this::escapeWord); + escaper = ReservedWordSymbolProvider.builder() .nameReservedWords(reservedClassNames) .memberReservedWords(reservedMemberNamesBuilder.build()) @@ -92,13 +95,8 @@ public PythonSymbolProvider(Model model, PythonSettings settings) { .escapePredicate((shape, symbol) -> !StringUtils.isEmpty(symbol.getDefinitionFile())) .buildEscaper(); - // Reserved words that only apply to error members. - ReservedWords reservedErrorMembers = reservedMemberNamesBuilder - .put("code", "code_") - .build(); - errorMemberEscaper = ReservedWordSymbolProvider.builder() - .memberReservedWords(reservedErrorMembers) + .memberReservedWords(reservedErrorMembers.build()) .escapePredicate((shape, symbol) -> !StringUtils.isEmpty(symbol.getDefinitionFile())) .buildEscaper(); } diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/ServiceErrorGenerator.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/ServiceErrorGenerator.java index 6bc84a761..8cc09d1d3 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/ServiceErrorGenerator.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/ServiceErrorGenerator.java @@ -4,7 +4,6 @@ */ package software.amazon.smithy.python.codegen.generators; -import java.util.Set; import software.amazon.smithy.codegen.core.WriterDelegator; import software.amazon.smithy.python.codegen.CodegenUtils; import software.amazon.smithy.python.codegen.PythonSettings; @@ -30,38 +29,15 @@ public void run() { var serviceError = CodegenUtils.getServiceError(settings); writers.useFileWriter(serviceError.getDefinitionFile(), serviceError.getNamespace(), writer -> { writer.addDependency(SmithyPythonDependency.SMITHY_CORE); - writer.addImport("smithy_core.exceptions", "SmithyException"); + writer.addImport("smithy_core.exceptions", "ModeledException"); writer.write(""" - class $L(SmithyException): - ""\"Base error for all errors in the service.""\" - pass - """, serviceError.getName()); - }); - - var apiError = CodegenUtils.getApiError(settings); - writers.useFileWriter(apiError.getDefinitionFile(), apiError.getNamespace(), writer -> { - writer.addStdlibImports("typing", Set.of("Literal", "ClassVar")); - var unknownApiError = CodegenUtils.getUnknownApiError(settings); - - writer.write(""" - @dataclass - class $1L($2T): - ""\"Base error for all API errors in the service.""\" - code: ClassVar[str] - fault: ClassVar[Literal["client", "server"]] + class $L(ModeledException): + ""\"Base error for all errors in the service. - message: str - - def __post_init__(self) -> None: - super().__init__(self.message) - - - @dataclass - class $3L($1L): - ""\"Error representing any unknown api errors.""\" - code: ClassVar[str] = 'Unknown' - fault: ClassVar[Literal["client", "server"]] = "client" - """, apiError.getName(), serviceError, unknownApiError.getName()); + Some exceptions do not extend from this class, including + synthetic, implicit, and shared exception types. + ""\" + """, serviceError.getName()); }); } } diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/SetupGenerator.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/SetupGenerator.java index fedc5b612..021ba4019 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/SetupGenerator.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/SetupGenerator.java @@ -451,7 +451,6 @@ private static void writeIndexes(GenerationContext context, String projectName) writeIndexFile(context, "docs/models/index.rst", "Models"); } - /** * Write the readme in the docs folder describing instructions for generation * @@ -461,18 +460,18 @@ private static void writeDocsReadme( GenerationContext context ) { context.writerDelegator().useFileWriter("docs/README.md", writer -> { - writer.write(""" - ## Generating Documentation - - Sphinx is used for documentation. You can generate HTML locally with the - following: - - ``` - $$ uv pip install ".[docs]" - $$ cd docs - $$ make html - ``` - """); + writer.write(""" + ## Generating Documentation + + Sphinx is used for documentation. You can generate HTML locally with the + following: + + ``` + $$ uv pip install ".[docs]" + $$ cd docs + $$ make html + ``` + """); }); } diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/StructureGenerator.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/StructureGenerator.java index e0e3d2dcd..726739cb1 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/StructureGenerator.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/StructureGenerator.java @@ -130,31 +130,26 @@ private void renderError() { writer.addStdlibImports("typing", Set.of("Literal", "ClassVar")); writer.addStdlibImport("dataclasses", "dataclass"); - // TODO: Implement protocol-level customization of the error code var fault = errorTrait.getValue(); - var code = shape.getId().getName(); var symbol = symbolProvider.toSymbol(shape); - var apiError = CodegenUtils.getApiError(settings); + var baseError = CodegenUtils.getServiceError(settings); writer.pushState(new ErrorSection(symbol)); writer.write(""" @dataclass(kw_only=True) class $1L($2T): - ${5C|} + ${4C|} + + fault: Literal["client", "server"] | None = $3S - code: ClassVar[str] = $3S - fault: ClassVar[Literal["client", "server"]] = $4S + ${5C|} - message: str ${6C|} ${7C|} - ${8C|} - """, symbol.getName(), - apiError, - code, + baseError, fault, writer.consumer(w -> writeClassDocs(true)), writer.consumer(w -> writeProperties()), @@ -325,7 +320,9 @@ private void writeMemberDocs(MemberShape member) { String memberName = symbolProvider.toMemberName(member); String docs = writer.formatDocs(String.format(":param %s: %s%s", - memberName, descriptionPrefix, trait.getValue())); + memberName, + descriptionPrefix, + trait.getValue())); writer.write(docs); }); } diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java index b8841d26f..ccfc99097 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java @@ -136,26 +136,24 @@ public static void generateErrorDispatcher( var transportResponse = context.applicationProtocol().responseType(); var delegator = context.writerDelegator(); var errorDispatcher = context.protocolGenerator().getErrorDeserializationFunction(context, operation); - var apiError = CodegenUtils.getApiError(context.settings()); - var unknownApiError = CodegenUtils.getUnknownApiError(context.settings()); + var apiError = CodegenUtils.getServiceError(context.settings()); var canReadResponseBody = canReadResponseBody(operation, context.model()); delegator.useFileWriter(errorDispatcher.getDefinitionFile(), errorDispatcher.getNamespace(), writer -> { writer.pushState(new ErrorDispatcherSection(operation, errorShapeToCode, errorMessageCodeGenerator)); writer.write(""" async def $1L(http_response: $2T, config: $3T) -> $4T: - ${6C|} + ${5C|} match code.lower(): - ${7C|} + ${6C|} case _: - return $5T(f"{code}: {message}") + return $4T(f"{code}: {message}") """, errorDispatcher.getName(), transportResponse, configSymbol, apiError, - unknownApiError, writer.consumer(w -> errorMessageCodeGenerator.accept(context, w, canReadResponseBody)), writer.consumer(w -> errorCases(context, w, operation, errorShapeToCode))); writer.popState(); diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/writer/MarkdownToRstDocConverter.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/writer/MarkdownToRstDocConverter.java index 52415c8d8..b2ab2173d 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/writer/MarkdownToRstDocConverter.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/writer/MarkdownToRstDocConverter.java @@ -77,7 +77,7 @@ public void head(Node node, int depth) { String text = textNode.text(); if (!text.trim().isEmpty()) { if (text.startsWith(":param ")) { - int secondColonIndex = text.indexOf(':', 1); + int secondColonIndex = text.indexOf(':', 1); writer.write(text.substring(0, secondColonIndex + 1)); //TODO right now the code generator gives us a mixture of // RST and HTML (for instance :param xyz:

docs @@ -85,7 +85,7 @@ public void head(Node node, int depth) { // starts a newline. We account for that with this if/else // statement, but we should refactor this in the future to // have a more elegant codepath. - if (secondColonIndex +1 == text.strip().length()) { + if (secondColonIndex + 1 == text.strip().length()) { writer.indent(); writer.ensureNewline(); } else { @@ -97,8 +97,8 @@ public void head(Node node, int depth) { } else { writer.writeInline(text); } - // Account for services making a paragraph tag that's empty except - // for a newline + // Account for services making a paragraph tag that's empty except + // for a newline } else if (node.parent() instanceof Element && ((Element) node.parent()).tagName().equals("p")) { writer.writeInline(text.replaceAll("[ \\t]+", "")); } diff --git a/codegen/core/src/main/resources/software/amazon/smithy/python/codegen/reserved-error-member-names.txt b/codegen/core/src/main/resources/software/amazon/smithy/python/codegen/reserved-error-member-names.txt new file mode 100644 index 000000000..e1b05dff9 --- /dev/null +++ b/codegen/core/src/main/resources/software/amazon/smithy/python/codegen/reserved-error-member-names.txt @@ -0,0 +1,4 @@ +is_retry_safe +retry_after +is_throttle +fault diff --git a/packages/smithy-core/src/smithy_core/exceptions.py b/packages/smithy-core/src/smithy_core/exceptions.py index 0c07e30d3..bf010e4c0 100644 --- a/packages/smithy-core/src/smithy_core/exceptions.py +++ b/packages/smithy-core/src/smithy_core/exceptions.py @@ -1,9 +1,57 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 +from dataclasses import dataclass, field +from typing import Literal, Protocol + + class SmithyException(Exception): """Base exception type for all exceptions raised by smithy-python.""" +@dataclass(kw_only=True) +class RetryInfo(Protocol): + is_retry_safe: bool | None = None + """Whether the exception is safe to retry. + + A value of True does not mean a retry will occur, but rather that a retry is allowed + to occur. + + A value of None indicates that there is not enough information available to + determine if a retry is safe. + """ + + retry_after: float | None = None + """The amount of time that should pass before a retry. + + Retry strategies MAY choose to wait longer. + """ + + is_throttle: bool = False + """Whether the error is a throttling error.""" + + +@dataclass(kw_only=True) +class CallException(SmithyException, RetryInfo): + """Base exceptio to be used in application-level errors.""" + + fault: Literal["client", "server"] | None = None + """Whether the client or server is at fault.""" + + message: str = field(default="", kw_only=False) + """The message of the error.""" + + def __post_init__(self): + super().__init__(self.message) + + +@dataclass(kw_only=True) +class ModeledException(CallException): + """Base excetpion to be used for modeled errors.""" + + fault: Literal["client", "server"] | None = "client" + """Whether the client or server is at fault.""" + + class SerializationException(Exception): """Base exception type for exceptions raised during serialization.""" From 4c4cdf80c598d7df43475c5a0ddbe1ed9a356675 Mon Sep 17 00:00:00 2001 From: JordonPhillips Date: Thu, 17 Apr 2025 18:05:12 +0200 Subject: [PATCH 2/3] Use CallException for unknown errors --- .../amazon/smithy/python/codegen/CodegenUtils.java | 5 +---- .../integrations/HttpProtocolGeneratorUtils.java | 14 ++++++++------ packages/smithy-core/src/smithy_core/exceptions.py | 12 +++++++++--- 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/CodegenUtils.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/CodegenUtils.java index 47ed4d70d..689215ece 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/CodegenUtils.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/CodegenUtils.java @@ -89,10 +89,7 @@ public static Symbol getPluginSymbol(PythonSettings settings) { /** * Gets the service error symbol. * - *

This error is the top-level error for the client. Errors surfaced by the client - * MUST be a subclass of this or SmithyException so that customers can reliably catch - * all the exceptions a client throws. The request pipeline will wrap exceptions of - * other types. + *

This error is the top-level error for modeled client errors. * * @param settings The client settings, used to account for module configuration. * @return Returns the symbol for the client's error class. diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java index ccfc99097..313dc3ce1 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java @@ -136,24 +136,26 @@ public static void generateErrorDispatcher( var transportResponse = context.applicationProtocol().responseType(); var delegator = context.writerDelegator(); var errorDispatcher = context.protocolGenerator().getErrorDeserializationFunction(context, operation); - var apiError = CodegenUtils.getServiceError(context.settings()); var canReadResponseBody = canReadResponseBody(operation, context.model()); delegator.useFileWriter(errorDispatcher.getDefinitionFile(), errorDispatcher.getNamespace(), writer -> { writer.pushState(new ErrorDispatcherSection(operation, errorShapeToCode, errorMessageCodeGenerator)); + writer.addImport("smithy_core.exceptions", "CallException"); writer.write(""" - async def $1L(http_response: $2T, config: $3T) -> $4T: - ${5C|} + async def $1L(http_response: $2T, config: $3T) -> CallException: + ${4C|} match code.lower(): - ${6C|} + ${5C|} case _: - return $4T(f"{code}: {message}") + return CallException( + message=f"{code}: {message}", + fault="client" if http_response.status < 500 else "server" + ) """, errorDispatcher.getName(), transportResponse, configSymbol, - apiError, writer.consumer(w -> errorMessageCodeGenerator.accept(context, w, canReadResponseBody)), writer.consumer(w -> errorCases(context, w, operation, errorShapeToCode))); writer.popState(); diff --git a/packages/smithy-core/src/smithy_core/exceptions.py b/packages/smithy-core/src/smithy_core/exceptions.py index bf010e4c0..5e7eb325a 100644 --- a/packages/smithy-core/src/smithy_core/exceptions.py +++ b/packages/smithy-core/src/smithy_core/exceptions.py @@ -35,7 +35,10 @@ class CallException(SmithyException, RetryInfo): """Base exceptio to be used in application-level errors.""" fault: Literal["client", "server"] | None = None - """Whether the client or server is at fault.""" + """Whether the client or server is at fault. + + If None, then there was not enough information to determine fault. + """ message: str = field(default="", kw_only=False) """The message of the error.""" @@ -46,10 +49,13 @@ def __post_init__(self): @dataclass(kw_only=True) class ModeledException(CallException): - """Base excetpion to be used for modeled errors.""" + """Base exception to be used for modeled errors.""" fault: Literal["client", "server"] | None = "client" - """Whether the client or server is at fault.""" + """Whether the client or server is at fault. + + If None, then there was not enough information to determine fault. + """ class SerializationException(Exception): From 3e7115d783074fcd2e009765976b41462c64f217 Mon Sep 17 00:00:00 2001 From: JordonPhillips Date: Thu, 17 Apr 2025 19:54:52 +0200 Subject: [PATCH 3/3] Add design doc and a few generator tweaks --- .../generators/StructureGenerator.java | 15 +++ .../HttpProtocolGeneratorUtils.java | 6 +- designs/exceptions.md | 124 ++++++++++++++++++ .../smithy-core/src/smithy_core/exceptions.py | 22 ++-- 4 files changed, 157 insertions(+), 10 deletions(-) create mode 100644 designs/exceptions.md diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/StructureGenerator.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/StructureGenerator.java index 726739cb1..410dcb0cc 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/StructureGenerator.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/generators/StructureGenerator.java @@ -31,6 +31,7 @@ import software.amazon.smithy.model.traits.InputTrait; import software.amazon.smithy.model.traits.OutputTrait; import software.amazon.smithy.model.traits.RequiredTrait; +import software.amazon.smithy.model.traits.RetryableTrait; import software.amazon.smithy.model.traits.SensitiveTrait; import software.amazon.smithy.model.traits.StreamingTrait; import software.amazon.smithy.python.codegen.CodegenUtils; @@ -134,12 +135,26 @@ private void renderError() { var symbol = symbolProvider.toSymbol(shape); var baseError = CodegenUtils.getServiceError(settings); writer.pushState(new ErrorSection(symbol)); + writer.putContext("retryable", false); + writer.putContext("throttling", false); + + var retryableTrait = shape.getTrait(RetryableTrait.class); + if (retryableTrait.isPresent()) { + writer.putContext("retryable", true); + writer.putContext("throttling", retryableTrait.get().getThrottling()); + } writer.write(""" @dataclass(kw_only=True) class $1L($2T): ${4C|} fault: Literal["client", "server"] | None = $3S + ${?retryable} + is_retry_safe: bool | None = True + ${?throttling} + is_throttle: bool = True + ${/throttling} + ${/retryable} ${5C|} diff --git a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java index 313dc3ce1..c8de9b77d 100644 --- a/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java +++ b/codegen/core/src/main/java/software/amazon/smithy/python/codegen/integrations/HttpProtocolGeneratorUtils.java @@ -140,6 +140,7 @@ public static void generateErrorDispatcher( delegator.useFileWriter(errorDispatcher.getDefinitionFile(), errorDispatcher.getNamespace(), writer -> { writer.pushState(new ErrorDispatcherSection(operation, errorShapeToCode, errorMessageCodeGenerator)); writer.addImport("smithy_core.exceptions", "CallException"); + // TODO: include standard retry-after in the pure-python version of this writer.write(""" async def $1L(http_response: $2T, config: $3T) -> CallException: ${4C|} @@ -148,9 +149,12 @@ public static void generateErrorDispatcher( ${5C|} case _: + is_throttle = http_response.status == 429 return CallException( message=f"{code}: {message}", - fault="client" if http_response.status < 500 else "server" + fault="client" if http_response.status < 500 else "server", + is_throttle=is_throttle, + is_retry_safe=is_throttle or None, ) """, errorDispatcher.getName(), diff --git a/designs/exceptions.md b/designs/exceptions.md new file mode 100644 index 000000000..97acbb3ff --- /dev/null +++ b/designs/exceptions.md @@ -0,0 +1,124 @@ +# Exceptions + +Exceptions are a necessary aspect of any software product (Go notwithstanding), +and care must be taken in how they're exposed. This document describes how +smithy-python clients will expose exceptions to customers. + +## Goals + +* Every exception raised by a Smithy client should be catchable with a single, + specific catch statement (that is, not just `except Exception`). +* Every modeled exception raised by a service should be catchable with a single, + specific catch statement. +* Exceptions should contain information about retryablility where relevant. + +## Specification + +Every exception raised by a Smithy client MUST inherit from `SmithyException`. + +```python +class SmithyException(Exception): + """Base exception type for all exceptions raised by smithy-python.""" +``` + +If an exception that is not a `SmithyException` is thrown while executing a +request, that exception MUST be wrapped in a `SmithyException` and the +`__cause__` MUST be set to the original exception. + +Just as in normal Python programming, different exception types SHOULD be made +for different kinds of exceptions. `SerializationException`, for example, will +serve as the exception type for any exceptions that occur while serializing a +request. + +### Retryability + +Not all exceptions need to include information about retryability, as most will +not be retryable at all. To avoid overly complicating the class hierarchy, +retryability properties will be standardized as a `Protocol` that exceptions MAY +implement. + +```python +@dataclass(kw_only=True) +@runtime_checkable +class RetryInfo(Protocol): + is_retry_safe: bool | None = None + """Whether the exception is safe to retry. + + A value of True does not mean a retry will occur, but rather that a retry is allowed + to occur. + + A value of None indicates that there is not enough information available to + determine if a retry is safe. + """ + + retry_after: float | None = None + """The amount of time that should pass before a retry. + + Retry strategies MAY choose to wait longer. + """ + + is_throttle: bool = False + """Whether the error is a throttling error.""" +``` + +If an exception with `RetryInfo` is received while attempting to send a +serialized request to the server, the contained information will be used to +inform the next retry. + +### Service Exceptions + +Exceptions returned by the service MUST be a `CallException`. `CallException`s +include a `fault` property that indicates whether the client or server is +responsible for the exception. HTTP protocols can determine this based on the +status code. + +Similarly, protocols can and should determine retry information. HTTP protocols +can generally be confident that a status code 429 is a throttling error and can +also make use of the `Retry-After` header. Specific protocols may also include +more information in protocol-specific headers. + +```python +type Fault = Literal["client", "server"] | None +"""Whether the client or server is at fault. + +If None, then there was not enough information to determine fault. +""" + + +@dataclass(kw_only=True) +class CallException(SmithyException, RetryInfo): + fault: Fault = None + message: str = field(default="", kw_only=False) +``` + +#### Modeled Exceptions + +Most exceptions thrown by a service will be present in the Smithy model for the +service. These exceptions will all be generated into the client package. Each +modeled exception will be inherit from a generated exception named +`ServiceException` which itself inherits from the static `ModeledException`. + +```python +@dataclass(kw_only=True) +class ModeledException(CallException): + """Base exception to be used for modeled errors.""" +``` + +The Smithy model itself can contain fault information in the +[error trait](https://smithy.io/2.0/spec/type-refinement-traits.html#smithy-api-error-trait) +and retry information in the +[retryable trait](https://smithy.io/2.0/spec/behavior-traits.html#retryable-trait). +This information will be statically generated onto the exception. + +```python +@dataclass(kw_only=True) +class ServiceException(ModeledException): + pass + + +@dataclass(kw_only=True) +class ThrottlingException(ServcieException): + fault: Fault = "client" + is_retry_safe: bool | None = True + is_throttle: bool = True +``` diff --git a/packages/smithy-core/src/smithy_core/exceptions.py b/packages/smithy-core/src/smithy_core/exceptions.py index 5e7eb325a..63467b715 100644 --- a/packages/smithy-core/src/smithy_core/exceptions.py +++ b/packages/smithy-core/src/smithy_core/exceptions.py @@ -1,14 +1,22 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 from dataclasses import dataclass, field -from typing import Literal, Protocol +from typing import Literal, Protocol, runtime_checkable class SmithyException(Exception): """Base exception type for all exceptions raised by smithy-python.""" +type Fault = Literal["client", "server"] | None +"""Whether the client or server is at fault. + +If None, then there was not enough information to determine fault. +""" + + @dataclass(kw_only=True) +@runtime_checkable class RetryInfo(Protocol): is_retry_safe: bool | None = None """Whether the exception is safe to retry. @@ -32,11 +40,11 @@ class RetryInfo(Protocol): @dataclass(kw_only=True) class CallException(SmithyException, RetryInfo): - """Base exceptio to be used in application-level errors.""" + """Base exception to be used in application-level errors.""" - fault: Literal["client", "server"] | None = None + fault: Fault = None """Whether the client or server is at fault. - + If None, then there was not enough information to determine fault. """ @@ -51,11 +59,7 @@ def __post_init__(self): class ModeledException(CallException): """Base exception to be used for modeled errors.""" - fault: Literal["client", "server"] | None = "client" - """Whether the client or server is at fault. - - If None, then there was not enough information to determine fault. - """ + fault: Fault = "client" class SerializationException(Exception):