WIP: Issue 841: Rework of Verb #969
Conversation
|
| routeR | ||
| :: Proxy api | ||
| -> Context context | ||
| -> Request |
There was a problem hiding this comment.
We should be careful with Request, since the body may have already been consumed.
(Also, if I'm understanding this correctly, is there any use for the Request here?)
There was a problem hiding this comment.
We should, but we can do "bad" things withRequest in HasServer too.
servant-server/example/greet.hs
Outdated
| -- returns a Greet as JSON or redirects | ||
| type HelloEndpoint = | ||
| "hello" :> Capture "name" Text :> QueryParam "capital" Bool :> | ||
| Verb' 'GET (Redirect :> Result 200 '[JSON] Greet) |
There was a problem hiding this comment.
Should we maybe use :<|> instead of :>, since it's either one or the other, rather than one and the other?
There was a problem hiding this comment.
It might be just me, but I find both of them equally confusing (not terribly though, I could get used to both being used there). To me, and in very vague terms (i.e don't pay too much attention to this), the :> let us chain constraints on the input (request), while :<|> lets us "or" constraints, but then neither corresponds to what feels like we're doing under that Verb, which would be something like "stating a few properties of the output". :& maybe? Not sure.
There was a problem hiding this comment.
I used :> because we already have it.
:<|> because of the sum doesn't really make sense, as we can do products too. something like :>< or :* (as something "near enough to" ⊗), but those are also bad as the operator should convey it's fixity (not associative here!), and :> does it. :< would have "wrong" feeling.
Also having less operators is not a bad thing.
There was a problem hiding this comment.
:<|>because of the sum doesn't really make sense, as we can do products too.
What I mean is that both :> and :<|> would be allowed inside the Verb. The first would correspond to products, the second to sums. That makes a little clearer what the expected handler type would be from the API type, and allows us in the instances to not have to pattern match on individual LHSs.
servant/src/Servant/API/Redirect.hs
Outdated
| -- | TODO: TBW... | ||
| -- | ||
| -- TODO: Add to ComprehensiveAPI | ||
| newtype Redirect = Redirect URI deriving (Typeable, Eq, Ord, Show) |
There was a problem hiding this comment.
I think this should take a status code as well (since there are multiple types of redirect) or a type indicating one of the status codes.
servant/src/Servant/API/Verbs.hs
Outdated
| -- | @Verb@ is a general type for representing HTTP verbs (a.k.a. methods). For | ||
| -- convenience, type synonyms for each verb with a 200 response code are | ||
| -- provided, but you are free to define your own: | ||
| -- | ||
| -- >>> type Post204 contentTypes a = Verb 'POST 204 contentTypes a | ||
| data Verb (method :: k1) (statusCode :: Nat) (contentTypes :: [*]) (a :: *) | ||
| type Verb (method :: k1) (statusCode :: Nat) (contentTypes :: [*]) (a :: *) |
There was a problem hiding this comment.
It's very nice that Verb can be nearly backwards compatible in this way. But maybe it's Get, Put, etc that should have this sort of synonym, and we can just redefine Verb, since Verb is much less user-facing?
There was a problem hiding this comment.
You are probably right. I don't remember writing Verb that often myself.
|
|
||
| instance HasServerR api => HasServerR (Redirect :> api) where | ||
| type ServerR (Redirect :> api) = Either Redirect (ServerR api) | ||
|
|
There was a problem hiding this comment.
It's strange that both this and the above instance are needed. And also a little sad that we might end up with nested Eithers. If we switch to having both :> and :<|> inside the Verb, I guess we could just map uniformly over :<|> into a sumtype? And something to consider is maybe using an extensible sum type?
There was a problem hiding this comment.
We can have unconditional and optional Redirects. There fore two instances.
One option is to have yet another class for so
instance (HasClientR api, HasClientRR combinator)
=> HasClient (combinator :> api)That would be cool as we won't need as meny FlexibleInstances (we can do same factoring for HasClient (combinator :> api) instances as well. We'll end up with 4 classes then. It's only a naming problem.
Ending up with nested sums (Redirect) and products (Headers) is inevitable, and I'll keep api simple. It's not even that contrived to have
"endpoint" :> Verb' 'Get
( Headers '[ Header "X-API-Version"]
:> Redirect
:> Headers '[ Header "X-API-Something"]
:> Result 200 '[JSON] Value)
|
API design question, when So
|
|
Another question is how to deal with "right extensions". As I commented above I think we don't need to complicate things with
I think we should be consistent and use one of following options consistently: Separate custom product/sum for each combinator-- combinator
data Headers hs
-- term
data ResponseHeaders hs a = ResponseHeaders (HList hs) a -- with field names thoughNote: that Side comment This change let us unify handling of request and response combinators, and I will do that. Users can decide whether they want single Note: we'll need two types for right and left: data Headers hs
data ResponseHeaders hs a = MkResponseHeaders (HList hs) a
newtype RequestHeaders hs = MkRequestHeaders (HList hs)
type ServerT (Headers hs :> api) m = RequestHeaders hs -> ServerT api m
type ServerR (Headers hs :> api) = ResponseHeaders hs (ServerR api)the Pro of this approach is that specific types often result in better error messages. Embrace
|
On the one hand, it feels like
I really like the simplicity of your second suggestion and how it makes |
|
I agree The way I think of it, with sums at least, it'd be nice to have an Would be really nice. I know there are problems with this:
|
|
I agree that having to chain multiple |
|
I think NS/NP is the way to go here honestly. However, perhaps we want a default mode where people don't have to bother with heterogeneous lists. that is, you should be able to use servant as you used to, but only need to bother about heterogeneous lists when they want to have the return types encoded in the API. |
|
Multiple redirects (I guess with different statuses?) can be handler with new combinator We can have About "endpoint" :> Verb' 'Get
( Headers '[ Header "X-API-Version"]
:> (Redirect :<|>
Headers '[ Header "X-API-Something"]
:> Result 200 '[JSON] Value))but it opens a possibility to "endpoint" :> Verb' 'Get (Result 200 '[JSON] Value :<|> Result 200 '[CSV] [Text])and it doesn't work out. Client tells it wants I mentioned ⊗, as it overloads × and +. But actually i'm seeking for a (right-)module like thing of a "tensor production", but I guess there aren't a notation for those. So in summary:
|
|
About plurality, examples: Verb' 'Get (Redirect 300 :> Redirect 301 :> Result ...)
Verb' 'Get (Redirects '[300,301] :> Result ...)
Verb' 'Get (Header "foo" Int :> Header "bar" String :> Result ...)
Verb' 'Get (Headers '[Header "foo" Int, Header "bar" String] :> Result ...)And IMO it depends on the details which, nested sums or products, or bundled NS/NP are "better". We can have all of those above. Different issue of smashing different combinators into same NS or NP will be also difficult to implement. Returning NS I '[Redirect, Error err1, Error err2, I MyValue]
Either (NS I '[Redirect, Error err1, Error err2]) MyValueI think that term level inconvenience is way easier to abstract than to make some crazy stuff to produce "flatter" handler types. Note I prototyped current design in https://github.com/phadej/servant-tiny/tree/more-comb, feel free to explore desing space based on it (it's simpler), but I won't bike-shedding prevent me proceeding with current design. I.e. MTL / Magical NS-smashing etc is out without "existential proof" it will work. (Note how little extensions are used there!) |
|
How's about
using prime is a little boring, but we already do that for |
|
In the specific case of content types, I think they should be *outside* of
Result, with the Verb. This is because we check for content types *before*
running the Handler (as we should) and therefore don't know what Result (if
any) we'll get.
This also makes people be sure to implement instances for error types that
match what they claim to handle, which is good - the server can thus also
respond with the appropriate format for errors.
Not sure if that solves the problem generally though.
Oleg Grenrus <notifications@github.com> schrieb am Mi., 13. Juni 2018,
13:21:
… Multiple redirects (I guess with different statuses?) can be handler with
new combinator Redirects (plural) which bundles them into single NS,
while :> (or whatever) is encoded as NS.
We can have A * (B + (C * D)) like expressions, potentially.
About :> and :<|>. We *could* have, rewriting my previous example
"endpoint" :> Verb' 'Get
( Headers '[ Header "X-API-Version"]
:> (Redirect :<|>
Headers '[ Header "X-API-Something"]
:> Result 200 '[JSON] Value))
but it opens a possibility to
"endpoint" :> Verb' 'Get (Result 200 '[JSON] Value :<|> Result 200 '[CSV] [Text])
and it doesn't work out. Client tells it wants JSON, so we cannot return
CSV. We'd need to have a pair handler there, not an Either (it's not our
choice, but theirs!)! It might be possible to encode, but it will
definitely make implementation more complex, for a little benefit.
I mentioned *⊗*, as it *overloads* *×* and *+*. But actually i'm seeking
for a (right-)module like thing of a "tensor production", but I guess there
aren't a notation for those.
So in summary:
- :<|> is problematic
- :> or :>< or :<> or .. choice is irrelevant, but we already have :>,
but we can add something else too. Note: than we'd need to add TypeError
(Text "use :><") => HasServerR (comb :> api), as I'm sure many (me
including) will write wrong operator in wrong place).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#969 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABlKmmxI9Uof0pwibidxAqDPUQ1k0Uzaks5t8PWegaJpZM4UXrY8>
.
|
|
Then we'll need to have a way to bypass Content-Type negotiation for |
|
Is it horrible if we fail with |
|
I'll try, maybe having `Verb method ctypes` and `VerbAnyCT method` would be good compromise
…Sent from my iPhone
On 13 Jun 2018, at 15.31, Julian Arni ***@***.***> wrote:
Is it horrible if we fail with Not Acceptable in a handler that only redirects/returns no content if the list of content-types does not include the requested content-type? That is, NoContent and Redirect would both be instances of all serializations, but you'd still have to mention the ones you want to support.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Only missing part are Streaming stuff at this point
|
I refactored Verb 'GET '[JSON] (Result 400 Text :<|> Result 200 Greet)IMHO it's enough for core |
|
FWIW it's possible to have type GET = Verb 'GETbut I'm not sure if that will be more confusing than helpful? |
|
Verb 'GET '[JSON] (Result 400 Text :<|> Result 200 Greet)I think we'll need to dispatch on status code, so we'll need some kind of |
|
Yes, we need to do some kind of "inversed/dual routing". Fortunately it's very simple, as the status code is the only thing we will ever look at to decide what to try to decode the response to. |
|
I think we do not need something like {-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE PolyKinds #-}
{-# LANGUAGE GADTs #-}
{-# LANGUAGE DataKinds #-}
import Data.Proxy
import GHC.TypeLits
data StatusCode = X200 | X401 | X403
data SStatusCode :: StatusCode -> * where
S200 :: SStatusCode 'X200
S401 :: SStatusCode 'X401
S403 :: SStatusCode 'X403
data Get (xs :: [(StatusCode, *)])
type family Lkup (x :: StatusCode) (xs :: [(StatusCode, *)]) :: * where
Lkup x '[] = TypeError ('Text "Status code not specified in API type!")
Lkup 'X200 ('( 'X200, x) ': xs) = x
Lkup 'X401 ('( 'X401, x) ': xs) = x
Lkup 'X403 ('( 'X403, x) ': xs) = x
Lkup a (x ': xs) = Lkup a xs
data OneOf :: [(StatusCode, *)] -> * where
For :: SStatusCode x -> Lkup x xs -> OneOf xs
class HasServer layout where
type Server layout :: *
route :: Proxy layout -> Server layout -> [String] -> Maybe (IO String)
instance HasServer (Get xs) where
type Server (Get xs) = IO (OneOf xs)
data User = User String String
data AuthError = PasswordWrong
type API = Get '[ '( 'X200, User), '( 'X401, AuthError)]
checkPassword :: IO Bool
checkPassword = undefined
hasPermissions :: IO String
hasPermissions = undefined
handleLogin :: Server API
handleLogin = do
correctPassword <- checkPassword
if correctPassword
then pure $ For S200 (User "arian" "lol123")
else pure $ For S401 PasswordWrongIf the user writes a server that is not conformant to the API, then you get a nice error:
handleLoginNotConformToAPI :: Server API
handleLoginNotConformToAPI = do
correctPassword <- checkPassword
if correctPassword
then pure $ For S200 (User "arian" "lol123")
else pure $ For S403 PasswordWrong
servant.hs:63:26: error:
• Status code not specified in API type!
• In the second argument of ‘For’, namely ‘PasswordWrong’
In the second argument of ‘($)’, namely ‘For S403 PasswordWrong’
In the expression: pure $ For S403 PasswordWrong
|
63 | else pure $ For S403 PasswordWrong
| ^^^^^^^^^^^^^
We get a nice API for clients as well, However, Haskell complains about -- Lets for now assume that Server and Client type family are the same
-- which is a fair assumption
client :: IO ()
client = do
-- x :: (OneOf '['('X200, User), '('X401, AuthError)])
x <- handleLogin
case x of
For S200 (User name _pw) -> putStrLn name
For S401 PasswordWrong -> putStrLn "wrong password"
-- x :: Lkup x '['('X200, User), '('X401, AuthError)]
-- which is uninhabited, which the exhausitveness checker should figure out
-- but it doesn't
For S403 x -> error "Haskell complains about incomplete patterns if we omit"
However, if we try to use the result of misbehavingClient :: IO ()
misbehavingClient = do
-- x :: (OneOf '['('X200, User), '('X401, AuthError)])
x <- handleLogin
case x of
For S200 (User name _pw) -> putStrLn name
For S401 PasswordWrong -> putStrLn "wrong password"
For S403 x -> putStrLn x
servant.hs:77:28: error:
• Status code not specified in API type!
• In the first argument of ‘putStrLn’, namely ‘x’
In the expression: putStrLn x
In a case alternative: For S403 x -> putStrLn x
|
77 | For S403 x -> putStrLn x
|
|
|
I think since we have https://github.com/haskell-servant/servant-uverb now, this can be closed? Please reopen if I'm wrong. |
this is WIP, but I open this for preview.
Note:
#include "overlapping-compat.h"is removed fromServant.Server.Internal.EDIT If I will work on something in ZuriHac, this will probably be it.
RedirectRedirect(s) toComprehensiveAPIVerb, no need forVerb'. We can break that.contentTypesinVerborResult?