WIP: feat(server/auth): cache auth context results in Redis #3882
Conversation
|
📸 Preview service has generated an image. |
|
📸 Preview service has generated an image. |
|
📸 Preview service has generated an image. |
|
📸 Preview service has generated an image. |
|
📸 Preview service has generated an image. |
iainsproat
changed the title
|
📸 Preview service has generated an image. |
| } | ||
| }): RetrieveFromCache<T> => { | ||
| const { options, retrieveFromSource, cache } = deps | ||
| const reqLogger = maybeLoggerWithContext({ logger: deps.options.logger }) |
There was a problem hiding this comment.
I know it was created in a different PR, but maybeLoggerWithContext() typing is a bit lacking, as shown in this PR - even tho deps.options.logger is definitely defined, the return type is optional.
That would be fixed through generics:
export const maybeLoggerWithContext = <L extends Optional<Logger>>({ logger }: { logger: L }): L => {
...
}But honestly it's not clear to me why you'd want to return undefined if no logger was passed in anyway. Wouldn't it be better to create a new logger instance, if one wasn't passed in?
| req.log = req.log.child({ authContext: loggedContext }) | ||
| if (!authContext.auth && authContext.err) { | ||
| const defaultMessage = 'Unknown Auth context error' | ||
| //NOTE due to the possibility of the auth context being rehydrated from cache, we cannot assert the error with `instanceof` and must check the name |
There was a problem hiding this comment.
i don't like that it breaks the error in the first place, but also the fact that AuthContext - the TS type - is now conditionally wrong, because if its retrieved from cache the error key most certainly won't be an Error subtype
can we properly rebuild the error, even if its just new Error(oldMessage)? retrieveViaCache() could support a custom formatter, if it doesn't already
Description & motivation
Caches the result of the whole auth pipeline in Redis for a configurable amount of time. Defaults to a couple of seconds.
Changes:
FactorykeywordTo-do before merge:
Screenshots:
Validation of changes:
Logs (read from bottom up):
Showing a cache miss and then the subsequent auth-related database calls and the cache being set before getting to the business-related database calls.
Showing a cache hit, and no auth-related database calls:
Checklist:
References