Support JSON-formatted string as messages #25

makimoto · 2017-06-02T04:05:35Z

Using BarbequeClient for enqueuing a message, it might be Array or Hash
because it is posted as JSON format with Content-Type: application/json header.
However in some cases, e.g. posted as query parameter style or enqueued
via SNS subscriptions, the message comes as JSON-formatted string. So
this patch make a message which is String parse as JSON.

@cookpad/dev-infra @k0kubun How do you think about it?

Using BarbequeClient for enqueuing a message, it might be Array or Hash because it is posted as JSON format with `Content-Type: application/json` header. However in some cases, e.g. posted as query parameter style or enqueued via SNS subscriptions, the message comes as JSON-formatted string. So this patch make a message which is String parse as JSON.

k0kubun · 2017-06-02T04:07:30Z

enqueued via SNS subscriptions

Ah~, will take a look later

makimoto · 2017-06-02T08:35:59Z

Travis CI's building with Ruby 2.3.1 has failed by a rubygems bug. https://travis-ci.org/cookpad/barbeque/jobs/238602336 https://bugs.ruby-lang.org/issues/12326
So I update .travis to change target rubies. c7f4ef5

taiki45 · 2017-06-02T09:00:50Z

lib/barbeque/message/job_execution.rb

@@ -13,7 +13,11 @@ def assign_body(message_body)
        super
        @application = message_body['Application']
        @job  = message_body['Job']
-        @body = message_body['Message']
+        @body = if message_body['Message'].is_a?(String)
+                  JSON.parse(message_body['Message'])


Q. Are there any cases a non-JSON string comes here? In that case, what will be happened?

Basically no. If non-JSON string came, the exception handling (shows the follow) world occur.

barbeque/lib/barbeque/message.rb

Lines 17 to 20 in 8ea3873

rescue JSON::ParserError => e

ExceptionHandler.handle_exception(e)

InvalidMessage.new(raw_message, {})

end

taiki45

Looks reasonable to me, as far as I know.

It's good if you show us an example HTTP requests which SNS made without credentials.

makimoto · 2017-06-02T10:39:47Z

It's good if you show us an example HTTP requests which SNS made without credentials.

In Barbeque SNS subscription feature, SQS queues catch SNS notifications directly (#20), so HTTP requests from SNS are not made explicitly.

k0kubun · 2017-06-02T12:12:28Z

I firstly designed Barbeque to be compatible with SNS.

But at some moment I might forget it and changed the API's interface and internal data structure regardless of that. So this change made sense...

k0kubun · 2017-06-02T12:14:27Z

lib/barbeque/message/job_execution.rb

@@ -13,7 +13,11 @@ def assign_body(message_body)
        super
        @application = message_body['Application']
        @job  = message_body['Job']
-        @body = message_body['Message']
+        @body = if message_body['Message'].is_a?(String)


Why are both Barbeque::Message::JobExecution and Barbeque::Message::Notification modified? Isn't changing only Notification sufficient?

Ah, "posted as query parameter style"...

I think garage_client will send with Content-Body: application/json. Is there really such a use case?

Honestly there're no use case for us. So I should get rid of this fix for JobExecution message...

Then, let's remove it. Unnecessary code just increases maintenance cost. Future maintainers will be confused about its background and not be able to remove it easily for consistency once such a behavior is introduced.

Even if we change to accept Content-Type: application/x-www-form-urlencoded in API, the enqueued data format can (and should) be unified, again, for maintenance cost.

Make sense. Removed in fc54849.

k0kubun · 2017-06-02T12:17:06Z

lib/barbeque/message/notification.rb

@@ -21,7 +21,11 @@ def set_params_from_subscription(subscription)
      def assign_body(message_body)
        super
        @topic_arn = message_body['TopicArn']
-        @body = message_body['Message']
+        @body = if message_body['Message'].is_a?(String)
+                  JSON.parse(message_body['Message'])


In your company, this is prohibited. ™️

https://github.com/cookpad/styleguide/blob/092ff807549c16b7ca6d63e11e5514d540f974ba/ruby.en.md#control-structures
[MUST] If you write a control structure just after an assignment operator such as =, obey the following form denoted as "good".

# bad result = if condition body_code end

k0kubun · 2017-06-02T12:20:35Z

lib/barbeque/message/notification.rb

+        @body = if message_body['Message'].is_a?(String)
+                  JSON.parse(message_body['Message'])
+                else
+                  message_body['Message']


Is this line necessary? If "enqueued via SNS subscriptions, the message comes as JSON-formatted string" is true, the line will be never used. Though I don't understand how it works now.

Exactly... Fixed it in 5a95022

There're no use case for now.

k0kubun

👍

makimoto requested a review from k0kubun June 2, 2017 04:05

Update target rubies for Travis CI

c7f4ef5

taiki45 reviewed Jun 2, 2017

View reviewed changes

k0kubun reviewed Jun 2, 2017

View reviewed changes

Remove unreachable code block

5a95022

makimoto force-pushed the json-formatted-message branch from 3339d04 to 5a95022 Compare June 2, 2017 12:31

makimoto added 2 commits June 2, 2017 21:41

Fix spec

f701898

Get rid of JSON-formatted message for JobExecution message class

fc54849

There're no use case for now.

k0kubun approved these changes Jun 2, 2017

View reviewed changes

makimoto merged commit 013d1ce into cookpad:master Jun 2, 2017

makimoto deleted the json-formatted-message branch June 2, 2017 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support JSON-formatted string as messages #25

Support JSON-formatted string as messages #25

makimoto commented Jun 2, 2017

k0kubun commented Jun 2, 2017 •

edited

Loading

makimoto commented Jun 2, 2017 •

edited

Loading

taiki45 Jun 2, 2017

makimoto Jun 2, 2017

taiki45 left a comment

makimoto commented Jun 2, 2017

k0kubun commented Jun 2, 2017

k0kubun Jun 2, 2017

k0kubun Jun 2, 2017

k0kubun Jun 2, 2017

makimoto Jun 2, 2017

k0kubun Jun 2, 2017

k0kubun Jun 2, 2017

makimoto Jun 2, 2017

k0kubun Jun 2, 2017 •

edited

Loading

makimoto Jun 2, 2017

k0kubun Jun 2, 2017

makimoto Jun 2, 2017

k0kubun left a comment

	rescue JSON::ParserError => e
	ExceptionHandler.handle_exception(e)
	InvalidMessage.new(raw_message, {})
	end

Support JSON-formatted string as messages #25

Support JSON-formatted string as messages #25

Conversation

makimoto commented Jun 2, 2017

k0kubun commented Jun 2, 2017 • edited Loading

makimoto commented Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taiki45 left a comment

Choose a reason for hiding this comment

makimoto commented Jun 2, 2017

k0kubun commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k0kubun Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k0kubun left a comment

Choose a reason for hiding this comment

k0kubun commented Jun 2, 2017 •

edited

Loading

makimoto commented Jun 2, 2017 •

edited

Loading

k0kubun Jun 2, 2017 •

edited

Loading