refactor: Add DB error predicates and drop TypeORM imports from execution and credential services (#38940)

This commit is contained in:
Andreas Fitzek 2026-09-22 05:30:05 +00:00 committed by GitHub
parent 872b145ea8
commit 32aa4629de
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
14 changed files with 303 additions and 76 deletions

View File

@ -15,6 +15,8 @@ export { chunkIds } from './utils/chunk-ids';
export { dbNowLiteral, dbNowPlusMsLiteral, parseDbTime } from './utils/dialect-time';
export { escapeLike, LIKE_ESCAPE_CLAUSE } from './utils/escape-like';
export { generateHostInstanceId } from './utils/generators';
export { isEntityNotFoundError } from './utils/is-entity-not-found-error';
export { isForeignKeyConstraintError } from './utils/is-foreign-key-constraint-error';
export { isStringArray } from './utils/is-string-array';
export { isUniqueConstraintError } from './utils/is-unique-constraint-error';
export { isValidEmail } from './utils/is-valid-email';

View File

@ -24,6 +24,17 @@ export class WorkflowHistoryRepository extends BaseRepository<WorkflowHistory> {
return await this.delete({ createdAt: LessThan(date) });
}
async findVersionSummaries(
workflowId: string,
versionIds: string[],
): Promise<Array<{ versionId: string; name: string | null; createdAt: Date }>> {
return await this.find({
where: { workflowId, versionId: In(versionIds) },
select: ['versionId', 'name', 'createdAt'],
order: { createdAt: 'DESC' },
});
}
/**
* Name and optionally describe a single version. Scoped by `workflowId` too
* so a version of another workflow can never be touched, and returns the

View File

@ -0,0 +1,23 @@
import { EntityNotFoundError } from '@n8n/typeorm';
import { isEntityNotFoundError } from '../is-entity-not-found-error';
class DummyEntity {}
describe('isEntityNotFoundError', () => {
it('returns true for an EntityNotFoundError', () => {
const error = new EntityNotFoundError(DummyEntity, { id: 'missing' });
expect(isEntityNotFoundError(error)).toBe(true);
});
it('returns false for an unrelated error', () => {
const error = new Error('Connection terminated due to connection timeout');
expect(isEntityNotFoundError(error)).toBe(false);
});
it('returns false for non-error values', () => {
expect(isEntityNotFoundError(null)).toBe(false);
expect(isEntityNotFoundError(undefined)).toBe(false);
expect(isEntityNotFoundError('not found')).toBe(false);
});
});

View File

@ -0,0 +1,71 @@
import { QueryFailedError } from '@n8n/typeorm';
import { isForeignKeyConstraintError } from '../is-foreign-key-constraint-error';
describe('isForeignKeyConstraintError', () => {
const makeQueryFailedError = (
message: string,
driverError: { code?: string } = {},
): QueryFailedError => {
return new QueryFailedError('Query', [], Object.assign(new Error(message), driverError));
};
describe('returns true for actual database foreign-key-constraint violations', () => {
it('SQLite extended code SQLITE_CONSTRAINT_FOREIGNKEY', () => {
const error = makeQueryFailedError('FOREIGN KEY constraint failed', {
code: 'SQLITE_CONSTRAINT_FOREIGNKEY',
});
expect(isForeignKeyConstraintError(error)).toBe(true);
});
it('SQLite base code SQLITE_CONSTRAINT when message mentions FOREIGN KEY constraint', () => {
const error = makeQueryFailedError('SQLITE_CONSTRAINT: FOREIGN KEY constraint failed', {
code: 'SQLITE_CONSTRAINT',
});
expect(isForeignKeyConstraintError(error)).toBe(true);
});
it('PostgreSQL foreign_key_violation (code 23503)', () => {
const error = makeQueryFailedError(
'insert or update on table "annotation_tag_mapping" violates foreign key constraint',
{ code: '23503' },
);
expect(isForeignKeyConstraintError(error)).toBe(true);
});
});
describe('returns false for non-QueryFailedError values', () => {
it('plain Error whose message mentions "foreign key"', () => {
const error = new Error('foreign key lookup failed upstream');
expect(isForeignKeyConstraintError(error)).toBe(false);
});
it('non-error values', () => {
expect(isForeignKeyConstraintError(null)).toBe(false);
expect(isForeignKeyConstraintError(undefined)).toBe(false);
expect(isForeignKeyConstraintError('FOREIGN KEY constraint failed')).toBe(false);
});
});
describe('returns false for unrelated database errors', () => {
it('SQLite base code SQLITE_CONSTRAINT for a UNIQUE violation', () => {
const error = makeQueryFailedError('UNIQUE constraint failed: workflow_entity.name', {
code: 'SQLITE_CONSTRAINT',
});
expect(isForeignKeyConstraintError(error)).toBe(false);
});
it('PostgreSQL unique_violation (code 23505)', () => {
const error = makeQueryFailedError(
'duplicate key value violates unique constraint "users_email_key"',
{ code: '23505' },
);
expect(isForeignKeyConstraintError(error)).toBe(false);
});
it('QueryFailedError with no driver code', () => {
const error = makeQueryFailedError('Connection timeout after 30000ms');
expect(isForeignKeyConstraintError(error)).toBe(false);
});
});
});

View File

@ -0,0 +1,8 @@
import { EntityNotFoundError } from '@n8n/typeorm';
/**
* Whether `error` is a TypeORM "entity not found" error, e.g. from `findOneOrFail`.
*/
export function isEntityNotFoundError(error: unknown): boolean {
return error instanceof EntityNotFoundError;
}

View File

@ -0,0 +1,13 @@
import { isQueryFailedErrorOfKind } from './is-query-failed-error-of-kind';
/**
* Whether `error` is a foreign-key-constraint violation, across PostgreSQL and SQLite.
*/
export function isForeignKeyConstraintError(error: unknown): boolean {
return isQueryFailedErrorOfKind(error, {
// PostgreSQL: 23503 = foreign_key_violation.
postgresCode: '23503',
sqliteExtendedCode: 'SQLITE_CONSTRAINT_FOREIGNKEY',
sqliteMessagePattern: /FOREIGN KEY constraint/i,
});
}

View File

@ -0,0 +1,34 @@
import { QueryFailedError } from '@n8n/typeorm';
/**
* Whether `error` is a `QueryFailedError` violating the constraint kind identified
* by `postgresCode`/`sqliteExtendedCode`/`sqliteMessagePattern`, across PostgreSQL
* and SQLite. Shared by the per-kind predicates (`isUniqueConstraintError`,
* `isForeignKeyConstraintError`, …) so the driver-error narrowing and SQLite
* disambiguation logic have one implementation instead of drifting copies.
*/
export function isQueryFailedErrorOfKind(
error: unknown,
{
postgresCode,
sqliteExtendedCode,
sqliteMessagePattern,
}: { postgresCode: string; sqliteExtendedCode: string; sqliteMessagePattern: RegExp },
): boolean {
if (!(error instanceof QueryFailedError)) return false;
// TypeORM types `driverError` as `any`; narrow it via `unknown` so the
// property checks below stay type-safe without an `as` cast.
const driverError: unknown = error.driverError;
if (typeof driverError !== 'object' || driverError === null) return false;
const code =
'code' in driverError && typeof driverError.code === 'string' ? driverError.code : undefined;
if (code === postgresCode) return true;
// SQLite: the extended code is unambiguous; the base code covers all constraint
// kinds (NOT NULL, FK, CHECK, UNIQUE), so disambiguate via the message.
if (code === sqliteExtendedCode) return true;
return code === 'SQLITE_CONSTRAINT' && sqliteMessagePattern.test(error.message);
}

View File

@ -1,26 +1,13 @@
import { QueryFailedError } from '@n8n/typeorm';
import { isQueryFailedErrorOfKind } from './is-query-failed-error-of-kind';
/**
* Whether `error` is a unique-constraint violation, across PostgreSQL and SQLite.
*/
export function isUniqueConstraintError(error: unknown): boolean {
if (!(error instanceof QueryFailedError)) return false;
// TypeORM types `driverError` as `any`; narrow it via `unknown` so the
// property checks below stay type-safe without an `as` cast.
const driverError: unknown = error.driverError;
if (typeof driverError !== 'object' || driverError === null) return false;
const code =
'code' in driverError && typeof driverError.code === 'string' ? driverError.code : undefined;
// PostgreSQL: 23505 = unique_violation.
if (code === '23505') return true;
// SQLite: the extended code is unambiguous; the base code covers all constraint
// kinds (NOT NULL, FK, CHECK, UNIQUE), so disambiguate via the message.
if (code === 'SQLITE_CONSTRAINT_UNIQUE') return true;
if (code === 'SQLITE_CONSTRAINT' && /UNIQUE constraint/i.test(error.message)) return true;
return false;
return isQueryFailedErrorOfKind(error, {
// PostgreSQL: 23505 = unique_violation.
postgresCode: '23505',
sqliteExtendedCode: 'SQLITE_CONSTRAINT_UNIQUE',
sqliteMessagePattern: /UNIQUE constraint/i,
});
}

View File

@ -240,7 +240,6 @@ export default defineConfig(
// NEVER add to this list — a new leak must fail CI. Entries are removed as each file migrates.
files: [
// credentials/
'./src/credentials-helper.ts',
'./src/credentials/credential-connection-status-provider.interface.ts',
'./src/credentials/credential-connection-status-proxy.ts',
'./src/credentials/credential-dependency.service.ts',
@ -331,7 +330,6 @@ export default defineConfig(
'./src/executions/execution-data/db-store.ts',
'./src/executions/execution-persistence.ts',
'./src/executions/execution-recovery.service.ts',
'./src/executions/execution.service.ts',
'./src/instance-settings-loader/loaders/log-streaming.instance-settings-loader.ts',
'./src/modules/agents/agents.service.ts',
'./src/modules/chat-hub/chat-hub-agent.service.ts',

View File

@ -1,12 +1,12 @@
/* eslint-disable @typescript-eslint/no-unsafe-argument */
/* eslint-disable @typescript-eslint/no-unsafe-member-access */
import { SYSTEM_RESOLVER_ID } from '@n8n/api-types';
import { LicenseState } from '@n8n/backend-common';
import type { CredentialsEntity, ICredentialsDb } from '@n8n/db';
import { CredentialsRepository, SecretsProviderConnectionRepository } from '@n8n/db';
import {
CredentialsRepository,
isEntityNotFoundError,
SecretsProviderConnectionRepository,
} from '@n8n/db';
import { Service } from '@n8n/di';
import { EntityNotFoundError } from '@n8n/typeorm';
import { Credentials, getAdditionalKeys } from 'n8n-core';
import type {
CredentialInformation,
@ -343,7 +343,7 @@ export class CredentialsHelper extends ICredentialsHelper {
type,
});
} catch (error) {
if (error instanceof EntityNotFoundError) {
if (isEntityNotFoundError(error)) {
throw new CredentialNotFoundError(nodeCredential.id, type);
}

View File

@ -11,7 +11,6 @@ import type {
Project,
User,
WorkflowHistoryRepository,
WorkflowHistory,
} from '@n8n/db';
import { Container } from '@n8n/di';
import { QueryFailedError } from '@n8n/typeorm';
@ -865,31 +864,26 @@ describe('ExecutionService', () => {
const result = await executionService.getExecutedVersions(workflowId);
expect(result).toEqual([]);
expect(workflowHistoryRepository.find).not.toHaveBeenCalled();
expect(workflowHistoryRepository.findVersionSummaries).not.toHaveBeenCalled();
});
it('should return versions with metadata from workflow history', async () => {
const versionIds = ['v1', 'v2'];
executionRepository.getDistinctVersionIds.mockResolvedValue(versionIds);
const historyVersions = [
mock<WorkflowHistory>({ versionId: 'v2', name: null, createdAt: new Date('2025-01-02') }),
mock<WorkflowHistory>({
versionId: 'v1',
name: 'Release 1',
createdAt: new Date('2025-01-01'),
}),
const historySummaries = [
{ versionId: 'v2', name: null, createdAt: new Date('2025-01-02') },
{ versionId: 'v1', name: 'Release 1', createdAt: new Date('2025-01-01') },
];
workflowHistoryRepository.find.mockResolvedValue(historyVersions);
workflowHistoryRepository.findVersionSummaries.mockResolvedValue(historySummaries);
const result = await executionService.getExecutedVersions(workflowId);
expect(executionRepository.getDistinctVersionIds).toHaveBeenCalledWith(workflowId);
expect(workflowHistoryRepository.find).toHaveBeenCalledWith({
where: { workflowId, versionId: expect.anything() },
select: ['versionId', 'name', 'createdAt'],
order: { createdAt: 'DESC' },
});
expect(workflowHistoryRepository.findVersionSummaries).toHaveBeenCalledWith(
workflowId,
versionIds,
);
expect(result).toHaveLength(2);
expect(result[0].versionId).toBe('v2');
expect(result[0].name).toBeNull();
@ -899,7 +893,7 @@ describe('ExecutionService', () => {
it('should return empty array when version IDs have no matching history', async () => {
executionRepository.getDistinctVersionIds.mockResolvedValue(['orphan-v1']);
workflowHistoryRepository.find.mockResolvedValue([]);
workflowHistoryRepository.findVersionSummaries.mockResolvedValue([]);
const result = await executionService.getExecutedVersions(workflowId);
@ -1107,16 +1101,37 @@ describe('ExecutionService', () => {
]);
});
it('should map QueryFailedError to NotFoundError for missing tags', async () => {
it('should map a foreign-key-constraint violation to NotFoundError for missing tags', async () => {
executionPersistence.findOneInWorkflows.mockResolvedValue(mock<IExecutionBase>({ id: '1' }));
executionAnnotationRepository.findOneOrFail.mockResolvedValue({ id: 42 } as never);
annotationTagMappingRepository.overwriteTags.mockRejectedValue(
new QueryFailedError('INSERT', [], new Error('FK')),
new QueryFailedError(
'INSERT',
[],
Object.assign(new Error('FOREIGN KEY constraint failed'), {
code: 'SQLITE_CONSTRAINT_FOREIGNKEY',
}),
),
);
await expect(
executionService.updateExecutionTags('1', ['missing'], ['wf-1']),
).rejects.toThrow('Some tags not found');
});
it('should let a non-foreign-key QueryFailedError propagate instead of mapping it to a 404', async () => {
executionPersistence.findOneInWorkflows.mockResolvedValue(mock<IExecutionBase>({ id: '1' }));
executionAnnotationRepository.findOneOrFail.mockResolvedValue({ id: 42 } as never);
const dbError = new QueryFailedError(
'INSERT',
[],
Object.assign(new Error('Connection timeout after 30000ms'), {}),
);
annotationTagMappingRepository.overwriteTags.mockRejectedValue(dbError);
await expect(executionService.updateExecutionTags('1', ['tag-1'], ['wf-1'])).rejects.toThrow(
dbError,
);
});
});
});

View File

@ -14,12 +14,11 @@ import {
AnnotationTagMappingRepository,
ExecutionAnnotationRepository,
ExecutionRepository,
In,
isForeignKeyConstraintError,
WorkflowHistoryRepository,
WorkflowRepository,
} from '@n8n/db';
import { Service } from '@n8n/di';
import { QueryFailedError } from '@n8n/typeorm';
import { ensureError } from '@n8n/utils/errors/ensure-error';
import { stringify } from 'flatted';
import { validate as jsonSchemaValidate } from 'jsonschema';
@ -811,7 +810,7 @@ export class ExecutionService {
try {
await this.annotationTagMappingRepository.overwriteTags(annotation.id, tagIds);
} catch (error) {
if (error instanceof QueryFailedError) {
if (isForeignKeyConstraintError(error)) {
throw new NotFoundError('Some tags not found');
}
throw error;
@ -836,11 +835,10 @@ export class ExecutionService {
const versionIds = await this.executionRepository.getDistinctVersionIds(workflowId);
if (versionIds.length === 0) return [];
const versions = await this.workflowHistoryRepository.find({
where: { workflowId, versionId: In(versionIds) },
select: ['versionId', 'name', 'createdAt'],
order: { createdAt: 'DESC' },
});
const versions = await this.workflowHistoryRepository.findVersionSummaries(
workflowId,
versionIds,
);
return versions.map((v) => ({
versionId: v.versionId,

View File

@ -1,6 +1,11 @@
import { BaseRepository, type OperationContext, TransactionRunner } from '@n8n/db';
import {
BaseRepository,
isForeignKeyConstraintError,
type OperationContext,
TransactionRunner,
} from '@n8n/db';
import { Service } from '@n8n/di';
import { DataSource, QueryFailedError } from '@n8n/typeorm';
import { DataSource } from '@n8n/typeorm';
import { PromotionProvider } from '../entities/promotion-provider.entity';
import { PromotionConflictError } from '../promotion-conflict.error';
@ -63,23 +68,3 @@ export class PromotionProviderRepository extends BaseRepository<PromotionProvide
return { count, data };
}
}
/** Mirrors `isUniqueConstraintError` in `@n8n/db`, for foreign keys. */
function isForeignKeyConstraintError(error: unknown): boolean {
if (!(error instanceof QueryFailedError)) return false;
// TypeORM types `driverError` as `any`, so narrow it via `unknown`.
const driverError: unknown = error.driverError;
if (typeof driverError !== 'object' || driverError === null) return false;
const code =
'code' in driverError && typeof driverError.code === 'string' ? driverError.code : undefined;
// PostgreSQL: 23503 = foreign_key_violation.
if (code === '23503') return true;
// SQLite: the extended code is unambiguous; the base code covers every
// constraint kind, so disambiguate via the message.
if (code === 'SQLITE_CONSTRAINT_FOREIGNKEY') return true;
return code === 'SQLITE_CONSTRAINT' && /FOREIGN KEY constraint/i.test(error.message);
}

View File

@ -481,4 +481,86 @@ describe('WorkflowHistoryRepository', () => {
}
});
});
describe('findVersionSummaries', () => {
it('scopes by workflowId, filters by versionId, selects versionId/name/createdAt, and orders by createdAt desc', async () => {
const workflowA = await createWorkflow({
versionId: uuid(),
nodes: [testNode1],
});
const workflowB = await createWorkflow({
versionId: uuid(),
nodes: [testNode1],
});
const vOldest = uuid();
const vMiddle = uuid();
const vNewest = uuid();
const vNotRequested = uuid();
const vOtherWorkflow = uuid();
const threeDaysAgo = new Date();
threeDaysAgo.setDate(threeDaysAgo.getDate() - 3);
const twoDaysAgo = new Date();
twoDaysAgo.setDate(twoDaysAgo.getDate() - 2);
const oneDayAgo = new Date();
oneDayAgo.setDate(oneDayAgo.getDate() - 1);
await createWorkflowHistory(workflowA, undefined, undefined, {
versionId: vOldest,
name: null,
createdAt: threeDaysAgo,
});
await createWorkflowHistory(workflowA, undefined, undefined, {
versionId: vMiddle,
name: 'Release 1',
createdAt: twoDaysAgo,
});
await createWorkflowHistory(workflowA, undefined, undefined, {
versionId: vNewest,
name: null,
createdAt: oneDayAgo,
});
// Belongs to workflowA but not passed in versionIds — must be excluded
await createWorkflowHistory(workflowA, undefined, undefined, {
versionId: vNotRequested,
name: 'Should not appear',
createdAt: oneDayAgo,
});
// Passed in versionIds but belongs to workflowB — must be excluded by workflowId scoping
await createWorkflowHistory(workflowB, undefined, undefined, {
versionId: vOtherWorkflow,
name: 'Wrong workflow',
createdAt: oneDayAgo,
});
const repository = Container.get(WorkflowHistoryRepository);
const result = await repository.findVersionSummaries(workflowA.id, [
vOldest,
vMiddle,
vNewest,
vOtherWorkflow,
uuid(), // not present in the DB at all
]);
expect(result).toHaveLength(3);
expect(result.map((v) => v.versionId)).toEqual([vNewest, vMiddle, vOldest]);
expect(result[0]).toMatchObject({ versionId: vNewest, name: null });
expect(result[1]).toMatchObject({ versionId: vMiddle, name: 'Release 1' });
expect(result[2]).toMatchObject({ versionId: vOldest, name: null });
// `select` should be applied: only versionId/name/createdAt are populated,
// not the full entity (e.g. nodes/authors).
expect(result[0]).not.toHaveProperty('nodes');
expect(result[0]).not.toHaveProperty('authors');
});
it('returns an empty array when no versionIds match', async () => {
const workflow = await createWorkflow({ versionId: uuid(), nodes: [testNode1] });
const repository = Container.get(WorkflowHistoryRepository);
const result = await repository.findVersionSummaries(workflow.id, [uuid()]);
expect(result).toEqual([]);
});
});
});