Skip to content

Fix SQS queue creation attributes and specific attribute retrieval#2005

Merged
whummer merged 2 commits intolocalstack:masterfrom
ImFlog:fix-sqs-attributes
Feb 3, 2020
Merged

Fix SQS queue creation attributes and specific attribute retrieval#2005
whummer merged 2 commits intolocalstack:masterfrom
ImFlog:fix-sqs-attributes

Conversation

@ImFlog
Copy link
Contributor

@ImFlog ImFlog commented Jan 31, 2020

Hello,

This PR fix two issues with the attributes:

  • When we create a queue, attributes not supported by ElasticMQ are not saved
  • When we query specific attributes, the unsupported attributes are all appended to the response

Also ElasticMQ now support tag operations (https://github.com/softwaremill/elasticmq/blob/master/rest/rest-sqs/src/main/scala/org/elasticmq/rest/sqs/TagQueueDirectives.scala), so i removed the hack around the tag and list methods.
There is still an issue around tagging on queue creation but it's on ElasticMQ side (softwaremill/elasticmq#333).

Thanks

@ImFlog ImFlog requested a review from whummer January 31, 2020 21:36
@ImFlog
Copy link
Contributor Author

ImFlog commented Jan 31, 2020

@whummer the build is failing but during the docker-build phase (s3 not starting, but not related to my code).
Could you please re-trigger the build ?

Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for these changes @ImFlog ! Only added a few minor comments.

queue_url = self._queue_url(path, req_data, headers)
self._set_queue_attributes(queue_url, req_data)
elif action == 'DeleteQueue':
del QUEUE_ATTRIBUTES[self._queue_url(path, req_data, headers)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit safer, to avoid KeyError:

QUEUE_ATTRIBUTES.pop(self._queue_url(path, req_data, headers), None)

key = 'AttributeName.%s' % i
if key not in req_data:
break
result.add(req_data[key][0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming we're accessing index 0 here because req_data is created from urlparse which creates values as lists. We should probably update the documentation comment of this method then: ... 'AttributeName.1': ['Policy'] ....

In the future, we may apply some small preprocessing to extract all values from the lists contained in req_data, as usually they only contain a single item anyway.

creation_attributes = self.client.get_queue_attributes(QueueUrl=queue_url, AttributeNames=['All'])

# assertion
self.assertIn('MessageRetentionPeriod', creation_attributes['Attributes'].keys())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: We could also assert that the correct value is returned here (604800). Same for the test test_get_specific_queue_attribute_response below.

@coveralls
Copy link

coveralls commented Feb 1, 2020

Coverage Status

Coverage increased (+0.2%) to 50.905% when pulling 3082646 on ImFlog:fix-sqs-attributes into 81a409b on localstack:master.

@whummer
Copy link
Member

whummer commented Feb 3, 2020

Great, thanks again for these enhancements and the quick turnaround!

@whummer whummer merged commit 9a772a0 into localstack:master Feb 3, 2020
@ImFlog ImFlog deleted the fix-sqs-attributes branch February 3, 2020 13:01
jgbmattos pushed a commit to jgbmattos/localstack that referenced this pull request Mar 10, 2020
For  mine redrive policy to work on lambda. It was necessary or the raise the  lambda_api.process_sns_notification try except or to handle  the exception on sns_listener.
Since the only place that process_sns_notification is called is on sns_listener i decide to remove the try except inside lamda_api

Minor fix on tests.
Created tests for redrive policy in SNS.
Changed the position of try in case of sqs.
For the lambda execution i would appreciate an opinion.
I discover that the function proccess_sns_notification has two possible return, None, ou an HTTP response. I pretend to check if lambda behaviour that way, or not. Because i guess it would be better if it always return an HTTP Response.

resolve CloudFormation attributes starting with lower case (localstack#2008)

refactor persistence logic; use single file for persistence (localstack#2011)

Fix SQS queue creation attributes and specific attribute retrieval (localstack#2005)

Update elasticmq in order to fix sqs tag on creation (localstack#2017)

fix regex for replacement of S3 ETag hashes (localstack#2021)

Add windows support to the MakeFile (localstack#2024)

Refactor CloudFormation dependency resolution (localstack#2026)

fix resolution of CF stack parameters (localstack#2028)

fix CF unit test

Fix SNS tag listing to remove duplicate tags (localstack#2014)

Fix CloudFormation dependency resolution loop; async stack deployment (localstack#2031)

add ExportName to CloudFormation stack outputs (localstack#2033)

Fix CloudFormation support for IAM::Role (localstack#2034)

Remove None strings from SNS results; refactor resolution of CF resource name placeholders (localstack#2036)

fix persistence for ES API (localstack#2040)

Prefix CloudWatch event file names with timestamps (localstack#2035)

Mark Java LocalstackExtension deprecated (localstack#2047)

add persistence for Elasticsearch Service API calls (localstack#2048)

add API to confirm SNS subscriptions (localstack#2043)

fix setting of empty SQS queue attribute values (localstack#2052)

Expose java options for local lambda executors (localstack#2050)

Fix kinesis stream get_cfn_attribute (localstack#2063)

Upgrade testcontainers Maven dependency to version 1.12.5 (localstack#2059)

minor: fix base image and add sasl libs (localstack#2065)

support ExtendedS3DestinationConfiguration in Firehose streams (localstack#2068)

fix RawMessageDelivery subscription values for SNS - SQS integration (localstack#2067)

Check for None before iterating (localstack#2064)

Fix s3 notification event object size (localstack#2069)

Update PYTHONPATH for Python 3.8 (localstack#2070)

support static refs in CloudFormation Fn::Sub strings (localstack#2076)

Fix CloudWatch log streams lambda timstamp format (localstack#2078)

Return ConsumedCapacity for DynamoDB Query action (localstack#2071)

Add basic /health check endpoint (localstack#2080)

move Java sources to separate project (localstack#2084)

update README

Return SQS maxReceiveCount attribute as integer (localstack#2081)

Allow deleting a specific version of an object in S3 (localstack#2087)

update exports on CF stack update (localstack#2097)

fix handler lookup for "provided" Lambda runtime (localstack#2098)

Consider LAMBDA_REMOVE_CONTAINERS config for docker-reuse Lambda executor (localstack#2094)

fix returned attributes on ReturnValues=ALL_OLD for DynamoDB PutItem; Fix CreationTime for CloudFormation stacks (localstack#2103)

Configure nodejs Lambdas to skip SSL verification; add CF support for S3::BucketPolicy (localstack#2104)

Fix SNS subscription confirmation message to include signature details (localstack#2100)

Fix s3 notification event objectsize (localstack#2105)

Fix notifications for s3 uploads made with presigned post requests (localstack#1640)

Optimize plugin loading to speed up boot time (localstack#2109)

fix deployment of EC2 subnets with CidrIpv6 (localstack#2112)

Make Lambda batch size configurable for Kinesis event source mappings (localstack#2110)

Fix creation of SQS tags via CloudFormation (localstack#2114)

Release version 0.10.8; minor fix in DynamoDB ListStreams (localstack#2117)

Fix forwarding of Lambda output into CloudWatch Logs (localstack#2118)

Fix empty response for PutTargets in CloudWatch Events (localstack#2129)

Fix setting of attributes in existing SQS queues (localstack#2130)

integrate S3 starter into multiserver to improve performance (localstack#2132)

Generate default Lambda FunctionName in CloudFormation (localstack#2134)

Fix order of resource checks for CF deployment in single process (localstack#2136)

update elasticmq to 0.15.5 in base Docker image (localstack#2137)

add postgresql-dev to base Docker image

update elasticmq, assert messages with invalid characters are rejected by SQS (localstack#2135)

Support S3 bucket notifications in CF deployments (localstack#2138)

add generated ReceiptHandle for SQS messages forwarded to Lambda (localstack#2139)

Import fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants