Apigw NG create method request handler #11096
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 22m 46s ⏱️ - 1h 13m 50s Results for commit 15f8768. ± Comparison against base commit 02302df. This pull request removes 2508 tests.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
LGTM! Nice refactor of the code 🚀 Just a minor comment regarding the testing, still not sure what our strategy should be regarding testing of handlers... I'd lean towards testing handlers in isolation, and should we need it, only instantiate a chain if we need different handlers executed one after another? What do you think?
localstack-core/localstack/services/apigateway/next_gen/execute_api/handlers/method_request.py
Outdated
Show resolved
Hide resolved
| @pytest.fixture | ||
| def method_handler_chain() -> RestApiGatewayHandlerChain: | ||
| """Returns a dummy chain for testing.""" | ||
| method_handler_chain = RestApiGatewayHandlerChain(request_handlers=[MethodRequestHandler()]) | ||
| # Sets raise_on_error to allow capturing the Exceptions raised by the validator | ||
| method_handler_chain.raise_on_error = True | ||
| return method_handler_chain |
There was a problem hiding this comment.
nit: should we use the handler chain with only handler, or should we directly instantiate the handler in the tests, so that we would not need to set method_handler_chain.error = None after every invocation?
In our Gateway, we instantiate a new handler chain for every incoming request.
We could maybe make the intent clearer by only testing the handler itself?
There was a problem hiding this comment.
I think you are right, I will see how I can make it cleaner by calling the handler itself! 😄
There was a problem hiding this comment.
Very neat update 💯 I like that, I think we should follow the same pattern for new tests, with the invoker!
|
I think you are right for the chain creation. My first thoughts were to also use the gateway response handlers and make assertion on the responses. But decided against it as it will be better as integration and snapshot tests later. I updated the tests to use only the handler, I think it is much cleaner that way! Thanks for the comment 🚀 |
45dd6be
to
15f8768
Compare
Motivation
This PR ports the functionality from
apigateway/invocations.py:RequestValidatorto a method request handler.The old invocation process wasn't routing the request before validation so some "passthrough" were required when no matching method were found. Cleaning up some of that code will make it cleaner and more obvious as to what is happening.
We can also start raising error from the gateway responses to be prepared for it's future implmentation.
Changes
RequestValidatorto ensure the functionality hasn't regressed.ModelResolverfails to find it's ref. There might be more work to do here, but that is the statement that helped me understand why my model wasn't resolving! :)Out of scope
While there could be an argument to include validation of api key as part of this handler as they are configured together, they are separate enough in implementation that we can simplify readability/maintainability of the code by further breaking it down into it's own handler. All of the api key validation is done before request validation so a new handler seems like the best solution.