Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route processing refactor #8463

Merged
merged 34 commits into from Dec 14, 2022
Merged

Route processing refactor #8463

merged 34 commits into from Dec 14, 2022

Conversation

yawkat
Copy link
Member

@yawkat yawkat commented Dec 6, 2022

This is a refactor of route execution, excluding filters (those will follow in #8422). The goals are:

  • Moving away from reactive APIs where possible.
  • Streamlining route execution so that it will be possible to have pre-route filters, and filters that read the request body.

This initial patch factors most of the routing code out of RoutingInboundHandler (the response writing code remains), and moves away from reactive for HttpContentProcessor and the route parameter fulfillment code (now called BaseRouteCompleter and FormRouteCompleter).

The goal of this PR is to get to a point where I am confident that it will be easy to delay filter execution until the body is available. Ideally the full request lifecycle, from before route resolution, through filter execution, to route execution, will be contained in the RouteRunner class.

edit, For reviewers: Much of the RouteExecutor code was moved to RequestLifecycle. Much of the RoutingInboundHandler code was moved to NettyRequestLifecycle and the two completer classes. Additionally, there is a new MicronautHttpData class that replaces netty's HttpData implementation, and HttpContentProcessor was refactored to not be reactive, which helps avoid concurrency considerations.

@yawkat
Copy link
Member Author

yawkat commented Dec 7, 2022

jackson-xml pr to fix up content processor: micronaut-projects/micronaut-jackson-xml#235

@yawkat
Copy link
Member Author

yawkat commented Dec 7, 2022

servlet pr to fix up RouteExecutor dependency: micronaut-projects/micronaut-servlet#363

@yawkat
Copy link
Member Author

yawkat commented Dec 7, 2022

This PR is now ready. The request routing code is now concentrated in RequestLifecycle. This should make it much easier to implement additional filtering behavior, such as reading the request body.

@yawkat yawkat marked this pull request as ready for review December 7, 2022 12:08
@yawkat yawkat requested review from timyates and removed request for dstepanov December 7, 2022 13:38
@yawkat
Copy link
Member Author

yawkat commented Dec 9, 2022

micronaut-function-aws-api-proxy requests include the host in getUri. This leads to no match with findAny. This causes the AVAILABLE_HTTP_METHODS to be empty, which causes a test failure with CorsFilter in the aws module.

Unfortunately I can't test this using CorsFilterSpec in this repo, because CorsFilterSpec calls findAny directly instead of using the findRouteMatch code path. There is also a PR for refactoring CorsFilterSpec (#8473) but it does not get rid of the mocking either.
@yawkat
Copy link
Member Author

yawkat commented Dec 10, 2022

In the AWS module, I've replaced most of the custom request processing code with the existing code in RouteExecutor. They are mostly the same, but there are a few differences.

The biggest change is about uses of request.getUri().toString(). For netty requests, this is usually identical to the path of the request. However with the aws proxy module, it includes the request host, so any route processing that treats the uri as a path breaks.

I've adjusted a few of these use sites related to route selection to use request.getPath() in 4a040af and 686de0f.

@dstepanov
Copy link
Contributor

I think we need to have more changes done if we want to support reading the request body in the pre-route filters. Probably introducing some internal structure that can represent the request body in different stages (fetched, reactive fetching) and different body contents (JSON, Form etc). That would likely eliminate receivedContent and receivedData from the NettyHttpRequest.

@yawkat
Copy link
Member Author

yawkat commented Dec 12, 2022

@dstepanov yes more changes are needed. but i think they can be limited to RequestLifecycle and NettyRequestLifecycle now. All previous users of RouteExecutor (netty, netty websocket, jetty, aws) now use RL, and e.g. filter processing is encapsulated (no more calls to filterPublisher), so it should be possible to limit changes to RL.

However this PR is too big already. It will be in follow-up PRs.

@dstepanov
Copy link
Contributor

Going to approve it as nothing is blocking it. I feel like classes like HttpContentProcessorAsReactiveProcessor FormRouteCompleter StreamingDataSubscriber are a bit complex and hard to understand. Maybe we can revisit it after we have all the changes that are needed for Microanut 4.

@yawkat
Copy link
Member Author

yawkat commented Dec 13, 2022

im still testing some changes for your other comments, so please don't merge yet :)

@graemerocher graemerocher marked this pull request as draft December 13, 2022 11:53
@yawkat yawkat marked this pull request as ready for review December 13, 2022 11:55
@yawkat
Copy link
Member Author

yawkat commented Dec 13, 2022

@graemerocher just finished it, all comments from @dstepanov addressed

@sonarcloud
Copy link

sonarcloud bot commented Dec 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

85.7% 85.7% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants