refactor: env process into config file #1189
Conversation
Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com>
Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com>
* fix: added api param validations Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> * fix: removed unnecessary validations Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> * fix:resolved validations issue Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> * added comment on function for understanding Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> * fix: resolve orgid validations Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> * fix: added response message Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> * fix: added space in response messages Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> --------- Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com>
Signed-off-by: pallavighule <pallavi.ghule@ayanworks.com>
* fix: removed prisma commands in build stage Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> * feat: create yml file for all services Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> * Updated Dockerfiles Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> * Updated yml to push docker images Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> * updated yml files Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> --------- Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com>
* fix: used single yml file using matrix instead of multiple files Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> * fix: renamed yml file to cicd.yml Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> * fix: renamed yml file Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> * fix: renamed yml file Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> * chore: removed id-token and attestations Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com> --------- Signed-off-by: sahil.kamble@ayanworks.com <sahil.kamble@ayanworks.com>
* chore: add aws into monorepo Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com> * chore: update tsconfig Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com> * refactor:tsconfig.build.json file Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com> * refactor: remove redundant .nvmrc file Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com> --------- Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com> Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com> Co-authored-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Sai Ranjit Tummalapalli <sairanjit.tummalapalli@ayanworks.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
…alhost) Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
GHkrishna
changed the title
Signed-off-by: rockito10 <rockito10.mfd@gmail.com>
|
Hello! I think the branch is ready to be merged, please tell me if there's any troubles with it, or anything I can improve |
|
Hello @rockito10 |
Signed-off-by: rockito10 <107514701+rockito10@users.noreply.github.com>
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis update introduces a comprehensive runtime environment validation framework in the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Config as config/config.ts
participant Env as process.env
participant Schema as envDemoSchema
participant V as v (validation framework)
participant Error as VError
App->>Config: Import env
Config->>Env: Read environment variables
Config->>Schema: Define validation schema
Config->>V: Use v.schema to validate process.env
V->>Env: Validate each variable with StringBuilder rules
V-->>Config: Return success or VError
alt Validation fails
Config->>Error: Throw VError with details
else Validation passes
Config->>App: Export parsed env config
end
Poem
✨ Finishing Touches
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. 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
config/core/helpers/str-helpers.ts (1)
13-15: Clarify the purpose of _isOptional function.The function always returns
true, which suggests it might be a placeholder or has a specific use case in the validation framework.Consider adding a comment to explain the purpose:
+// Always returns true - used for optional validation scenarios function _isOptional(): boolean { return true; }config/core/helpers/url-helpers.ts (2)
19-29: Improve robustness of localhost validation.The current port extraction logic using
split(':').at(-1).split('/').at(0)could be fragile with edge cases.Consider a more robust approach:
function _isLocalhost(input: string | undefined): boolean { const regex = /^(http:\/\/)?(localhost|127\.0\.0\.1|::1)(:\d{1,5})?(\/)?$/; if (!regex.test(input || '')) { return false; } - const port = input.split(':').at(-1).split('/').at(0); + const portMatch = input.match(/:(\d+)/); + const port = portMatch ? portMatch[1] : undefined; - return _isPort(port); + return !port || _isPort(port); }
53-55: Consider edge cases in multiple URL validation.The current implementation splits by comma and trims each URL, but it might not handle edge cases like empty strings after splitting.
Consider adding a check for empty URLs:
function _isMultipleURL(input: string | undefined): boolean { - return input?.split(',').every((url) => _isURL(url.trim()) || _isLocalhost(url.trim())); + return input?.split(',') + .map(url => url.trim()) + .filter(url => url.length > 0) + .every((url) => _isURL(url) || _isLocalhost(url)) ?? false; }config/core/validators.ts (1)
176-188: Consider expanding the valid port range.The current validation restricts ports to 1024-65536, but valid port numbers are 1-65535. While ports 1-1023 are reserved for system services, applications might legitimately need to use them in certain contexts.
Consider making the port range more flexible or configurable:
export function port(): Validator<string> { return (input) => { const success = _URL._isPort(input); if (!success) { return { - expected: 'A number between 1024 and 65536', + expected: 'A number between 1 and 65535', received: input, - message: 'Invalid port. Port numbers must be between 1024 and 65536.' + message: 'Invalid port. Port numbers must be between 1 and 65535.' }; } }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.eslintrc.js(1 hunks)apps/api-gateway/src/issuance/dtos/issuance.dto.ts(1 hunks)apps/api-gateway/src/issuance/issuance.controller.ts(1 hunks)apps/api-gateway/src/verification/verification.controller.ts(1 hunks)config/config.ts(1 hunks)config/core/error.ts(1 hunks)config/core/helpers/str-helpers.ts(1 hunks)config/core/helpers/url-helpers.ts(1 hunks)config/core/index.ts(1 hunks)config/core/validators.ts(1 hunks)config/test/.env.test.sample(1 hunks)config/test/config.spec.ts(1 hunks)libs/common/src/NATSClient.ts(0 hunks)
💤 Files with no reviewable changes (1)
- libs/common/src/NATSClient.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tipusinghaw
PR: credebl/platform#1297
File: apps/api-gateway/src/tracer.ts:24-28
Timestamp: 2025-06-24T11:15:29.468Z
Learning: In the credebl/platform repository, the team prefers to create utilities for validating required environment variables rather than implementing inline validation. This approach promotes reusability across different parts of the application.
config/config.ts (1)
Learnt from: tipusinghaw
PR: credebl/platform#1297
File: apps/api-gateway/src/tracer.ts:24-28
Timestamp: 2025-06-24T11:15:29.468Z
Learning: In the credebl/platform repository, the team prefers to create utilities for validating required environment variables rather than implementing inline validation. This approach promotes reusability across different parts of the application.
config/test/config.spec.ts (1)
Learnt from: tipusinghaw
PR: credebl/platform#1297
File: apps/api-gateway/src/tracer.ts:24-28
Timestamp: 2025-06-24T11:15:29.468Z
Learning: In the credebl/platform repository, the team prefers to create utilities for validating required environment variables rather than implementing inline validation. This approach promotes reusability across different parts of the application.
🧬 Code Graph Analysis (4)
config/config.ts (1)
config/core/index.ts (1)
v(184-187)
config/test/config.spec.ts (1)
config/core/index.ts (1)
v(184-187)
config/core/helpers/url-helpers.ts (2)
config/core/index.ts (3)
port(88-91)host(78-81)url(53-56)config/core/validators.ts (3)
port(176-188)host(160-172)url(208-220)
config/core/validators.ts (4)
config/core/error.ts (1)
Issue(3-8)config/core/helpers/str-helpers.ts (1)
_STR(23-29)config/core/index.ts (16)
v(184-187)boolean(48-51)notEmpty(38-41)number(43-46)optional(110-113)startsWith(98-101)domain(103-106)endpoint(93-96)host(78-81)port(88-91)protocol(73-76)url(53-56)localhost(83-86)multipleUrl(58-61)postgresUrl(63-66)config/core/helpers/url-helpers.ts (1)
_URL(66-78)
🪛 Biome (1.9.4)
apps/api-gateway/src/issuance/dtos/issuance.dto.ts
[error] 18-18: Declarations inside of a import declaration may not have duplicates
a second declaration of IsUUID is not allowed
IsUUID is first declared here
(parse)
🔇 Additional comments (12)
apps/api-gateway/src/issuance/issuance.controller.ts (1)
84-84: LGTM - Formatting improvement.The added blank line improves code readability and maintains consistency with formatting standards.
.eslintrc.js (1)
104-104: LGTM - Formatting improvement.Adding a trailing newline is a good practice and improves file formatting consistency.
apps/api-gateway/src/verification/verification.controller.ts (1)
220-220: LGTM - Formatting improvement.The added blank line improves code readability by providing visual separation between the object construction and the service call.
config/core/helpers/str-helpers.ts (1)
17-19: LGTM - Proper optional chaining implementation.The function correctly uses optional chaining to handle undefined inputs safely.
config/test/.env.test.sample (3)
20-21: Review the expected behavior for optional validation.The test expects an empty string to fail for optional validation (
TEST_OPTIONAL_1= # FAIL), but typically optional fields should allow empty values.Please verify if this is the intended behavior for optional field validation. If optional fields should allow empty values, this test case might need adjustment.
16-18: Verify port range validation logic.The test expects ports 1000 and 99999 to fail, suggesting a specific valid port range. Please ensure this aligns with your application's port validation requirements.
Consider documenting the valid port range in comments for clarity:
-TEST_PORT_1=1000 # FAIL -TEST_PORT_2=99999 # FAIL -TEST_PORT_3=3000 # PASS +TEST_PORT_1=1000 # FAIL (below minimum allowed port) +TEST_PORT_2=99999 # FAIL (above maximum allowed port) +TEST_PORT_3=3000 # PASS (within valid range)
1-61: LGTM - Comprehensive test coverage.The test file provides good coverage of various validation scenarios including protocols, URLs, emails, booleans, numbers, and PostgreSQL URLs. The expected pass/fail cases generally align with typical validation rules.
config/core/error.ts (1)
1-49: Well-structured error handling implementation.The custom
VErrorclass provides excellent structured error handling for validation issues. The design allows for detailed error reporting with path-based grouping through theflatten()method, which will be very useful for debugging configuration issues.config/config.ts (1)
1-170: Excellent implementation of environment validation utilities.This implementation perfectly aligns with the team's preference for creating reusable utilities for environment variable validation. The comprehensive schema covers all necessary environment variables with appropriate validators, and the error handling with
flatten()will provide clear feedback for configuration issues.The organization by functional areas (API_GATEWAY, PLATFORM, DATABASE, etc.) makes the configuration easy to understand and maintain.
config/core/index.ts (1)
1-188: Excellent validation framework design.The validation framework demonstrates excellent design patterns:
- Fluent API: The
StringBuilderclass provides an intuitive chaining interface for building complex validators.- Type Safety: Generic types with
InferShape<T>ensure compile-time type safety for validated objects.- Error Handling: Proper integration with the
VErrorclass for structured error reporting.- Separation of Concerns: Clean separation between string validation and object validation logic.
The public API through the
vobject provides a clean interface that will be easy to use across the application.config/core/validators.ts (2)
1-7: Excellent validation framework implementation.The overall design and implementation of the validation framework is well-structured with:
- Consistent error message formatting
- Proper TypeScript typing with the generic
Validator<T>- Good separation of concerns between generic and URL-specific validators
- Comprehensive error objects with expected/received values
- Modular design leveraging helper functions
This aligns well with the team's preference for creating reusable validation utilities rather than inline validation logic.
Also applies to: 13-106, 112-271
208-220: Localhost URLs aren’t covered by_isURL— both checks are needed
The defaultvalidator.isURLcall (used by_URL._isURL) requires a TLD and thus returns false for"localhost"or"localhost:3000"._URL._isLocalhostexplicitly handles those cases. Keeping both checks is correct and the performance impact of two quick boolean calls is negligible.Likely an incorrect or invalid review comment.
| @@ -15,6 +15,7 @@ import { | |||
| IsString, | |||
| IsUUID, | |||
| IsUrl, | |||
| IsUUID, | |||
There was a problem hiding this comment.
Remove duplicate IsUUID from the import list
IsUUID is already imported on line 16. The second occurrence triggers a compilation error (TS2300: Duplicate identifier) and linter failure. Delete the redundant entry to restore a valid import statement.
- IsUUID,
- IsUrl,
- IsUUID,
+ IsUUID,
+ IsUrl,Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Declarations inside of a import declaration may not have duplicates
a second declaration of IsUUID is not allowed
IsUUID is first declared here
(parse)
🤖 Prompt for AI Agents
In apps/api-gateway/src/issuance/dtos/issuance.dto.ts at line 18, remove the
duplicate import of IsUUID since it is already imported on line 16. Delete the
redundant IsUUID entry from the import list to fix the TS2300 duplicate
identifier compilation error and resolve the linter failure.
| function _isNotEmpty(input: string | undefined): boolean { | ||
| return '' !== input?.trim(); | ||
| } |
There was a problem hiding this comment.
Fix incorrect handling of undefined inputs.
The function returns true for undefined inputs because '' !== undefined is true, which is likely not the intended behavior.
Apply this fix to properly handle undefined inputs:
-function _isNotEmpty(input: string | undefined): boolean {
- return '' !== input?.trim();
-}
+function _isNotEmpty(input: string | undefined): boolean {
+ return input !== undefined && '' !== input.trim();
+}📝 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.
| function _isNotEmpty(input: string | undefined): boolean { | |
| return '' !== input?.trim(); | |
| } | |
| function _isNotEmpty(input: string | undefined): boolean { | |
| return input !== undefined && '' !== input.trim(); | |
| } |
🤖 Prompt for AI Agents
In config/core/helpers/str-helpers.ts around lines 9 to 11, the function
_isNotEmpty incorrectly returns true for undefined inputs because it compares an
empty string to input?.trim(), which results in true when input is undefined. To
fix this, explicitly check if the input is defined before trimming and
comparing, ensuring the function returns false for undefined inputs and only
true for non-empty strings.
| function _isNumber(input: string | undefined): boolean { | ||
| return !Number.isNaN(Number(input)); | ||
| } |
There was a problem hiding this comment.
Fix edge case with whitespace-only strings.
The current implementation considers whitespace-only strings as valid numbers because Number(" ") returns 0.
Apply this fix to handle whitespace-only strings properly:
-function _isNumber(input: string | undefined): boolean {
- return !Number.isNaN(Number(input));
-}
+function _isNumber(input: string | undefined): boolean {
+ return input?.trim() !== '' && !Number.isNaN(Number(input));
+}📝 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.
| function _isNumber(input: string | undefined): boolean { | |
| return !Number.isNaN(Number(input)); | |
| } | |
| function _isNumber(input: string | undefined): boolean { | |
| return input?.trim() !== '' && !Number.isNaN(Number(input)); | |
| } |
🤖 Prompt for AI Agents
In config/core/helpers/str-helpers.ts around lines 5 to 7, the _isNumber
function incorrectly treats whitespace-only strings as valid numbers because
Number(" ") returns 0. To fix this, add a check to return false if the input is
undefined or consists only of whitespace before converting it to a number. This
ensures that strings with only whitespace are not considered valid numbers.
| function _includes(inputs: string[], value: string | undefined): boolean { | ||
| return inputs?.includes(value); | ||
| } |
There was a problem hiding this comment.
Fix type safety issue with undefined values.
The function accepts string | undefined but inputs.includes(undefined) behavior depends on whether the array actually contains undefined values. This could lead to unexpected results.
Consider this fix to handle undefined values explicitly:
-function _includes(inputs: string[], value: string | undefined): boolean {
- return inputs?.includes(value);
-}
+function _includes(inputs: string[], value: string | undefined): boolean {
+ return value !== undefined && inputs?.includes(value);
+}📝 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.
| function _includes(inputs: string[], value: string | undefined): boolean { | |
| return inputs?.includes(value); | |
| } | |
| function _includes(inputs: string[], value: string | undefined): boolean { | |
| return value !== undefined && inputs?.includes(value); | |
| } |
🤖 Prompt for AI Agents
In config/core/helpers/str-helpers.ts around lines 1 to 3, the function
_includes accepts a value that can be string or undefined, but calling includes
with undefined may cause unexpected behavior. Update the function to explicitly
check if the value is undefined and handle that case separately, returning false
or handling it as appropriate, before calling includes on the inputs array.
| test('Boolean success, is uppercase false.', () => { | ||
| const mockSchema = v.schema({ | ||
| TEST_BOOLEAN_4: v.str().boolean() | ||
| }); | ||
|
|
||
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
Fix contradictory test description and expectation.
The test description says "Boolean success, is uppercase false" but expects toBeFalsy(), which indicates failure, not success.
Apply this diff to fix the test description:
-test('Boolean success, is uppercase false.', () => {
+test('Boolean error, is uppercase false.', () => {
const mockSchema = v.schema({
TEST_BOOLEAN_4: v.str().boolean()
});
expect(mockSchema.safeParse(process.env).success).toBeFalsy();
});📝 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.
| test('Boolean success, is uppercase false.', () => { | |
| const mockSchema = v.schema({ | |
| TEST_BOOLEAN_4: v.str().boolean() | |
| }); | |
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | |
| }); | |
| test('Boolean error, is uppercase false.', () => { | |
| const mockSchema = v.schema({ | |
| TEST_BOOLEAN_4: v.str().boolean() | |
| }); | |
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | |
| }); |
🤖 Prompt for AI Agents
In config/test/config.spec.ts around lines 309 to 315, the test description
states "Boolean success, is uppercase false" but the expectation is toBeFalsy(),
which contradicts the description. Update the test description to accurately
reflect that the test expects failure by changing it to something like "Boolean
failure, is uppercase false" or similar wording that matches the toBeFalsy()
expectation.
| test('Host Error exceeds value of valid IP', () => { | ||
| const mockSchema = v.schema({ | ||
| TEST_HOST_1: v.str().protocol() | ||
| }); | ||
|
|
||
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | ||
| }); | ||
|
|
||
| test('Host Error contains letters', () => { | ||
| const mockSchema = v.schema({ | ||
| TEST_HOST_2: v.str().protocol() | ||
| }); | ||
|
|
||
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | ||
| }); | ||
|
|
||
| test('Host Error contains letters', () => { | ||
| const mockSchema = v.schema({ | ||
| TEST_HOST_3: v.str().protocol() | ||
| }); | ||
|
|
||
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
Fix validator methods and test naming issues.
Several issues in the host error tests:
- Lines 46, 54, 62: These tests are checking for "Host Error" but using
.protocol()instead of.host()validator. - Line 60: Duplicate test name "Host Error contains letters".
Apply this diff to fix the validator methods and test names:
test('Host Error exceeds value of valid IP', () => {
const mockSchema = v.schema({
- TEST_HOST_1: v.str().protocol()
+ TEST_HOST_1: v.str().host()
});
expect(mockSchema.safeParse(process.env).success).toBeFalsy();
});
test('Host Error contains letters', () => {
const mockSchema = v.schema({
- TEST_HOST_2: v.str().protocol()
+ TEST_HOST_2: v.str().host()
});
expect(mockSchema.safeParse(process.env).success).toBeFalsy();
});
-test('Host Error contains letters', () => {
+test('Host Error invalid format', () => {
const mockSchema = v.schema({
- TEST_HOST_3: v.str().protocol()
+ TEST_HOST_3: v.str().host()
});
expect(mockSchema.safeParse(process.env).success).toBeFalsy();
});📝 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.
| test('Host Error exceeds value of valid IP', () => { | |
| const mockSchema = v.schema({ | |
| TEST_HOST_1: v.str().protocol() | |
| }); | |
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | |
| }); | |
| test('Host Error contains letters', () => { | |
| const mockSchema = v.schema({ | |
| TEST_HOST_2: v.str().protocol() | |
| }); | |
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | |
| }); | |
| test('Host Error contains letters', () => { | |
| const mockSchema = v.schema({ | |
| TEST_HOST_3: v.str().protocol() | |
| }); | |
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | |
| }); | |
| test('Host Error exceeds value of valid IP', () => { | |
| const mockSchema = v.schema({ | |
| TEST_HOST_1: v.str().host() | |
| }); | |
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | |
| }); | |
| test('Host Error contains letters', () => { | |
| const mockSchema = v.schema({ | |
| TEST_HOST_2: v.str().host() | |
| }); | |
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | |
| }); | |
| test('Host Error invalid format', () => { | |
| const mockSchema = v.schema({ | |
| TEST_HOST_3: v.str().host() | |
| }); | |
| expect(mockSchema.safeParse(process.env).success).toBeFalsy(); | |
| }); |
🤖 Prompt for AI Agents
In config/test/config.spec.ts lines 44 to 66, the tests for host errors
incorrectly use the .protocol() validator instead of .host(), and there is a
duplicate test name "Host Error contains letters". Update the validator calls
from .protocol() to .host() for TEST_HOST_1, TEST_HOST_2, and TEST_HOST_3. Also,
rename the third test to a unique descriptive name to avoid duplication.
| expected: | ||
| 'A valid postgresURL with the format: postgresql://{postgres.user}:{postgres.password}@{your-ip}:{postgres.port}/{database-name}/schema={}', | ||
| received: input, | ||
| message: 'Invalid collection of URLs.' |
There was a problem hiding this comment.
Fix incorrect error message in postgresUrl() validator.
The error message incorrectly states "Invalid collection of URLs" when it should be about an invalid PostgreSQL URL format.
- message: 'Invalid collection of URLs.'
+ message: 'Invalid PostgreSQL URL format.'📝 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.
| message: 'Invalid collection of URLs.' | |
| message: 'Invalid PostgreSQL URL format.' |
🤖 Prompt for AI Agents
In config/core/validators.ts at line 267, the error message for the
postgresUrl() validator incorrectly states "Invalid collection of URLs." Update
this message to accurately reflect the issue by changing it to indicate an
invalid PostgreSQL URL format, such as "Invalid PostgreSQL URL format."
| export function optional(): Validator<string> { | ||
| return (input) => { | ||
| const success = _STR._isOptional(); | ||
| if (!success) { | ||
| return { | ||
| expected: 'An optional value', | ||
| received: input, | ||
| message: 'Must be a valid optional value.' | ||
| }; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix the optional() validator logic.
The optional() validator has a critical flaw: it calls _STR._isOptional() without passing the input parameter, and the logic seems incorrect since it always returns an error.
export function optional(): Validator<string> {
return (input) => {
- const success = _STR._isOptional();
- if (!success) {
- return {
- expected: 'An optional value',
- received: input,
- message: 'Must be a valid optional value.'
- };
- }
+ const success = _STR._isOptional(input);
+ if (!success) {
+ return {
+ expected: 'An optional value',
+ received: input,
+ message: 'Must be a valid optional value.'
+ };
+ }
};
}📝 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.
| export function optional(): Validator<string> { | |
| return (input) => { | |
| const success = _STR._isOptional(); | |
| if (!success) { | |
| return { | |
| expected: 'An optional value', | |
| received: input, | |
| message: 'Must be a valid optional value.' | |
| }; | |
| } | |
| }; | |
| } | |
| export function optional(): Validator<string> { | |
| return (input) => { | |
| const success = _STR._isOptional(input); | |
| if (!success) { | |
| return { | |
| expected: 'An optional value', | |
| received: input, | |
| message: 'Must be a valid optional value.' | |
| }; | |
| } | |
| }; | |
| } |
🤖 Prompt for AI Agents
In config/core/validators.ts around lines 79 to 90, the optional() validator
incorrectly calls _STR._isOptional() without passing the input parameter and
always returns an error. Fix this by passing the input to
_STR._isOptional(input) and adjust the logic to return undefined (indicating
success) when the input is optional, and return the error object only when the
input is not optional.
| export function localhost(): Validator<string> { | ||
| return (input) => { | ||
| const port = input.split(':').at(-1); | ||
| const success = _URL._isLocalhost(input) && _URL._isPort(port); | ||
|
|
||
| if (!success) { | ||
| return { | ||
| expected: 'A valid localhost (e.g., http://localhost:aPortNumber, 127.0.0.1:aPortNumber)', | ||
| received: input, | ||
| message: 'Invalid Localhost.' | ||
| }; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review localhost validation for IPv6 compatibility.
The current implementation splits by ':' and takes the last part as the port, but this approach won't work correctly for IPv6 addresses like [::1]:8080 where colons are part of the IP address format.
Consider testing with IPv6 localhost addresses to ensure the validation works correctly:
🏁 Script executed:
#!/bin/bash
# Description: Check if IPv6 localhost addresses are handled correctly in the codebase
# Search for IPv6 localhost usage patterns
rg -i "localhost.*:.*:" --type ts
rg -i "\[::1\]" --type ts
rg -i "127\.0\.0\.1" --type tsLength of output: 1083
🏁 Script executed:
#!/bin/bash
# Display import section and context for _URL in validators.ts
sed -n '1,50p' config/core/validators.tsLength of output: 1413
🏁 Script executed:
#!/bin/bash
# Show _isLocalhost implementation in url-helpers.ts
sed -n '1,200p' config/core/helpers/url-helpers.tsLength of output: 2084
Add IPv6 bracketed localhost support in the localhost validator
The current logic in
config/core/helpers/url-helpers.ts→_isLocalhostconfig/core/validators.ts→localhost()
only matches unbracketed ::1 and naïvely splits on ':' to grab the port. As a result, inputs like
• http://[::1]:8080
• [::1]:8080
will fail validation or mis-extract the port.
Suggested fixes:
• In _isLocalhost, extend the regex to allow optional brackets around ::1, e.g.:
const regex = /^(http:\/\/)?(\[?::1\]?|localhost|127\.0\.0\.1)(:\d{1,5})?(\/)?$/;• Update port extraction to strip the bracket suffix before splitting, for example:
// inside _isLocalhost
const hostPort = input.replace(/^\[?(.+?)\]?$/, '$1'); // removes surrounding [ ]
const port = hostPort.split(':').at(-1).split('/').at(0);• In config/core/validators.ts, update the expected message to include the IPv6 example:
'A valid localhost (e.g., http://localhost:3000, http://127.0.0.1:3000, http://[::1]:3000)'
Locations needing change:
- config/core/helpers/url-helpers.ts — function
_isLocalhost - config/core/validators.ts — function
localhost(), errorexpectedstring
🤖 Prompt for AI Agents
In config/core/helpers/url-helpers.ts within the _isLocalhost function, update
the regex to allow optional brackets around ::1 by using
/^(http:\/\/)?(\[?::1\]?|localhost|127\.0\.0\.1)(:\d{1,5})?(\/)?$/. Also, modify
the port extraction logic to first remove surrounding brackets from the host
portion before splitting on ':' to get the port. In config/core/validators.ts
inside the localhost() function (lines 224-237), update the expected error
message string to include the IPv6 bracketed localhost example like 'A valid
localhost (e.g., http://localhost:3000, http://127.0.0.1:3000,
http://[::1]:3000)'. These changes will correctly handle bracketed IPv6
localhost addresses and improve validation accuracy.
|
Hello @rockito10 We were working on some releases, which meant we couldn't merge some PRs. Since that part is about to be finished, we can now merge appropriate PRs. Do you mind having a look at the suggestions from coderabbit and also some DCO issues |
|
Hello! I read the reviews and worked a bit on them, just a couple of things: -Regarding the DCO, I did sign my previous commits, these last unsigned ones seem to be from the last merge we did on this branch, from other people. Im not sure if I am allowed to change their commit messages to have signatures. -Regarding the CodeRabbit review, I did most of the things it requested, except the isOptional conflict, because if the function would receive the input parameter, there'll be no way to use it (since what I am checking is that it could be null or undefined but it could also be anything else), so I just return true in this case. Still, I can change it if you want, no problem on that. |
Summary by CodeRabbit
New Features
Tests
Chores