handle_tunnel_request: small code cleanup #391
Conversation
src/tunnel/server/server.rs
Outdated
| } | ||
| }; | ||
| let remote = RemoteAddr::try_from(jwt.claims) | ||
| .inspect_err(|err| warn!("Rejecting connection with bad tunnel info: {err} {}", req.uri())) |
There was a problem hiding this comment.
It is a matter of taste, but I don't really like inspect_err followed by map_err. It is less clear than the match, as it splits the error block in 2 for no reason.
src/tunnel/server/utils.rs
Outdated
| } | ||
|
|
||
| #[inline] | ||
| pub(super) fn extract_path_prefix(req: &Request<Incoming>) -> Result<&str, ()> { | ||
| pub(super) fn extract_path_prefix(req: &Request<Incoming>) -> Result<&str, HttpResponse> { |
There was a problem hiding this comment.
Those functions should not return HttpResponse as Err variant.
It is leaking abstraction, as this error should be known only from the above/callee and not be a inner detail of those function.
The goal of Result<&str, ()> is to let the callee handle/set the error. It is not of the responsibility of those functions to create the error that will be returned.
There was a problem hiding this comment.
i see your point.
however, this is a helper function used only by handle_tunnel_request(). and there are two things which should probably moved to the caller: warn logs and the error creation (as you requested).
i'll try to make the helper function do the one thing it is supposed to do: extract_path_prefix and also move the warn to the caller.
| @@ -17,7 +17,9 @@ use tracing::{error, info, warn}; | |||
| use url::Host; | |||
| use uuid::Uuid; | |||
|
|
|||
| pub(super) fn bad_request() -> Response<Either<String, BoxBody<Bytes, anyhow::Error>>> { | |||
| pub type HttpResponse = Response<Either<String, BoxBody<Bytes, anyhow::Error>>>; | |||
more idiomatic, less code, better readability
tsnoam
force-pushed
the
handle_tunnel_request
branch
from
c0d3485 to
9c65248
Compare
|
@erebe |
|
LGTM, thank you ;) |
more idiomatic, less code, better readability