-
Notifications
You must be signed in to change notification settings - Fork 79
WIP: Trait for HTTP request handler #221
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a trait-based approach for HTTP request handlers in nginx modules, simplifying handler registration and providing type-safe error handling. The new HttpRequestHandler trait allows handlers to return arbitrary types (Option<ngx_int_t> or Result<ngx_int_t, E>) with automatic conversion to nginx's native return type, replacing the previous macro-based approach.
Key changes:
- Added
HttpRequestHandlertrait with associated types for module and phase configuration - Implemented automatic error logging for
Resultreturn types viaHttpHandlerWrappertrait - Introduced
NgxHttpPhasesenum for type-safe phase specification - Migrated all example modules (curl, awssig, async) to use the new trait system
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/http/status.rs | Added From<HTTPStatus> implementation for ngx_int_t to support status code conversion |
| src/http/request.rs | Defined HttpRequestHandler and HttpHandlerWrapper traits with default implementations for Option and Result return types |
| nginx-sys/src/http.rs | Added NgxHttpPhases enum representing nginx HTTP processing phases |
| examples/curl.rs | Converted from macro-based handler to HttpRequestHandler trait implementation |
| examples/awssig.rs | Converted from macro-based handler to HttpRequestHandler trait implementation |
| examples/async.rs | Converted from macro-based handler to HttpRequestHandler trait implementation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
c57f8ea to
8878948
Compare
| } | ||
|
|
||
| /// Implementation of `HttpHandlerWrapper` for `Result<ngx_int_t, E>`. | ||
| impl<T, E> HttpHandlerWrapper<Result<ngx_int_t, E>> for T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result<HTTPStatus, E>?
Also, the bounds for E can be relaxed to Display. I doubt we'll need anything more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bounds relaxed. I'm not sure about HTTPStatus here. Probably we need to have more generic status type, but this maybe out of the scope of current PR.
8878948 to
9ed8efc
Compare
9ed8efc to
6ab1de8
Compare
Trait
HttpRequestHandler<ReturnType>is defined. This trait allows to implement HTTP request handler with arbitrary return type. Special auxiliary traitHttpHandlerWrapper<ReturnType>adds function to convert this return type tongx_int_t. Two conversions are predefined: forOption<ngx_int_t>and forResult<ngx_int_t, E>. Conversion function may include any desired additional actions common for all handlers. For instance, convertor forResult<ngx_int_t, E>logs the error, if any.Trait
HttpRequestHandler<ReturnType>also includes associated type for the HTTP module and associated constant for the handler phase.Handler registration is simplified to a single call of
ngx::http::add_phase_handler::<H: crate::http::HttpHandlerWrapper<R>, R>()function which is implemented by default.All examples are modified to use this trait.
There are some unclear moments:
struct, what can be done with data members of this structure? Are they usable at all?Statusandinto()is acceptable?