Address PR feedback

Author: arandito

codegen/core/src/main/java/software/amazon/smithy/python/codegen/HttpProtocolTestGenerator.java

@@ -424,8 +424,8 @@ private void compareMediaBlob(HttpMessageTestCase testCase, PythonWriter writer)
         if (contentType.equals("application/x-www-form-urlencoded")) {
             writer.addStdlibImport("urllib.parse", "parse_qsl");
             writer.write("""
-                    actual_params = sorted(parse_qsl(actual_body_content.decode()))
-                    expected_params = sorted(parse_qsl(expected_body_content.decode()))
+                    actual_params = sorted(parse_qsl(actual_body_content.decode(), keep_blank_values=True))
+                    expected_params = sorted(parse_qsl(expected_body_content.decode(), keep_blank_values=True))
                     assert actual_params == expected_params
 
                     """);

packages/smithy-aws-core/src/smithy_aws_core/_private/query/errors.py

@@ -1,16 +1,38 @@
 #  Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 #  SPDX-License-Identifier: Apache-2.0
-from typing import Any
+from typing import TYPE_CHECKING, Any
 from xml.etree.ElementTree import Element, ParseError, fromstring
 
 from smithy_core.documents import TypeRegistry
-from smithy_core.exceptions import CallError, ExpectationNotMetError, ModeledError
+from smithy_core.exceptions import (
+    CallError,
+    ExpectationNotMetError,
+    MissingDependencyError,
+    ModeledError,
+)
+from smithy_core.interfaces import TypedProperties
 from smithy_core.schemas import APIOperation
 from smithy_core.shapes import ShapeID
-from smithy_xml import XMLCodec
 
 from ...traits import AwsQueryErrorTrait
 
+try:
+    from smithy_xml import XMLCodec
+
+    _HAS_XML = True
+except ImportError:
+    _HAS_XML = False  # type: ignore
+
+if TYPE_CHECKING:
+    from smithy_xml import XMLCodec
+
+
+def _assert_xml() -> None:
+    if not _HAS_XML:
+        raise MissingDependencyError(
+            "Attempted to use XML codec, but smithy-xml is not installed."
+        )
+
 
 def _local_name(tag: str) -> str:
     """Strip namespace URI from an element tag: {uri}local -> local."""
@@ -76,6 +98,7 @@ def create_aws_query_error(
     default_namespace: str,
     wrapper_elements: tuple[str, ...],
     status: int,
+    context: TypedProperties,
 ) -> CallError:
     """Create a modeled or generic CallError from an awsQuery error response."""
     code = _parse_aws_query_error_code(body, wrapper_elements)
@@ -94,6 +117,7 @@ def create_aws_query_error(
                     f"but got {error_shape}"
                 )
 
+            _assert_xml()
             deserializer = XMLCodec().create_deserializer(
                 body, wrapper_elements=wrapper_elements
             )
@@ -102,5 +126,15 @@ def create_aws_query_error(
     message = f"Unknown error for operation {operation.schema.id} - status: {status}"
     if code is not None:
         message += f", code: {code}"
-    fault = "client" if 400 <= status < 500 else "server"
-    return CallError(message=message, fault=fault)
+
+    is_timeout = status == 408
+    is_throttle = status == 429
+    fault = "client" if status < 500 else "server"
+
+    return CallError(
+        message=message,
+        fault=fault,
+        is_throttling_error=is_throttle,
+        is_timeout_error=is_timeout,
+        is_retry_safe=is_throttle or is_timeout or None,
+    )

packages/smithy-aws-core/src/smithy_aws_core/aio/protocols.py

@@ -76,14 +76,14 @@
 def _assert_json() -> None:
     if not _HAS_JSON:
         raise MissingDependencyError(
-            "Attempted to use JSON protocol support, but smithy-json is not installed."
+            "Attempted to use JSON codec, but smithy-json is not installed."
         )
 
 
 def _assert_xml() -> None:
     if not _HAS_XML:
         raise MissingDependencyError(
-            "Attempted to use XML protocol support, but smithy-xml is not installed."
+            "Attempted to use XML codec, but smithy-xml is not installed."
         )
 
 
@@ -114,17 +114,21 @@ def identify(
         return None
 
 
-class AWSJSONDocument(JSONDocument):
-    @property
-    def discriminator(self) -> ShapeID:
-        if self.shape_type is ShapeType.STRUCTURE:
-            return self._schema.id
-        parsed = parse_document_discriminator(self, self._settings.default_namespace)
-        if parsed is None:
-            raise DiscriminatorError(
-                f"Unable to parse discriminator for {self.shape_type} document."
+if _HAS_JSON:
+
+    class AWSJSONDocument(JSONDocument):
+        @property
+        def discriminator(self) -> ShapeID:
+            if self.shape_type is ShapeType.STRUCTURE:
+                return self._schema.id
+            parsed = parse_document_discriminator(
+                self, self._settings.default_namespace
             )
-        return parsed
+            if parsed is None:
+                raise DiscriminatorError(
+                    f"Unable to parse discriminator for {self.shape_type} document."
+                )
+            return parsed
 
 
 class RestJsonClientProtocol(HttpBindingClientProtocol):
@@ -242,6 +246,14 @@ def __init__(self, service_schema: Schema, version: str) -> None:
     def id(self) -> ShapeID:
         return self._id
 
+    @property
+    def payload_codec(self) -> "XMLCodec":
+        return self._codec
+
+    @property
+    def content_type(self) -> str:
+        return self._content_type
+
     def serialize_request[
         OperationInput: SerializeableShape,
         OperationOutput: DeserializeableShape,
@@ -271,7 +283,7 @@ def serialize_request[
             destination=_URI(host="", path="/"),
             fields=tuples_to_fields(
                 [
-                    ("content-type", self._content_type),
+                    ("content-type", self.content_type),
                     ("content-length", str(content_length)),
                 ]
             ),
@@ -292,36 +304,46 @@ async def deserialize_response[
     ) -> OperationOutput:
         body = await response.consume_body_async()
 
-        if response.status >= 300:
-            raise self._create_error(
+        if not self._is_success(operation, context, response):
+            raise await self._create_error(
                 operation=operation,
                 response=response,
                 response_body=body,
                 error_registry=error_registry,
+                context=context,
             )
 
         if len(body) == 0:
             return operation.output.deserialize(
                 HTTPResponseDeserializer(
-                    payload_codec=self._codec,
+                    payload_codec=self.payload_codec,
                     response=response,
                     body=body,
                 )
             )
 
         wrapper_elements = self._response_wrapper_elements(operation)
-        deserializer = self._codec.create_deserializer(
+        deserializer = self.payload_codec.create_deserializer(
             body, wrapper_elements=wrapper_elements
         )
         return operation.output.deserialize(deserializer)
 
-    def _create_error(
+    def _is_success(
+        self,
+        operation: APIOperation[Any, Any],
+        context: TypedProperties,
+        response: HTTPResponse,
+    ) -> bool:
+        return 200 <= response.status < 300
+
+    async def _create_error(
         self,
         *,
         operation: APIOperation[Any, Any],
         response: HTTPResponse,
         response_body: bytes,
         error_registry: TypeRegistry,
+        context: TypedProperties,
     ) -> CallError:
         return create_aws_query_error(
             body=response_body,
@@ -330,6 +352,7 @@ def _create_error(
             default_namespace=self._default_namespace,
             wrapper_elements=self._error_wrapper_elements(),
             status=response.status,
+            context=context,
         )
 
     def _action_name(

packages/smithy-aws-core/src/smithy_aws_core/traits.py

@@ -57,15 +57,15 @@ class AwsQueryErrorTrait(Trait, id=ShapeID("aws.protocols#awsQueryError")):
     def __post_init__(self):
         assert isinstance(self.document_value, Mapping)
         assert isinstance(self.document_value.get("code"), str)
-        assert isinstance(self.document_value.get("httpResponseCode"), int | None)
+        assert isinstance(self.document_value.get("httpResponseCode"), int)
 
     @property
     def code(self) -> str:
         return self.document_value["code"]  # type: ignore
 
     @property
-    def http_response_code(self) -> int | None:
-        return self.document_value.get("httpResponseCode")  # type: ignore
+    def http_response_code(self) -> int:
+        return self.document_value["httpResponseCode"]  # type: ignore
 
 
 @dataclass(init=False, frozen=True)

packages/smithy-aws-core/tests/unit/aio/test_protocols.py

@@ -23,7 +23,7 @@
 from smithy_core.traits import Trait
 from smithy_core.types import TypedProperties
 from smithy_http import Fields, tuples_to_fields
-from smithy_http.aio import HTTPResponse
+from smithy_http.aio import HTTPRequest, HTTPResponse
 from smithy_json import JSONSettings
 
 
@@ -199,60 +199,80 @@ async def test_aws_query_serializes_base_request_shape() -> None:
     assert body == b"Action=TestOperation&Version=2020-01-08&name=example"
 
 
-def test_aws_query_resolves_modeled_error_from_query_error_trait() -> None:
+@pytest.mark.asyncio
+async def test_aws_query_resolves_modeled_error_from_query_error_trait() -> None:
     protocol = AwsQueryClientProtocol(_SERVICE_SCHEMA, "2020-01-08")
-    error = getattr(protocol, "_create_error")(
-        operation=_mock_operation(
-            _operation_schema("FailingOperation"),
-            error_schemas=[_INVALID_ACTION_ERROR_SCHEMA],
-        ),
-        response=HTTPResponse(status=400, fields=tuples_to_fields([])),
-        response_body=(
-            b"<ErrorResponse><Error><Code>InvalidAction</Code>"
-            b"<message>bad request</message></Error></ErrorResponse>"
-        ),
-        error_registry=TypeRegistry(
-            {ShapeID("com.test#InvalidActionError"): _ModeledQueryError}
-        ),
-    )
-
-    assert isinstance(error, _ModeledQueryError)
-    assert error.message == "bad request"
+    with pytest.raises(_ModeledQueryError) as exc_info:
+        await protocol.deserialize_response(
+            operation=_mock_operation(
+                _operation_schema("FailingOperation"),
+                error_schemas=[_INVALID_ACTION_ERROR_SCHEMA],
+            ),
+            request=cast(HTTPRequest, Mock()),
+            response=HTTPResponse(
+                status=400,
+                fields=tuples_to_fields([]),
+                body=(
+                    b"<ErrorResponse><Error><Code>InvalidAction</Code>"
+                    b"<message>bad request</message></Error></ErrorResponse>"
+                ),
+            ),
+            error_registry=TypeRegistry(
+                {ShapeID("com.test#InvalidActionError"): _ModeledQueryError}
+            ),
+            context=TypedProperties(),
+        )
+
+    assert exc_info.value.message == "bad request"
 
 
-def test_aws_query_resolves_modeled_error_from_default_namespace_fallback() -> None:
+@pytest.mark.asyncio
+async def test_aws_query_resolves_modeled_error_from_default_namespace_fallback() -> (
+    None
+):
     protocol = AwsQueryClientProtocol(_SERVICE_SCHEMA, "2020-01-08")
-    error = getattr(protocol, "_create_error")(
-        operation=_mock_operation(_operation_schema("FailingOperation")),
-        response=HTTPResponse(status=503, fields=tuples_to_fields([])),
-        response_body=(
-            b"<ErrorResponse><Error><Code>ServiceUnavailable</Code>"
-            b"<message>try again</message></Error></ErrorResponse>"
-        ),
-        error_registry=TypeRegistry(
-            {ShapeID("com.test#ServiceUnavailable"): _ModeledQueryError}
-        ),
-    )
+    with pytest.raises(_ModeledQueryError) as exc_info:
+        await protocol.deserialize_response(
+            operation=_mock_operation(_operation_schema("FailingOperation")),
+            request=cast(HTTPRequest, Mock()),
+            response=HTTPResponse(
+                status=503,
+                fields=tuples_to_fields([]),
+                body=(
+                    b"<ErrorResponse><Error><Code>ServiceUnavailable</Code>"
+                    b"<message>try again</message></Error></ErrorResponse>"
+                ),
+            ),
+            error_registry=TypeRegistry(
+                {ShapeID("com.test#ServiceUnavailable"): _ModeledQueryError}
+            ),
+            context=TypedProperties(),
+        )
+
+    assert exc_info.value.message == "try again"
 
-    assert isinstance(error, _ModeledQueryError)
-    assert error.message == "try again"
 
-
-def test_aws_query_returns_generic_error_for_unknown_code() -> None:
+@pytest.mark.asyncio
+async def test_aws_query_returns_generic_error_for_unknown_code() -> None:
     protocol = AwsQueryClientProtocol(_SERVICE_SCHEMA, "2020-01-08")
-    error = getattr(protocol, "_create_error")(
-        operation=_mock_operation(_operation_schema("FailingOperation")),
-        response=HTTPResponse(status=500, fields=tuples_to_fields([])),
-        response_body=(
-            b"<ErrorResponse><Error><Code>UnknownThing</Code>"
-            b"<message>bad request</message></Error></ErrorResponse>"
-        ),
-        error_registry=TypeRegistry({}),
-    )
-
-    assert isinstance(error, CallError)
-    assert not isinstance(error, ModeledError)
-    assert error.message == (
+    with pytest.raises(CallError) as exc_info:
+        await protocol.deserialize_response(
+            operation=_mock_operation(_operation_schema("FailingOperation")),
+            request=cast(HTTPRequest, Mock()),
+            response=HTTPResponse(
+                status=500,
+                fields=tuples_to_fields([]),
+                body=(
+                    b"<ErrorResponse><Error><Code>UnknownThing</Code>"
+                    b"<message>bad request</message></Error></ErrorResponse>"
+                ),
+            ),
+            error_registry=TypeRegistry({}),
+            context=TypedProperties(),
+        )
+
+    assert not isinstance(exc_info.value, ModeledError)
+    assert exc_info.value.message == (
         "Unknown error for operation com.test#FailingOperation"
         " - status: 500, code: UnknownThing"
     )

packages/smithy-core/.changes/next-release/smithy-core-feature-12dccc844ab2484a8d5ee087b6375817.json

@@ -0,0 +1,4 @@
+{
+  "type": "feature",
+  "description": "Added `error_schemas` to `APIOperation` to expose the operation\u2019s modeled error schemas."
+}

…nt-80e80edee8a7461d99ca5f28c2547e59.json …re-f854c5df00ef4bdf9b0947f09b2f53f0.jsonpackages/smithy-core/.changes/next-release/smithy-core-enhancement-80e80edee8a7461d99ca5f28c2547e59.json renamed to packages/smithy-core/.changes/next-release/smithy-core-feature-f854c5df00ef4bdf9b0947f09b2f53f0.json packages/smithy-core/.changes/next-release/smithy-core-enhancement-80e80edee8a7461d99ca5f28c2547e59.json renamed to packages/smithy-core/.changes/next-release/smithy-core-feature-f854c5df00ef4bdf9b0947f09b2f53f0.json

@@ -1,4 +1,4 @@
 {
-  "type": "enhancement",
+  "type": "feature",
   "description": "Added XML binding traits: `XMLNameTrait`, `XMLNamespaceTrait`, `XMLFlattenedTrait`, and `XMLAttributeTrait`."
-}
+}