-
-
Notifications
You must be signed in to change notification settings - Fork 18
Backend redis improvements #1450
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
Conversation
…tments in connection parameters
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.
Pull request overview
This PR adds support for Redis connection URLs (formats like redis://... or rediss://...) as an alternative to the traditional host/port/password connection configuration. This enhancement allows users to provide all Redis connection details in a single URL string, which is a common pattern for Redis connections.
Key Changes
- Added
isRedisConnectionUrl()helper function to detect Redis URL format - Modified required field validation to make port and password optional when using a connection URL
- Updated password validation logic across multiple files to skip password checks for Redis URLs
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
shared-code/src/data-access-layer/shared/create-data-access-object.ts |
Added isRedisConnectionUrl() helper function and updated required keys validation to make port/password optional for Redis URLs |
shared-code/src/data-access-layer/data-access-objects/data-access-object-redis.ts |
Modified Redis client creation to support both socket-based and URL-based connections |
backend/src/entities/connection/use-cases/test-connection.use.case.ts |
Updated password validation to allow missing passwords for Redis connection URLs |
backend/src/entities/connection/connection.controller.ts |
Added Redis URL checks to password validation and parameter validation logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return connection; | ||
| } | ||
|
|
||
| export function isRedisConnectionUrl(host: string): boolean { |
Copilot
AI
Nov 25, 2025
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.
The isRedisConnectionUrl function does not handle null or undefined values for the host parameter. If connectionParams.host is null or undefined, calling .test() on it will throw a runtime error. Consider adding a null check:
export function isRedisConnectionUrl(host: string): boolean {
if (!host) {
return false;
}
const redisUrlPattern = /^rediss?:\/\/.+/i;
return redisUrlPattern.test(host);
}| export function isRedisConnectionUrl(host: string): boolean { | |
| export function isRedisConnectionUrl(host: string): boolean { | |
| if (!host) { | |
| return false; | |
| } |
| password: isConnectionUrl ? undefined : this.connection.password ? this.connection.password : undefined, | ||
| username: isConnectionUrl ? undefined : this.connection.username ? this.connection.username : undefined, |
Copilot
AI
Nov 25, 2025
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.
Redundant ternary expression. The condition this.connection.password ? this.connection.password : undefined can be simplified to just this.connection.password || undefined or even simpler this.connection.password since empty strings are falsy in JavaScript. The same applies to line 888 for username.
Consider simplifying to:
password: isConnectionUrl ? undefined : this.connection.password || undefined,
username: isConnectionUrl ? undefined : this.connection.username || undefined,| password: isConnectionUrl ? undefined : this.connection.password ? this.connection.password : undefined, | |
| username: isConnectionUrl ? undefined : this.connection.username ? this.connection.username : undefined, | |
| password: isConnectionUrl ? undefined : this.connection.password, | |
| username: isConnectionUrl ? undefined : this.connection.username, |
| if (isRedisConnectionUrl(connectionData.host)) { | ||
| return errors; | ||
| } |
Copilot
AI
Nov 25, 2025
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.
[nitpick] When a Redis connection URL is detected, the validation returns early without validating the port field. However, the subsequent code (lines 740-742) still expects port to be a valid number. This could lead to issues if someone provides a Redis URL but also provides an invalid port value.
Consider either:
- Validating that port is not provided when using a connection URL, or
- Skipping port validation specifically for Redis URLs by checking the connection type as well:
if (isRedisConnectionUrl(connectionData.host)) {
return errors;
}
if (typeof connectionData.port !== 'number') errors.push(Messages.PORT_FORMAT_INCORRECT);Or move the port validation before the early return.
No description provided.