[aws-json] Migrate SNS SetSubscriptionAttributes API #321
Conversation
app/gosns/gosns_test.go
Outdated
There was a problem hiding this comment.
Finally we can delete this old test file 👍
| // FIXME: This response is literally strange but it's the same behavior with old SetSubscriptionAtteributes. | ||
| return utils.CreateErrorResponseV1("SubscriptionNotFound", false) |
There was a problem hiding this comment.
Current behavior returns "SubscriptionNotFound" error when an unsupported attribute name is specified.
Should we define another appropriate error?
There was a problem hiding this comment.
Hm, good find. Yeah that is weird. Maybe InvalidParameter? The thing is, that I think there's a lot of other attributes they could be targeting that I guess we don't support either.
I think I would suggest catching all those other ones and maybe just logging that they are unsupported and return. Then our default could be the InvalidParameterValue?
There was a problem hiding this comment.
Agree! I will apply it 👍
| func getSubscription(subsArn string) *app.Subscription { | ||
| for _, topic := range app.SyncTopics.Topics { | ||
| for _, sub := range topic.Subscriptions { | ||
| if sub.SubscriptionArn == subsArn { | ||
| return sub | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I tried to separate the loop indent from the following logic.
There was a problem hiding this comment.
Nice! If you want to throw it in the utils or somewhere more general it might be helpful in the other handlers?
There was a problem hiding this comment.
OK, I will apply the idea in this PR. At lease, I will apply it on GetSubscriptionAttributes API.
There was a problem hiding this comment.
I shared the getSubscription to just only GetSubscriptionAttributes API.
Unsubscribe API logic also has the same loop and indent but the API requires the subscription index in a topic and modify them by topic level perspective. I thought it will be too much commonize for the API.
| uuid := uuid.NewString() | ||
| respStruct := models.SetSubscriptionAttributesResponse{ | ||
| Xmlns: models.BASE_XMLNS, | ||
| Metadata: app.ResponseMetadata{RequestId: uuid}} |
There was a problem hiding this comment.
I realized that the "RequestId" in the response is different between APIs (random uuid or models.BASE_RESPONSE_METADATA).
We can align it into which one before merging the feature branch into master.
There was a problem hiding this comment.
Yeah, I know. They're all different between SQS and SNS. Let's match to whatever is currently here right now, and I'll go back and untangle them later.
|
Hi @Admiral-Piett , this is ready for review! |
app/gosns/gosns_test.go
Outdated
| // FIXME: This response is literally strange but it's the same behavior with old SetSubscriptionAtteributes. | ||
| return utils.CreateErrorResponseV1("SubscriptionNotFound", false) |
There was a problem hiding this comment.
Hm, good find. Yeah that is weird. Maybe InvalidParameter? The thing is, that I think there's a lot of other attributes they could be targeting that I guess we don't support either.
I think I would suggest catching all those other ones and maybe just logging that they are unsupported and return. Then our default could be the InvalidParameterValue?
| uuid := uuid.NewString() | ||
| respStruct := models.SetSubscriptionAttributesResponse{ | ||
| Xmlns: models.BASE_XMLNS, | ||
| Metadata: app.ResponseMetadata{RequestId: uuid}} |
There was a problem hiding this comment.
Yeah, I know. They're all different between SQS and SNS. Let's match to whatever is currently here right now, and I'll go back and untangle them later.
| func getSubscription(subsArn string) *app.Subscription { | ||
| for _, topic := range app.SyncTopics.Topics { | ||
| for _, sub := range topic.Subscriptions { | ||
| if sub.SubscriptionArn == subsArn { | ||
| return sub | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Nice! If you want to throw it in the utils or somewhere more general it might be helpful in the other handlers?
kojisaikiAtSony
force-pushed
the
feature-aws-json-set-subscription-attributes
branch
from
bbedcf3 to
a5aa87e
Compare
|
@Admiral-Piett Updated! |
Admiral-Piett
merged commit e54a44f
into
Admiral-Piett:feature-aws-json
kojisaikiAtSony
deleted the
feature-aws-json-set-subscription-attributes
branch
No description provided.