[#177116764] Allow caching EYCA CCDB API's SessionId #39
Conversation
Affected stories
|
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
==========================================
+ Coverage 82.89% 83.27% +0.37%
==========================================
Files 36 40 +4
Lines 1076 1238 +162
Branches 121 139 +18
==========================================
+ Hits 892 1031 +139
- Misses 179 202 +23
Partials 5 5
Continue to review full report at Codecov.
|
| import { getTask, setWithExpirationTask } from "../utils/redis_storage"; | ||
|
|
||
| export const CCDB_SESSION_ID_KEY = "CCDB_SESSION_ID"; | ||
| export const CCDB_SESSION_ID_TTL = 1500; |
There was a problem hiding this comment.
The CCDB_SESSION session have a TTL of 30 minutes (1800 seconds), why we are using 1500? Can we set this parameter with an ENV configuration?
There was a problem hiding this comment.
I'd rather to maintain it as an hardcoded constant. It will never change. Therefore I will change the value to 1700 seconds avoiding that a minimum delay can compromise the token validity :)
| eycaClient: ReturnType<EycaAPIClient>, | ||
| username: NonEmptyString, | ||
| password: NonEmptyString | ||
| ) => |
There was a problem hiding this comment.
Can we add a return type definition? The function behaviour becomes more readable.
| export const CCDB_SESSION_ID_KEY = "CCDB_SESSION_ID"; | ||
| export const CCDB_SESSION_ID_TTL = 1500; | ||
|
|
||
| const ccdbLogin = ( |
| const retrieveCcdbSessionId = ( | ||
| redisClient: RedisClient, | ||
| eycaClient: ReturnType<EycaAPIClient>, | ||
| username: NonEmptyString, | ||
| password: NonEmptyString | ||
| ) => |
There was a problem hiding this comment.
The same consideration of ccdbLogin about description and return type.
| password: NonEmptyString | ||
| ) => | ||
| getTask(redisClient, CCDB_SESSION_ID_KEY).foldTaskEither( | ||
| () => ccdbLogin(eycaClient, username, password), |
There was a problem hiding this comment.
Are we using ccdbLogin as fallback on Redis errors? We can only return an error, retrieveCcdbSessionId is used inside SuccessEycaActivationActivity that has a retry logic.
There was a problem hiding this comment.
Yep I will restore the default behaviour. So we let the activity to retry and log errors in case Redis get or set fails.
| setWithExpirationTask( | ||
| redisClient, | ||
| CCDB_SESSION_ID_KEY, | ||
| _, | ||
| CCDB_SESSION_ID_TTL | ||
| ).foldTaskEither( | ||
| () => taskEither.of(_), |
There was a problem hiding this comment.
If we have an error we can't see it. We don't have any logs.
| activationDate: new Date(), | ||
| expirationDate: extractEycaExpirationDate(aFiscalCode).value as Date | ||
| expirationDate: extractEycaExpirationDate(aFiscalCode).value as Date, |
There was a problem hiding this comment.
Sometime in the future, all the tests that use this value start to fail.
| ccdbLogin(eycaClient, username, password).chain(_ => | ||
| setWithExpirationTask( | ||
| redisClient, | ||
| CCDB_SESSION_ID_KEY, | ||
| _, | ||
| CCDB_SESSION_ID_TTL | ||
| ).map(() => _) |
There was a problem hiding this comment.
Only a small readability improvement
| ccdbLogin(eycaClient, username, password).chain(_ => | |
| setWithExpirationTask( | |
| redisClient, | |
| CCDB_SESSION_ID_KEY, | |
| _, | |
| CCDB_SESSION_ID_TTL | |
| ).map(() => _) | |
| ccdbLogin(eycaClient, username, password).chain(sessionId => | |
| setWithExpirationTask( | |
| redisClient, | |
| CCDB_SESSION_ID_KEY, | |
| sessionId, | |
| CCDB_SESSION_ID_TTL | |
| ).map(() => sessionId) |
List of Changes
eycamodulesession_idauth paramsession_idthrough Redissession_idfrom Redis before all CCDB Api callsMotivation and Context
In order to optimize authentication through EYCA CCDB API's platform, we must handle and cache a
session_idtoken for all subsequent api calls afterauthLoginoperation (performed using username and password credentials). Please note that a newsession_idis valid for 30 minutes after it is released and a newauthLoginoperation does not invalidate previous released session_id until its expiration has not reached.In case of Redis isn't available we fallback to
authLoginoperation avoiding retries for missingsession_idhit in cache.See this story for further informations.
How Has This Been Tested?
Unit tests done.
Types of changes
Checklist: