Make Server::into_router() public for TLS support#12
Make Server::into_router() public for TLS support#12ivonnyssen wants to merge 1 commit intoRReverser:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR widens the server API to support custom socket binding (including TLS termination) by exposing the internal Axum router so downstream consumers can run the server with their own listener/acceptor setup.
Changes:
- Made
Server::into_router(self) -> Routerpublic. - Added rustdoc to explain the intended TLS/binding use case and discovery-server responsibility.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Consumes the server and returns the underlying [`axum::Router`]. | ||
| /// | ||
| /// Use this when you need to handle socket binding and TLS wrapping | ||
| /// yourself instead of using [`Self::bind`]. The caller is responsible | ||
| /// for starting the Alpaca discovery server separately if needed. | ||
| #[expect(clippy::too_many_lines)] | ||
| fn into_router(self) -> Router { | ||
| pub fn into_router(self) -> Router { | ||
| let devices = Arc::new(self.devices); |
There was a problem hiding this comment.
Making into_router public exposes axum::Router in this crate’s public API surface. That effectively couples semver compatibility to Axum’s Router type (e.g., an Axum major upgrade could become a breaking change for this crate even if internal usage stays the same). If you want to keep the public API more stable, consider returning a crate-defined wrapper/type alias (and/or gating this behind a feature) so downstream code doesn’t need to name axum::Router directly.
RReverser
left a comment
There was a problem hiding this comment.
I actually have same concern as Copilot surfaced here. I intentionally tried to keep axum an implementation detail - it should be possible to switch server implementations if we decide to, without worrying about public API.
Is there a smaller-scoped change that could be made to still provide the desired functionality? At the very least changing the return signature to impl tower_service::Service would already limit the API surface.
|
Thanks for the review. I'd like to take this further than a direct signature swap so the decoupling is structural rather than nominal. Quick note on why I don't think With that in mind, three shapes I've considered. I think A is the best fit; listing B and C for completeness since I don't think either is better than A. A (recommended): opaque service newtypepub struct AlpacaService { /* private — wraps the internal router */ }
impl Clone for AlpacaService { ... }
impl<B> tower_service::Service<http::Request<B>> for AlpacaService
where
B: http_body::Body<Data = Bytes> + Send + 'static,
B::Error: Into<BoxError>,
{
type Response = http::Response<UnsyncBoxBody<Bytes, BoxError>>;
type Error = Infallible;
type Future = BoxFuture<'static, Result<Self::Response, Infallible>>;
}
impl Server {
pub fn into_service(self) -> AlpacaService { ... }
}Body conversion stays inside B: runner-style API, service never leaves the crateimpl Server {
pub async fn serve_on(self, listener: TcpListener) -> Result<Infallible>;
pub async fn serve_tls(self, listener: TcpListener, cfg: rustls::ServerConfig) -> Result<Infallible>;
}Also hides axum, simpler than A, but rigid — each new integration needs a new upstream method, and middleware composition isn't covered out of the box. Concrete example: a downstream service I maintain wraps the Alpaca routes with HTTP Basic Auth middleware (config-driven C: callback-style bindingpub fn bind_with<F, Out>(self, build: F) -> Out
where F: FnOnce(AlpacaService) -> Out;Hands the service to a user closure and serves whatever comes back. Covers all shapes in principle, but the trait bounds get awkward and the common case is harder to document than A. If A sounds reasonable, I'll rework the PR along those lines. Happy to pivot if you'd prefer a different shape — e.g., keeping |
But Router's Service implementation is already generic over B. If you just change the signature to be |
|
You're right — One bit to decide: the Response type pins to // Simple — response body projects to axum::body::Body
pub fn into_router<B>(self) -> impl Service<http::Request<B>, Error = Infallible> + Clone + Send + 'static
where
B: http_body::Body<Data = Bytes> + Send + 'static,
B::Error: Into<BoxError>,// Boxed response body — fully axum-free
pub fn into_router<B>(self) -> impl Service<
http::Request<B>,
Response = http::Response<UnsyncBoxBody<Bytes, BoxError>>,
Error = Infallible,
> + Clone + Send + 'static
where
B: http_body::Body<Data = Bytes> + Send + 'static,
B::Error: Into<BoxError>,The second is a one-liner |
|
I don't really want to dig into details - feel free to investigate, I just want the API to remain as clean as possible and untied from axum :) |
Add `Server::into_service()` returning a new `AlpacaService` newtype that implements `tower_service::Service` for any `http::Request<B>` with `B: Body<Data = Bytes>`. Responses use a type-erased `UnsyncBoxBody<Bytes, BoxError>` so callers don't see axum types. This supports TLS termination and custom middleware composition without leaking axum as part of the public API surface. Co-Authored-By: Claude Opus 4.7 <[email protected]>
20da95f to
c4d787c
Compare
Understood. I think the latest push should do that. No axum in the API and we have a way to support TLS and authentication. |
Summary
Changes
Server::into_router()fromfntopub fn, allowing consumers to obtain the underlyingaxum::Routerand handle socket binding and TLS wrapping themselves.Motivation
The current API only exposes
Server::listen()andServer::bind(), both of which bind to a plain TCP socket internally. For TLS termination at the Alpaca server (e.g. usingaxum-serverwithrustls), consumers need access to the router so they can wrap it in their ownaxum_server::bind_rustls()call without duplicating the entire route setup.Changes
src/server/mod.rs: Changedfn into_router(self) -> Routertopub fn into_router(self) -> Routerand added documentation explaining the use case. The caller is responsible for starting the discovery server separately if needed.Test plan
cargo clippypasses (full CI feature matrix verified)