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

New combinator to return routed path in response headers #1561

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nbacquey
Copy link
Contributor

@nbacquey nbacquey commented Mar 14, 2022

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' 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:

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).

This PR also introduces Spec tests for the WithRoutingHeader combinator, which showcase some of its possible uses.

This PR is based upon #1556, and should remain WIP until it is merged.

Closes #1553

@nbacquey nbacquey force-pushed the server-routing-header branch from 99356af to d09215b Compare March 14, 2022 14:54
@tchoutri
Copy link
Contributor

I would love to read a bit more on practical uses of this feature!
Do you use this in tracing scenarios? Observability and Telemetry?

@nbacquey
Copy link
Contributor Author

I would love to read a bit more on practical uses of this feature! Do you use this in tracing scenarios? Observability and Telemetry?

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.
For instance, I once needed to have a portion of my API failing fast on a certain error, instead of defaulting to subsequent choices defined by :<|>.
With this PR merged, this behavior would be really easy to implement: you'd only need a new combinator that uses an EnvRouter, a new field in RouterEnv env, and the appropriate switch in runChoice

@tchoutri
Copy link
Contributor

@nbacquey This is music to my ears.

@@ -235,7 +235,7 @@ hoistServer p = hoistServerWithContext p (Proxy :: Proxy '[])
-- > │ └─ e/
-- > │ └─•
-- > ├─ b/
-- > │ └─ <capture>/
-- > │ └─ <x::CaptureSingle>/
Copy link
Contributor

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? :)

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thank you :)

Copy link
Contributor

@tchoutri tchoutri left a 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)

@gdeest
Copy link
Contributor

gdeest commented Mar 15, 2022

For instance, I once needed to have a portion of my API failing fast on a certain error, instead of defaulting to subsequent choices defined by :<|>.

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 Raw combinators and turned into 404 errors by serveDirectoryFileServer.

@nbacquey
Copy link
Contributor Author

I don't remember my specific usecase, but I hadn't thought about using tweakResponse like that, thanks for the tip!

@nbacquey nbacquey force-pushed the server-routing-header branch from 6146f74 to 37cf3fd Compare March 15, 2022 17:12
@nbacquey nbacquey requested a review from tchoutri March 15, 2022 17:14
Copy link
Contributor

@tchoutri tchoutri left a 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

@alpmestan
Copy link
Contributor

With this PR merged, this behavior would be really easy to implement: you'd only need a new combinator that uses an EnvRouter, a new field in RouterEnv env, and the appropriate switch in runChoice

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?

@nbacquey
Copy link
Contributor Author

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.

@nbacquey nbacquey changed the title [WIP] New combinator to return routed path in response headers New combinator to return routed path in response headers Mar 16, 2022
@nbacquey nbacquey force-pushed the server-routing-header branch from 37cf3fd to 6c51cf8 Compare March 21, 2022 10:18
@nbacquey
Copy link
Contributor Author

I rebased on the new version of #1556, which reinstates "real" type hints, instead of CaptureSingle/CaptureList

@nbacquey nbacquey force-pushed the server-routing-header branch 4 times, most recently from 738cfbe to 203671d Compare March 25, 2022 11:11
@nbacquey nbacquey force-pushed the server-routing-header branch from 203671d to 809c8ce Compare March 28, 2022 13:35
@nbacquey nbacquey force-pushed the server-routing-header branch from 809c8ce to ed40f9d Compare April 12, 2022 12:47
@nbacquey
Copy link
Contributor Author

Added a fix to prevent WithRoutingHeader from breaking IsElem

@nbacquey nbacquey force-pushed the server-routing-header branch from ed40f9d to b1e6748 Compare April 12, 2022 13:42
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).
@nbacquey nbacquey force-pushed the server-routing-header branch from b1e6748 to 801f075 Compare April 19, 2022 12:04
@tchoutri
Copy link
Contributor

@alpmestan @gdeest I would appreciate if we could bring this PR to a conclusion :)

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.

Expose some information about API resolution to middlewares
4 participants