Redirect combinator ?

[alpmestan]

Hi everyone,

This has been discussed a bit offline but I thought I should open an issue and throw some code in here for discussion and potential inclusion in servant. The deal here is to offer a proper generic combinator for redirection.

One thing however is that we have some decent machinery for generating type safe links, which of course looks like a good basis to build the redirect combinator on.

For a servant-0.4 compatible solution, one could consider such an implementation:

class KnownMethod (c :: [*] -> * -> *) where
  methodOf :: Proxy c -> Method

instance KnownMethod Delete where
  methodOf _ = methodDelete

instance KnownMethod Get where
  methodOf _ = methodGet

instance KnownMethod Patch where
  methodOf _ = methodPatch

instance KnownMethod Post where
  methodOf _ = methodPost

instance KnownMethod Put where
  methodOf _ = methodPut

data Redirect (c :: [*] -> * -> *) lnk api

instance (KnownMethod c, IsElem lnk api, HasLink lnk)
        => HasServer (Redirect c lnk api) where

  type ServerT (Redirect c lnk api) m
    = m URI

  route Proxy getLink req respond
    | null (pathInfo req) && requestMethod req == methodOf pc = do
        res <- runEitherT getLink
        case res of
          Left err  -> respond . succeedWith $ responseServantErr err
          Right lnk -> respond . succeedWith $
            responseLBS seeOther303 [("Location", fromString ("/" ++ show lnk))] "" 
    | null (pathInfo req) && requestMethod req /= methodOf pc =
        respond $ failWith WrongMethod 
    | otherwise = respond $ failWith NotFound

    where pc = Proxy :: Proxy c

(complete usable code here)

The only thing I'm not happy about is that I hardcode the 303 status code. We could probably take an additional parameter of kind Natural (type-level natural number) and only have the HasServer instance when that number is a valid status code used for redirects.

Thoughts?

PS: I use the above combinator in a work project already and hence would be keen to eventually include such a combinator in servant itself since I would otherwise have to maintain that combinator independently and adapt it for any change to servant/servant-server.

[jkarni]

I like this a lot. Here are some thoughts:

• Why not name it Redirect303 or SeeOther, or something along those lines? We can add other redirects later.
• If we can abstract away commonality of redirects, and then wrap more specific combinators into newtypes, great. I don't think we should make that a blocker, though.
• The 303 redirect should check that lnk is a GET endpoint (cf. RFC 7231)

[alpmestan]

• Why not, actually. This idea hasn't even crossed my mind for some reason =) It's not like there would be hundreds of them, we were talking about absolutely having to support 4 or 5 status codes the other day, right?
• We could however have these be type synonyms for an application of a generic Redirect combinator that would take a status code parameter, and have some KnownStatusCode class do the discrimination on the status code to allow the HasServer instance to be picked.
• Oh, I wasn't aware of that, although I have not violated this principle. Instinct, I guess. Should we do that with a simple type family IsGetLink :: * -> Bool and have a IsGetLink lnk ~ 'True constraint on the instance?

[alpmestan]

Had a little idea about this. If we don't want to be too opinionated about redirects, we can do:

data Redirect (status :: Natural) (c :: [*] -> * -> *) lnk api
    deriving (Typeable)

class KnownMethod (c :: [*] -> * -> *) where
  methodOf :: Proxy c -> Method

instance KnownMethod Delete where
  methodOf _ = methodDelete

instance KnownMethod Get where
  methodOf _ = methodGet

instance KnownMethod Patch where
  methodOf _ = methodPatch

instance KnownMethod Post where
  methodOf _ = methodPost

instance KnownMethod Put where
  methodOf _ = methodPut

instance (KnownMethod c, 300 <= st, st <= 307, IsElem lnk api, HasLink lnk)
        => HasServer (Redirect st c lnk api) where

[jkarni]

Nice. For type-safe conformance to the relevant RFCs, I think we'd really want something like

 (..., (st == 303 `And` IsGet lnk) `Or` (st == 307 `And` c ~ MethodOf lnk), ...)

&c. That way, we have a single instance, but all the nuances of the differences between code checked and built-in.

Of course, we would probably factor it out as a IsGoodRedirect type family so it doesn't look as ugly. @alpmestan what do you think?

[alpmestan]

Do you really want to enforce that the method of the endpoint we redirect from should be the same as the link's? That defeats the typical POST new entity -> redirect to its uri. But I'm totally onboard with your suggestion of having types enforce the RFC.

[jkarni]

307 does not allowing changing of method. 303 requires changing to GET. See https://tools.ietf.org/html/rfc7538

[jkarni]

Any updates on this? I have a little time and could do it.

[alpmestan]

Oh, if you want to, please be my guest! I'd definitely be glad to review and then actually use the final Redirect combinator as described in the last few comments here :)

[jkarni]

Ok, @alpmestan and I discussed this a little more. The current proposal doesn't tie the link you actually use with the link in the type at all, which is bad. One option is to get the type for safeLink from the combinator, which would stay much as it is. The consequences are that

1. The link would have to be static
2. We'd have to pass the handler a continuation so that it could provide any missing params

The alternative is to change the Link type so that we can tag it with the API type it was constructed for, and then just require one type param for the Redirect combinator (the API type), and check that that and the Link type match. The second involves more changes, but seems more general. Any thoughts?