fix: suppress logging on the validate token command #1550
Conversation
WalkthroughThe changes introduce logic to suppress all console output in the logging service when running "validate-token" CLI commands. Additionally, error and success logging in the validate-token command are unified under a new method that restores console methods before outputting structured JSON, ensuring consistent behavior for both error and success cases. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant LogService
participant ValidateTokenCommand
User->>CLI: Run "validate-token" command
CLI->>LogService: Initialize LogService
LogService->>LogService: Detect "validate-token" command
LogService->>LogService: Suppress console output methods
CLI->>ValidateTokenCommand: Execute run()
ValidateTokenCommand->>LogService: Attempt to log
LogService-->>ValidateTokenCommand: No output (suppressed)
ValidateTokenCommand->>ValidateTokenCommand: Restore console methods
ValidateTokenCommand->>User: Output JSON result (success or error)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Claude finished @elibosley's task —— View job PR Review: Suppress Logging on Validate Token CommandReviewing the changes to suppress logging functionality in the validate token command. Todo List:
Review Feedback🎯 Code Quality & Best PracticesConcerns:
Suggestions: // Better approach: Use a flag-based system instead of global overrides
private shouldSuppressOutput(): boolean {
return this.isValidateTokenCommand;
}
log(...messages: unknown[]): void {
if (!this.shouldSuppressOutput() && this.shouldLog('info')) {
this.logger.log(...messages);
}
}🐛 Potential Bugs & Issues
⚡ Performance ConsiderationsGood:
Minor:
🔒 Security ConcernsMedium Risk:
Recommendations:
🧪 Test CoverageMissing:
Recommended Tests: // ValidateTokenCommand tests needed:
- Token validation with valid JWT tokens
- Error handling for invalid tokens
- Proper JSON output format
- Exit code verification
- Console suppression behavior
// LogService tests needed:
- Verify console methods are overridden for validate-token command
- Verify normal logging works for other commands
- Test the command detection logic💡 Alternative ApproachConsider a cleaner implementation: // In LogService
export class LogService {
private suppressOutput = false;
constructor() {
this.suppressOutput = this.isValidateTokenCommand();
}
private output(method: 'log' | 'error', ...args: unknown[]) {
if (this.suppressOutput) return;
console[method](...args);
}
}
// In ValidateTokenCommand
private exit(data: object, isError = false) {
const json = JSON.stringify(data);
if (isError) {
console.error(json);
process.exit(1);
} else {
console.log(json);
process.exit(0);
}
}📋 SummaryBlocking Issues:
Recommendations:
The intent of the PR is good, but the implementation needs refinement to avoid potential side effects and improve maintainability. |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/src/unraid-api/cli/log.service.ts (1)
11-31: Simplify command detection logic and consider potential side effects of global console overriding.The command detection logic contains redundant conditions - lines 18-20 duplicate the checks from lines 14-16 within the SSO condition. Additionally, globally overriding console methods could affect other parts of the application that rely on console output.
Apply this diff to simplify the command detection:
- this.isValidateTokenCommand = - process.argv.includes('validate-token') || - process.argv.includes('validate') || - process.argv.includes('v') || - (process.argv.includes('sso') && - (process.argv.includes('validate-token') || - process.argv.includes('validate') || - process.argv.includes('v'))); + const hasValidateCommand = + process.argv.includes('validate-token') || + process.argv.includes('validate') || + process.argv.includes('v'); + + this.isValidateTokenCommand = + hasValidateCommand || + (process.argv.includes('sso') && hasValidateCommand);Consider storing original console methods for potential restoration:
+ private readonly originalConsole = { + log: console.log, + error: console.error, + warn: console.warn, + info: console.info, + debug: console.debug + };api/src/unraid-api/cli/sso/validate-token.command.ts (2)
28-47: LGTM! Clean unified approach for structured output.The
logAndExitmethod provides a clean unified approach for handling both success and error cases with structured JSON output. The console method restoration usingconsole.constructor.prototypeis a clever solution to coordinate with the LogService suppression.Consider adding error handling for edge cases:
- const json = JSON.stringify(data); + const json = JSON.stringify(data, null, 0);The explicit
null, 0parameters ensure consistent formatting and make the intent clear.
96-96: Remove unnecessary return statement.The
returnstatement is unnecessary sincelogAndExitcallsprocess.exit().Apply this diff:
- return this.logAndExit({ error: 'No ID found in token', valid: false }); + this.logAndExit({ error: 'No ID found in token', valid: false });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/src/unraid-api/cli/log.service.ts(1 hunks)api/src/unraid-api/cli/sso/validate-token.command.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
api/**/*: Use pnpm as the only package manager
Always run scripts from api/package.json unless specifically requested otherwise
Files:
api/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/cli/log.service.ts
api/src/unraid-api/**/*
📄 CodeRabbit Inference Engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the NestJS repository located at api/src/unraid-api/ instead of the legacy code
Prefer adding new files to the NestJS repo located at api/src/unraid-api/ instead of the legacy code
Files:
api/src/unraid-api/cli/sso/validate-token.command.tsapi/src/unraid-api/cli/log.service.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: mdatelle
PR: unraid/api#1219
File: api/src/unraid-api/auth/cookie.strategy.ts:19-20
Timestamp: 2025-03-10T17:24:06.914Z
Learning: In the auth system, CSRF token validation and cookie validation have been unified in the `validateCookiesCasbin()` method in the AuthService class, which takes the entire FastifyRequest object and performs both validations sequentially.
Learnt from: pujitm
PR: unraid/api#1047
File: api/src/unraid-api/graph/sandbox-plugin.ts:57-57
Timestamp: 2025-01-15T21:34:00.006Z
Learning: In the GraphQL sandbox (api/src/unraid-api/graph/sandbox-plugin.ts), CSRF token validation should fail silently with a fallback value to maintain sandbox accessibility, as it's a development tool where strict security measures aren't required.
Learnt from: elibosley
PR: unraid/api#1120
File: plugin/scripts/build-plugin-and-txz.ts:132-147
Timestamp: 2025-02-06T15:32:09.488Z
Learning: For validation steps in build scripts where the goal is to fail fast on invalid state, simple error handling (console.error + process.exit) is appropriate since we want to abort the build process immediately.
Learnt from: pujitm
PR: unraid/api#1116
File: api/src/unraid-api/cli/log.service.ts:52-56
Timestamp: 2025-02-04T20:06:52.290Z
Learning: In Node.js logging, console.trace() is specifically for outputting stack traces to stderr and should not be used for general trace-level logging. For trace-level logging of general messages, console.log() is the appropriate choice.
api/src/unraid-api/cli/sso/validate-token.command.ts (3)
Learnt from: elibosley
PR: #942
File: api/src/unraid-api/auth/api-key.service.ts:62-70
Timestamp: 2024-11-05T14:49:07.308Z
Learning: In api/src/unraid-api/auth/api-key.service.ts, when handling read errors in the findById method, throw a GraphQLError instead of an InternalServerErrorException.
Learnt from: mdatelle
PR: #942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts, methods like removeRoleFromApiKey rely on service-level error handling.
Learnt from: elibosley
PR: #1120
File: plugin/scripts/build-plugin-and-txz.ts:132-147
Timestamp: 2025-02-06T15:32:09.488Z
Learning: For validation steps in build scripts where the goal is to fail fast on invalid state, simple error handling (console.error + process.exit) is appropriate since we want to abort the build process immediately.
api/src/unraid-api/cli/log.service.ts (7)
Learnt from: elibosley
PR: #942
File: api/src/unraid-api/auth/api-key.service.ts:176-188
Timestamp: 2024-11-15T16:22:03.485Z
Learning: Atomic writes are not required for the saveApiKey method in ApiKeyService (api/src/unraid-api/auth/api-key.service.ts) unless specifically needed.
Learnt from: mdatelle
PR: #942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:44:46.432Z
Learning: When modifying apiKey.roles in removeRoleFromApiKey and addRoleToApiKey within api/src/unraid-api/auth/auth.service.ts, concurrency issues are not a concern because the keys are stored in the file system.
Learnt from: mdatelle
PR: #942
File: api/src/unraid-api/auth/auth.service.ts:42-105
Timestamp: 2024-11-04T22:00:41.946Z
Learning: In the 'AuthService' within api/src/unraid-api/auth/auth.service.ts, rate limiting is not required because the API is not public facing.
Learnt from: pujitm
PR: #1047
File: api/src/unraid-api/graph/sandbox-plugin.ts:57-57
Timestamp: 2025-01-15T21:34:00.006Z
Learning: In the GraphQL sandbox (api/src/unraid-api/graph/sandbox-plugin.ts), CSRF token validation should fail silently with a fallback value to maintain sandbox accessibility, as it's a development tool where strict security measures aren't required.
Learnt from: mdatelle
PR: #942
File: api/src/unraid-api/auth/auth.service.ts:0-0
Timestamp: 2024-11-04T20:41:22.303Z
Learning: In api/src/unraid-api/auth/auth.service.ts, the addRoleToApiKey function operates on API keys stored as JSON files in a directory, not a database, so concurrency is not a concern for modifying apiKey.roles.
Learnt from: elibosley
PR: #1222
File: api/src/unraid-api/graph/resolvers/logs/logs.resolver.ts:37-65
Timestamp: 2025-03-17T13:41:52.496Z
Learning: The LogsService in Unraid API uses a reference counting mechanism to track log file subscriptions, ensuring file watchers remain active until all subscribers have unsubscribed.
Learnt from: elibosley
PR: #1101
File: api/src/unraid-api/unraid-file-modifier/modifications/test/generic-modification.spec.ts:55-56
Timestamp: 2025-02-03T18:57:28.442Z
Learning: Using console.log is acceptable in test files, especially for debugging test execution. The logger instances in test files are often used as mocks/dependencies for the classes being tested, not for actual logging.
🔇 Additional comments (1)
api/src/unraid-api/cli/log.service.ts (1)
34-36: LGTM!The conditional logic in the clear() method is consistent with the overall approach of suppressing console operations during validate-token command execution.
1 participant
Summary by CodeRabbit