Skip to content

Conversation

@valkirilov
Copy link
Member

@valkirilov valkirilov commented Nov 28, 2025

What

Fixes the issue where users see raw JavaScript errors like "Cannot read properties of null (reading 'split')" when adding cloud databases with invalid endpoints. Now returns user-friendly error messages instead.

Testing

Prerequisites: You need a Redis Cloud account with a PRO-tier subscription database with a disabled public endpoint.

  1. Open Redis Insight and login to Redis Cloud (top right corner)
  2. From the top right corner, click on your avatar and then click "Import Cloud Databases"
  3. Select your subscription from the "Redis Cloud Subscriptions" screen
  4. Select your databases from the "Redis Cloud Databases" screen

You'll be redirected to the "Redis Enterprise Databases Added" screen, once the import is done. Observe the errors in the Results column.

Before After
image Screenshot 2025-12-01 at 17 04 07

Note

Introduce CloudDatabaseEndpointInvalidException and use it across autodiscovery and cloud jobs; add corresponding error code/message, UI code, tests, and docs updates.

  • Backend:
    • Custom exception: Add CloudDatabaseEndpointInvalidException (api/src/modules/cloud/job/exceptions/cloud-database-endpoint-invalid.exception.ts) and export from exceptions/index.ts.
    • Autodiscovery: In cloud-autodiscovery.service.ts, validate publicEndpoint; on missing value, return fail response using the new exception instead of splitting the endpoint.
    • Cloud jobs: In create-free-database.cloud-job.ts and import-free-database.cloud-job.ts, throw CloudDatabaseEndpointInvalidException when publicEndpoint is absent.
    • Constants: Add CustomErrorCodes.CloudDatabaseEndpointInvalid and ERROR_MESSAGES.CLOUD_DATABASE_ENDPOINT_INVALID.
  • Tests:
    • cloud-autodiscovery.service.spec.ts: Add parameterized tests for null/undefined publicEndpoint; ensure proper error structure and no DB creation.
  • Frontend:
    • Update ui/src/constants/customErrorCodes.ts with CloudDatabaseEndpointInvalid code.
  • Docs:
    • Backend rules: add guidance on custom exceptions with example.
    • Testing rules: add it.each parameterized tests section.
    • Commits rules: emphasize keeping commit bodies concise.

Written by Cursor Bugbot for commit 90f362c. This will update automatically on new commits. Configure here.

Validate publicEndpoint before calling split() to prevent null reference errors.
Return user-friendly error messages instead of raw JavaScript errors.

References: #RI-7778
- Introduced a section on custom exceptions, emphasizing their benefits for consistent error handling.
- Added examples for creating custom exceptions in TypeScript.
- Included a new section on parameterized tests using `it.each`, highlighting its advantages for maintainability and readability.
@github-actions
Copy link
Contributor

github-actions bot commented Nov 28, 2025

Code Coverage - Integration Tests

Status Category Percentage Covered / Total
🟢 Statements 81.63% 16290/19954
🟡 Branches 64.57% 7347/11377
🟡 Functions 70.61% 2285/3236
🟢 Lines 81.28% 15327/18857

@github-actions
Copy link
Contributor

github-actions bot commented Nov 28, 2025

Code Coverage - Backend unit tests

St.
Category Percentage Covered / Total
🟢 Statements 92.3% 13889/15048
🟡 Branches 74.02% 4193/5665
🟢 Functions 85.81% 2135/2488
🟢 Lines 92.1% 13277/14416

Test suite run success

2967 tests passing in 287 suites.

Report generated by 🧪jest coverage report action from 90f362c

@valkirilov valkirilov marked this pull request as ready for review November 28, 2025 12:50
@valkirilov valkirilov self-assigned this Nov 28, 2025
publicEndpoint: string | null | undefined,
): boolean {
return (
!!publicEndpoint && !!publicEndpoint.trim() && publicEndpoint.includes(':')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the best way to check it or there can be a regex with more rules?

Copy link
Member Author

@valkirilov valkirilov Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, all we care about is whether there is a value for the publicEndpoint, but since we split it later in the code by :, the AI considered it makes sense to check it here as well.

For sure, we can make the validation smarter, but I don't have the requirements for it, so here we're just improvising.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say we just need to check if we have not empty string. It should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done. I have simplified the validation so we check only whether the publicEndpoint is present 👍

Comment on lines +170 to +173
message,
statusCode: HttpStatus.BAD_REQUEST,
error: 'FeatureInvalid',
errorCode: CustomErrorCodes.FeatureInvalid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense to enforce that these fields are provided for all exceptions (e.g. extract and implement a common interface)? Currently it's typed as Record<string, any> and if we have a common interface, we can have more predictive api responses

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the current pattern, so I just plugged into it.

What you suggest sounds reasonable to me, but I believe @ArtemHoruzhenko can advise on architectural changes like that.

CLOUD_DATABASE_IMPORT_FORBIDDEN:
'Adding your Redis Cloud database to Redis Insight is disabled due to a setting restricting database connection management.',
CLOUD_DATABASE_ENDPOINT_INVALID:
'Database endpoint is not available or not fully provisioned yet.',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the message?
I would say that we must say something like: "Public endpoint is disabled for this database"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, we can put whatever we want.

Copy link
Member Author

@valkirilov valkirilov Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After an internal sync, we'll go with the following error message (90f362c):

Database endpoint is unavailable. It may still be provisioning or has been disabled.

publicEndpoint: string | null | undefined,
): boolean {
return (
!!publicEndpoint && !!publicEndpoint.trim() && publicEndpoint.includes(':')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say we just need to check if we have not empty string. It should be enough.

@valkirilov valkirilov merged commit cae44d7 into main Dec 1, 2025
33 checks passed
@valkirilov valkirilov deleted the be/bugfix/RI-7778/proper-error-messages-cloud-databases branch December 1, 2025 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants