add AWS json protocol support for SQS developer endpoints#11726
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 5m 31s ⏱️ - 36m 45s Results for commit 131770e. ± Comparison against base commit 56e0b77. This pull request removes 1037 and adds 18 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
bentsku
left a comment
There was a problem hiding this comment.
I'll only comment as I've updated the PR a bit, let me know what you think @thrau! I've tried to remove the logic of the protocol outside of the method to avoid having nested functions, we still need it for the parser though. Happy to get feedback!
| @@ -652,20 +690,14 @@ def list_messages(self, request: Request) -> ReceiveMessageResult: | |||
| return self._get_and_serialize_messages(request, region, account_id, queue_name) | |||
|
|
|||
| @route("/_aws/sqs/messages/<region>/<account_id>/<queue_name>") | |||
There was a problem hiding this comment.
q: not sure if it's normal that this method accept all HTTP methods?
There was a problem hiding this comment.
Perhaps we make this the same as the /_aws/sqs/messages route?
| @route("/_aws/sqs/messages/<region>/<account_id>/<queue_name>") | |
| @route("/_aws/sqs/messages/<region>/<account_id>/<queue_name>", methods=["GET", "POST"]) |
| # only parse the request using a parser if it comes from an AWS client | ||
| protocol = get_sqs_protocol(request) | ||
| operation, service_request = create_parser( | ||
| load_service("sqs", protocol=protocol) |
There was a problem hiding this comment.
nit: I've updated it, as we're moving away from the sqs-query service name to the "service name" + "protocol" way of creating/selecting serializer and parser.
| return "json" if content_type == "application/x-amz-json-1.0" else "query" | ||
|
|
||
|
|
||
| def sqs_auto_protocol_aws_response_serializer(service_name: str, operation: str): |
There was a problem hiding this comment.
this is a bit verbose and copies the logic of aws_response_serializer to get the request out of it, I'd like to make it cleaner but not sure how 🤔 there's no way of getting the proper protocol before we're inside the method itself.
Motivation
There was a long-standing TODO for the sqs developer endpoints
/_aws/sqs/messagesto also support the modern x-amz-json-1.0 protocol introduced in #8268. This PR adds this functionality.The implementation is not pretty but the best I could come up with without reworking the entire serialization mechanism. Basically through the use of the
@aws_response_serializerdecorator the protocol used to serialize the response was hard-coded, but needed to be dynamic per-request.It's still a bit awkward because we documented that when calling this endpoint with
application/jsoncontent type, it will return a JSON serialization of the query protocol response. So basically we have three protocols now: query (XML), x-amz-json-1.0 (for modern clients), and our weird/wrong json protocol that just returns the XML responses as json. That needs to be addressed at some point but is technically a breaking change.Changes
localhost:4566/_aws/sqs/messages, the endpoint now correctly returns the JSON response.TODO
What's left to do: