-
-
Notifications
You must be signed in to change notification settings - Fork 413
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
New combinator to return routed path in response headers #1561
base: master
Are you sure you want to change the base?
Conversation
99356af
to
d09215b
Compare
I would love to read a bit more on practical uses of this feature! |
d09215b
to
6146f74
Compare
Indeed, my first motivation for this was to be able to use a middleware to observe the usage statistics and response time of the endpoints in my own API. I didn't want to use servant-ekg, because I wanted my middleware to be aware of Servant's fallback feature when capture fails ; I also preferred the routing to be done only once. Also, I think this PR lays the groundwork for a whole new class of interesting features. |
@nbacquey This is music to my ears. |
servant-server/src/Servant/Server.hs
Outdated
@@ -235,7 +235,7 @@ hoistServer p = hoistServerWithContext p (Proxy :: Proxy '[]) | |||
-- > │ └─ e/ | |||
-- > │ └─• | |||
-- > ├─ b/ | |||
-- > │ └─ <capture>/ | |||
-- > │ └─ <x::CaptureSingle>/ |
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.
Unrelated, but do you have any intuition on how hard it would be to add HTTP method information next to each leaf? :)
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.
I don't think it would be really hard ; it would look a lot like what I've done in #1556, except that you would have to add another argument to StaticRouter
, and pass it in the HasServer
instance for Verb
. I expect the new router constructor to look like
StaticRouter (Map Text (Router' env a)) [(RouterEnv env -> a, HTTPMethod)]
Then the information would get passed along routerStructure
, then to routerLayout
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.
Excellent, thank you :)
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.
Wonderful PR, thank you very much.
I left some requests.
For the @since
annotations, you can target 0.20.0.0 (informally known as 0.20)
I don't know what you exactly have in mind, but as a side note: it is already possible to do so to some extent. E.g.: data StrictMethodMatch
instance HasServer api context => HasServer (StrictMethodMatch :> api) context where
type ServerT (StrictMethodMatch :> api) m = ServerT api m
route Proxy ctx denv = tweakResponse turn405IntoFatalErrors (route (Proxy @api) ctx denv)
where turn405IntoFatalErrors (Fail e@(ServerError{errHTTPCode = 405})) = FailFatal e
turn405IntoFatalErrors response = response
hoistServerWithContext _ pctx nat = hoistServerWithContext (Proxy @api) pctx nat This combinator captures 405 errors from a sub-API, and turns them into fatal failures instead of continuing routing. It prevents 405 errors from, e.g., being swallowed by |
I don't remember my specific usecase, but I hadn't thought about using |
6146f74
to
37cf3fd
Compare
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.
My comments have been addressed
Do I understand correctly that this would require changing a type in servant and that the new combinator you describe would need to be based off a patched servant? |
You are correct, this hypothetical new combinator would need to be based off a patched servant. My point was that the patch would be quite simple to write. |
37cf3fd
to
6c51cf8
Compare
I rebased on the new version of #1556, which reinstates "real" type hints, instead of |
738cfbe
to
203671d
Compare
203671d
to
809c8ce
Compare
809c8ce
to
ed40f9d
Compare
Added a fix to prevent |
ed40f9d
to
b1e6748
Compare
This commit introduces a new type-level combinator, `WithRoutingHeader`. It modifies the behaviour of the following sub-API, such that all endpoint of said API return an additional routing header in their response. A routing header is a header that specifies which endpoint the incoming request was routed to. Endpoint are designated by their path, in which `Capture'` and `CaptureAll` combinators are replaced by a capture hint. This header can be used by downstream middlewares to gather information about individual endpoints, since in most cases a routing header uniquely identifies a single endpoint. Example: ```haskell type MyApi = WithRoutingHeader :> "by-id" :> Capture "id" Int :> Get '[JSON] Foo -- GET /by-id/1234 will return a response with the following header: -- ("Servant-Routed-Path", "/by-id/<id::Int>") ``` To achieve this, two refactorings were necessary: * Introduce a type `RouterEnv env` to encapsulate the `env` type (as in `Router env a`), which contains a tuple-encoded list of url pieces parsed from the incoming request. This type makes it possible to pass more information throughout the routing process, and the computation of the `Delayed env c` associated with each request. * Introduce a new kind of router, which only modifies the RouterEnv, and doesn't affect the routing process otherwise. `EnvRouter (RouterEnv env -> RouterEnv env) (Router' env a)` This new router is used when encountering the `WithRoutingHeader` combinator in an API, to notify the endpoints of the sub-API that they must produce a routing header (this behaviour is disabled by default).
b1e6748
to
801f075
Compare
@alpmestan @gdeest I would appreciate if we could bring this PR to a conclusion :) |
This commit introduces a new type-level combinator,
WithRoutingHeader
.It modifies the behaviour of the following sub-API, such that all endpoints of said API return an additional routing header in their response.
A routing header is a header that specifies which endpoint the incoming request was routed to.
Endpoint are designated by their path, in which
Capture'
andCaptureAll
combinators are replaced by a capture hint.This header can be used by downstream middlewares to gather information about individual endpoints, since in most cases
a routing header uniquely identifies a single endpoint.
Example:
To achieve this, two refactorings were necessary:
RouterEnv env
to encapsulate theenv
type (as inRouter env a
), which contains a tuple-encoded list of url pieces parsed from the incoming request. This type makes it possible to pass more information throughout the routing process, and the computation of theDelayed env c
associated with each request.EnvRouter (RouterEnv env -> RouterEnv env) (Router' env a)
.This new router is used when encountering the
WithRoutingHeader
combinator in an API, to notify the endpoints of the sub-API that they must produce a routing header (this behaviour is disabled by default).This PR also introduces
Spec
tests for theWithRoutingHeader
combinator, which showcase some of its possible uses.This PR is based upon #1556, and should remain WIP until it is merged.
Closes #1553