fix(11576): fixing bug in standardqueue put message #11717
Conversation
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
All contributors have signed the CLA ✍️ ✅ |
ad43249 to
c117a3f
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
Hi @thrau , @baermat , @gregfurman Could you please add the |
There was a problem hiding this comment.
hey @kshitijkohli, thanks for the contribution!
would you please add a snapshot test case so we can avoid regressions? you can find out more in our contributor docs here.
something like this:
@markers.aws.unknown
def test_send_message_to_standard_queue_with_empty_message_group_id(self, sqs_create_queue, aws_client, snapshot):
queue = sqs_create_queue()
with pytest.raises(ClientError) as e:
aws_client.sqs.send_message(QueueUrl=queue, MessageBody="message", MessageGroupId="")
snapshot.match("error", e.value.response)i think if we add and run this we may find that the error message isn't quite correct either. once we understand the full scope of the change i'm happy to set the semver label!
|
@thrau Pushed the snapshot tests |
|
Hey @kshitijkohli 😄 When you ran your parity test, did you remember to point your client to snapshot against AWS? The relevant snippet of the contributing docs that Thomas linked is the How to write Parity tests:
In addition, this section on Integration Testing (against AWS) is definitely also relevant! Once you've setup your testing suite to be able to snapshot against AWS, you can run the test again with those flags enabled: make TEST_TARGET=AWS_CLOUD SNAPSHOT_UPDATE=1 TEST_PATH="tests/aws/services/sqs/test_sqs.py::TestSqsProvider::test_send_message_to_standard_queue_with_empty_message_group_id"Otherwise, I'd also be happy to generate the snapshot for you and push to your feature branch if that's something you'd prefer 🙂 |
85acdf2 to
a75d885
Compare
Motivation
Raising a fix for an open issue in SQS flow of Localstack.
Fixes #11576
Changes
There was a bug in Localstack SQS put message in Standard SQS flow. Principally, standard SQS put calls do not support messagegroupid parameter to be passed. Current implementation of localstack allows "" ie empty string to be passed as messagegroupid while publishing to localstack, whereas same api request to actual AWS sqs throws an exception. Added an additional check in code which checks messagegroupid should not be any string ( having values or empty ) in case of standard sqs put message. If the parameter is an instance of the string, throw exception