-
-
Notifications
You must be signed in to change notification settings - Fork 669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: remove logs for secret and change invalid token query logic #6907
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 3 findings(s) 🚩
if (!token) { | ||
if (isPast(nextAllowedQuery)) { | ||
this.logger.info(`Token not found in cache, querying database`); | ||
if (this.queryAfter.size > 1000) { | ||
// establish a max limit for queryAfter size to prevent memory leak | ||
this.logger.info( | ||
'queryAfter size exceeded 1000, clearing cache', | ||
); | ||
this.queryAfter.clear(); | ||
} | ||
|
||
const stopCacheTimer = this.timer('getTokenWithCache.query'); | ||
token = await this.store.get(secret); | ||
if (token) { | ||
if (token?.expiresAt && isPast(token.expiresAt)) { | ||
this.logger.info('Token has expired'); | ||
// prevent querying the same invalid secret multiple times. Expire after 5 minutes | ||
this.queryAfter.set(secret, addMinutes(new Date(), 5)); | ||
token = undefined; | ||
} else { | ||
this.activeTokens.push(token); | ||
} | ||
} else { | ||
// prevent querying the same invalid secret multiple times. Expire after 5 minutes | ||
this.queryAfter.set(secret, addMinutes(new Date(), 5)); | ||
} | ||
stopCacheTimer(); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Getting worse: Complex Method
ApiTokenService.getTokenWithCache increases in cyclomatic complexity from 9 to 12, threshold = 9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code scene kinda has a point here imo
if (!token) { | ||
if (isPast(nextAllowedQuery)) { | ||
this.logger.info(`Token not found in cache, querying database`); | ||
if (this.queryAfter.size > 1000) { | ||
// establish a max limit for queryAfter size to prevent memory leak | ||
this.logger.info( | ||
'queryAfter size exceeded 1000, clearing cache', | ||
); | ||
this.queryAfter.clear(); | ||
} | ||
|
||
const stopCacheTimer = this.timer('getTokenWithCache.query'); | ||
token = await this.store.get(secret); | ||
if (token) { | ||
if (token?.expiresAt && isPast(token.expiresAt)) { | ||
this.logger.info('Token has expired'); | ||
// prevent querying the same invalid secret multiple times. Expire after 5 minutes | ||
this.queryAfter.set(secret, addMinutes(new Date(), 5)); | ||
token = undefined; | ||
} else { | ||
this.activeTokens.push(token); | ||
} | ||
} else { | ||
// prevent querying the same invalid secret multiple times. Expire after 5 minutes | ||
this.queryAfter.set(secret, addMinutes(new Date(), 5)); | ||
} | ||
stopCacheTimer(); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Bumpy Road Ahead
ApiTokenService.getTokenWithCache has 2 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one too
if (!token) { | ||
if (isPast(nextAllowedQuery)) { | ||
this.logger.info(`Token not found in cache, querying database`); | ||
if (this.queryAfter.size > 1000) { | ||
// establish a max limit for queryAfter size to prevent memory leak | ||
this.logger.info( | ||
'queryAfter size exceeded 1000, clearing cache', | ||
); | ||
this.queryAfter.clear(); | ||
} | ||
|
||
const stopCacheTimer = this.timer('getTokenWithCache.query'); | ||
token = await this.store.get(secret); | ||
if (token) { | ||
if (token?.expiresAt && isPast(token.expiresAt)) { | ||
this.logger.info('Token has expired'); | ||
// prevent querying the same invalid secret multiple times. Expire after 5 minutes | ||
this.queryAfter.set(secret, addMinutes(new Date(), 5)); | ||
token = undefined; | ||
} else { | ||
this.activeTokens.push(token); | ||
} | ||
} else { | ||
// prevent querying the same invalid secret multiple times. Expire after 5 minutes | ||
this.queryAfter.set(secret, addMinutes(new Date(), 5)); | ||
} | ||
stopCacheTimer(); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Deep, Nested Complexity
ApiTokenService.getTokenWithCache has a nested complexity depth of 4, threshold = 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, though, do we really want our tests to write to console.log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM comments optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 3 findings(s) 🚩
Yes, I'm aware of the additional complexity but for the sake of time and moving on to other important tasks I'll let this one slip |
About the changes
What's going on is the following:
unleash/src/lib/services/api-token-service.ts
Line 162 in 54383a6
unleash/src/lib/services/api-token-service.ts
Lines 164 to 165 in 54383a6
The token was added to the cache because we were not checking if it had expired. Now we added a check and we also have a log for expired tokens. Some improvement opportunities: