refactor(core): Migrate all errors in cli package to new hierarchy #13478

ivov · 2025-02-24T16:03:35Z

Follow-up to: #12267

Exempting webhook-helpers.ts which is currently being refactored.

packages/cli/src/decorators/on-shutdown.ts

packages/cli/src/external-secrets.ee/external-secrets-manager.ee.ts

…ierarchy

codecov · 2025-02-25T10:39:44Z

Codecov Report

Attention: Patch coverage is 58.33333% with 130 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ee/source-control/source-control-git.service.ee.ts	4.76%	20 Missing ⚠️
...source-control/source-control-export.service.ee.ts	33.33%	6 Missing ⚠️
...ges/cli/src/services/community-packages.service.ts	40.00%	6 Missing ⚠️
...e-control/source-control-preferences.service.ee.ts	16.66%	5 Missing ⚠️
packages/cli/src/commands/execute.ts	0.00%	4 Missing ⚠️
...kages/cli/src/databases/utils/migration-helpers.ts	20.00%	4 Missing ⚠️
...nts.ee/source-control/source-control.service.ee.ts	20.00%	4 Missing ⚠️
packages/cli/src/commands/import/workflow.ts	50.00%	3 Missing ⚠️
packages/cli/src/credentials-helper.ts	25.00%	3 Missing ⚠️
...source-control/source-control-import.service.ee.ts	25.00%	3 Missing ⚠️
... and 45 more

📢 Thoughts on this report? Let us know!

tomi

Few comments about which error class to use. Didn't go through all of them as there are so many. In the future maybe we can do these in smaller batches so they are easier to review.

tomi · 2025-02-25T10:43:58Z

packages/cli/src/active-workflow-manager.ts

@@ -238,7 +238,7 @@ export class ActiveWorkflowManager {
 		});

 		if (workflowData === null) {
-			throw new ApplicationError('Could not find workflow', { extra: { workflowId } });
+			throw new UnexpectedError('Could not find workflow', { extra: { workflowId } });


I think we could change the .findOne above to .findOneOrFail and remove this if, as non BaseErrors are treated as unexpected

I'd prefer not to change any logic in this PR if possible.

packages/workflow/src/errors/base/runtime.error.ts

tomi · 2025-02-25T10:56:26Z

packages/cli/src/errors/aborted-execution-retry.error.ts


-export class AbortedExecutionRetryError extends ApplicationError {
+export class AbortedExecutionRetryError extends UnexpectedError {


Isn't this error transient by nature? If so it should extend OperationalError instead

This error is part of a workaround for the root issue that we should not be sending requests to retry an execution that was cancelled before starting.

tomi · 2025-02-25T11:00:38Z

packages/cli/src/errors/external-secrets-provider-not-found.error.ts


-export class ExternalSecretsProviderNotFoundError extends ApplicationError {
+export class ExternalSecretsProviderNotFoundError extends UnexpectedError {


The provider name seems to be coming in a request to the controller. Hence that makes this a UserError as it can fail due to input from user

The frontend should never allow sending an external secrets provider name that does not exist.

packages/cli/src/errors/max-stalled-count.error.ts

tomi · 2025-02-25T11:15:59Z

packages/cli/src/executions/execution.service.ts

@@ -146,7 +146,7 @@ export class ExecutionService {
 		if (!execution.data.executionData) throw new AbortedExecutionRetryError();

 		if (execution.finished) {
-			throw new ApplicationError('The execution succeeded, so it cannot be retried.');
+			throw new UnexpectedError('The execution succeeded, so it cannot be retried.');


This is transient? So OperationalError?

This error is part of a workaround for the root issue that we should not be sending requests to retry an execution with success status.

tomi · 2025-02-25T11:16:47Z

packages/cli/src/executions/execution.service.ts

@@ -188,7 +188,7 @@ export class ExecutionService {
 			})) as IWorkflowBase;

 			if (workflowData === undefined) {
-				throw new ApplicationError(
+				throw new UnexpectedError(


Could this happen if the execution or workflow gets deleted? Should it be OperationalError or UserError instead?

In this and the above cases, we've been assuming that trying to operate on a resource that no longer exists is a UserError. I don't fully agree and would rather err on the side of caution, but I'll go with UserError to be consistent.

tomi · 2025-02-25T11:23:03Z

packages/cli/src/executions/parse-range-query.middleware.ts

@@ -44,7 +44,7 @@ export const parseRangeQuery = (

 			if (jsonFilter.waitTill) jsonFilter.waitTill = Boolean(jsonFilter.waitTill);

-			if (!isValid(jsonFilter)) throw new ApplicationError('Query does not match schema');
+			if (!isValid(jsonFilter)) throw new UnexpectedError('Query does not match schema');


This input is coming from users --> UserError

This is coming from our own frontend. Cases like I'd think this is unexpected and we'd want to report it?

tomi · 2025-02-25T11:27:03Z

packages/cli/src/task-runners/task-broker/errors/task-deferred.error.ts


-export class TaskDeferredError extends ApplicationError {
+export class TaskDeferredError extends UserError {


I think this is transient --> OperationalError

This is a special case in that we're relying on this to give time to the launcher to start a runner. So not really an error at all, so not sure any of the existing categories fit?

packages/cli/src/task-runners/task-broker/errors/task-runner-timeout.error.ts

…ierarchy

refactor(core): Migrate all errors in cli package to new hierarchy

4368e55

ivov commented Feb 24, 2025

View reviewed changes

packages/cli/src/decorators/on-shutdown.ts Outdated Show resolved Hide resolved

ivov commented Feb 24, 2025

View reviewed changes

packages/cli/src/external-secrets.ee/external-secrets-manager.ee.ts Show resolved Hide resolved

ivov added 2 commits February 24, 2025 17:18

Go through all extends

26f2194

Disregard file to refactor

2d6e89b

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Feb 24, 2025

ivov added 6 commits February 24, 2025 17:35

Improve error reporter

3ebc3a9

Introduce RuntimeError

dd35329

Improve error reporter

7e9d8b9

Fix tests

a532a71

Merge branch 'master' into migrate-all-errors-in-cli-package-to-new-h…

1e900fd

…ierarchy

Fix test

d0d75fe

ivov marked this pull request as ready for review February 25, 2025 10:31

ivov requested a review from a team as a code owner February 25, 2025 10:31

tomi reviewed Feb 25, 2025

View reviewed changes

ivov added 4 commits February 25, 2025 16:57

Merge branch 'master' into migrate-all-errors-in-cli-package-to-new-h…

ddf7d08

…ierarchy

Remove RuntimeError

86e31ae

Recategorize some errors

950064c

Set level based on error code

e563580

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Migrate all errors in cli package to new hierarchy #13478

refactor(core): Migrate all errors in cli package to new hierarchy #13478

ivov commented Feb 24, 2025

codecov bot commented Feb 25, 2025

tomi left a comment

tomi Feb 25, 2025

ivov Feb 25, 2025

tomi Feb 25, 2025

ivov Feb 25, 2025 •

edited

Loading

tomi Feb 25, 2025

ivov Feb 25, 2025 •

edited

Loading

tomi Feb 25, 2025

ivov Feb 25, 2025

tomi Feb 25, 2025

ivov Feb 25, 2025

tomi Feb 25, 2025

ivov Feb 25, 2025

tomi Feb 25, 2025

ivov Feb 25, 2025 •

edited

Loading


		export class AbortedExecutionRetryError extends ApplicationError {
		export class AbortedExecutionRetryError extends UnexpectedError {


		export class ExternalSecretsProviderNotFoundError extends ApplicationError {
		export class ExternalSecretsProviderNotFoundError extends UnexpectedError {


		export class TaskDeferredError extends ApplicationError {
		export class TaskDeferredError extends UserError {

refactor(core): Migrate all errors in cli package to new hierarchy #13478

Are you sure you want to change the base?

refactor(core): Migrate all errors in cli package to new hierarchy #13478

Conversation

ivov commented Feb 24, 2025

codecov bot commented Feb 25, 2025

Codecov Report

tomi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

ivov Feb 25, 2025 •

edited

Loading

ivov Feb 25, 2025 •

edited

Loading

ivov Feb 25, 2025 •

edited

Loading