Sns publish batch #314
Sns publish batch #314
Conversation
|
ref #270 |
|
One thing I noticed is you were adding support for aws json so this may need to be added to the list of things which need aws json support |
|
@goddenrich Thanks for working on this! I really appreciate it. As you pointed out we're in the middle of the JSON upgrade. We're not merging any PRs against master until that's out to minimize the confusion. Let's revisit this after that update is complete, it's almost done, but there's a pattern to those changes that we're going to enforce on all future endpoints, so we'll need to adhere to it too. Happy to talk through that with you and provide examples, if you want to get ahead or have any questions about it. Thanks again for taking the initiative! |
Sure I'm glad that's getting attention. It's blocking us upgrading aws-sdk-go too so pleased that looks like it will be done soon. I'll take a closer look at the patterns from the other json submissions and may give reworking this to align it so it's ready when the other APIs are implemented. I'll let you know if I have any questions |
goddenrich
force-pushed
the
SNS-PublishBatch
branch
from
53fe8c4
to
91e6ead
Compare
goddenrich
force-pushed
the
SNS-PublishBatch
branch
from
91e6ead
to
2fbb393
Compare
|
fyi @Admiral-Piett Ive mostly refactored things to follow the aws json feature work. I have also rebased this onto that feature branch. I still need to add smoke tests but would appreciate it if you could take a look at some point |
There was a problem hiding this comment.
Looks pretty good! I had a couple questions and thoughts, but let me know what you think. Looking forward to those smoke tests too if you can - they're going to be super helpful to make sure we're up to date with the SDK from here on.
Thanks again for looking at all this!
app/gosns/publish.go
Outdated
| @@ -74,8 +75,7 @@ func PublishV1(req *http.Request) (int, interfaces.AbstractResponseBody) { | |||
| return http.StatusOK, respStruct | |||
| } | |||
|
|
|||
| func publishSQS(subscription *app.Subscription, topicName string, requestBody *models.PublishRequest) error { | |||
| messageAttributes := utils.ConvertToOldMessageAttributeValueStructure(requestBody.MessageAttributes) | |||
| func publishSQS(subscription *app.Subscription, message string, messageAttributes map[string]app.MessageAttributeValue, subject string, topicName string, messageStructure string) error { | |||
There was a problem hiding this comment.
I just undid all this argument fatigue in favor of referring to the models.PublishRequest request body. If you're changing it back because we don't have a PublishRequest in the batch flow, I understand, but let's come up with something else to hold all this. Perhaps an interface or something. Having a bazillion arguments makes this difficult to juggle in my opinion.
If you really want to get slick, I wouldn't mind a helper method like publishMessage that takes a topic name, and all the message(s) data, then publishes them however they need to go. It could route between the SQS and HTTP code and return successes VS failures. It would reduce a lot of this duplicate complexity, but I leave that choice to you.
app/gosns/publish_batch.go
Outdated
|
|
||
| seen := make(map[string]bool) | ||
|
|
||
| for _, entry := range requestBody.PublishBatchRequestEntries { |
There was a problem hiding this comment.
Can this whole loop be merged with the similar loop below? Feels like we could just add the following condition below at the start of the second loop and be good?
if seen[entry.ID] {
return utils.CreateErrorResponseV1("BatchEntryIdsNotDistinct", false)
}
There was a problem hiding this comment.
I don't think so because we will publish entries before realising there is a duplicate entry.ID when I believe we should return an error response and not publish any previous entities.
goddenrich
force-pushed
the
SNS-PublishBatch
branch
from
27628b4
to
4b9e312
Compare
goddenrich
force-pushed
the
SNS-PublishBatch
branch
from
0a2b60e
to
93af1b3
Compare
goddenrich
force-pushed
the
SNS-PublishBatch
branch
from
6de0ddd
to
07b9b9b
Compare
|
@Admiral-Piett I have made the changes you have suggested or responded. I have also added the smoke tests (modeled on sns_publish_test.go. Could you take another look. I also notice none of the smoke tests within smoke_tests directory pass but I guess that is a known problem given the state of all the other tests. |
Thanks for taking another look at this! I appreciate the thoroughness. I see your one note, and I have one more question about it otherwise I think this is ok to move on with, at least pending the following thought. What do you mean about the smoke_tests not working? That's not known to me anyway. They work for me when I run them. What error(s) are you getting? What command are you using? |
Ah I have just checked again and noticed the README.md with the bash exports. All other tests are now running and passing the publish batch ones aren't so I will figure that out and let you know. Thanks |
goddenrich
force-pushed
the
SNS-PublishBatch
branch
from
deedeb9
to
a3fed24
Compare
Squashed commits: [7342cdd] Update [f1de347] Add tests [28011fb] fix test [a2d30b4] Add SetAttributesFromForm test [8464892] Add xml smoke test [fe667e4] Update UT [14c8798] Update by reviews [29dc396] Add a test about message size exceeding [9083ad0] Add a test [4efb39a] Fix MessageAttributes propagation [85b0f0b] separate unit test [29b45a3] move model into same model files + update a test [06db0ee] add 1 smoke test [031e67e] test commit [f3062f3] Migrate SQS SendMessage w/o smoke test test commit add 1 smoke test move model into same model files + update a test separate unit test Fix MessageAttributes propagation Add a test Add a test about message size exceeding Update by reviews Update UT Add xml smoke test Add SetAttributesFromForm test fix test Add tests Update add TODO comment
fixes comment
added ut fix review ref review ref resolve conflicts rm sqs_messages.go
minor fixes add not implemented comment to sns model reset tests affecting new tests add debug message when listing sns topics remove merge remnants, minor fixups update listtopics test to handle new mock config
add request transform test to delete topic revise comments - change deletetopicv1 println to log.Info - remove unnecessary deletetopicv1 request setAttributesFromForm update deletetopics test to handle new mock config
add bad request ut case to listsubscriptionsv1 rename listSubscriptions test case review ref
rename review ref review ref
review ref
update udpate fixed update
|
@Admiral-Piett I have quickly merged changes from the json feature branch and master into this PR and reset it to merge into master rather than the feature branch |
Thanks @goddenrich - could you also squash your commits? Otherwise I'll get lost in the sauce so to speak. Thanks! |
|
Sorry I am going to have to copy this into a new branch. The way we this kind of merged in will wreak havoc in the master branch. No sweat though, I appreciate the start. I'll pull it over and finish it off. Thanks @goddenrich ! |
This is a clone of #274 but with the test assert.Lenght fixed