Refactor for CreateQueue for V1 JSON support #289

Admiral-Piett · 2024-01-17T23:01:24Z

This is a pretty rough draft of what I had in mind for our previous conversations. If we like where this is headed, I'll fix it up a bit, rename everything away from just V1, and fix all the TODO's.

Preliminarily I think it works for both XML and JSON flows.

Matthew-Davey · 2024-01-22T12:13:51Z

AWS SDK	goaws	Pass/Fail
3.7.200.65	admiralpiett/goaws:v0.4.5	✔️
3.7.300.39	admiralpiett/goaws:v0.4.5	✖️
3.7.200.65	feature-aws-json-v1 `docker build .`	✔️
3.7.300.39	feature-aws-json-v1 `docker build .`	✖️

It looks like a lot of progress has been made here ( 😃 ), I can see in the docker logs that many SQS operations seem to be successful, however my tests are failing at the teardown stage as they are unable to purge queues.

docker logs goaws

{"level":"info","msg":"Putting Message in Queue: callback-service-queue","time":"2024-01-22T11:57:34Z"}
{"level":"info","msg":"2024-01-22 11:57:34: Queue: callback-service-queue, Message: {...},"time":"2024-01-22T11:57:34Z"}
{"level":"info","msg":"Getting Message from Queue: callback-service-queue","time":"2024-01-22T11:57:34Z"}
{"level":"info","msg":"Deleting Message, Queue: callback-service-queue , ReceiptHandle: 3d777ccc-e9e8-4a89-b123-9020dd9b40d2#a1a55fe4-cd92-4635-9a40-f86e4a818a04","time":"2024-01-22T11:57:34Z"}
{"level":"info","msg":"FIFO Queue callback-service-queue unlocking group :","time":"2024-01-22T11:57:34Z"}
{"level":"info","msg":"Purging Queue: ","time":"2024-01-22T11:59:30Z"}
{"level":"info","msg":"Purge Queue:  , queue does not exist!!!","time":"2024-01-22T11:59:30Z"}

Admiral-Piett · 2024-01-30T19:50:43Z

@Matthew-Davey Could you give me some info on how you are testing this? Like - which SDK are you using? Also, have you got this automated somehow? (If so that would be huuuuuuuge here. I've been meaning to do that for ages. 😄 )

I'm looking through the SDK's now, and the python one at least (boto3) still seems to be sending Forms for the time being, at least as far as I can tell. I want to make sure I get on board with this contract before I take this much further.

Admiral-Piett · 2024-02-13T20:06:27Z

@Matthew-Davey Following up on the above, some information would be helpful when you can. Thanks!

Admiral-Piett · 2024-02-16T21:40:31Z

Ok, I think ffa95eb is the last of the required code for the CreateQueue action and the basis of the others. Though I still want to smoke test this against an SDK that's doing JSON now. All the one's I've tried say they support it but aren't actually making calls with it - at least not to CreateQueue. Feedback/word on this would be appreciated.

NOTE: there are things we can improve on later and if you see things feel free to shout/make issues/etc., but I think this is workable for now.

In the meantime, future endpoints more or less could be accomplished with the following steps. (Probably missing some steps, but this should be the bulk - I will update if we find things)

Create a model for your new Request struct
a. Implement <NewStruct>.SetAttributesFromForm - even if it just returns without doing anything.
b. You may also need to handle UnmarshalJSON for any nested request attributes/objects.
Create a RequestFunctionV1 function
Swap the reference in the routing table to the new V1 routing table
Copy the code from the old function
Call utils.REQUEST_TRANSFORMER with your new struct
Update fetches from the request Form to look at the new struct
Make the new V1 function return a status and response struct
Update any references to createErrorResponse to use createErrorResponseV1
Delete old/non-V1 function
Update/Create/Remove unit tests
Smoke Test with the SDK
Add automated tests in smoke_tests/... (use the v2 SDK as much as possible)
PR

kojisaikiAtSony · 2024-02-26T04:18:44Z

@dhumphreys01 @Admiral-Piett
The smoke-test mechanism included in this PR is great!
However, there are many differences in one PR, and it is mixed with the discussion of aws-json, making it difficult to review.

How about creating a separate PR corresponding to smoke-test and adding it to master bransh first?

kojisaikiAtSony

@Admiral-Piett @dhumphreys01
I think this idea would work! 👍

My ideal architecture was to solve the protocol concerns around the router.go and have gosqs.go gosns.go not depend on the http package (For example, the argument of CreateQueueV1 would be models.CreateQueueRequest).

However, my idea will be much bigger than this, so I think it is better to apply your current idea to other SQS/SNS APIs and test it working first!

kojisaikiAtSony · 2024-02-26T05:24:33Z

app/conf/config.go

+	if app.CurrentEnvironment.QueueAttributeDefaults.MessageRetentionPeriod <= 0 {
+		app.CurrentEnvironment.QueueAttributeDefaults.MessageRetentionPeriod = 345600 // 4 days
+	}
+
+	if app.CurrentEnvironment.QueueAttributeDefaults.ReceiveMessageWaitTimeSeconds <= 0 {
+		app.CurrentEnvironment.QueueAttributeDefaults.ReceiveMessageWaitTimeSeconds = 0
+	}


@Admiral-Piett @dhumphreys01
Would it be good to support these latest QueueAttributes in master branch separately from the aws-json consideration?

If I understand correctly, you can currently use these attributes with the aws-query protocol in the Amazon SQS service, right?
https://docs.aws.amazon.com/AWSSimpleQueueService/latest/APIReference/API_SetQueueAttributes.html#API_SetQueueAttributes_RequestSyntax

I think these attributes were all technically supported before, these are just the defaults for if there wasn't anything in the config files. I was trying to avoid adding new things for your point there, but I had some bugs that pushed me into this territory. There might be one of them that's mentioned in some comments that I added, but it doesn't actually do anything I don't think it just gets captured.

Admiral-Piett · 2024-02-26T21:18:49Z

Thanks for taking a look at this @kojisaikiAtSony! In general I think you're right about trying to break a lot of this up - but you're going to have to forgive me, because I just don't have the bandwidth to take it all in steps the way I would like to. I don't have time to go and write them all up as smoke tests before hand - since doing that means I have to plumb the depths of each endpoint and I can only afford to do that once. So I think we need to fill them in as we go - because I want this huge change to be backed by automated tests. I can't keep up with it all otherwise. I know it's annoying, and a lot to sift through, and I'm sorry, but I think we've just got to grit our teeth and cut that corner.

If you have any trouble following this let me know - we can even arrange a zoom call to talk through some of it if that's helpful.

Also, in terms of the V1 tag - I plan to delete all that at the end. It's there for now, to indicate what belongs to the new code and what is outdated, in a searchable way and/or at a glance.

kojisaikiAtSony · 2024-02-27T05:50:27Z

I tried V1 flow on SQS.SendMessage API on #291.
I don't think the amount of code required to rewrite one API is too large, but it's better to try also on SNS API to acknowledge the V1 flow will work 👍

kojisaikiAtSony · 2024-02-28T03:01:02Z

#289 (comment)

Smoke Test with the SDK

Add automated tests in smoke_tests/...

@Admiral-Piett
I found https://github.com/Admiral-Piett/goaws/blob/master/app/servertest/server_test.go has end-to-end level testing with AWS SDK.
We can add more test methods in server_test.go, but is this able to be an alternative to the smoke test you want to achieve?

A concern with testing with the AWS SDK is that we can only use one SDK version, so we cannot test aws-json and aws-query at the same time. Because AWS SDK cannot switch aws-json and aws-query at the same time. Sadly, the protocol is replaced completely...

Change to aws-json in sdk v1
aws/aws-sdk-go@4da2672#diff-1c51913d8f0186051e412a0acfbd318ebf076884ef3864cd5e82fc52f7273c7e
Change to aws-json in sdk v1
aws/aws-sdk-go-v2@f3c195f#diff-47873a43267c27afd0b22b7c24a5155822a7dc602eaebe105f954285cbeb4bc6

We can find the SDK version here:
https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-json-faqs.html#json-protocol-getting-started

kojisaikiAtSony · 2024-02-28T05:19:07Z

It may be a bit rough, but by taking advantage of the fact that there are v1 and v2 of separeted AWS SDK Go, we might be able to test aws-query using SDK v1 1.47.6 and aws-json using SDK v2 1.28.0.
I'll try this idea separately.

kojisaikiAtSony · 2024-02-28T07:57:20Z

It may be a bit rough, but by taking advantage of the fact that there are v1 and v2 of separeted AWS SDK Go, we might be able to test aws-query using SDK v1 1.47.6 and aws-json using SDK v2 1.28.0. I'll try this idea separately.

I created the PoC on the E2E testing idea: #292

In the PoC, I was convinced that it was necessary to separate the protocol context from gosqs and gosns...

Admiral-Piett · 2024-03-01T20:17:51Z

It may be a bit rough, but by taking advantage of the fact that there are v1 and v2 of separeted AWS SDK Go, we might be able to test aws-query using SDK v1 1.47.6 and aws-json using SDK v2 1.28.0. I'll try this idea separately.

I created the PoC on the E2E testing idea: #292

In the PoC, I was convinced that it was necessary to separate the protocol context from gosqs and gosns...

@kojisaikiAtSony I really liked your ideas I used several of them, though I think my style differed slightly. If you prefer some style changes that will make it easier to consume (or any others of course) just shout - I'm open to chatting about it. I I tested this against both styles of the SDK (luckily the python one hasn't updated to JSON and I had that handy, along with the Golang one), looks good prelimiarily. I got the memory issue in the smoke tests too I think - watch out for the global memory resources. They are hold-over even across instances of the server. I wrote a utility to wipe them that you can call at the end of tests until I can figure out a long term strategy.

Otherwise, I really liked your idea about incorporating the SDK into the automated tests - so I'm going to try to work on that at least for the JSON requests and go from there. After that I think it's about there. So let me know what you think about this? If you agree, hopefully this will put us in a good space to knock out the rest more easily.

Thanks for sticking with this!

kojisaikiAtSony · 2024-03-04T06:38:17Z

I got the memory issue in the smoke tests too I think - watch out for the global memory resources. They are hold-over even across instances of the server. I wrote a utility to wipe them that you can call at the end of tests until I can figure out a long term strategy

Nice workaround! 👍

I really liked your idea about incorporating the SDK into the automated tests - so I'm going to try to work on that at least for the JSON requests and go from there

OK!

The latest end-to-end app behavior looks good. I also think the testing approach is good, so please continue the work 👍
The only thing I'm concerned about is that the xml response from gosqs.go is further converted to json (the xml protocol infiltrates the SQS business layer). This is simply because I'm too particular about details. 😓

Next time, I'll try updating the SendMessage implementation (#291). At that time, I will consider the idea of layer division a little.

Admiral-Piett · 2024-03-05T20:25:58Z

@kojisaiki Ok, this is ready for review I think - finally. You may have noticed my other question out here - #288 (comment). Let me know what you think there? I don't want to step on your toes, but these changes look fairly old now.

If we can remove them then this is ready for your opinion and hopefully we can get it merged shortly and divide and conquer the rest of it.

kojisaikiAtSony · 2024-03-08T00:39:33Z

@kojisaiki Ok, this is ready for review I think - finally. You may have noticed my other question out here - #288 (comment). Let me know what you think there? I don't want to step on your toes, but these changes look fairly old now.

If we can remove them then this is ready for your opinion and hopefully we can get it merged shortly and divide and conquer the rest of it.

No problem, you can remove my old commits/codes completely that are from #285 (I closed the PR) 👍

kojisaikiAtSony · 2024-03-08T06:27:39Z

smoke_tests/sqs_create_queue_test.go

+func Test_CreateQueueV1_xml_with_attributes(t *testing.T) {
+	server := generateServer()
+	defer func() {
+		server.Close()
+		utils.ResetResources()
+	}()
+
+	utils.InitializeDecoders()
+
+	e := httpexpect.Default(t, server.URL)
+
+	e.POST("/").
+		WithHeaders(map[string]string{
+			"Content-Type": "application/x-amz-json-1.0",


The test name is ...xml..., but the test request looks using aws-json.

It seems that the test name and the protocol actually used for the other tests are different, so could you please review them?

That is just to create a basic queue first so that I can set it as the Dead Letter Queue in the attributes below. If that queue doesn't exist when I make the second request it will error. But it's not particularly important in and of itself.

Below, the next queue is created with XML which is the one with attributes populated - and the one I am asserting on at the end.

Admiral-Piett mentioned this pull request Jan 17, 2024

[WIP] Feature aws json #288

Merged

24 tasks

kojisaikiAtSony reviewed Feb 26, 2024

View reviewed changes

kojisaikiAtSony mentioned this pull request Feb 27, 2024

[aws-json V1 flow] SQS SendMessage #291

Closed

Admiral-Piett force-pushed the feature-aws-json-v1 branch from 3675f0f to 1babc4e Compare March 1, 2024 20:38

kojisaikiAtSony mentioned this pull request Mar 5, 2024

E2E test with AWS SDK on aws-query/aws-json both #292

Closed

Admiral-Piett force-pushed the feature-aws-json-v1 branch from fa5ea7b to 714dcb8 Compare March 5, 2024 19:53

kojisaiki and others added 5 commits March 5, 2024 14:54

add sample test

6844e32

super roughly passed

fcb9e47

update attribute format on aws-json protocol

1555426

Try routing refactoring

d03d7a9

Refactor for CreateQueue for V1 JSON support

a323e5f

Admiral-Piett force-pushed the feature-aws-json-v1 branch from 714dcb8 to a323e5f Compare March 5, 2024 19:54

Admiral-Piett force-pushed the feature-aws-json branch 2 times, most recently from 6733100 to 44e22ae Compare March 5, 2024 20:03

Admiral-Piett changed the title ~~Rough Draft: Refactor for V1 flow supporting AWS Json~~ Refactor for CreateQueue for V1 JSON support Mar 5, 2024

Admiral-Piett marked this pull request as ready for review March 7, 2024 21:31

kojisaikiAtSony suggested changes Mar 8, 2024

View reviewed changes

Admiral-Piett force-pushed the feature-aws-json branch from 44e22ae to d6568e7 Compare March 13, 2024 21:28

Admiral-Piett merged commit 5b55f9f into feature-aws-json Mar 13, 2024

Admiral-Piett deleted the feature-aws-json-v1 branch March 13, 2024 21:30

Refactor for CreateQueue for V1 JSON support #289

Refactor for CreateQueue for V1 JSON support #289

Uh oh!

Conversation

Admiral-Piett commented Jan 17, 2024

Uh oh!

Matthew-Davey commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Admiral-Piett commented Jan 30, 2024

Uh oh!

Admiral-Piett commented Feb 13, 2024

Uh oh!

Admiral-Piett commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kojisaikiAtSony commented Feb 26, 2024

Uh oh!

kojisaikiAtSony left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kojisaikiAtSony Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

Admiral-Piett Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

Admiral-Piett commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kojisaikiAtSony commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kojisaikiAtSony commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kojisaikiAtSony commented Feb 28, 2024

Uh oh!

kojisaikiAtSony commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Admiral-Piett commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kojisaikiAtSony commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Admiral-Piett commented Mar 5, 2024

Uh oh!

kojisaikiAtSony commented Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kojisaikiAtSony Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

kojisaikiAtSony Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

Admiral-Piett Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Matthew-Davey commented Jan 22, 2024 •

edited

Loading

Admiral-Piett commented Feb 16, 2024 •

edited

Loading

kojisaikiAtSony left a comment •

edited

Loading

Admiral-Piett commented Feb 26, 2024 •

edited

Loading

kojisaikiAtSony commented Feb 27, 2024 •

edited

Loading

kojisaikiAtSony commented Feb 28, 2024 •

edited

Loading

kojisaikiAtSony commented Feb 28, 2024 •

edited

Loading

Admiral-Piett commented Mar 1, 2024 •

edited

Loading

kojisaikiAtSony commented Mar 4, 2024 •

edited

Loading

kojisaikiAtSony commented Mar 8, 2024 •

edited

Loading