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

Add message_attributes to Message object as per SQS Message struct #33

Closed
wants to merge 2 commits into from

Conversation

nritholtz
Copy link
Contributor

This is an adaption with some changes and added specs from #15

@@ -193,7 +211,7 @@
)
expect(nothing.messages.size).to eq 0

sleep(5)
sleep(7)

Choose a reason for hiding this comment

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

Is 7 more lucky than 5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #32, the spec randomly fails on travis at 5, most likely because it just misses 5 second mark for the run_timer.

I didn't want to risk the chance again of the travis build failing on this commit, and people would assume it's because of my changes.

Choose a reason for hiding this comment

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

Fair enough, a comment in the code would be nice for the next person that wonders "why 7 seconds?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a commit in #32 with the comment in the spec.

I made the commit there, since that's where I detailed/explained the original change in the PR.

@raffopazzo
Copy link

Hi there, I'm trying out fake-sqs and I found out that it doesn't support message attributes, which my project relies on. Any chance this PR can get in? 👍

@felixbuenemann
Copy link

@nritholtz The calculation of the checksum does not match that of the current aws sdk.

The following fix is needed to avoid a checksum failure:

diff --git a/lib/fake_sqs/message.rb b/lib/fake_sqs/message.rb
index 7fd2de6..7e4c0df 100644
--- a/lib/fake_sqs/message.rb
+++ b/lib/fake_sqs/message.rb
@@ -53,12 +53,14 @@ module FakeSQS

     def add_string_to_buffer(string, buffer)
       string_bytes = string.force_encoding('UTF-8').bytes.to_a
-      buffer.concat string_bytes.pack("N").bytes.to_a
+      buffer.concat [string_bytes.size].pack("N").bytes.to_a
+      buffer.concat string_bytes
     end

     def add_binary_to_buffer(binary, buffer)
       binary_bytes = binary.unpack("m*")[0].bytes.to_a
-      buffer.concat binary_bytes.pack("N").bytes.to_a
+      buffer.concat [binary_bytes.size].pack("N").bytes.to_a
+      buffer.concat binary_bytes
     end

     private

Also calling bytes.to_a is redundant, because bytes already returns an array.

@nritholtz nritholtz force-pushed the message_attributes branch 2 times, most recently from 10a00ac to cd43bd3 Compare August 31, 2016 11:44
Newer version of sinatra (2+) depend on rack 2+ which requires Ruby version >= 2.2.2
breaking the travis build on older versions of ruby
See https://travis-ci.org/iain/fake_sqs/jobs/156489863

Activesupport 5+ requires Ruby version >= 2.2.2
See https://travis-ci.org/iain/fake_sqs/jobs/156491614
@nritholtz
Copy link
Contributor Author

@felixbuenemann Thanks for the information. Unfortunately, this PR is almost a year old, so this MD5 constraint didn't exist in the client then.

I've updated my commit with your snippet. Please note, that bytes actually returns an enumerator in ruby 1.9.3. Since the travis build supports ruby 1.9.3 also, the specs would've failed something like this:

 1) FakeSQS::Message#message_attributes has message attributes
     Failure/Error: expect(message.message_attributes_md5).to eq "6d31a67b8fa3c1a74d030c5de73fd7e2"
     TypeError:
       can't convert Enumerator into Array
     # ./lib/fake_sqs/message.rb:56:in `concat'
     # ./lib/fake_sqs/message.rb:56:in `add_string_to_buffer'
     # ./lib/fake_sqs/message.rb:40:in `block in message_attributes_md5'
     # ./lib/fake_sqs/message.rb:39:in `each'
     # ./lib/fake_sqs/message.rb:39:in `each_with_object'
     # ./lib/fake_sqs/message.rb:39:in `message_attributes_md5'
     # ./spec/unit/message_spec.rb:35:in `block (3 levels) in <top (required)>'

In addition, I had to add a commit to lock down Sinatra and ActiveSupport gem versions, since the gemspec currently allows the latest and there were plenty of new versions since my last commit a year ago. Both of these gem's latest versions ultimately require ruby version >= 2.2.2. Until older versions of ruby are dropped from this gem, I just added checks within the gemspec.

@miteshsondhi
Copy link

@nritholtz Thanks it works. Please merge this.

@nritholtz
Copy link
Contributor Author

Closed in preference to #59

@nritholtz nritholtz closed this Oct 10, 2019
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.

5 participants