Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify MD5 checksums returned by SQS #1093

Merged
merged 1 commit into from Feb 19, 2016
Merged

Conversation

tawan
Copy link
Contributor

@tawan tawan commented Feb 11, 2016

Verify MD5 checksums returned by SQS for received messages.

@tawan tawan force-pushed the issue-1085-sqs-md5 branch 2 times, most recently from 7ac8542 to 958856e Compare February 15, 2016 05:35
@trevorrowe
Copy link
Member

I took a moment to review this. The code looks good. I had one piece of feedback above we need to address and then I'm happy to merge this in. Good work!

Verify MD5 checksums returned by SQS for received messages.
 * Verify checksum of message body.
 * Verify checksum of message attributes if present.

 Refer to aws#1085
attributes = entry[:message_attributes]
message_response = response.successful.select { |r| r.id == id }[0]
unless message_response.nil?
validate_single_message(body, attributes, message_response)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trevorrowe This is the line you mentioned in the previous commit: I simple added a nil check. Only messages which are which are placed in the successful array are verified.

@tawan
Copy link
Contributor Author

tawan commented Feb 19, 2016

@trevorrowe Thanks for your feedback. I incorporated your suggestion. I amended my changes and rebased, therefore your inline comment seems to be gone. I commented the line in question again for some context.

trevorrowe added a commit that referenced this pull request Feb 19, 2016
Verify MD5 checksums returned by SQS
@trevorrowe trevorrowe merged commit 75228d5 into aws:master Feb 19, 2016
@trevorrowe
Copy link
Member

Thanks for the contribution! This should go out with our next release.

I'm going to do a small amount of refactoring on the tests. We've been moving away from unit testing the handlers in isolation and instead functionally test the client class they are applied to. This has the benefit of ensuring the plugin is applied correctly. Your tests are organized well enough that moving this should be pretty straightforward.

trevorrowe added a commit that referenced this pull request Feb 24, 2016
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.

None yet

3 participants