HandleSQSSend allow for array body #9
Conversation
proxy/sqs.go
Outdated
| http.Error(w, fmt.Sprintf("Error sending SQS message: %v", err), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| // Check if body starts with '[' to detect JSON array (multiple messages) |
There was a problem hiding this comment.
What if the message starts with [. This will break existing clients.
A safe and "non-magical" option is to check for a new header SQS-Batch-Message and if set (value is not important) AND the Content-Type header starts with application/json (not == since you can have things like ; charset=utf-8 suffix — best to use ParseMediaType from stdlib to properly parse the MIME type), THEN and only then is the request valid and will be treated as a JSON array of string objects to be sent as a batch of messages to SQS.
Why new header
Since we allowed HTTP requests with any content and any content type to be treated as a byte string and to be sent on the queue, we essentially removed any possibility of the content type OR the content to define anything meaningful.
Therefore, we need an extra signal. The SQS-Batch-Message is that signal.
Why Content-Type: application/json
You should never assume the content type of an HTTP request based on its content. HTTP content is typed, unlike say a file on disk. Compliant clients/servers will treat the same bytes in the body differently based on what the content type is. It's therefore best to build conforming servers/clients that don't fall over when interacting with unforeseen peers in the future.
proxy/sqs.go
Outdated
| var allEntries []sqstypes.SendMessageBatchRequestEntry | ||
| for i, msg := range messages { | ||
| allEntries = append(allEntries, sqstypes.SendMessageBatchRequestEntry{ | ||
| Id: aws.String(fmt.Sprintf("msg-%d", i)), |
There was a problem hiding this comment.
Why the extra msg- prefix? This only needs to be unique within the batch and we know they are messages.
proxy/sqs.go
Outdated
| // Check if body starts with '[' to detect JSON array (multiple messages) | ||
| if len(body) > 0 && body[0] == '[' { | ||
| // Multiple messages - parse and use batch send | ||
| var messages []json.RawMessage |
There was a problem hiding this comment.
This is assuming each message itself is also JSON. Why? This will be limiting what you can send to a queue unnecessarily and requires the receiving end to parse the message as JSON.
One alternative is to accept JSON array of strings:
var messages []string
proxy/sqs.go
Outdated
| } | ||
|
|
||
| // Send in batches of 10 (SQS limit) | ||
| for i := 0; i < len(allEntries); i += 10 { |
There was a problem hiding this comment.
Avoid magic numbers and define these as file level const:
const maxSQSBatchSize = 10
proxy/sqs.go
Outdated
|
|
||
| // Build entries for batch send (split into groups of 10) | ||
| var allEntries []sqstypes.SendMessageBatchRequestEntry | ||
| for i, msg := range messages { |
There was a problem hiding this comment.
nit: You can construct the message instances in the next loop where you're actually sending them to SQS 10 at a time, instead of all at once, taking up memory.
proxy/sqs.go
Outdated
| return | ||
| } | ||
|
|
||
| if len(messages) == 0 { |
There was a problem hiding this comment.
Unless there is a significant performance advantage, I suggest accepting up to the maximum of 10 messages allowed by SQS API and thus making a single call to SQS.
There was a problem hiding this comment.
Would the best way to determine this be to deploy a simple lambdafied function in the dev account and run requests against it?
proxy/sqs.go
Outdated
| Entries: batch, | ||
| }); err != nil { | ||
| log.Printf("error sending SQS message batch: %v", err) | ||
| http.Error(w, fmt.Sprintf("Error sending SQS message batch: %v", err), http.StatusInternalServerError) |
There was a problem hiding this comment.
This is swallowing individual errors of the batch send. The client would want to know if some of the items managed to get through.
proxy/sqs.go
Outdated
|
|
||
| log.Printf("sent an SQS message to '%s'", qURL) | ||
| log.Printf("sent %d SQS messages to '%s'", len(messages), qURL) |
There was a problem hiding this comment.
readability: should return early here and unindent the rest of the code instead of using else. Better yet, move the single message case to the top since it's smaller and easier to read and return early after that.
|
@msmans so far I've addressed everything except the number of messages accepted and partial failure handling. |
Allows for JSON array to be sent to the sqs send handler.
This is backwards compatible with single message send
You can put as many messages in a single request to the handler. It will split them into 10 or less batches for relaying to the SQS queue with SendMessageBatch