search

Admiral-Piett / goaws

AWS (SQS/SNS) Clone for Development testing
MIT License
767 stars 144 forks source link

Refactor for CreateQueue for V1 JSON support #289

Closed Admiral-Piett closed 3 months ago

Admiral-Piett commented 5 months ago

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 commented 5 months ago
AWS SDK goaws Pass/Fail
3.7.200.65 admiralpiett/goaws:v0.4.5 :heavy_check_mark:
3.7.300.39 admiralpiett/goaws:v0.4.5 :heavy_multiplication_x:
3.7.200.65 feature-aws-json-v1 docker build . :heavy_check_mark:
3.7.300.39 feature-aws-json-v1 docker build . :heavy_multiplication_x:

It looks like a lot of progress has been made here ( :smiley: ), 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 commented 5 months ago

@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 commented 4 months ago

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

Admiral-Piett commented 4 months ago

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)

  1. 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.
  2. Create a RequestFunctionV1 function
  3. Swap the reference in the routing table to the new V1 routing table
  4. Copy the code from the old function
  5. Call utils.REQUEST_TRANSFORMER with your new struct
  6. Update fetches from the request Form to look at the new struct
  7. Make the new V1 function return a status and response struct
  8. Update any references to createErrorResponse to use createErrorResponseV1
  9. Delete old/non-V1 function
  10. Update/Create/Remove unit tests
  11. Smoke Test with the SDK
  12. Add automated tests in smoke_tests/... (use the v2 SDK as much as possible)
  13. PR
kojisaikiAtSony commented 4 months ago

@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?

Admiral-Piett commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

https://github.com/Admiral-Piett/goaws/pull/289#issuecomment-1949372726

  1. Smoke Test with the SDK
  2. 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...

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

kojisaikiAtSony commented 4 months ago

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 commented 4 months ago

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: https://github.com/Admiral-Piett/goaws/pull/292

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

Admiral-Piett commented 4 months ago

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 commented 4 months ago

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 (https://github.com/Admiral-Piett/goaws/pull/291). At that time, I will consider the idea of layer division a little.

Admiral-Piett commented 4 months ago

@kojisaiki Ok, this is ready for review I think - finally. You may have noticed my other question out here - https://github.com/Admiral-Piett/goaws/pull/288#issuecomment-1979569818. 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 commented 4 months ago

@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 https://github.com/Admiral-Piett/goaws/pull/285 (I closed the PR) 👍