servant icon indicating copy to clipboard operation
servant copied to clipboard

WIP: Introduce `Redirect` combinator

Open nbacquey opened this issue 2 years ago • 8 comments

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

nbacquey avatar Mar 31 '22 12:03 nbacquey

Prior art/discussions on this: https://github.com/haskell-servant/servant/issues/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.

alpmestan avatar Mar 31 '22 13:03 alpmestan

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 avatar Mar 31 '22 15:03 gdeest

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

gdeest avatar Mar 31 '22 16:03 gdeest

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

alpmestan avatar Mar 31 '22 16:03 alpmestan

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

gdeest avatar Apr 03 '22 22:04 gdeest

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?

alpmestan avatar Apr 04 '22 15:04 alpmestan

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.

gdeest avatar Apr 13 '22 08:04 gdeest

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.

alpmestan avatar Apr 14 '22 06:04 alpmestan