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

WIP: Introduce Redirect combinator #1575

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

Conversation

nbacquey
Copy link
Contributor

This PR introduces a new combinator, to ease the implementation of correct HTTP redirections:

data Redirect (location :: Symbol)

Current implementation

It modifies the behaviour of the nested sub-APIs, such that all endpoints of said API return a "Location" header, set to the value of the location type variable. An API using the Redirect combinator does not typecheck if any of the endpoints after the combinator returns a status code outside the 3xx range, or if it is used to redirect a Raw API (because we cannot guarantee anything about those).
For instance, the following API doesn't have a HasServer instance:

type BadApi
  =    "old-api" :> Redirect "/new-api" :> Get '[JSON] Foo
  :<|> "new-api" :> Get '[JSON] Foo
    -- `Get` is an alias for `Verb 'GET 200`

Whereas this one does:

type GoodApi
  =    "old-api" :> Redirect "/new-api" :> Verb 'GET 301 '[JSON] Foo
  :<|> "new-api" :> Get '[JSON] Foo
    -- GET /old-api will return a response with status 301 and the following header:
    --   ("Location", "/new-api")

Alternative implementations

Have a Verb-like combinator

In this alternative, Redirect would replace Verb, UVerb, and Stream, and have a syntax like:

data Redirect (location :: Symbol) (method :: k1) (statusCode :: Nat) (contentTypes :: [*]) (a :: *)

It would only allow 3xx status codes, and would behave the same as Verb does, only forcing a Location header in the response.

The API example above would then be re-written as:

type Api
  =    "old-api" :> Redirect "/new-api" 'GET 301 '[JSON] Foo
  :<|> "new-api" :> Get '[JSON] Foo

Pros:

  • One would immediately notice which endpoints are redirected, and which are not.
  • All information about the redirection (status + location) would be located at the same place.

Cons:

  • The boilerplate and instances for Verb would have to be duplicated for Redirect.
  • Maybe some usecases would need a UVerb-like or Stream-like way to return a body, thus creating the need for RedirectVerb, RedirectUVerb, and RedirectStream, with more boilerplate and instances.
  • It would be tedious to redirect large parts of an API, because each endpoint would have to be rewritten.

Specify a status code with the Redirect combinator

In this case, the combinator would have a syntax like:

data Redirect (location :: Symbol) (statusCode :: Nat)

It would still be used "in the middle" of an API, and would only accept 3xx status codes. The status code would then overwrite the status code of individual endpoints. For instance, on the following API:

type Api
  =    "old-api" :> Redirect "/new-api" 301 :> Get '[JSON] Foo
  :<|> "new-api" :> Get '[JSON] Foo
    -- `Get` is an alias for `Verb 'GET 200`

A call to GET /old-api would return a 301 status code instead of a 200, with a header {"Location": "/new-api"}.

Pros:

  • All information about the redirection (status + location) would be located at the same place.
  • It would be easy to redirect large parts of an API: no need to change the status codes individually, nor the verbs.

Cons:

  • It might be confusing to have an endpoint return a status code different than the one declared in its Verb.

I would appreciate your input on the relative merits of those alternative implementations.

This PR is based on #1561, it uses the enriched RouterEnv env, and the EnvRouter Constructor.
Relates to #1550

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).
@alpmestan
Copy link
Contributor

alpmestan commented Mar 31, 2022

Prior art/discussions on this: #117

I'm not entirely a fan because it's pretty common to have to "inject" a dynamic value in the URL. Think of a POST endpoint to create a new something than then redirects you to /something/:id. The use "in the middle" like this to inject a static url for what is essentially just a response header + a 3xx status code, seems to have to narrow a scope to be shipped out of the box.

If Redirect as presented here works for your projects it's great, and the point of servant's open design is exactly for things like this to be able to live outside the core libraries, but I am not entirely sure we want to offer something too limited in the standard toolbox. This could live in a servant-redirect-static package or something.

@gdeest
Copy link
Contributor

gdeest commented Mar 31, 2022

I tend to agree that a static redirect location is a very limited use-case that likely doesn't match the needs of most users.

I wonder if we could craft something based on HasLink to ensure that the returned URL belongs to a specific API ?

data Redirect (statusCode :: Nat) (api :: *) 

with something like:

class HasServer (Redirect statusCode api) ctx where
  type ServerT m (Redirect statusCode api) = m (EndpointOf api) 

  route =  -- Run handler and pattern match on `EndpointOf` to retrieve all the evidence to call `safeLink`.

data EndpointOf api where
   EndpointOf :: (IsElem endpoint api, HasLink endpoint) => Proxy endpoint -> EndpointOf api

If I am not mistaken, this would give users more flexibility while also ensuring that the link belongs to a specific API. Do you think this is doable / useful ?

Of course, this doesn't solve all the problems, such as required / forbidden method changes.

@gdeest
Copy link
Contributor

gdeest commented Mar 31, 2022

The proposal above is actually bogus, because it does not give us any way to bundle capture arguments, etc. I still wonder if something along those lines could work.

EDIT:
I think this does the trick:

data EndpointOf api where
   EndpointOf
     :: (IsElem endpoint api, HasLink endpoint)
     => Proxy endpoint -> (forall a. MkLink endpoint a -> a) -> EndpointOf api

@alpmestan
Copy link
Contributor

@gdeest This was brought up in the ticket I linked to above IIRC.

@gdeest
Copy link
Contributor

gdeest commented Apr 3, 2022

@alpmestan Not exactly in this form, as far as I understand — the idea I was playing with would allow the Redirect combinator to dynamically select the redirection endpoint within a given API, instead of fixing it as a type parameter, which I think is feasible.

@alpmestan
Copy link
Contributor

Indeed, not in this form, but this then prevents Redirect from being used to send the user somewhere else than in the current API (type). Which may or may not be a good idea.

All in all, I'd find it perhaps desirable that we have a fairly flexible building block on top of which we could have something that crafts carefully statically-checked redirects to endpoints from the same API, or something that just sends people to some "random" URL. From that perspective, having Redirect be a parametrized synonym for an empty response with some Location header and a 3xx status code seems simpler, and then we could have two functions for constructing responses, one that only lets you target a valid endpoint from an API (and its signature would be similar to your GADT constructor), and another that just takes any URL. Would we lose anything along the way?

@gdeest
Copy link
Contributor

gdeest commented Apr 13, 2022

this then prevents Redirect from being used to send the user somewhere else than in the current API (type)

It is (unfortunately) true. It wouldn't that much of a problem if we could just wrap the API in a newtype and derive instances for HasServer etc. But HasServer in particular is problematic, because the API type is not the last type class parameter (the context is). I don't think there is a satisfying solution to this problem.

Adding a type synonym is easy enough, but I fail to see how it constitutes a satisfying building block to adding more elaborate static checks.

@alpmestan
Copy link
Contributor

but I fail to see how it constitutes a satisfying building block to adding more elaborate static checks.

Functions for constructing Redirect responses could have any "endpoint must belong to API"-style constraints if desired. But one could also just redirect to a random URL through some "raw"-er function.

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.

None yet

3 participants