Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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 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,50 @@ describe('CloudAutodiscoveryService', () => {
mockImportCloudDatabaseResponseFixed,
]);
});
it.each([
{
description: 'null',
publicEndpoint: null,
},
{
description: 'undefined',
publicEndpoint: undefined,
},
])(
'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 @@ import {
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 Down Expand Up @@ -222,6 +223,16 @@ export class CloudAutodiscoveryService {
databaseDetails: database,
};
}
if (!publicEndpoint) {
const exception = new CloudDatabaseEndpointInvalidException();
return {
...dto,
status: ActionStatus.Fail,
message: exception.message,
error: exception?.getResponse(),
databaseDetails: database,
};
}
const [host, port] = publicEndpoint.split(':');

await this.databaseService.create(sessionMetadata, {
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 Down Expand Up @@ -58,86 +59,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 (!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 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)

🌿 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,10 @@
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';

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

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

const { publicEndpoint, name, password } = cloudDatabase;

if (!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 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)

🌿 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