refactor: migrate environment variable access to appConfig…#1801
Merged
Conversation
…ed configuration management - Introduced appConfig to centralize configuration management and replace direct process.env access. - Updated various services and utilities to utilize appConfig for retrieving configuration values. - Removed deprecated helper functions for environment variable access. - Enhanced test environment handling by utilizing isTest() method from appConfig. - Ensured all database connection configurations are now sourced from appConfig. - Improved readability and maintainability of the codebase by consolidating configuration logic.
📝 WalkthroughWalkthroughThis PR migrates the backend from scattered ChangesCentralized Configuration System Refactoring
🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes environment-based configuration into a single appConfig module and migrates scattered process.env access (and related helpers) to that shared configuration layer, improving consistency across services/middlewares and simplifying validation/TypeORM setup.
Changes:
- Added
AppConfig(appConfig) + globalConfigModule/ConfigServicewrapper to provide typed, centralized configuration and validation. - Migrated many services, middlewares, and utilities from direct
process.envreads toappConfig/isTest()/isSaaS(). - Removed deprecated env helper/validator utilities (
get-process-variable,get-requeired-env-variable,required-environment-variables.validator).
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/shared/services/turnstile.service.ts | Uses appConfig for Turnstile secret instead of process.env. |
| backend/src/shared/database/database.service.ts | Switches test-environment check to isTest(). |
| backend/src/shared/config/config.service.ts | Replaced old dotenv-based config service with getters over appConfig. |
| backend/src/shared/config/config.module.ts | Introduces global ConfigModule that validates config on initialization. |
| backend/src/shared/config/app-config.ts | Adds centralized config reader/validator + TypeORM config builder. |
| backend/src/microservices/gateways/saas-gateway.ts/utils/generate-saas-jwt.ts | Reads microservice JWT secret from appConfig with explicit missing-secret error. |
| backend/src/microservices/gateways/saas-gateway.ts/base-saas-gateway.service.ts | Uses appConfig for SaaS base URL defaulting. |
| backend/src/main.ts | Validates required config via appConfig.validate() and uses config-backed values for Sentry/global prefix. |
| backend/src/interceptors/timeout.interceptor.ts | Uses isTest() to pick default timeout values. |
| backend/src/helpers/validators/validation-helper.ts | Uses isTest() to relax password strength in tests. |
| backend/src/helpers/validators/required-environment-variables.validator.ts | Removed in favor of appConfig.validate(). |
| backend/src/helpers/validators/is-action-url-host-allowed.ts | Uses isTest() to bypass host checks in tests. |
| backend/src/helpers/slack/slack-post-message.ts | Reads Slack token and test flag from appConfig. |
| backend/src/helpers/get-process-variable.ts | Removed deprecated env accessor helper. |
| backend/src/helpers/encryption/encryptor.ts | Reads private key / intercom key from appConfig. |
| backend/src/helpers/constants/constants.ts | Migrates test-connection config + app domain address to appConfig; simplifies SaaS gating. |
| backend/src/helpers/app/is-test.ts | Implements isTest() via appConfig. |
| backend/src/helpers/app/is-saas.ts | Implements isSaaS() via appConfig. |
| backend/src/helpers/app/get-requeired-env-variable.ts | Removed deprecated required-env helper. |
| backend/src/entities/user/utils/get-cookie-domain-options.ts | Reads cookie domain from appConfig. |
| backend/src/entities/user/utils/generate-gwt-token.ts | Reads JWT secrets from appConfig. |
| backend/src/entities/table/use-cases/find-tables-in-connection.use.case.ts | Uses isTest() to gate side-effect saving table info. |
| backend/src/entities/table/use-cases/find-tables-in-connection-v2.use.case.ts | Uses isTest() similarly; minor formatting changes. |
| backend/src/entities/table-schema/utils/dynamodb-schema-op.ts | Uses isTest() for DynamoDB region override in tests. |
| backend/src/entities/table-actions/table-action-rules-module/use-cases/update-action-rule-with-actions-and-events.use.case.ts | Uses isTest() to relax URL validation in tests. |
| backend/src/entities/table-actions/table-action-rules-module/use-cases/create-action-rule.use.case.ts | Uses isTest() to relax URL validation in tests. |
| backend/src/entities/table-actions/table-action-rules-module/application/dto/create-action-rules-with-actions-and-events-body.dto.ts | Uses isTest() to conditionally apply IsUrl. |
| backend/src/entities/logging/winston-logger.ts | Reads log level from appConfig. |
| backend/src/entities/email/email/email.service.ts | Uses appConfig for “from” address; isTest() to skip sending in tests. |
| backend/src/entities/email/email-config/email-config.service.ts | Uses appConfig for SMTP config resolution. |
| backend/src/entities/connection/utils/validate-create-connection-data.ts | Uses isTest() to alter host/type validation behavior in tests. |
| backend/src/entities/connection/utils/is-host-allowed.ts | Uses isTest() in allow/deny logic. |
| backend/src/entities/connection/connection.entity.ts | Uses isTest() to set deterministic signing key in tests. |
| backend/src/entities/company-info/use-cases/invite-user-in-company.use.case.ts | Uses isTest() to expose verification string in tests. |
| backend/src/entities/company-info/company-info-helper.service.ts | Uses isTest() to bypass SaaS invitation limits in tests. |
| backend/src/entities/amplitude/amplitude.service.ts | Reads Amplitude key from appConfig; skips in tests via isTest(). |
| backend/src/entities/ai/user-ai-requests-v2.controller.ts | Uses isTest() to set AI endpoint timeouts. |
| backend/src/entities/agent/repository/custom-agent-repository-extension.ts | Uses isTest() for test token behavior. |
| backend/src/authorization/temporary-auth.middleware.ts | Uses isTest() for test bypass; reads JWT secret from appConfig. |
| backend/src/authorization/saas-auth.middleware.ts | Reads microservice JWT secret from appConfig. |
| backend/src/authorization/non-scoped-auth.middleware.ts | Uses isTest() for test bypass; reads JWT secret from appConfig. |
| backend/src/authorization/basic-auth.middleware.ts | Reads basic auth creds from appConfig. |
| backend/src/authorization/auth.middleware.ts | Uses isTest() for test bypass; reads JWT secret from appConfig. |
| backend/src/authorization/auth-with-api.middleware.ts | Reads JWT secret from appConfig. |
| backend/src/app.module.ts | Registers global ConfigModule in the main module imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+236
to
+242
| public get isTest(): boolean { | ||
| return process.env.NODE_ENV === 'test'; | ||
| } | ||
|
|
||
| public get isSaaS(): boolean { | ||
| return !!process.env.IS_SAAS; | ||
| } |
Comment on lines
+161
to
+168
| this.email = Object.freeze({ | ||
| configString: readString('EMAIL_CONFIG_STRING'), | ||
| host: readString('EMAIL_SERVICE_HOST'), | ||
| port: readInt('EMAIL_SERVICE_PORT') ?? DEFAULT_EMAIL_PORT, | ||
| username: readString('EMAIL_SERVICE_USERNAME'), | ||
| password: readString('EMAIL_SERVICE_PASSWORD'), | ||
| nonSecure: readString('NON_SSL_EMAIL') === null, | ||
| from: readString('EMAIL_FROM') ?? AUTOADMIN_SUPPORT_MAIL, |
| port: emailServicePort, | ||
| host: host, | ||
| port: port, | ||
| secure: nonSecure, |
Comment on lines
+268
to
+271
| ? path.join(process.cwd(), ...pgLiteFolderPath.split('/')) | ||
| : path.join(__dirname, '..', '..', '..', pgLiteFolderPath); | ||
| console.info('\nPg Lite Folder Patch: ', pgLiteFolderPath, '\n'); | ||
| const resolvedPath = path.resolve(fullPath); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
backend/src/shared/config/config.module.ts (1)
10-14: ⚡ Quick winAnnotate
useFactoryreturn type explicitly.Please add an explicit return type on the factory callback to match the TypeScript typing guideline.
♻️ Proposed change
- useFactory: () => { + useFactory: (): ConfigService => { const service = new ConfigService(); appConfig.validate(); return service; },As per coding guidelines, "Always add type annotations to function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/shared/config/config.module.ts` around lines 10 - 14, The factory callback for the Nest provider lacks an explicit return type; update the useFactory declaration to explicitly return the ConfigService type (i.e., annotate the callback as returning ConfigService) so the function signature for useFactory: () => ConfigService matches TypeScript guidelines and the created instance from new ConfigService() and appConfig.validate() is correctly typed.backend/src/entities/table-actions/table-action-rules-module/application/dto/create-action-rules-with-actions-and-events-body.dto.ts (1)
23-31: ⚡ Quick winAdd explicit return type to
IsUrlIfNotTest.
IsUrlIfNotTestcurrently relies on inference. Please annotate the return type explicitly for consistency and stronger contracts.♻️ Proposed change
-function IsUrlIfNotTest(validationOptions?: IsURLOptions) { - return (object: NonNullable<unknown>, propertyName: string) => { +function IsUrlIfNotTest(validationOptions?: IsURLOptions): PropertyDecorator { + return (object: object, propertyName: string | symbol): void => { const decorators = [IsString()]; if (!isTest()) { decorators.push(IsUrl(validationOptions)); } applyDecorators(...decorators)(object, propertyName); }; }As per coding guidelines, "Always add type annotations to function parameters and return types in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/entities/table-actions/table-action-rules-module/application/dto/create-action-rules-with-actions-and-events-body.dto.ts` around lines 23 - 31, The function IsUrlIfNotTest lacks an explicit return type; change its signature to declare the decorator return type (e.g. add : PropertyDecorator) so the compiler knows it returns a property decorator. Update the declaration to function IsUrlIfNotTest(validationOptions?: IsURLOptions): PropertyDecorator { ... } (the inner function can remain as-is and continue calling applyDecorators with IsString and conditional IsUrl).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/entities/email/email/email.service.ts`:
- Line 38: The test-mode guard currently does "if (isTest()) return;" which
returns undefined but callers expect null for "no mail sent"; change that branch
to "if (isTest()) return null;" so the method in email.service.ts (the isTest()
check inside the email sending method) returns null explicitly in test mode to
match the method contract and avoid breaking null-only checks.
In `@backend/src/helpers/slack/slack-post-message.ts`:
- Line 8: The guard currently uses appConfig.isTest (a function reference) which
always evaluates truthy; change the condition to call the function (use
appConfig.isTest()) so the short-circuit only happens when tests are actually
detected or when slackBotToken is missing; update the conditional in
slack-post-message.ts (the if that references appConfig.isTest and
slackBotToken) to use appConfig.isTest() while preserving the existing ||
!slackBotToken logic.
In `@backend/src/shared/config/app-config.ts`:
- Line 270: Fix the typo in the console.info message: update the console.info
call that logs pgLiteFolderPath (the console.info('\nPg Lite Folder Patch: ',
pgLiteFolderPath, '\n') statement) to say "Path" instead of "Patch" so the log
reads "Pg Lite Folder Path: " while leaving pgLiteFolderPath and surrounding
formatting unchanged.
- Line 167: The nonSecure flag is inverted: currently nonSecure is true when
readString('NON_SSL_EMAIL') === null; change the logic so nonSecure reflects the
presence of the env var by setting nonSecure to a truthy check of the env value
(e.g., nonSecure: !!readString('NON_SSL_EMAIL') or nonSecure:
readString('NON_SSL_EMAIL') !== null) so that when NON_SSL_EMAIL is set,
nonSecure becomes true; update the property in the same config object where
nonSecure is defined to use this corrected condition.
- Around line 324-342: The parseTypeORMUrl function uses parse.parse() (from
parse) which can return nullable fields and currently types ssl as any; update
the return type to be strict (e.g., ssl: boolean | undefined or a specific SSL
config type) and defensively handle nullable parse results from parse.parse(url)
by checking for null/undefined for host, port, user, password, database and
providing safe defaults or throwing a clear error; ensure port is converted with
Number(...) or conditional parseInt only when port is a string and fall back to
a default number or undefined to match the return type; reference
parseTypeORMUrl and parse.parse in your changes.
---
Nitpick comments:
In
`@backend/src/entities/table-actions/table-action-rules-module/application/dto/create-action-rules-with-actions-and-events-body.dto.ts`:
- Around line 23-31: The function IsUrlIfNotTest lacks an explicit return type;
change its signature to declare the decorator return type (e.g. add :
PropertyDecorator) so the compiler knows it returns a property decorator. Update
the declaration to function IsUrlIfNotTest(validationOptions?: IsURLOptions):
PropertyDecorator { ... } (the inner function can remain as-is and continue
calling applyDecorators with IsString and conditional IsUrl).
In `@backend/src/shared/config/config.module.ts`:
- Around line 10-14: The factory callback for the Nest provider lacks an
explicit return type; update the useFactory declaration to explicitly return the
ConfigService type (i.e., annotate the callback as returning ConfigService) so
the function signature for useFactory: () => ConfigService matches TypeScript
guidelines and the created instance from new ConfigService() and
appConfig.validate() is correctly typed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cb0a464-bd93-4092-971a-1f9af9565d7f
📒 Files selected for processing (45)
backend/src/app.module.tsbackend/src/authorization/auth-with-api.middleware.tsbackend/src/authorization/auth.middleware.tsbackend/src/authorization/basic-auth.middleware.tsbackend/src/authorization/non-scoped-auth.middleware.tsbackend/src/authorization/saas-auth.middleware.tsbackend/src/authorization/temporary-auth.middleware.tsbackend/src/entities/agent/repository/custom-agent-repository-extension.tsbackend/src/entities/ai/user-ai-requests-v2.controller.tsbackend/src/entities/amplitude/amplitude.service.tsbackend/src/entities/company-info/company-info-helper.service.tsbackend/src/entities/company-info/use-cases/invite-user-in-company.use.case.tsbackend/src/entities/connection/connection.entity.tsbackend/src/entities/connection/utils/is-host-allowed.tsbackend/src/entities/connection/utils/validate-create-connection-data.tsbackend/src/entities/email/email-config/email-config.service.tsbackend/src/entities/email/email/email.service.tsbackend/src/entities/logging/winston-logger.tsbackend/src/entities/table-actions/table-action-rules-module/application/dto/create-action-rules-with-actions-and-events-body.dto.tsbackend/src/entities/table-actions/table-action-rules-module/use-cases/create-action-rule.use.case.tsbackend/src/entities/table-actions/table-action-rules-module/use-cases/update-action-rule-with-actions-and-events.use.case.tsbackend/src/entities/table-schema/utils/dynamodb-schema-op.tsbackend/src/entities/table/use-cases/find-tables-in-connection-v2.use.case.tsbackend/src/entities/table/use-cases/find-tables-in-connection.use.case.tsbackend/src/entities/user/utils/generate-gwt-token.tsbackend/src/entities/user/utils/get-cookie-domain-options.tsbackend/src/helpers/app/get-requeired-env-variable.tsbackend/src/helpers/app/is-saas.tsbackend/src/helpers/app/is-test.tsbackend/src/helpers/constants/constants.tsbackend/src/helpers/encryption/encryptor.tsbackend/src/helpers/get-process-variable.tsbackend/src/helpers/slack/slack-post-message.tsbackend/src/helpers/validators/is-action-url-host-allowed.tsbackend/src/helpers/validators/required-environment-variables.validator.tsbackend/src/helpers/validators/validation-helper.tsbackend/src/interceptors/timeout.interceptor.tsbackend/src/main.tsbackend/src/microservices/gateways/saas-gateway.ts/base-saas-gateway.service.tsbackend/src/microservices/gateways/saas-gateway.ts/utils/generate-saas-jwt.tsbackend/src/shared/config/app-config.tsbackend/src/shared/config/config.module.tsbackend/src/shared/config/config.service.tsbackend/src/shared/database/database.service.tsbackend/src/shared/services/turnstile.service.ts
💤 Files with no reviewable changes (3)
- backend/src/helpers/validators/required-environment-variables.validator.ts
- backend/src/helpers/app/get-requeired-env-variable.ts
- backend/src/helpers/get-process-variable.ts
|
|
||
| public async sendEmailToUser(letterContent: IMessage): Promise<SMTPTransport.SentMessageInfo | null> { | ||
| if (process.env.NODE_ENV === 'test') return; | ||
| if (isTest()) return; |
There was a problem hiding this comment.
Return null explicitly in test mode.
Line 38 currently returns undefined in a method contract that uses null for “no mail sent”, which can break callers that only branch on null.
Proposed fix
public async sendEmailToUser(letterContent: IMessage): Promise<SMTPTransport.SentMessageInfo | null> {
- if (isTest()) return;
+ if (isTest()) return null;
const mailResult = await this.sendEmailWithTimeout(letterContent);
if (mailResult) {
return mailResult;
}
+ return null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (isTest()) return; | |
| public async sendEmailToUser(letterContent: IMessage): Promise<SMTPTransport.SentMessageInfo | null> { | |
| if (isTest()) return null; | |
| const mailResult = await this.sendEmailWithTimeout(letterContent); | |
| if (mailResult) { | |
| return mailResult; | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/entities/email/email/email.service.ts` at line 38, The test-mode
guard currently does "if (isTest()) return;" which returns undefined but callers
expect null for "no mail sent"; change that branch to "if (isTest()) return
null;" so the method in email.service.ts (the isTest() check inside the email
sending method) returns null explicitly in test mode to match the method
contract and avoid breaking null-only checks.
| const slackBotToken = process.env.SLACK_BOT_ACCESS_TOKEN; | ||
| if (process.env.NODE_ENV === 'test' || !slackBotToken) { | ||
| const slackBotToken = appConfig.thirdParty.slackBotAccessToken; | ||
| if (appConfig.isTest || !slackBotToken) { |
There was a problem hiding this comment.
Call isTest() in the guard to avoid always short-circuiting.
Line [8] checks appConfig.isTest directly. If isTest is a function (as used elsewhere), this branch always evaluates truthy and prevents Slack messages in all environments.
Proposed fix
- if (appConfig.isTest || !slackBotToken) {
+ if (appConfig.isTest() || !slackBotToken) {
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (appConfig.isTest || !slackBotToken) { | |
| if (appConfig.isTest() || !slackBotToken) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/helpers/slack/slack-post-message.ts` at line 8, The guard
currently uses appConfig.isTest (a function reference) which always evaluates
truthy; change the condition to call the function (use appConfig.isTest()) so
the short-circuit only happens when tests are actually detected or when
slackBotToken is missing; update the conditional in slack-post-message.ts (the
if that references appConfig.isTest and slackBotToken) to use appConfig.isTest()
while preserving the existing || !slackBotToken logic.
| port: readInt('EMAIL_SERVICE_PORT') ?? DEFAULT_EMAIL_PORT, | ||
| username: readString('EMAIL_SERVICE_USERNAME'), | ||
| password: readString('EMAIL_SERVICE_PASSWORD'), | ||
| nonSecure: readString('NON_SSL_EMAIL') === null, |
There was a problem hiding this comment.
Logic inversion for nonSecure configuration.
The current logic sets nonSecure: true when NON_SSL_EMAIL is not set. Typically, setting NON_SSL_EMAIL env var would indicate you want non-SSL email. The condition appears inverted.
🐛 Proposed fix
- nonSecure: readString('NON_SSL_EMAIL') === null,
+ nonSecure: readString('NON_SSL_EMAIL') !== null,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/shared/config/app-config.ts` at line 167, The nonSecure flag is
inverted: currently nonSecure is true when readString('NON_SSL_EMAIL') === null;
change the logic so nonSecure reflects the presence of the env var by setting
nonSecure to a truthy check of the env value (e.g., nonSecure:
!!readString('NON_SSL_EMAIL') or nonSecure: readString('NON_SSL_EMAIL') !==
null) so that when NON_SSL_EMAIL is set, nonSecure becomes true; update the
property in the same config object where nonSecure is defined to use this
corrected condition.
| const fullPath = this.isTest | ||
| ? path.join(process.cwd(), ...pgLiteFolderPath.split('/')) | ||
| : path.join(__dirname, '..', '..', '..', pgLiteFolderPath); | ||
| console.info('\nPg Lite Folder Patch: ', pgLiteFolderPath, '\n'); |
There was a problem hiding this comment.
Typo in log message.
"Patch" should be "Path".
✏️ Proposed fix
- console.info('\nPg Lite Folder Patch: ', pgLiteFolderPath, '\n');
+ console.info('\nPg Lite Folder Path: ', pgLiteFolderPath, '\n');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.info('\nPg Lite Folder Patch: ', pgLiteFolderPath, '\n'); | |
| console.info('\nPg Lite Folder Path: ', pgLiteFolderPath, '\n'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/shared/config/app-config.ts` at line 270, Fix the typo in the
console.info message: update the console.info call that logs pgLiteFolderPath
(the console.info('\nPg Lite Folder Patch: ', pgLiteFolderPath, '\n') statement)
to say "Path" instead of "Patch" so the log reads "Pg Lite Folder Path: " while
leaving pgLiteFolderPath and surrounding formatting unchanged.
Comment on lines
+324
to
+342
| private parseTypeORMUrl(url: string): { | ||
| host: string; | ||
| port: number; | ||
| username: string; | ||
| password: string; | ||
| database: string; | ||
| ssl: any; | ||
| } { | ||
| const parsingResult = parse.parse(url); | ||
| const { host, port, user, password, database, ssl } = parsingResult; | ||
| return { | ||
| host, | ||
| port: parseInt(port, 10), | ||
| username: user, | ||
| password, | ||
| database, | ||
| ssl, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Type safety issues in parseTypeORMUrl.
ssl: anyviolates the "avoid any types" guideline.parse.parse()can returnnullforhost,port,user,password, anddatabase. If port is null,parseInt(port, 10)returnsNaN, which doesn't match the declared return type.
🛡️ Proposed fix with proper null handling and types
private parseTypeORMUrl(url: string): {
host: string;
port: number;
username: string;
password: string;
database: string;
- ssl: any;
+ ssl: boolean | Record<string, unknown> | undefined;
} {
const parsingResult = parse.parse(url);
const { host, port, user, password, database, ssl } = parsingResult;
+ if (!host || !port || !user || !password || !database) {
+ throw new Error('DATABASE_URL is missing required components (host, port, user, password, or database)');
+ }
return {
host,
port: parseInt(port, 10),
username: user,
password,
database,
- ssl,
+ ssl: ssl as boolean | Record<string, unknown> | undefined,
};
}As per coding guidelines: "Avoid any types - use specific types or generics instead"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/shared/config/app-config.ts` around lines 324 - 342, The
parseTypeORMUrl function uses parse.parse() (from parse) which can return
nullable fields and currently types ssl as any; update the return type to be
strict (e.g., ssl: boolean | undefined or a specific SSL config type) and
defensively handle nullable parse results from parse.parse(url) by checking for
null/undefined for host, port, user, password, database and providing safe
defaults or throwing a clear error; ensure port is converted with Number(...) or
conditional parseInt only when port is a string and fall back to a default
number or undefined to match the return type; reference parseTypeORMUrl and
parse.parse in your changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…for improved configuration management
Summary by CodeRabbit