refactor: remove auth middleware #4059
Conversation
This commit removes auth middleware as it hides side effects and obscures logic. The auth operations are now done in its own stage in the request-response cycle.
| Wai.setApacheRequestFilter (\_ res -> shouldLogResponse logLevel $ Wai.responseStatus res) & | ||
| Wai.setApacheUserGetter getAuthRole | ||
| Wai.setApacheRequestFilter (\_ res -> shouldLogResponse logLevel $ Wai.responseStatus res) |
There was a problem hiding this comment.
Now with this approach we can't get the role here, I think we should do this change as a separate break/fix?
There was a problem hiding this comment.
I think we should not do the change at all. Why remove the role from the log? It was added there on purpose, I even worked upstream (IIRC wai-logger) to support it.
There was a problem hiding this comment.
I guess I couldn't figure out yet how to get the role after removing the auth middleware. So, I removed it temporarily. I'll try to restore this change.
There was a problem hiding this comment.
Let me put it this way: I remember introducing the middleware precisely to be able to add the role to the log output. So maybe this refactor is a dead-end. But if you come with a nice way to do it and will overall make the code better, sure.
| @@ -974,35 +976,35 @@ def test_log_level(level, defaultenv): | |||
| output[0], | |||
| ) | |||
| assert re.match( | |||
| r'- - postgrest_test_anonymous \[.+\] "GET /unknown HTTP/1.1" 404 \d+ "" "python-requests/.+"', | |||
| r'- - - \[.+\] "GET /unknown HTTP/1.1" 404 \d+ "" "python-requests/.+"', | |||
There was a problem hiding this comment.
This is not a refactor, this is a regression. The user's role was supposed to be in the log here and that's not the case anymore.
There was a problem hiding this comment.
Yeah, sorry, forgot to mark this PR as draft. This changed isn't supposed to be done with this refactor. I changed this here to discuss about this.
|
Hmm, it looks like we also want to remove the postgrest/src/PostgREST/Logger.hs Lines 58 to 63 in b582538 |
Benefits: - Removes `unsafePerformIO` from Logger - Reuses our cached time getter (`stateGetZTime`) instead of having another unnecessary one. See https://github.com/yesodweb/wai//blob/d50e1184600631a114e3d2ad119abdf0ef08834b/wai-extra/Network/Wai/Middleware/RequestLogger/Internal.hs#L22-L37. - Reuses Observation module, simplified logic - Will help with log buffering PostgREST#4061 TODO: - [ ] Currently we vendor `apacheLogStr`, but might be possible to expose this function upstream. If not, it's not a big problem since the function is small. - [ ] Time in logging differs a bit: ``` Old: postgrest_test_anonymous [04/May/2025:14:43:06 -0500] New: postgrest_test_anonymous [2025-05-04 22:43:01.28098247 -05] ``` + Otherwise all logging works the same. - [ ] Auth error logging relies on old logger middleware + We might need to put `observer` in Auth.hs + Or otherwise try to combine this with PostgREST#4059
Benefits: - Removes `unsafePerformIO` from Logger - Reuses our cached time getter (`stateGetZTime`) instead of having another unnecessary one. See https://github.com/yesodweb/wai//blob/d50e1184600631a114e3d2ad119abdf0ef08834b/wai-extra/Network/Wai/Middleware/RequestLogger/Internal.hs#L22-L37. - Reuses Observation module, simplified logic - Will help with log buffering PostgREST#4061 TODO: - [ ] Currently we vendor `apacheLogStr`, but might be possible to expose this function upstream. If not, it's not a big problem since the function is small. - [ ] Time in logging differs a bit: ``` Old: postgrest_test_anonymous [04/May/2025:14:43:06 -0500] New: postgrest_test_anonymous [2025-05-04 22:43:01.28098247 -05] ``` + Otherwise all logging works the same. - [ ] Auth error logging relies on old logger middleware + We might need to put `observer` in Auth.hs + Or otherwise try to combine this with PostgREST#4059
|
@taimoorzaeem Gave a shot at removing Logger middleware: #4062. I think we need to solve that one first. |
|
Closing as continued on #4064. |
Refactor for #3507.
This commit removes
Auth.middlewareas it hides side effects and obscures logic. The auth operations are now done in its own stage in the request-response cycle.