Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions .ai/rules/backend.md
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,43 @@ Use appropriate exception types:
- `ConflictException` - 409
- `InternalServerErrorException` - 500

### Custom Exceptions

**Prefer custom exceptions over generic ones** when you need:

- Consistent error codes for frontend handling
- Specific error messages from constants
- Consistent error structure across the codebase

Create custom exceptions following existing patterns:

```typescript
// src/modules/feature/exceptions/feature-invalid.exception.ts
import {
HttpException,
HttpExceptionOptions,
HttpStatus,
} from '@nestjs/common';
import ERROR_MESSAGES from 'src/constants/error-messages';
import { CustomErrorCodes } from 'src/constants';

export class FeatureInvalidException extends HttpException {
constructor(
message = ERROR_MESSAGES.FEATURE_INVALID,
options?: HttpExceptionOptions,
) {
const response = {
message,
statusCode: HttpStatus.BAD_REQUEST,
error: 'FeatureInvalid',
errorCode: CustomErrorCodes.FeatureInvalid,
Comment on lines +170 to +173
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.

};

super(response, response.statusCode, options);
}
}
```

### Error Logging

```typescript
Expand Down
1 change: 1 addition & 0 deletions .ai/rules/commits.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ chore: upgrade React to version 18.2
- Atomic changes (one logical change per commit)
- Reference issue/ticket in body
- Explain **why**, not just **what**
- **Keep it concise** - Don't list every file change in the body

```bash
feat(ui): add user profile editing
Expand Down
24 changes: 24 additions & 0 deletions .ai/rules/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,30 @@ jest.mock('uiSrc/services/api', () => ({
}));
```

### Parameterized Tests with `it.each`

**Use `it.each` for multiple tests with the same body but different inputs:**

```typescript
// ✅ GOOD: Parameterized tests
it.each([
{ description: 'null', value: null },
{ description: 'undefined', value: undefined },
{ description: 'empty string', value: '' },
{ description: 'whitespace only', value: ' ' },
])('should return error when input is $description', async ({ value }) => {
const result = await service.processInput(value);
expect(result.status).toBe('error');
});
```

**Benefits:**

- DRY: Single test body shared across all cases
- Maintainability: Changes to test logic only need to be made once
- Readability: Test cases are clearly defined in a table
- Easier to extend: Adding new test cases is just adding a new row

### Test Edge Cases

Always test:
Expand Down
1 change: 1 addition & 0 deletions redisinsight/api/src/constants/custom-error-codes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export enum CustomErrorCodes {
CloudJobNotFound = 11_113,
CloudSubscriptionAlreadyExistsFree = 11_114,
CloudDatabaseImportForbidden = 11_115,
CloudDatabaseEndpointInvalid = 11_116,

// General database errors [11200, 11299]
DatabaseAlreadyExists = 11_200,
Expand Down
2 changes: 2 additions & 0 deletions redisinsight/api/src/constants/error-messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ export default {
CLOUD_DATABASE_ALREADY_EXISTS_FREE: 'Free database already exists',
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.

CLOUD_PLAN_NOT_FOUND_FREE: 'Unable to find free cloud plan',
CLOUD_SUBSCRIPTION_ALREADY_EXISTS_FREE: 'Free subscription already exists',
COMMON_DEFAULT_IMPORT_ERROR: 'Unable to import default data',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import { CloudApiUnauthorizedException } from 'src/modules/cloud/common/exceptio
import { CloudUserCapiService } from 'src/modules/cloud/user/cloud-user.capi.service';
import { CloudSubscriptionCapiService } from 'src/modules/cloud/subscription/cloud-subscription.capi.service';
import { CloudDatabaseCapiService } from 'src/modules/cloud/database/cloud-database.capi.service';
import ERROR_MESSAGES from 'src/constants/error-messages';
import { CustomErrorCodes } from 'src/constants';

describe('CloudAutodiscoveryService', () => {
let service: CloudAutodiscoveryService;
Expand Down Expand Up @@ -364,5 +366,62 @@ describe('CloudAutodiscoveryService', () => {
mockImportCloudDatabaseResponseFixed,
]);
});
it.each([
{
description: 'null',
publicEndpoint: null,
},
{
description: 'undefined',
publicEndpoint: undefined,
},
{
description: 'empty string',
publicEndpoint: '',
},
{
description: 'whitespace only',
publicEndpoint: ' ',
},
{
description: 'does not contain colon',
publicEndpoint: 'hostname',
},
])(
'should return error when publicEndpoint is $description',
async ({ publicEndpoint }) => {
cloudDatabaseCapiService.getDatabase.mockResolvedValueOnce({
...mockCloudDatabase,
publicEndpoint,
status: CloudDatabaseStatus.Active,
});

const result = await service.addRedisCloudDatabases(
mockSessionMetadata,
mockCloudCapiAuthDto,
[mockImportCloudDatabaseDto],
);

expect(result).toEqual([
{
...mockImportCloudDatabaseResponse,
status: ActionStatus.Fail,
message: ERROR_MESSAGES.CLOUD_DATABASE_ENDPOINT_INVALID,
error: {
message: ERROR_MESSAGES.CLOUD_DATABASE_ENDPOINT_INVALID,
statusCode: 400,
error: 'CloudDatabaseEndpointInvalid',
errorCode: CustomErrorCodes.CloudDatabaseEndpointInvalid,
},
databaseDetails: {
...mockCloudDatabase,
publicEndpoint,
status: CloudDatabaseStatus.Active,
},
},
]);
expect(databaseService.create).not.toHaveBeenCalled();
},
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Logger,
ServiceUnavailableException,
} from '@nestjs/common';
import { CloudDatabaseEndpointInvalidException } from 'src/modules/cloud/job/exceptions';
import { uniqBy } from 'lodash';
import ERROR_MESSAGES from 'src/constants/error-messages';
import {
Expand All @@ -29,6 +30,7 @@
CloudDatabase,
CloudDatabaseStatus,
} from 'src/modules/cloud/database/models';
import { isValidCloudDatabaseEndpoint } from 'src/modules/cloud/database/utils';
import config from 'src/utils/config';

const cloudConfig = config.get('cloud');
Expand Down Expand Up @@ -222,6 +224,16 @@
databaseDetails: database,
};
}
if (!isValidCloudDatabaseEndpoint(publicEndpoint)) {
const exception = new CloudDatabaseEndpointInvalidException();
return {
...dto,
status: ActionStatus.Fail,
message: exception.message,
error: exception?.getResponse(),

Check warning on line 233 in redisinsight/api/src/modules/cloud/autodiscovery/cloud-autodiscovery.service.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🌿 Branch is not covered

Warning! Not covered branch
databaseDetails: database,
};
}
const [host, port] = publicEndpoint.split(':');

await this.databaseService.create(sessionMetadata, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ export function convertRedisCloudModuleName(name: string): string {
return REDIS_CLOUD_MODULES_NAMES[name] ?? name;
}

export function isValidCloudDatabaseEndpoint(
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 👍

);
}

export const parseCloudDatabaseCapiResponse = (
database: ICloudCapiDatabase,
tags: ICloudCapiDatabaseTag[],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import {
HttpException,
HttpExceptionOptions,
HttpStatus,
} from '@nestjs/common';
import ERROR_MESSAGES from 'src/constants/error-messages';
import { CustomErrorCodes } from 'src/constants';

export class CloudDatabaseEndpointInvalidException extends HttpException {
constructor(
message = ERROR_MESSAGES.CLOUD_DATABASE_ENDPOINT_INVALID,
options?: HttpExceptionOptions,
) {
const response = {
message,
statusCode: HttpStatus.BAD_REQUEST,
error: 'CloudDatabaseEndpointInvalid',
errorCode: CustomErrorCodes.CloudDatabaseEndpointInvalid,
};

super(response, response.statusCode, options);
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export * from './cloud-database-already-exists-free.exception';
export * from './cloud-database-endpoint-invalid.exception';
export * from './cloud-database-import-forbidden.exception';
export * from './cloud-database-in-failed-state.exception';
export * from './cloud-database-in-unexpected-state.exception';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import { CloudJobName } from 'src/modules/cloud/job/constants';
import { CloudJobStatus, CloudJobStep } from 'src/modules/cloud/job/models';
import {
CloudDatabaseEndpointInvalidException,
CloudDatabaseImportForbiddenException,
CloudJobUnexpectedErrorException,
CloudTaskNoResourceIdException,
Expand All @@ -30,6 +31,7 @@
import { DatabaseInfoService } from 'src/modules/database/database-info.service';
import { FeatureService } from 'src/modules/feature/feature.service';
import { KnownFeatures } from 'src/modules/feature/constants';
import { isValidCloudDatabaseEndpoint } from 'src/modules/cloud/database/utils';

const cloudConfig = config.get('cloud');

Expand Down Expand Up @@ -58,86 +60,90 @@

async iteration(sessionMetadata: SessionMetadata): Promise<Database> {
let freeSubscription: CloudSubscription;
try {
this.logger.debug('Create free database');

this.checkSignal();

this.changeState({ step: CloudJobStep.Database });

this.logger.debug('Getting subscription metadata');

freeSubscription =
await this.dependencies.cloudSubscriptionCapiService.getSubscription(
this.options.capiCredentials,
this.data.subscriptionId,
CloudSubscriptionType.Fixed,
);
let cloudDatabase: CloudDatabase;

let createFreeDatabaseTask =
await this.dependencies.cloudDatabaseCapiService.createFreeDatabase(
this.options.capiCredentials,
{
subscriptionId: freeSubscription.id,
subscriptionType: freeSubscription.type,
},
);

this.checkSignal();

createFreeDatabaseTask = await this.runChildJob(
sessionMetadata,
WaitForTaskCloudJob,
{
taskId: createFreeDatabaseTask.taskId,
},
this.options,
);

const freeDatabaseId = createFreeDatabaseTask?.response?.resourceId;

if (!freeDatabaseId) {
throw new CloudTaskNoResourceIdException();
}

cloudDatabase = {
databaseId: freeDatabaseId,
} as CloudDatabase;

if (!cloudDatabase) {
throw new CloudJobUnexpectedErrorException(
'Unable to create free cloud database',
);
}

this.checkSignal();

cloudDatabase = await this.runChildJob(
sessionMetadata,
WaitForActiveDatabaseCloudJob,
{
databaseId: cloudDatabase.databaseId,
subscriptionId: this.data.subscriptionId,
subscriptionType: CloudSubscriptionType.Fixed,
},
this.options,
);

this.checkSignal();

const isDatabaseManagementEnabled =
await this.dependencies.featureService.isFeatureEnabled(
sessionMetadata,
KnownFeatures.DatabaseManagement,
);

if (!isDatabaseManagementEnabled) {
throw new CloudDatabaseImportForbiddenException();
}

const { publicEndpoint, name, password } = cloudDatabase;

if (!isValidCloudDatabaseEndpoint(publicEndpoint)) {
throw new CloudDatabaseEndpointInvalidException();

Check warning on line 144 in redisinsight/api/src/modules/cloud/job/jobs/create-free-database.cloud-job.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered

Warning! Not covered statement
}

Check warning on line 145 in redisinsight/api/src/modules/cloud/job/jobs/create-free-database.cloud-job.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered

Warning! Not covered statement

Check warning on line 145 in redisinsight/api/src/modules/cloud/job/jobs/create-free-database.cloud-job.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🌿 Branch is not covered

Warning! Not covered branch

const [host, port] = publicEndpoint.split(':');

const database = await this.dependencies.databaseService.create(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@
import { SessionMetadata } from 'src/common/models';
import { KnownFeatures } from 'src/modules/feature/constants';
import { FeatureService } from 'src/modules/feature/feature.service';
import { CloudDatabaseImportForbiddenException } from 'src/modules/cloud/job/exceptions';
import {
CloudDatabaseEndpointInvalidException,
CloudDatabaseImportForbiddenException,
} from 'src/modules/cloud/job/exceptions';
import { isValidCloudDatabaseEndpoint } from 'src/modules/cloud/database/utils';

const cloudConfig = config.get('cloud');

Expand Down Expand Up @@ -82,6 +86,10 @@

const { publicEndpoint, name, password } = cloudDatabase;

if (!isValidCloudDatabaseEndpoint(publicEndpoint)) {
throw new CloudDatabaseEndpointInvalidException();

Check warning on line 90 in redisinsight/api/src/modules/cloud/job/jobs/import-free-database.cloud-job.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered

Warning! Not covered statement
}

Check warning on line 91 in redisinsight/api/src/modules/cloud/job/jobs/import-free-database.cloud-job.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🧾 Statement is not covered

Warning! Not covered statement

Check warning on line 91 in redisinsight/api/src/modules/cloud/job/jobs/import-free-database.cloud-job.ts

View workflow job for this annotation

GitHub Actions / Coverage annotations (🧪 jest-coverage-report-action)

🌿 Branch is not covered

Warning! Not covered branch

const [host, port] = publicEndpoint.split(':');

const database = await this.dependencies.databaseService.create(
Expand Down
1 change: 1 addition & 0 deletions redisinsight/ui/src/constants/customErrorCodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export enum CustomErrorCodes {
CloudJobNotFound = 11_113,
CloudSubscriptionAlreadyExistsFree = 11_114,
CloudDatabaseImportForbidden = 11_115,
CloudDatabaseEndpointInvalid = 11_116,

// General database errors [11200, 11299]
DatabaseAlreadyExists = 11_200,
Expand Down
Loading