Backend exceptions refactoring#1827
Merged
Merged
Conversation
…w exception types
…er error handling
…on across multiple use cases and controllers - Introduced ConnectionNotFoundException to handle connection not found errors more consistently. - Updated various use cases and controllers to throw ConnectionNotFoundException instead of HttpException with Messages.CONNECTION_NOT_FOUND. - Removed unnecessary imports of Messages from several files. - Ensured that the new exception provides a clear and consistent error message and HTTP status code.
…roved error handling in SaasCompanyGatewayService
… and enhance error translation
📝 WalkthroughWalkthroughIntroduces a custom exception framework with internal codes/types, integrates domain-error translation in the global filter, and replaces generic Nest exceptions across modules (auth, connections, tables, visualizations, microservices). Adds repository/domain error emits and comprehensive unit tests. ChangesDomain Exceptions Refactor and Adoption
Sequence Diagram(s)sequenceDiagram
participant UseCase
participant Repo
participant Filter
participant Client
UseCase->>Repo: findAndDecryptConnection()
Repo-->>UseCase: throw MasterPasswordMissingError
UseCase-->>Filter: exception bubbles
Filter->>Filter: translateDomainError(...)
Filter-->>Client: 400 with {type: 'no_master_key', internalCode}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors backend exception handling to standardize error responses (status + internalCode + type + originalMessage) and decouple some lower-layer errors from NestJS HTTP exceptions, while adding unit tests around the new exception behaviors.
Changes:
- Introduces a
BaseRocketAdminExceptionand multiple new typed custom exceptions with internal codes/types. - Adds domain-layer master password errors plus a translator used by
AllExceptionsFilter. - Updates many use-cases/guards/gateways to throw the new custom exceptions; extends exception-message categorization with code/type metadata and adds AVA unit tests.
Reviewed changes
Copilot reviewed 101 out of 101 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/test/ava-tests/unit-tests/domain-error-translation.test.ts | New unit tests for domain-error → HttpException translation behavior. |
| backend/test/ava-tests/unit-tests/custom-exceptions.test.ts | New unit tests covering new/updated custom exception classes. |
| backend/test/ava-tests/unit-tests/categorize-exception-message.test.ts | New unit tests for exception-message categorization and message processing. |
| backend/src/microservices/saas-microservice/use-cases/update-hosted-connection-password.use.case.ts | Uses ConnectionNotFoundException for hosted-connection lookup failures. |
| backend/src/microservices/saas-microservice/use-cases/get-hosted-connection-credentials.use.case.ts | Uses ConnectionNotFoundException instead of generic Nest exceptions. |
| backend/src/microservices/saas-microservice/use-cases/delete-connection-for-hosted-db.use.case.ts | Uses ConnectionNotFoundException for missing hosted DB connection. |
| backend/src/microservices/gateways/saas-gateway.ts/saas-company-gateway.service.ts | Replaces raw HttpException with ExternalServiceException for upstream SaaS errors. |
| backend/src/guards/tables-receive.guard.ts | Throws ConnectionNotFoundException for missing connection in guard logic. |
| backend/src/exceptions/utils/process-exception-message.ts | Adds categorizeExceptionMessage returning message + internalCode + type. |
| backend/src/exceptions/domain-errors/translate-domain-error.ts | New translator from domain Errors to HTTP-layer exceptions. |
| backend/src/exceptions/domain-errors/master-password.errors.ts | New domain error classes for master password missing/incorrect. |
| backend/src/exceptions/custom-exceptions/validation-exception.ts | Migrates to BaseRocketAdminException and sets internal code. |
| backend/src/exceptions/custom-exceptions/unknown-sql-exception.ts | Uses categorization + internalCode/type and preserves originalMessage in base exception options. |
| backend/src/exceptions/custom-exceptions/two-fa-required-exception.ts | New typed exception for 2FA-required condition with internal code. |
| backend/src/exceptions/custom-exceptions/table-not-found-exception.ts | New typed exception for table-not-found with internal code. |
| backend/src/exceptions/custom-exceptions/primary-key-missing-exception.ts | New typed exception for missing PK with internal code. |
| backend/src/exceptions/custom-exceptions/non-available-in-free-plan-exception.ts | Migrates to BaseRocketAdminException and sets billing-related internal code. |
| backend/src/exceptions/custom-exceptions/master-password-missing-exception.ts | New typed exception for missing master password with internal code/type. |
| backend/src/exceptions/custom-exceptions/master-password-incorrect-exception.ts | New typed exception for incorrect master password with internal code/type. |
| backend/src/exceptions/custom-exceptions/external-service-exception.ts | New typed exception for upstream dependency failures with internal code + originalMessage. |
| backend/src/exceptions/custom-exceptions/exception-type.ts | Defines the shared ExceptionType union for backend error typing. |
| backend/src/exceptions/custom-exceptions/delete-row-exception.ts | Migrates to BaseRocketAdminException and uses categorization for FK vs unknown SQL errors. |
| backend/src/exceptions/custom-exceptions/custom-exceptions-internal-codes/exceptions-internal-codes.ts | Expands and documents internal exception codes and ranges. |
| backend/src/exceptions/custom-exceptions/connection-not-found-exception.ts | New typed exception for connection-not-found with internal code. |
| backend/src/exceptions/custom-exceptions/base-rocketadmin.exception.ts | Adds a base exception class carrying internalCode, type, and originalMessage. |
| backend/src/exceptions/all-exceptions.filter.ts | Integrates domain-error translation and supports extracting type from new exceptions. |
| backend/src/entities/widget/use-cases/find-table-widgets.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/widget/use-cases/create-update-delete-table-widgets.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel/use-cases/update-panel.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel/use-cases/test-db-query.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel/use-cases/find-panel.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel/use-cases/find-all-panels.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel/use-cases/execute-panel.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel/use-cases/delete-panel.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel/use-cases/create-panel.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel-position/use-cases/update-panel-position.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel-position/use-cases/generate-table-dashboard-with-ai.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel-position/use-cases/generate-panel-position-with-ai.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel-position/use-cases/delete-panel-position.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/panel-position/use-cases/create-panel-position.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/dashboard/use-cases/update-dashboard.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/dashboard/use-cases/find-dashboard.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/dashboard/use-cases/find-all-dashboards.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/dashboard/use-cases/delete-dashboard.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/visualizations/dashboard/use-cases/create-dashboard.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table/utils/validate-connection.util.ts | Uses ConnectionNotFoundException for missing connection and keeps frozen-plan exception path. |
| backend/src/entities/table/use-cases/get-table-structure.use.case.ts | Uses TableNotFoundException for missing tables. |
| backend/src/entities/table/use-cases/get-table-structure-without-cache.use.case.ts | Uses TableNotFoundException for missing tables. |
| backend/src/entities/table/use-cases/get-table-rows.use.case.ts | Uses TableNotFoundException for missing tables. |
| backend/src/entities/table/use-cases/get-row-by-primary-key.use.case.ts | Uses PrimaryKeyMissingException for missing primary key. |
| backend/src/entities/table/use-cases/find-tables-in-connection.use.case.ts | Uses master-password custom exceptions + ConnectionNotFoundException. |
| backend/src/entities/table/use-cases/find-tables-in-connection-v2.use.case.ts | Uses master-password custom exceptions + ConnectionNotFoundException. |
| backend/src/entities/table/use-cases/delete-rows-from-table.use.case.ts | Uses PrimaryKeyMissingException for missing primary keys. |
| backend/src/entities/table/use-cases/delete-row-from-table.use.case.ts | Uses PrimaryKeyMissingException for missing primary key. |
| backend/src/entities/table/use-cases/bulk-update-rows-in-table.use.case.ts | Uses PrimaryKeyMissingException for missing primary keys. |
| backend/src/entities/table/use-cases/add-row-in-table.use.case.ts | Uses TableNotFoundException for missing tables. |
| backend/src/entities/table/table.controller.ts | Uses ConnectionNotFoundException in controller-level connection checks. |
| backend/src/entities/table-settings/personal-table-settings/use-cases/find-personal-table-settings.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-settings/personal-table-settings/use-cases/create-update-personal-table-settings.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-settings/common-table-settings/use-cases/update-table-settings.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-settings/common-table-settings/use-cases/find-table-settings.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-settings/common-table-settings/use-cases/create-table-settings.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-schema/use-cases/rollback-schema-change.use-case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-schema/use-cases/rollback-batch-schema-changes.use-case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-schema/use-cases/generate-schema-change.use-case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-schema/use-cases/approve-batch-schema-changes.use-case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-schema/use-cases/approve-and-apply-schema-change.use-case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-filters/use-cases/update-table-filters.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-filters/use-cases/create-table-filters.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-categories/use-cases/find-table-categories-with-tables.use.case.ts | Uses master-password custom exceptions + ConnectionNotFoundException. |
| backend/src/entities/table-categories/use-cases/create-or-update-table-categories.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-actions/table-actions-module/table-action-activation.service.ts | Uses ConnectionNotFoundException for missing signing key/connection context. |
| backend/src/entities/table-actions/table-action-rules-module/use-cases/update-action-rule-with-actions-and-events.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-actions/table-action-rules-module/use-cases/create-action-rule.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/table-actions/table-action-rules-module/use-cases/activate-actions-in-rule.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/s3-widget/use-cases/get-s3-file-url.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/group/use-cases/update-group-title.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/group/use-cases/saas-add-user-in-group-v2.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/custom-field/use-cases/update-custom-field.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/custom-field/use-cases/create-custom-fields.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/use-cases/validate-connection-master-password.use.case.ts | Uses ConnectionNotFoundException in validation flow. |
| backend/src/entities/connection/use-cases/update-connection.use.case.ts | Uses ConnectionNotFoundException + MasterPasswordIncorrectException. |
| backend/src/entities/connection/use-cases/update-connection-title.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/use-cases/update-connection-master-password.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/use-cases/unfreeze-connection.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/use-cases/restore-connection-use.case.ts | Uses ConnectionNotFoundException for missing connections. |
| backend/src/entities/connection/use-cases/preview-connection-diagram.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/use-cases/get-user-permissions-for-group-in-connection.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/use-cases/get-connection-diagram.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/use-cases/find-one-connection.use.case.ts | Uses ConnectionNotFoundException + master-password custom exceptions. |
| backend/src/entities/connection/use-cases/delete-connection.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/use-cases/create-group-in-connection.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection/repository/custom-connection-repository-extension.ts | Throws domain master-password errors instead of string-based Errors. |
| backend/src/entities/connection-properties/use-cases/update-connection-properties.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/connection-properties/use-cases/create-connection-properties.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/cedar-authorization/cedar-permissions.service.ts | Uses ConnectionNotFoundException for missing group connection ID. |
| backend/src/entities/ai/use-cases/request-info-from-table-with-ai-v7.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/entities/ai/use-cases/request-ai-settings-and-widgets-creation.use.case.ts | Uses ConnectionNotFoundException for missing connection. |
| backend/src/authorization/auth.middleware.ts | Throws TwoFaRequiredException instead of generic bad request. |
| backend/src/authorization/auth-with-api.middleware.ts | Throws TwoFaRequiredException instead of generic bad request. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+24
to
+28
| DB_PERMISSION_DENIED = 1002, | ||
| /** Could not reach the database within the timeout window. */ | ||
| CONNECTION_TIMEOUT = 1003, | ||
| /** Database host could not be resolved or is unreachable (ENOTFOUND, ECONNREFUSED, etc.). */ | ||
| HOST_UNREACHABLE = 1004, |
Comment on lines
22
to
+26
| case msgInLowerCase.includes(PROCESSING_MESSAGES_FIND.ETIMEDOUT): | ||
| return { | ||
| message: PROCESSING_MESSAGES_REPLACE.getaddrinfo_ENOTFOUND, | ||
| internalCode: ExceptionsInternalCodes.CONNECTION_TIMEOUT, | ||
| type: 'query_timeout', |
Comment on lines
+29
to
33
| throw new ExternalServiceException( | ||
| Messages.SAAS_DELETE_COMPANY_FAILED_UNHANDLED_ERROR, | ||
| result.status, | ||
| result?.body?.message ? (result.body.message as string) : undefined, | ||
| ); |
Comment on lines
+49
to
53
| throw new ExternalServiceException( | ||
| Messages.SAAS_GET_COMPANY_ID_BY_CUSTOM_DOMAIN_FAILED_UNHANDLED_ERROR, | ||
| result.status, | ||
| result?.body?.message ? (result.body.message as string) : undefined, | ||
| ); |
Comment on lines
+70
to
74
| throw new ExternalServiceException( | ||
| Messages.SAAS_GET_COMPANY_CUSTOM_DOMAIN_BY_ID_FAILED_UNHANDLED_ERROR, | ||
| result.status, | ||
| result?.body?.message ? (result.body.message as string) : undefined, | ||
| ); |
Comment on lines
+91
to
95
| throw new ExternalServiceException( | ||
| Messages.SAAS_RECOUNT_USERS_IN_COMPANY_FAILED_UNHANDLED_ERROR, | ||
| result.status, | ||
| result?.body?.message ? (result.body.message as string) : undefined, | ||
| ); |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/exceptions/custom-exceptions/unknown-sql-exception.ts (1)
33-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAgent “no data” detection is too strict and can miss valid cases.
Exact string equality is brittle (prefix/suffix/casing differences), which can downgrade this path to
UNKNOWN_SQL_EXCEPTIONinstead ofAGENT_NO_DATA.Suggested fix
+const AGENT_NO_DATA_MARKER = 'no data returned from agent'; + function isAgentNoDataError(originalMessage: string): boolean { - return originalMessage === 'No data returned from agent'; + return originalMessage.toLowerCase().includes(AGENT_NO_DATA_MARKER); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/exceptions/custom-exceptions/unknown-sql-exception.ts` around lines 33 - 35, The isAgentNoDataError function currently does exact string equality which is brittle; update isAgentNoDataError to perform a resilient, case-insensitive match (e.g., trim the message and check for a substring or regex like /no\s+data\s+returned/i or startWith/contains checks) so variants such as extra whitespace, different casing, prefixes/suffixes, or minor phrasing still map to AGENT_NO_DATA; keep the function name and return boolean contract intact and use the normalized message for matching.
🧹 Nitpick comments (4)
backend/src/microservices/gateways/saas-gateway.ts/saas-company-gateway.service.ts (1)
29-33: ⚡ Quick winDeduplicate upstream error translation into a private helper.
The same
result.status > 299+ExternalServiceExceptionblock is repeated in four methods; extracting it will reduce drift risk and keep message extraction logic consistent.Also applies to: 49-53, 70-74, 91-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/microservices/gateways/saas-gateway.ts/saas-company-gateway.service.ts` around lines 29 - 33, Extract the repeated "result.status > 299" check and ExternalServiceException throw into a private helper (e.g., private handleUpstreamError(result: HttpResponse, message: string): void) that performs the status check and throws new ExternalServiceException(message, result.status, result?.body?.message ? (result.body.message as string) : undefined); then replace the four duplicated blocks with calls to handleUpstreamError(result, Messages.SAAS_DELETE_COMPANY_FAILED_UNHANDLED_ERROR) (or the appropriate Messages.* constant used in each method). Ensure the helper lives in the same class so callers like the methods currently throwing ExternalServiceException invoke it and no other behavior changes.backend/test/ava-tests/unit-tests/categorize-exception-message.test.ts (2)
96-96: 💤 Low valueConsider strengthening the message assertion.
The assertion
t.truthy(result.message)accepts any truthy value. Consider usingt.is(typeof result.message, 'string')andt.true(result.message.length > 0)to ensure it's a non-empty string, similar to line 117.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/unit-tests/categorize-exception-message.test.ts` at line 96, Replace the weak truthy assertion for result.message in the test with explicit checks that it is a non-empty string: instead of t.truthy(result.message, 'a user-facing message should be present') use assertions like t.is(typeof result.message, 'string') and t.true(result.message.length > 0) (same style as the check used later on line 117) to ensure result.message is a populated string in the test for categorize-exception-message.
24-27: ⚡ Quick winClarify the comment-to-type mismatch.
The comment says "plain timeout -> connection timeout" but the type is
'query_timeout'. Either the comment should say "query timeout" or there should be clarification about why a "connection timeout" maps to a "query_timeout" type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/unit-tests/categorize-exception-message.test.ts` around lines 24 - 27, The test comment and the asserted type disagree: the comment says "connection timeout" but the test sets type 'query_timeout' while using ExceptionsInternalCodes.CONNECTION_TIMEOUT; pick one consistent intent and update the test accordingly—either change the comment to "plain timeout -> query timeout" to match type 'query_timeout' (keeping ExceptionsInternalCodes.CONNECTION_TIMEOUT), or change the asserted type to 'connection_timeout' to match the comment and the constant; update the test case surrounding the raw value, the type string, and the inline comment next to the test name to reflect the chosen, consistent mapping.backend/src/exceptions/custom-exceptions/unknown-sql-exception.ts (1)
18-22: ⚡ Quick winConvert local helper declarations to arrow functions for consistency.
resolveSqlExceptionandisAgentNoDataErrorshould follow the project’s arrow-function convention.As per coding guidelines, "Prefer arrow functions over function declarations".
Also applies to: 33-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/exceptions/custom-exceptions/unknown-sql-exception.ts` around lines 18 - 22, The file uses function declarations; convert resolveSqlException and isAgentNoDataError into const arrow-function declarations to match project style: replace "function resolveSqlException(...){...}" with "const resolveSqlException = (originalMessage: string): { message: string; internalCode: ExceptionsInternalCodes; type?: ExceptionType } => { ... }" and similarly convert isAgentNoDataError to "const isAgentNoDataError = (msg: string) => boolean", preserving all type annotations, returned shapes, and any exports (e.g., export them as before or export const) and ensure any internal references remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@backend/src/entities/connection/use-cases/validate-connection-master-password.use.case.ts`:
- Line 33: The code throws ConnectionNotFoundException with
HttpStatus.INTERNAL_SERVER_ERROR for a missing connection; change the thrown
status to a client/resource error such as HttpStatus.NOT_FOUND (or BAD_REQUEST
if you prefer treating it as invalid input). Locate the throw site where
ConnectionNotFoundException is constructed (in
validate-connection-master-password.use.case, likely inside
ValidateConnectionMasterPasswordUseCase) and replace INTERNAL_SERVER_ERROR with
HttpStatus.NOT_FOUND to align with not-found semantics.
In
`@backend/src/entities/table-actions/table-actions-module/table-action-activation.service.ts`:
- Around line 198-200: Replace the misleading ConnectionNotFoundException thrown
when a connection exists but lacks a signing key: in the code that checks
foundConnection.signing_key (in TableActionActivationService or the method where
foundConnection is validated) stop throwing ConnectionNotFoundException and
throw a signing-key-specific error (e.g., SigningKeyMissingException or
ConnectionSigningKeyMissingException) with the same HttpStatus.BAD_REQUEST (or
appropriate status); if the new exception class does not exist, add it mirroring
existing exception patterns and use it where the signing key is absent to ensure
correct error typing and handling.
In
`@backend/src/entities/table-categories/use-cases/find-table-categories-with-tables.use.case.ts`:
- Around line 44-53: The catch block that inspects getErrorMessage(error) should
re-throw any unknown errors instead of letting them fall through and later
causing a ConnectionNotFoundException; update the error handling in the
find-table-categories-with-tables use-case so that after checking for
Messages.MASTER_PASSWORD_MISSING and Messages.MASTER_PASSWORD_INCORRECT
(throwing MasterPasswordMissingException or MasterPasswordIncorrectException
respectively), any other caught error is re-thrown (throw error), and only if no
error occurred and connection is falsy should you throw
ConnectionNotFoundException(HttpStatus.BAD_REQUEST); reference the existing
getErrorMessage, Messages, MasterPasswordMissingException,
MasterPasswordIncorrectException, and ConnectionNotFoundException symbols to
locate and apply this change.
In `@backend/src/entities/table/use-cases/delete-rows-from-table.use.case.ts`:
- Around line 49-50: The current guard only rejects null/undefined primaryKeys
but allows an empty array, causing no-op "success" responses; update the check
in the delete-rows-from-table use case to also reject empty arrays by throwing
PrimaryKeyMissingException when primaryKeys is falsy or an array with length 0
(e.g., replace the existing if (!primaryKeys) throw new
PrimaryKeyMissingException() with a condition like if (!primaryKeys ||
(Array.isArray(primaryKeys) && primaryKeys.length === 0)) throw new
PrimaryKeyMissingException()).
In
`@backend/src/entities/table/use-cases/find-tables-in-connection-v2.use.case.ts`:
- Around line 50-60: The current catch around findAndDecryptConnection swallows
all errors and only maps password-related messages to
MasterPasswordMissingException or MasterPasswordIncorrectException, causing
non-password failures to be ignored and later misreported as
ConnectionNotFoundException; update the catch in
find-tables-in-connection-v2.use.case.ts so that after checking
getErrorMessage(error) against Messages.MASTER_PASSWORD_MISSING and
Messages.MASTER_PASSWORD_INCORRECT and throwing MasterPasswordMissingException
or MasterPasswordIncorrectException respectively, any other error is rethrown
(or propagated) instead of being dropped; ensure
findAndDecryptConnection-related unexpected errors bubble up while leaving the
existing ConnectionNotFoundException logic for when connection is genuinely
null.
In `@backend/src/entities/table/use-cases/find-tables-in-connection.use.case.ts`:
- Around line 51-61: In find-tables-in-connection.use.case.ts inside the
find-tables-in-connection use case, the catch block only handles
MASTER_PASSWORD_MISSING and MASTER_PASSWORD_INCORRECT and then lets execution
fall through to throw ConnectionNotFoundException; modify the catch so that
after mapping those two known messages (using getErrorMessage and Messages) you
rethrow the original error for any other case instead of allowing the code to
continue—i.e., keep the checks that throw MasterPasswordMissingException and
MasterPasswordIncorrectException, and add a default rethrow of the caught error
(preserving the original exception) so unexpected repository/decryption failures
are propagated rather than remapped to ConnectionNotFoundException.
In
`@backend/src/exceptions/custom-exceptions/custom-exceptions-internal-codes/exceptions-internal-codes.ts`:
- Around line 35-36: Add a new validation internal code value in the enum for
the documented validation range (e.g., VALIDATOR_EXCEPTION_RANGE = 1100) and
update all throw sites that currently use VALIDATOR_EXCEPTION to use the new
code; keep the existing VALIDATOR_EXCEPTION = 1007 as a legacy alias for
backward compatibility so older consumers still see 1007, but move
runtime/validation error emissions to VALIDATOR_EXCEPTION_RANGE (or a clearly
named constant like VALIDATION_ERROR_CODE) and update any tests or mappings that
branch on the 1100–1199 range accordingly.
In `@backend/src/exceptions/domain-errors/translate-domain-error.ts`:
- Around line 6-14: Replace the function declaration with an exported arrow
function: change `export function translateDomainError(exception: unknown):
HttpException | null { ... }` to `export const translateDomainError =
(exception: unknown): HttpException | null => { ... }`, keeping the existing
body and return logic that checks `MasterPasswordMissingError` ->
`MasterPasswordMissingException` and `MasterPasswordIncorrectError` ->
`MasterPasswordIncorrectException`.
In `@backend/src/exceptions/utils/process-exception-message.ts`:
- Around line 12-13: Convert the two exported function declarations into
arrow-function exports: replace "export function
categorizeExceptionMessage(...)" with "export const categorizeExceptionMessage =
(...) => { ... }" and similarly replace "export function
processExceptionMessage(...)" with "export const processExceptionMessage = (...)
=> { ... }"; keep the existing parameter types, return types
(CategorizedExceptionMessage), and internal logic exactly the same so behavior
doesn't change, and ensure any internal references still work after changing
from hoisted declarations to const arrow functions.
---
Outside diff comments:
In `@backend/src/exceptions/custom-exceptions/unknown-sql-exception.ts`:
- Around line 33-35: The isAgentNoDataError function currently does exact string
equality which is brittle; update isAgentNoDataError to perform a resilient,
case-insensitive match (e.g., trim the message and check for a substring or
regex like /no\s+data\s+returned/i or startWith/contains checks) so variants
such as extra whitespace, different casing, prefixes/suffixes, or minor phrasing
still map to AGENT_NO_DATA; keep the function name and return boolean contract
intact and use the normalized message for matching.
---
Nitpick comments:
In `@backend/src/exceptions/custom-exceptions/unknown-sql-exception.ts`:
- Around line 18-22: The file uses function declarations; convert
resolveSqlException and isAgentNoDataError into const arrow-function
declarations to match project style: replace "function
resolveSqlException(...){...}" with "const resolveSqlException =
(originalMessage: string): { message: string; internalCode:
ExceptionsInternalCodes; type?: ExceptionType } => { ... }" and similarly
convert isAgentNoDataError to "const isAgentNoDataError = (msg: string) =>
boolean", preserving all type annotations, returned shapes, and any exports
(e.g., export them as before or export const) and ensure any internal references
remain unchanged.
In
`@backend/src/microservices/gateways/saas-gateway.ts/saas-company-gateway.service.ts`:
- Around line 29-33: Extract the repeated "result.status > 299" check and
ExternalServiceException throw into a private helper (e.g., private
handleUpstreamError(result: HttpResponse, message: string): void) that performs
the status check and throws new ExternalServiceException(message, result.status,
result?.body?.message ? (result.body.message as string) : undefined); then
replace the four duplicated blocks with calls to handleUpstreamError(result,
Messages.SAAS_DELETE_COMPANY_FAILED_UNHANDLED_ERROR) (or the appropriate
Messages.* constant used in each method). Ensure the helper lives in the same
class so callers like the methods currently throwing ExternalServiceException
invoke it and no other behavior changes.
In `@backend/test/ava-tests/unit-tests/categorize-exception-message.test.ts`:
- Line 96: Replace the weak truthy assertion for result.message in the test with
explicit checks that it is a non-empty string: instead of
t.truthy(result.message, 'a user-facing message should be present') use
assertions like t.is(typeof result.message, 'string') and
t.true(result.message.length > 0) (same style as the check used later on line
117) to ensure result.message is a populated string in the test for
categorize-exception-message.
- Around line 24-27: The test comment and the asserted type disagree: the
comment says "connection timeout" but the test sets type 'query_timeout' while
using ExceptionsInternalCodes.CONNECTION_TIMEOUT; pick one consistent intent and
update the test accordingly—either change the comment to "plain timeout -> query
timeout" to match type 'query_timeout' (keeping
ExceptionsInternalCodes.CONNECTION_TIMEOUT), or change the asserted type to
'connection_timeout' to match the comment and the constant; update the test case
surrounding the raw value, the type string, and the inline comment next to the
test name to reflect the chosen, consistent mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a52204c-0565-454b-827e-831da5349127
📒 Files selected for processing (101)
backend/src/authorization/auth-with-api.middleware.tsbackend/src/authorization/auth.middleware.tsbackend/src/entities/ai/use-cases/request-ai-settings-and-widgets-creation.use.case.tsbackend/src/entities/ai/use-cases/request-info-from-table-with-ai-v7.use.case.tsbackend/src/entities/cedar-authorization/cedar-permissions.service.tsbackend/src/entities/connection-properties/use-cases/create-connection-properties.use.case.tsbackend/src/entities/connection-properties/use-cases/update-connection-properties.use.case.tsbackend/src/entities/connection/repository/custom-connection-repository-extension.tsbackend/src/entities/connection/use-cases/create-group-in-connection.use.case.tsbackend/src/entities/connection/use-cases/delete-connection.use.case.tsbackend/src/entities/connection/use-cases/find-one-connection.use.case.tsbackend/src/entities/connection/use-cases/get-connection-diagram.use.case.tsbackend/src/entities/connection/use-cases/get-permissions-for-group-in-connection.use.case.tsbackend/src/entities/connection/use-cases/get-user-permissions-for-group-in-connection.use.case.tsbackend/src/entities/connection/use-cases/preview-connection-diagram.use.case.tsbackend/src/entities/connection/use-cases/restore-connection-use.case.tsbackend/src/entities/connection/use-cases/unfreeze-connection.use.case.tsbackend/src/entities/connection/use-cases/update-connection-master-password.use.case.tsbackend/src/entities/connection/use-cases/update-connection-title.use.case.tsbackend/src/entities/connection/use-cases/update-connection.use.case.tsbackend/src/entities/connection/use-cases/validate-connection-master-password.use.case.tsbackend/src/entities/custom-field/use-cases/create-custom-fields.use.case.tsbackend/src/entities/custom-field/use-cases/update-custom-field.use.case.tsbackend/src/entities/group/use-cases/saas-add-user-in-group-v2.use.case.tsbackend/src/entities/group/use-cases/update-group-title.use.case.tsbackend/src/entities/s3-widget/use-cases/get-s3-file-url.use.case.tsbackend/src/entities/table-actions/table-action-rules-module/use-cases/activate-actions-in-rule.use.case.tsbackend/src/entities/table-actions/table-action-rules-module/use-cases/create-action-rule.use.case.tsbackend/src/entities/table-actions/table-action-rules-module/use-cases/update-action-rule-with-actions-and-events.use.case.tsbackend/src/entities/table-actions/table-actions-module/table-action-activation.service.tsbackend/src/entities/table-categories/use-cases/create-or-update-table-categories.use.case.tsbackend/src/entities/table-categories/use-cases/find-table-categories-with-tables.use.case.tsbackend/src/entities/table-filters/use-cases/create-table-filters.use.case.tsbackend/src/entities/table-filters/use-cases/update-table-filters.use.case.tsbackend/src/entities/table-schema/use-cases/approve-and-apply-schema-change.use-case.tsbackend/src/entities/table-schema/use-cases/approve-batch-schema-changes.use-case.tsbackend/src/entities/table-schema/use-cases/generate-schema-change.use-case.tsbackend/src/entities/table-schema/use-cases/rollback-batch-schema-changes.use-case.tsbackend/src/entities/table-schema/use-cases/rollback-schema-change.use-case.tsbackend/src/entities/table-settings/common-table-settings/use-cases/create-table-settings.use.case.tsbackend/src/entities/table-settings/common-table-settings/use-cases/find-table-settings.use.case.tsbackend/src/entities/table-settings/common-table-settings/use-cases/update-table-settings.use.case.tsbackend/src/entities/table-settings/personal-table-settings/use-cases/create-update-personal-table-settings.use.case.tsbackend/src/entities/table-settings/personal-table-settings/use-cases/find-personal-table-settings.use.case.tsbackend/src/entities/table/table.controller.tsbackend/src/entities/table/use-cases/add-row-in-table.use.case.tsbackend/src/entities/table/use-cases/bulk-update-rows-in-table.use.case.tsbackend/src/entities/table/use-cases/delete-row-from-table.use.case.tsbackend/src/entities/table/use-cases/delete-rows-from-table.use.case.tsbackend/src/entities/table/use-cases/find-tables-in-connection-v2.use.case.tsbackend/src/entities/table/use-cases/find-tables-in-connection.use.case.tsbackend/src/entities/table/use-cases/get-row-by-primary-key.use.case.tsbackend/src/entities/table/use-cases/get-table-rows.use.case.tsbackend/src/entities/table/use-cases/get-table-structure-without-cache.use.case.tsbackend/src/entities/table/use-cases/get-table-structure.use.case.tsbackend/src/entities/table/utils/validate-connection.util.tsbackend/src/entities/visualizations/dashboard/use-cases/create-dashboard.use.case.tsbackend/src/entities/visualizations/dashboard/use-cases/delete-dashboard.use.case.tsbackend/src/entities/visualizations/dashboard/use-cases/find-all-dashboards.use.case.tsbackend/src/entities/visualizations/dashboard/use-cases/find-dashboard.use.case.tsbackend/src/entities/visualizations/dashboard/use-cases/update-dashboard.use.case.tsbackend/src/entities/visualizations/panel-position/use-cases/create-panel-position.use.case.tsbackend/src/entities/visualizations/panel-position/use-cases/delete-panel-position.use.case.tsbackend/src/entities/visualizations/panel-position/use-cases/generate-panel-position-with-ai.use.case.tsbackend/src/entities/visualizations/panel-position/use-cases/generate-table-dashboard-with-ai.use.case.tsbackend/src/entities/visualizations/panel-position/use-cases/update-panel-position.use.case.tsbackend/src/entities/visualizations/panel/use-cases/create-panel.use.case.tsbackend/src/entities/visualizations/panel/use-cases/delete-panel.use.case.tsbackend/src/entities/visualizations/panel/use-cases/execute-panel.use.case.tsbackend/src/entities/visualizations/panel/use-cases/find-all-panels.use.case.tsbackend/src/entities/visualizations/panel/use-cases/find-panel.use.case.tsbackend/src/entities/visualizations/panel/use-cases/test-db-query.use.case.tsbackend/src/entities/visualizations/panel/use-cases/update-panel.use.case.tsbackend/src/entities/widget/use-cases/create-update-delete-table-widgets.use.case.tsbackend/src/entities/widget/use-cases/find-table-widgets.use.case.tsbackend/src/exceptions/all-exceptions.filter.tsbackend/src/exceptions/custom-exceptions/base-rocketadmin.exception.tsbackend/src/exceptions/custom-exceptions/connection-not-found-exception.tsbackend/src/exceptions/custom-exceptions/custom-exceptions-internal-codes/exceptions-internal-codes.tsbackend/src/exceptions/custom-exceptions/delete-row-exception.tsbackend/src/exceptions/custom-exceptions/exception-type.tsbackend/src/exceptions/custom-exceptions/external-service-exception.tsbackend/src/exceptions/custom-exceptions/master-password-incorrect-exception.tsbackend/src/exceptions/custom-exceptions/master-password-missing-exception.tsbackend/src/exceptions/custom-exceptions/non-available-in-free-plan-exception.tsbackend/src/exceptions/custom-exceptions/primary-key-missing-exception.tsbackend/src/exceptions/custom-exceptions/table-not-found-exception.tsbackend/src/exceptions/custom-exceptions/two-fa-required-exception.tsbackend/src/exceptions/custom-exceptions/unknown-sql-exception.tsbackend/src/exceptions/custom-exceptions/validation-exception.tsbackend/src/exceptions/domain-errors/master-password.errors.tsbackend/src/exceptions/domain-errors/translate-domain-error.tsbackend/src/exceptions/utils/process-exception-message.tsbackend/src/guards/tables-receive.guard.tsbackend/src/microservices/gateways/saas-gateway.ts/saas-company-gateway.service.tsbackend/src/microservices/saas-microservice/use-cases/delete-connection-for-hosted-db.use.case.tsbackend/src/microservices/saas-microservice/use-cases/get-hosted-connection-credentials.use.case.tsbackend/src/microservices/saas-microservice/use-cases/update-hosted-connection-password.use.case.tsbackend/test/ava-tests/unit-tests/categorize-exception-message.test.tsbackend/test/ava-tests/unit-tests/custom-exceptions.test.tsbackend/test/ava-tests/unit-tests/domain-error-translation.test.ts
|
|
||
| if (!connection) { | ||
| throw new InternalServerErrorException(Messages.CONNECTION_NOT_FOUND); | ||
| throw new ConnectionNotFoundException(HttpStatus.INTERNAL_SERVER_ERROR); |
There was a problem hiding this comment.
Use a client/resource error status for missing connection.
Line 33 returns HttpStatus.INTERNAL_SERVER_ERROR for a not-found condition. This misclassifies the failure and can trigger incorrect retry/error handling paths. Align this with your not-found contract (typically HttpStatus.NOT_FOUND, or BAD_REQUEST if intentionally treated as invalid input) instead of 500.
Suggested fix
- throw new ConnectionNotFoundException(HttpStatus.INTERNAL_SERVER_ERROR);
+ throw new ConnectionNotFoundException(HttpStatus.NOT_FOUND);📝 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.
Suggested change
| throw new ConnectionNotFoundException(HttpStatus.INTERNAL_SERVER_ERROR); | |
| throw new ConnectionNotFoundException(HttpStatus.NOT_FOUND); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/connection/use-cases/validate-connection-master-password.use.case.ts`
at line 33, The code throws ConnectionNotFoundException with
HttpStatus.INTERNAL_SERVER_ERROR for a missing connection; change the thrown
status to a client/resource error such as HttpStatus.NOT_FOUND (or BAD_REQUEST
if you prefer treating it as invalid input). Locate the throw site where
ConnectionNotFoundException is constructed (in
validate-connection-master-password.use.case, likely inside
ValidateConnectionMasterPasswordUseCase) and replace INTERNAL_SERVER_ERROR with
HttpStatus.NOT_FOUND to align with not-found semantics.
Comment on lines
198
to
200
| if (!foundConnection.signing_key) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.CONNECTION_NOT_FOUND, | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new ConnectionNotFoundException(HttpStatus.BAD_REQUEST); | ||
| } |
There was a problem hiding this comment.
Use a signing-key-specific exception instead of ConnectionNotFoundException.
Line 198 checks for missing signing_key, but Line 199 throws a connection-not-found exception. In this refactor, that emits the wrong typed error metadata and can misroute client handling and monitoring.
Proposed fix
if (!foundConnection.signing_key) {
- throw new ConnectionNotFoundException(HttpStatus.BAD_REQUEST);
+ throw new HttpException(
+ {
+ message: 'Connection signing key is missing',
+ },
+ HttpStatus.INTERNAL_SERVER_ERROR,
+ );
}📝 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.
Suggested change
| if (!foundConnection.signing_key) { | |
| throw new HttpException( | |
| { | |
| message: Messages.CONNECTION_NOT_FOUND, | |
| }, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| throw new ConnectionNotFoundException(HttpStatus.BAD_REQUEST); | |
| } | |
| if (!foundConnection.signing_key) { | |
| throw new HttpException( | |
| { | |
| message: 'Connection signing key is missing', | |
| }, | |
| HttpStatus.INTERNAL_SERVER_ERROR, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/table-actions/table-actions-module/table-action-activation.service.ts`
around lines 198 - 200, Replace the misleading ConnectionNotFoundException
thrown when a connection exists but lacks a signing key: in the code that checks
foundConnection.signing_key (in TableActionActivationService or the method where
foundConnection is validated) stop throwing ConnectionNotFoundException and
throw a signing-key-specific error (e.g., SigningKeyMissingException or
ConnectionSigningKeyMissingException) with the same HttpStatus.BAD_REQUEST (or
appropriate status); if the new exception class does not exist, add it mirroring
existing exception patterns and use it where the signing key is absent to ensure
correct error typing and handling.
Comment on lines
44
to
+53
| const errMessage = getErrorMessage(error); | ||
| if (errMessage === Messages.MASTER_PASSWORD_MISSING) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.MASTER_PASSWORD_MISSING, | ||
| type: 'no_master_key', | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new MasterPasswordMissingException(); | ||
| } | ||
| if (errMessage === Messages.MASTER_PASSWORD_INCORRECT) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.MASTER_PASSWORD_INCORRECT, | ||
| type: 'invalid_master_key', | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new MasterPasswordIncorrectException(); | ||
| } | ||
| } | ||
| if (!connection) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.CONNECTION_NOT_FOUND, | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new ConnectionNotFoundException(HttpStatus.BAD_REQUEST); |
There was a problem hiding this comment.
Re-throw unknown connection-decryption errors instead of converting them to not-found.
The catch only remaps two known master-password messages; any other repository/decryption failure is swallowed and then misreported as ConnectionNotFoundException(HttpStatus.BAD_REQUEST).
Suggested fix
} catch (error) {
const errMessage = getErrorMessage(error);
if (errMessage === Messages.MASTER_PASSWORD_MISSING) {
throw new MasterPasswordMissingException();
}
if (errMessage === Messages.MASTER_PASSWORD_INCORRECT) {
throw new MasterPasswordIncorrectException();
}
+ throw error;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/table-categories/use-cases/find-table-categories-with-tables.use.case.ts`
around lines 44 - 53, The catch block that inspects getErrorMessage(error)
should re-throw any unknown errors instead of letting them fall through and
later causing a ConnectionNotFoundException; update the error handling in the
find-table-categories-with-tables use-case so that after checking for
Messages.MASTER_PASSWORD_MISSING and Messages.MASTER_PASSWORD_INCORRECT
(throwing MasterPasswordMissingException or MasterPasswordIncorrectException
respectively), any other caught error is re-thrown (throw error), and only if no
error occurred and connection is falsy should you throw
ConnectionNotFoundException(HttpStatus.BAD_REQUEST); reference the existing
getErrorMessage, Messages, MasterPasswordMissingException,
MasterPasswordIncorrectException, and ConnectionNotFoundException symbols to
locate and apply this change.
Comment on lines
49
to
+50
| if (!primaryKeys) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.PRIMARY_KEY_MISSING, | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new PrimaryKeyMissingException(); |
There was a problem hiding this comment.
Reject empty primary key arrays.
Line 49 only guards null/undefined. An empty array ([]) currently passes and can return success without deleting anything, which makes invalid input look successful.
Suggested fix
- if (!primaryKeys) {
+ if (!primaryKeys?.length) {
throw new PrimaryKeyMissingException();
}📝 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.
Suggested change
| if (!primaryKeys) { | |
| throw new HttpException( | |
| { | |
| message: Messages.PRIMARY_KEY_MISSING, | |
| }, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| throw new PrimaryKeyMissingException(); | |
| if (!primaryKeys?.length) { | |
| throw new PrimaryKeyMissingException(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/entities/table/use-cases/delete-rows-from-table.use.case.ts`
around lines 49 - 50, The current guard only rejects null/undefined primaryKeys
but allows an empty array, causing no-op "success" responses; update the check
in the delete-rows-from-table use case to also reject empty arrays by throwing
PrimaryKeyMissingException when primaryKeys is falsy or an array with length 0
(e.g., replace the existing if (!primaryKeys) throw new
PrimaryKeyMissingException() with a condition like if (!primaryKeys ||
(Array.isArray(primaryKeys) && primaryKeys.length === 0)) throw new
PrimaryKeyMissingException()).
Comment on lines
50
to
+60
| } catch (error) { | ||
| const errMessage = getErrorMessage(error); | ||
| if (errMessage === Messages.MASTER_PASSWORD_MISSING) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.MASTER_PASSWORD_MISSING, | ||
| type: 'no_master_key', | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new MasterPasswordMissingException(); | ||
| } | ||
| if (errMessage === Messages.MASTER_PASSWORD_INCORRECT) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.MASTER_PASSWORD_INCORRECT, | ||
| type: 'invalid_master_key', | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new MasterPasswordIncorrectException(); | ||
| } | ||
| } | ||
| if (!connection) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.CONNECTION_NOT_FOUND, | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new ConnectionNotFoundException(HttpStatus.BAD_REQUEST); |
There was a problem hiding this comment.
Do not swallow non-password decryption failures.
Errors from findAndDecryptConnection that are not password-related are ignored and later misreported as ConnectionNotFoundException. This masks real backend failures as client-side bad requests.
Suggested fix
} catch (error) {
const errMessage = getErrorMessage(error);
if (errMessage === Messages.MASTER_PASSWORD_MISSING) {
throw new MasterPasswordMissingException();
}
if (errMessage === Messages.MASTER_PASSWORD_INCORRECT) {
throw new MasterPasswordIncorrectException();
}
+ throw error;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/entities/table/use-cases/find-tables-in-connection-v2.use.case.ts`
around lines 50 - 60, The current catch around findAndDecryptConnection swallows
all errors and only maps password-related messages to
MasterPasswordMissingException or MasterPasswordIncorrectException, causing
non-password failures to be ignored and later misreported as
ConnectionNotFoundException; update the catch in
find-tables-in-connection-v2.use.case.ts so that after checking
getErrorMessage(error) against Messages.MASTER_PASSWORD_MISSING and
Messages.MASTER_PASSWORD_INCORRECT and throwing MasterPasswordMissingException
or MasterPasswordIncorrectException respectively, any other error is rethrown
(or propagated) instead of being dropped; ensure
findAndDecryptConnection-related unexpected errors bubble up while leaving the
existing ConnectionNotFoundException logic for when connection is genuinely
null.
Comment on lines
51
to
+61
| } catch (error) { | ||
| const errMessage = getErrorMessage(error); | ||
| if (errMessage === Messages.MASTER_PASSWORD_MISSING) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.MASTER_PASSWORD_MISSING, | ||
| type: 'no_master_key', | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new MasterPasswordMissingException(); | ||
| } | ||
| if (errMessage === Messages.MASTER_PASSWORD_INCORRECT) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.MASTER_PASSWORD_INCORRECT, | ||
| type: 'invalid_master_key', | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new MasterPasswordIncorrectException(); | ||
| } | ||
| } | ||
| if (!connection) { | ||
| throw new HttpException( | ||
| { | ||
| message: Messages.CONNECTION_NOT_FOUND, | ||
| }, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
| throw new ConnectionNotFoundException(HttpStatus.BAD_REQUEST); |
There was a problem hiding this comment.
Preserve unexpected decrypt errors instead of remapping to not-found.
The catch handles only two password messages, then execution continues and throws ConnectionNotFoundException. Unexpected repository/decryption failures should be propagated.
Suggested fix
} catch (error) {
const errMessage = getErrorMessage(error);
if (errMessage === Messages.MASTER_PASSWORD_MISSING) {
throw new MasterPasswordMissingException();
}
if (errMessage === Messages.MASTER_PASSWORD_INCORRECT) {
throw new MasterPasswordIncorrectException();
}
+ throw error;
}📝 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.
Suggested change
| } catch (error) { | |
| const errMessage = getErrorMessage(error); | |
| if (errMessage === Messages.MASTER_PASSWORD_MISSING) { | |
| throw new HttpException( | |
| { | |
| message: Messages.MASTER_PASSWORD_MISSING, | |
| type: 'no_master_key', | |
| }, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| throw new MasterPasswordMissingException(); | |
| } | |
| if (errMessage === Messages.MASTER_PASSWORD_INCORRECT) { | |
| throw new HttpException( | |
| { | |
| message: Messages.MASTER_PASSWORD_INCORRECT, | |
| type: 'invalid_master_key', | |
| }, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| throw new MasterPasswordIncorrectException(); | |
| } | |
| } | |
| if (!connection) { | |
| throw new HttpException( | |
| { | |
| message: Messages.CONNECTION_NOT_FOUND, | |
| }, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| throw new ConnectionNotFoundException(HttpStatus.BAD_REQUEST); | |
| } catch (error) { | |
| const errMessage = getErrorMessage(error); | |
| if (errMessage === Messages.MASTER_PASSWORD_MISSING) { | |
| throw new MasterPasswordMissingException(); | |
| } | |
| if (errMessage === Messages.MASTER_PASSWORD_INCORRECT) { | |
| throw new MasterPasswordIncorrectException(); | |
| } | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/entities/table/use-cases/find-tables-in-connection.use.case.ts`
around lines 51 - 61, In find-tables-in-connection.use.case.ts inside the
find-tables-in-connection use case, the catch block only handles
MASTER_PASSWORD_MISSING and MASTER_PASSWORD_INCORRECT and then lets execution
fall through to throw ConnectionNotFoundException; modify the catch so that
after mapping those two known messages (using getErrorMessage and Messages) you
rethrow the original error for any other case instead of allowing the code to
continue—i.e., keep the checks that throw MasterPasswordMissingException and
MasterPasswordIncorrectException, and add a default rethrow of the caught error
(preserving the original exception) so unexpected repository/decryption failures
are propagated rather than remapped to ConnectionNotFoundException.
Comment on lines
+35
to
36
| /** DTO / request validation failure. Legacy value 1007, kept for backward compatibility. */ | ||
| VALIDATOR_EXCEPTION = 1007, |
There was a problem hiding this comment.
Validation code currently violates the documented range contract.
VALIDATOR_EXCEPTION is still 1007, but the file documents validation as 1100–1199 and explicitly suggests frontend range-based branching. That causes validation errors to fall into the database bucket for range-based clients.
I’d recommend adding a validation-range code (e.g., 1100) and migrating throw sites to it while keeping 1007 only as a legacy compatibility alias.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/src/exceptions/custom-exceptions/custom-exceptions-internal-codes/exceptions-internal-codes.ts`
around lines 35 - 36, Add a new validation internal code value in the enum for
the documented validation range (e.g., VALIDATOR_EXCEPTION_RANGE = 1100) and
update all throw sites that currently use VALIDATOR_EXCEPTION to use the new
code; keep the existing VALIDATOR_EXCEPTION = 1007 as a legacy alias for
backward compatibility so older consumers still see 1007, but move
runtime/validation error emissions to VALIDATOR_EXCEPTION_RANGE (or a clearly
named constant like VALIDATION_ERROR_CODE) and update any tests or mappings that
branch on the 1100–1199 range accordingly.
Comment on lines
+6
to
+14
| export function translateDomainError(exception: unknown): HttpException | null { | ||
| if (exception instanceof MasterPasswordMissingError) { | ||
| return new MasterPasswordMissingException(); | ||
| } | ||
| if (exception instanceof MasterPasswordIncorrectError) { | ||
| return new MasterPasswordIncorrectException(); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether function declaration is still present in the changed file.
rg -nP '^export function\s+translateDomainError\s*\(' backend/src/exceptions/domain-errors/translate-domain-error.ts
# Expected after fix: no matches.Repository: rocket-admin/rocketadmin
Length of output: 151
Convert translateDomainError to an arrow function export
backend/src/exceptions/domain-errors/translate-domain-error.ts currently uses export function translateDomainError(...), which violates the project’s “prefer arrow functions over function declarations” rule.
Suggested change
-export function translateDomainError(exception: unknown): HttpException | null {
+export const translateDomainError = (exception: unknown): HttpException | null => {
if (exception instanceof MasterPasswordMissingError) {
return new MasterPasswordMissingException();
}
if (exception instanceof MasterPasswordIncorrectError) {
return new MasterPasswordIncorrectException();
}
return null;
-}
+};📝 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.
Suggested change
| export function translateDomainError(exception: unknown): HttpException | null { | |
| if (exception instanceof MasterPasswordMissingError) { | |
| return new MasterPasswordMissingException(); | |
| } | |
| if (exception instanceof MasterPasswordIncorrectError) { | |
| return new MasterPasswordIncorrectException(); | |
| } | |
| return null; | |
| } | |
| export const translateDomainError = (exception: unknown): HttpException | null => { | |
| if (exception instanceof MasterPasswordMissingError) { | |
| return new MasterPasswordMissingException(); | |
| } | |
| if (exception instanceof MasterPasswordIncorrectError) { | |
| return new MasterPasswordIncorrectException(); | |
| } | |
| return null; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/exceptions/domain-errors/translate-domain-error.ts` around lines
6 - 14, Replace the function declaration with an exported arrow function: change
`export function translateDomainError(exception: unknown): HttpException | null
{ ... }` to `export const translateDomainError = (exception: unknown):
HttpException | null => { ... }`, keeping the existing body and return logic
that checks `MasterPasswordMissingError` -> `MasterPasswordMissingException` and
`MasterPasswordIncorrectError` -> `MasterPasswordIncorrectException`.
Comment on lines
+12
to
13
| export function categorizeExceptionMessage(message: string): CategorizedExceptionMessage { | ||
| const msgInLowerCase = message.toLowerCase(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify remaining function declarations in exceptions TS files.
rg -n --type=ts '^\s*export\s+function\s+\w+\s*\(' backend/src/exceptionsRepository: rocket-admin/rocketadmin
Length of output: 494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="backend/src/exceptions/utils/process-exception-message.ts"
echo "---- $FILE (head 1-60) ----"
sed -n '1,60p' "$FILE" | nl -ba -w2 -s': '
echo "---- $FILE (lines 75-120) ----"
sed -n '75,120p' "$FILE" | nl -ba -w2 -s': 'Repository: rocket-admin/rocketadmin
Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="backend/src/exceptions/utils/process-exception-message.ts"
echo "---- $FILE (lines 1-60) ----"
sed -n '1,60p' "$FILE" | cat -n -w2 -s': '
echo "---- $FILE (lines 75-120) ----"
sed -n '75,120p' "$FILE" | cat -n -w2 -s': 'Repository: rocket-admin/rocketadmin
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="backend/src/exceptions/utils/process-exception-message.ts"
echo "---- $FILE (lines 1-60) ----"
sed -n '1,60p' "$FILE" | awk '{printf "%4d: %s\n", NR, $0}'
echo "---- $FILE (lines 75-120) ----"
sed -n '75,120p' "$FILE" | awk '{printf "%4d: %s\n", NR+74, $0}'Repository: rocket-admin/rocketadmin
Length of output: 4074
Convert exported exception-message helpers to arrow-function exports.
categorizeExceptionMessage (lines 12-89) and processExceptionMessage (lines 91-93) are exported as function declarations; switch to export const ... = (...) => ... to match the repo’s arrow-function convention.
Suggested refactor
-export function categorizeExceptionMessage(message: string): CategorizedExceptionMessage {
+export const categorizeExceptionMessage = (message: string): CategorizedExceptionMessage => {
const msgInLowerCase = message.toLowerCase();
switch (true) {
...
}
-}
+};
-export function processExceptionMessage(message: string): string {
+export const processExceptionMessage = (message: string): string => {
return categorizeExceptionMessage(message).message;
-}
+};📝 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.
Suggested change
| export function categorizeExceptionMessage(message: string): CategorizedExceptionMessage { | |
| const msgInLowerCase = message.toLowerCase(); | |
| export const categorizeExceptionMessage = (message: string): CategorizedExceptionMessage => { | |
| const msgInLowerCase = message.toLowerCase(); | |
| switch (true) { | |
| // ... switch statement continues ... | |
| } | |
| }; | |
| export const processExceptionMessage = (message: string): string => { | |
| return categorizeExceptionMessage(message).message; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/exceptions/utils/process-exception-message.ts` around lines 12 -
13, Convert the two exported function declarations into arrow-function exports:
replace "export function categorizeExceptionMessage(...)" with "export const
categorizeExceptionMessage = (...) => { ... }" and similarly replace "export
function processExceptionMessage(...)" with "export const
processExceptionMessage = (...) => { ... }"; keep the existing parameter types,
return types (CategorizedExceptionMessage), and internal logic exactly the same
so behavior doesn't change, and ensure any internal references still work after
changing from hoisted declarations to const arrow functions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor