-
Notifications
You must be signed in to change notification settings - Fork 15
Description
Hi!
The other day, I opened an issue with nodetrout
where I saw that if sent a Accept
with a MediaType that an endpoint couldn't be rendered as, the server would still execute the endpoint, but then respond with a 406
instead of the return value. In the ideal case, the request wouldn't even be processed. After all, we're giving the client a 4xx
error, which means that they supplied an unprocessable request, which means if we did some state-modifying operation, it would have been processed but then we told the client nothing happened.
When attempting to fix this I realized that the problem actually stems from the definition ofAllMimeRender
. In order to determine what MIMEs can be rendered for a given a
, we first have to produce an a
-- i.e., run the endpoint. To resolve this in Nodetrout, I created a new typeclass as well as some blanket instances for it:
class DeferredAllMimeRender a cts b | a -> b, cts -> b where
deferredMimeRenderers :: Proxy cts -> NonEmptyList (Tuple MediaType (a -> b))
instance deferredAllMimeRenderAlt ::
( HasMediaType ct1
, MimeRender a ct1 b
, DeferredAllMimeRender a ct2 b
) => DeferredAllMimeRender a (ct1 :<|> ct2) b where
deferredMimeRenderers _ = pure (Tuple (getMediaType p) (mimeRender p)) <> (deferredMimeRenderers p')
where p = Proxy :: Proxy ct1
p' = Proxy :: Proxy ct2
else instance deferredAllMimeRenderSingle ::
( HasMediaType ct
, MimeRender a ct b
) => DeferredAllMimeRender a ct b where
deferredMimeRenderers _ = pure (Tuple (getMediaType p) (mimeRender p))
where p = Proxy :: Proxy ct
Currently Nodetrout and Hypertrout both do the equivalent of runEndpoint >>= (\result -> negotiateContentType (allMimeRender (Proxy :: Proxy endpointContentTypes result)
. Changing the definition of AllMimeRender
to match the above would allow servers to determine if a response can be produced without running the endpoint, something like:
serve =
let contentTypeCandidates = determineContentTypeCandidates request
allValidRenderers = deferredMimeRenderers (Proxy:: Proxy endpointContentTypes)
processIfAcceptable =
case chooseBestContentType contentTypeCandidates allValidRenderers of
Nothing -> throwError error406
Just (Tuple mediaType renderContent) -> do
res <- runEndpoint
serveResponse (Tuple mediaType $ renderContent res)
in processIfAcceptable runEndpoint
which is effectively what I did in Nodetrout as part of my fix
A secondary benefit of this is there's no need to make a "single"/"non-alt" AllMimeRender
instance for every custom media type you define -- the instance resolution error message will say there's no instance for HasMediaType and/or MimeRender for your custom type regardless of which branch of the two candidate instances it takes.
Figure I'd leave this here for your thoughts -- after all changing the definition of such a critical typeclass will cause breakages, but it seems like this could go in the next major bump