Derive the MonadTrans instance for WithLogger #70
Conversation
There was a problem hiding this comment.
I think the reason we haven't needed these is that WithLogger is primarily used for DerivingVia. In that context, you are deriving for your own type, which would itself have/need instances like that. WithLogger only needs enough to implement the instance you're deriving through it.
runWithLogger is here though, and can be used directly, which is when more instances would be useful.
If that's a use-case we want to support better, I think I'd give this every reasonable instance that we're already taking on dependencies for: error, state, write, catch, throw, mask, unliftio...
Right?
|
the case in which I found this instance was needed was in this example -- Basic structure for our auth middleware
authMiddleware
:: HasCallStack
=> Logger
-> String
-> (AppContext -> Application)
-> Application
authMiddleware logger tokenServerUrl app request respond = do
let maybeBearerToken = bearerTokenFromRequest request
case maybeBearerToken of
Nothing -> do
runWithLogger logger $ logError $ "No bearer token found" :# []
respond unauthorizedResponse
Just token -> do
claims <- runJOSE @JWTError $ runWithLogger logger $ do
logInfo $ "decoding bearer token" :# ["token" .= decodeUtf8 token]
jwt <- decodeCompact $ BS.fromStrict token
logInfo $ "decoded bearer token" :# ["jwt" .= jwt]
verifyClaims @_ @_ @JWTError -- without the MonadTrans this line gives a compile error
insecureJWTValidationSettings
(TokenServerUrl tokenServerUrl)
jwt
case claims of
Left err -> do
runWithLogger logger
$ logError
$ "error with token claims" :# ["error" .= show err]
respond unauthorizedResponse
Right claims -> do
runWithLogger logger
$ logInfo
$ "successful token validation" :# ["claims" .= claims]
app (AppContext logger claims) request respond
where
bearerTokenFromRequest :: Wai.Request -> Maybe BS.ByteString
bearerTokenFromRequest =
fmap ((!! 1) . BS.split (BS.c2w ' ')) . lookup "Authorization" . requestHeaders |
|
Yup, it's because you're using That's fine, but uncommon. Usually, your code would look something like this: newtype AppM a = AppM
{ unwrap :: ReaderT AppContext IO a
}
deriving newtype
( -- ...
, MonadError
)
deriving (MonadLogger, MonadLoggerIO) via (WithLogger IO)
runAppM :: AppContext -> AppM a -> IO a
runAppT = undefined
claims <- runAppT app $ do
-- ...
verifyClaims -- this is now AppM, not WithLogger so it can use any of AppM's instancesIf lot of things get easier if you introduce and |
|
I think I can make that work, I was getting kind of confused about how to wrap the entire middleware in |
Yeah, this stuff is too confusing for me to know without playing with it myself.
Correct, but that's the point: you need it (and can just newtype-derive it) on |
|
yeah those were the two steps of my reasoning
, but also,
|
|
So sounds like the PR itself is not needed, but the discussion was helpful to me, thanks |
I'm maybe a little surprised this wasn't needed already. In any case it shouldn't be harmful to derive it
(it's truly the same thing as ReaderT modulo newtype wrappers)