purescript-trout icon indicating copy to clipboard operation
purescript-trout copied to clipboard

AllMimeRender currently doesn't solve the Content-Type negotiation problem

Open iostat opened this issue 5 years ago • 0 comments

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

iostat avatar Oct 28 '20 12:10 iostat