From c9774aae07a6dc101af0807d91aa0cd3ec00fc05 Mon Sep 17 00:00:00 2001 From: Andreas Fitzek Date: Thu, 22 Oct 2026 16:28:51 +0000 Subject: [PATCH] refactor(core): Compose repositories instead of extending BaseRepository (no-changelog) (#41842) Co-authored-by: Claude Fable 5.1 --- AGENTS.md | 30 ++++- packages/@n8n/db/AGENTS.md | 7 ++ .../db/src/repositories/base-repository.ts | 21 +++- .../repositories/deployment-key.repository.ts | 11 +- .../src/templates/backend/README.md.template | 4 +- packages/cli/AGENTS.md | 34 +++-- .../__tests__/trusted-source.gate.test.ts | 2 +- .../trusted-source-identity.repository.ts | 23 ++-- .../repositories/trusted-source.repository.ts | 24 ++-- .../inbound-auth-core/trusted-source.gate.ts | 2 +- .../inbound-auth-core/trusted-source.store.ts | 119 ++++++++++-------- .../__tests__/oauth-flow.service.api.test.ts | 17 +-- ...-trusted-source.seeder.integration.test.ts | 13 +- ...ource-identity.service.integration.test.ts | 4 +- .../backend-module/backend-module-guide.md | 38 ++++-- .../my-feature.repository.template | 31 +++-- .../my-feature.service.template | 2 +- 17 files changed, 242 insertions(+), 140 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 70e928a2438..48fdd2ab788 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -253,18 +253,36 @@ baseline. A new business-logic import fails CI. repository methods you call. `run` **requires** a context — pass an empty `{}` at the operation entry point, and reuse the one you were handed everywhere below it (a context that already carries a transaction is joined, not nested). - Repositories extend `BaseRepository` and resolve the right `EntityManager` with - `this.managerFor(ctx)`; the `Transaction` handle is opaque and never exposes a - driver type to business logic. See `oauth-token.service.ts` + - `oauth-*-token.repository.ts` for a worked example. + The `Transaction` handle is opaque and never exposes a driver type to + business logic. See `oauth-token.service.ts` for a worked service example + (its repositories predate the composition rule below; do not copy their shape). +- **Repositories compose, they don't extend.** A repository is the boundary + that keeps TypeORM out of business logic. If it extended `BaseRepository`, + every service that injects it would also get `find`, `save`, `delete`, + `createQueryBuilder` and `.manager` for free, and TypeORM would leak through + inferred types that no lint rule can see. So a repository is a plain + `@Service()` class that holds a **file-private `BaseRepository` subclass (the + table)** as a private field and resolves the manager with + `this.table.managerFor(ctx)`. The table is never exported or registered with + DI. Every data access is a named use-case method on the repository that + calls the table internally; there is no generic `find`/`save`/`delete` + unless you write it for one operation. See + `packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source.repository.ts` + for a worked example. The existing `extends BaseRepository` / `extends + Repository` repositories are legacy. Do not copy them, and do not convert them + in unrelated PRs. This rule is not yet lint-enforced. - **Anti-patterns reviewers reject** — they hide the dependency instead of removing it: - String-matching TypeORM errors, e.g. `error.name === 'QueryFailedError'`. - Relabeling the import from `@n8n/typeorm` to `@n8n/db` to silence the rule (`@n8n/db` re-exports several operators/types, but this relabels the dependency rather than removing it). - - Pushing `.manager` / `createQueryBuilder` into business logic to avoid an - operator import — trades a visible leak for an invisible one. + - Pushing `.manager` / `createQueryBuilder` / `repo.managerFor(ctx)` into + business logic to avoid an operator import — trades a visible leak for an + invisible one. + - A repository that `extends BaseRepository` / `extends Repository`, or that + exposes its table (public property, getter, or a `find(options)` + passthrough) — the inherited TypeORM surface must stay private to the table. ### ESLint configuration layers diff --git a/packages/@n8n/db/AGENTS.md b/packages/@n8n/db/AGENTS.md index ffcb2a7f0bd..dde3c2de361 100644 --- a/packages/@n8n/db/AGENTS.md +++ b/packages/@n8n/db/AGENTS.md @@ -16,6 +16,13 @@ type its parameters and return value in **domain terms** — don't surface add generic `find(options)` passthroughs. This keeps operators and query construction inside the persistence layer where they belong. +Repositories in this package follow the same rule as everywhere else: compose a +file-private `BaseRepository` subclass (the table), do not extend it. Otherwise +every injecting service inherits the full TypeORM surface and the boundary is +gone. `src/repositories/deployment-key.repository.ts` is the in-package example; +the root AGENTS.md "Repositories compose, they don't extend" bullet has the +rules. Do not add new `extends BaseRepository` repositories. + ## Transactions Both transaction primitives live here: the deprecated `withTransaction` helper diff --git a/packages/@n8n/db/src/repositories/base-repository.ts b/packages/@n8n/db/src/repositories/base-repository.ts index 368c53b56bc..1a124725154 100644 --- a/packages/@n8n/db/src/repositories/base-repository.ts +++ b/packages/@n8n/db/src/repositories/base-repository.ts @@ -6,12 +6,17 @@ import type { OperationContext, TransactionRunner } from '../services/transactio import { TypeOrmTransaction } from '../services/typeorm-transaction'; /** - * Base class for repositories that participate in {@link TransactionRunner} transactions. + * Base class for the table behind a repository. It participates in {@link TransactionRunner} + * transactions and owns the single seam that turns an opaque {@link Transaction} back into a + * native `EntityManager`: {@link BaseRepository.managerFor}. * - * It owns the single seam that turns an opaque {@link Transaction} back into a native - * `EntityManager`: {@link BaseRepository.managerFor}. That method is `protected`, so the - * unwrap is reachable only from repository subclasses — business logic can hold a - * `Transaction` but never resolve it to a driver handle. + * A repository does not extend this class. It declares a file-private subclass (the table), + * which is never exported or registered with DI, and holds it as a private field. Every data + * access is a named use-case method on the repository that calls the table internally. If the + * repository extended this class instead, every service that injects it would also get `find`, + * `save`, `delete`, `createQueryBuilder` and `.manager`, and TypeORM would leak through inferred + * types that no lint rule can see. See `trusted-source.repository.ts` in `packages/cli` for the + * pattern. */ export abstract class BaseRepository extends Repository { constructor( @@ -26,8 +31,12 @@ export abstract class BaseRepository extends Repos * The manager a query should run against: the context's active transaction if present, * otherwise this repository's default manager. `ctx` is always required — thread the one * received from `TransactionRunner.run`; a non-transactional caller passes the root `{}`. + * + * Public, because `.manager` and {@link runInTransaction} already expose the manager. The + * boundary is not this modifier: it is that the table is a private field of its repository + * and business logic never holds a `BaseRepository`. */ - protected managerFor(ctx: OperationContext): EntityManager { + managerFor(ctx: OperationContext): EntityManager { const { trx } = ctx; if (!trx) return this.manager; diff --git a/packages/@n8n/db/src/repositories/deployment-key.repository.ts b/packages/@n8n/db/src/repositories/deployment-key.repository.ts index 8f6ab136fb0..156d80c6d08 100644 --- a/packages/@n8n/db/src/repositories/deployment-key.repository.ts +++ b/packages/@n8n/db/src/repositories/deployment-key.repository.ts @@ -10,7 +10,6 @@ import { OAUTH_SIGNING_KEY_TYPE, } from '../entities/deployment-key'; import { DbLock, DbLockService } from '../services/db-lock.service'; -import type { OperationContext } from '../services/transaction'; import { TransactionRunner } from '../services/transaction'; /** @@ -36,10 +35,6 @@ class DeploymentKeyStore extends BaseRepository { constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { super(DeploymentKey, dataSource.manager, transactionRunner); } - - managerForContext(ctx: OperationContext) { - return this.managerFor(ctx); - } } @Service() @@ -124,7 +119,7 @@ export class DeploymentKeyRepository { */ async seedLegacyCbcKey(encryptedValue: string): Promise { await this.dbLockService.withLockContext(DbLock.DATA_ENCRYPTION_KEY_SEED, async (ctx) => { - const repo = this.store.managerForContext(ctx).getRepository(DeploymentKey); + const repo = this.store.managerFor(ctx).getRepository(DeploymentKey); const existing = await repo.findOne({ where: { type: 'data_encryption', algorithm: 'aes-256-cbc' }, }); @@ -255,7 +250,7 @@ export class DeploymentKeyRepository { status: 'active', }); return await this.transactionRunner.run({}, async (ctx) => { - const tx = this.store.managerForContext(ctx); + const tx = this.store.managerFor(ctx); await tx.update( DeploymentKey, { type: entity.type, status: 'active' }, @@ -267,7 +262,7 @@ export class DeploymentKeyRepository { async activateDataEncryptionKey(id: string): Promise { await this.transactionRunner.run({}, async (ctx) => { - const tx = this.store.managerForContext(ctx); + const tx = this.store.managerFor(ctx); const target = await tx.findOne(DeploymentKey, { where: { id, type: 'data_encryption' }, }); diff --git a/packages/@n8n/module-cli/src/templates/backend/README.md.template b/packages/@n8n/module-cli/src/templates/backend/README.md.template index 17db2ef5c76..9c95ee69b54 100644 --- a/packages/@n8n/module-cli/src/templates/backend/README.md.template +++ b/packages/@n8n/module-cli/src/templates/backend/README.md.template @@ -22,4 +22,6 @@ service, repository, entity, configuration, and testing patterns. - Keep the module entrypoint in the `./module` export. Do not export it from `src/index.ts`. Importing the entrypoint runs the `@BackendModule` decorator. - Use dynamic imports inside the module entrypoint. This keeps disabled modules unloaded. -- Keep TypeORM imports in entity and repository files. +- Keep TypeORM imports in entity and repository files. A repository composes a + file-private `BaseRepository` table and exposes only use-case methods. It does + not extend `BaseRepository`. diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index 55ce1eeb38a..733dfc43e77 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -7,11 +7,13 @@ for repo-wide conventions. TypeORM belongs in the **persistence layer**, not in business logic. -**Allowed to import `@n8n/typeorm`** — files that declare an `@Entity` class or a -repository class that extends TypeORM `Repository` or n8n `BaseRepository`. -Their folder and filename do not affect the rule. Tests and migration tooling use -Code Health exceptions. Helper-only persistence adapters need a narrow, -auditable entry in the `typeorm-persistence-boundary` rule. +**Allowed to import `@n8n/typeorm`** — files that declare an `@Entity` class or +a class that extends TypeORM `Repository` or n8n `BaseRepository`. Their folder +and filename do not affect the rule. The file-private table class of a +composition repository (see below) satisfies this check, so such a repository +needs no exception. Tests and migration tooling use Code Health exceptions. +Helper-only persistence adapters need a narrow, auditable entry in the +`typeorm-persistence-boundary` rule. **Not allowed** — business logic (services, controllers, public-api handlers, commands, factories) must not import `@n8n/typeorm` or `@n8n/typeorm/...` @@ -24,8 +26,26 @@ without decoupling anything. Existing leaks of both kinds are tracked in the shrink-only Code Health baseline. Do not add new baseline entries. Distinct from that shrink-only ratchet, `src/commands/db/revert.ts` is an exact -exception for CLI migration tooling. Composition-based repositories use exact -exceptions. Repository subclasses need no exception. +exception for CLI migration tooling. Adapters with no `Repository` or +`BaseRepository` subclass in the file at all (for example +`src/modules/data-table/data-table-rows.repository.ts`) also need an exact +`allowedFiles` entry in the rule. A repository with a file-private table class +needs none. + +## Repositories + +A repository is the boundary that keeps TypeORM out of business logic, so it +**composes** a `BaseRepository` and never extends one. Extending would hand +every injecting service `find`, `save`, `delete`, `createQueryBuilder` and +`.manager` for free, and TypeORM would leak through inferred types that no lint +rule can see. Declare a file-private subclass (the table), hold it as a private +field, and write every data access as a named use-case method that calls +`this.table.managerFor(ctx)` internally. The table is never exported or +registered with DI. See the root AGENTS.md section for the rules and +`src/modules/inbound-auth-core/database/repositories/trusted-source.repository.ts` +for a worked example. Most repositories in this package still extend +`BaseRepository` or `Repository`. They are legacy: do not copy them, and do not +convert them in unrelated PRs. Need an operator query (`In`, `IsNull`, `FindOptionsWhere`, …)? Add a use-case-named repository method (plain parameters, domain-shaped return) rather diff --git a/packages/cli/src/modules/inbound-auth-core/__tests__/trusted-source.gate.test.ts b/packages/cli/src/modules/inbound-auth-core/__tests__/trusted-source.gate.test.ts index 0356b34a683..c0f5b700a7e 100644 --- a/packages/cli/src/modules/inbound-auth-core/__tests__/trusted-source.gate.test.ts +++ b/packages/cli/src/modules/inbound-auth-core/__tests__/trusted-source.gate.test.ts @@ -76,7 +76,7 @@ describe('TrustedSourceDbGate', () => { describe('with a binding', () => { it('allows an active binding to the same user', async () => { expect(await gate.authorizeSealed({ userId: 'user-1', grant, binding })).toBe(true); - expect(identityRepository.findBinding).toHaveBeenCalledWith('source-1', 'idp-alice'); + expect(identityRepository.findBinding).toHaveBeenCalledWith('source-1', 'idp-alice', {}); }); it.each(['suspended', 'revoked'] as const)('denies a %s binding', async (status) => { diff --git a/packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source-identity.repository.ts b/packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source-identity.repository.ts index d9055eff2cd..48cfd4a0875 100644 --- a/packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source-identity.repository.ts +++ b/packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source-identity.repository.ts @@ -19,10 +19,6 @@ class TrustedSourceIdentityStore extends BaseRepository { return await this.store.managerFor(ctx).findOne(TrustedSourceIdentityEntity, { select: ['userId', 'status'], @@ -54,7 +50,7 @@ export class TrustedSourceIdentityRepository { }); } - async clearByTrustedSourceId(trustedSourceId: string, ctx: OperationContext = {}): Promise { + async clearByTrustedSourceId(trustedSourceId: string, ctx: OperationContext): Promise { await this.store .managerFor(ctx) .delete(TrustedSourceIdentityEntity, { sourceId: trustedSourceId }); @@ -64,7 +60,7 @@ export class TrustedSourceIdentityRepository { async findBySubject( sourceId: string, subject: string, - ctx: OperationContext = {}, + ctx: OperationContext, ): Promise { return await this.store.managerFor(ctx).findOne(TrustedSourceIdentityEntity, { where: { sourceId, subject }, @@ -77,18 +73,18 @@ export class TrustedSourceIdentityRepository { } /** Reads inside the identity transaction must reuse its connection: the pool may hold one. */ - async findUserByEmail(email: string, ctx: OperationContext = {}): Promise { + async findUserByEmail(email: string, ctx: OperationContext): Promise { return await this.store .managerFor(ctx) .findOne(User, { where: { email }, relations: { role: true } }); } - async globalRoleExists(slug: string, ctx: OperationContext = {}): Promise { + async globalRoleExists(slug: string, ctx: OperationContext): Promise { return await this.store.managerFor(ctx).exists(Role, { where: { slug, roleType: 'global' } }); } /** True when every slug names a project role. Reads inside the identity transaction. */ - async projectRolesExist(slugs: string[], ctx: OperationContext = {}): Promise { + async projectRolesExist(slugs: string[], ctx: OperationContext): Promise { const unique = [...new Set(slugs)]; if (unique.length === 0) return true; const count = await this.store @@ -101,7 +97,7 @@ export class TrustedSourceIdentityRepository { sourceId: string, subject: string, seenAt: Date, - ctx: OperationContext = {}, + ctx: OperationContext, ): Promise { await this.store .managerFor(ctx) @@ -109,10 +105,7 @@ export class TrustedSourceIdentityRepository { } /** Writes the binding unless one exists for `(sourceId, subject)`; an existing row wins silently. */ - async insertIfAbsent( - row: InsertTrustedSourceIdentityRow, - ctx: OperationContext = {}, - ): Promise { + async insertIfAbsent(row: InsertTrustedSourceIdentityRow, ctx: OperationContext): Promise { await this.store .managerFor(ctx) .createQueryBuilder() diff --git a/packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source.repository.ts b/packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source.repository.ts index 88dd8ce8587..64257717124 100644 --- a/packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source.repository.ts +++ b/packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source.repository.ts @@ -15,10 +15,6 @@ class TrustedSourceTable extends BaseRepository { constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { super(TrustedSourceEntity, dataSource.manager, transactionRunner); } - - override managerFor(ctx: OperationContext) { - return super.managerFor(ctx); - } } export type NewTrustedSourceRow = Pick< @@ -79,15 +75,15 @@ export class TrustedSourceRepository { this.isPostgres = config.type === 'postgresdb'; } - async findById(id: string, ctx: OperationContext = {}) { + async findById(id: string, ctx: OperationContext) { return await this.table.managerFor(ctx).findOneBy(TrustedSourceEntity, { id }); } - async findByIssuer(issuer: string, ctx: OperationContext = {}) { + async findByIssuer(issuer: string, ctx: OperationContext) { return await this.table.managerFor(ctx).findOneBy(TrustedSourceEntity, { issuer }); } - async findAll(ctx: OperationContext = {}) { + async findAll(ctx: OperationContext) { return await this.table.managerFor(ctx).find(TrustedSourceEntity, { order: { name: 'ASC' } }); } @@ -98,7 +94,7 @@ export class TrustedSourceRepository { */ async findDue( { errorRetrySeconds, healthyRefreshSeconds, limit }: DueQuery, - ctx: OperationContext = {}, + ctx: OperationContext, ): Promise { const errorCutoff = dbNowPlusMsLiteral(this.isPostgres, -errorRetrySeconds * 1000); const healthyCutoff = dbNowPlusMsLiteral(this.isPostgres, -healthyRefreshSeconds * 1000); @@ -119,7 +115,7 @@ export class TrustedSourceRepository { } /** `save`, not `insert`: the `@BeforeInsert` id generator only runs on an entity instance. */ - async insertRow(row: NewTrustedSourceRow, ctx: OperationContext = {}) { + async insertRow(row: NewTrustedSourceRow, ctx: OperationContext) { return await this.table.managerFor(ctx).save(TrustedSourceEntity, this.table.create(row)); } @@ -130,7 +126,7 @@ export class TrustedSourceRepository { */ async insertIfAbsent( row: NewTrustedSourceRow & Pick, - ctx: OperationContext = {}, + ctx: OperationContext, ) { await this.table .managerFor(ctx) @@ -142,11 +138,11 @@ export class TrustedSourceRepository { .execute(); } - async updateById(id: string, changes: TrustedSourceRowChanges, ctx: OperationContext = {}) { + async updateById(id: string, changes: TrustedSourceRowChanges, ctx: OperationContext) { await this.table.managerFor(ctx).update(TrustedSourceEntity, { id }, changes); } - async deleteById(id: string, ctx: OperationContext = {}) { + async deleteById(id: string, ctx: OperationContext) { await this.table.managerFor(ctx).delete(TrustedSourceEntity, { id }); } @@ -155,7 +151,7 @@ export class TrustedSourceRepository { id: string, token: string, leaseSeconds: number, - ctx: OperationContext = {}, + ctx: OperationContext, ): Promise { const staleBefore = dbNowPlusMsLiteral(this.isPostgres, -leaseSeconds * 1000); const result = await this.table @@ -179,7 +175,7 @@ export class TrustedSourceRepository { id: string, token: string, changes: DiscoveryRowChanges, - ctx: OperationContext = {}, + ctx: OperationContext, ): Promise { const result = await this.table .managerFor(ctx) diff --git a/packages/cli/src/modules/inbound-auth-core/trusted-source.gate.ts b/packages/cli/src/modules/inbound-auth-core/trusted-source.gate.ts index 3ef570ebc89..cd602c0557b 100644 --- a/packages/cli/src/modules/inbound-auth-core/trusted-source.gate.ts +++ b/packages/cli/src/modules/inbound-auth-core/trusted-source.gate.ts @@ -37,7 +37,7 @@ export class TrustedSourceDbGate extends TrustedSourceGate { } if (binding) { - const row = await this.identityRepository.findBinding(binding.sourceId, binding.subject); + const row = await this.identityRepository.findBinding(binding.sourceId, binding.subject, {}); if (row?.status !== 'active' || row.userId !== userId) { this.logger.debug('Sealed identity denied: the binding is not active for this user', { userId, diff --git a/packages/cli/src/modules/inbound-auth-core/trusted-source.store.ts b/packages/cli/src/modules/inbound-auth-core/trusted-source.store.ts index be7c7e7b302..59347e9bfac 100644 --- a/packages/cli/src/modules/inbound-auth-core/trusted-source.store.ts +++ b/packages/cli/src/modules/inbound-auth-core/trusted-source.store.ts @@ -81,7 +81,7 @@ export class TrustedSourceDbStore extends TrustedSourceStore { async getById(id: string): Promise { return await this.cacheService.get(idKey(id), { ttl: CACHE_TTL, - refreshFn: async () => await this.load(await this.trustedSourceRepository.findById(id)), + refreshFn: async () => await this.load(await this.trustedSourceRepository.findById(id, {})), }); } @@ -89,7 +89,7 @@ export class TrustedSourceDbStore extends TrustedSourceStore { return await this.cacheService.get(issuerKey(issuer), { ttl: CACHE_TTL, refreshFn: async () => - await this.load(await this.trustedSourceRepository.findByIssuer(issuer)), + await this.load(await this.trustedSourceRepository.findByIssuer(issuer, {})), }); } @@ -109,11 +109,14 @@ export class TrustedSourceDbStore extends TrustedSourceStore { /** Sources due for discovery, most urgent first; not cached, the task reads it once a minute. */ async listDue(limit: number): Promise { - const rows = await this.trustedSourceRepository.findDue({ - errorRetrySeconds: ERROR_RETRY_SECONDS, - healthyRefreshSeconds: HEALTHY_REFRESH_SECONDS, - limit, - }); + const rows = await this.trustedSourceRepository.findDue( + { + errorRetrySeconds: ERROR_RETRY_SECONDS, + healthyRefreshSeconds: HEALTHY_REFRESH_SECONDS, + limit, + }, + {}, + ); const sources = await Promise.all(rows.map(async (row) => await this.load(row))); return sources.filter((source): source is TrustedSource => source !== undefined); } @@ -125,6 +128,7 @@ export class TrustedSourceDbStore extends TrustedSourceStore { source.id, token, DISCOVERY_LEASE_SECONDS, + {}, ); if (!claimed) return null; await this.invalidateCache(source); @@ -138,11 +142,16 @@ export class TrustedSourceDbStore extends TrustedSourceStore { result: DiscoveryResult, ): Promise { // Metadata is never cleared: an error keeps the last good documents readable. - const recorded = await this.trustedSourceRepository.recordDiscovery(source.id, token, { - status: result.status, - lastError: result.lastError, - ...(result.metadata && { metadata: JSON.stringify(result.metadata) }), - }); + const recorded = await this.trustedSourceRepository.recordDiscovery( + source.id, + token, + { + status: result.status, + lastError: result.lastError, + ...(result.metadata && { metadata: JSON.stringify(result.metadata) }), + }, + {}, + ); if (recorded) await this.invalidateCache(source); return recorded; } @@ -151,18 +160,21 @@ export class TrustedSourceDbStore extends TrustedSourceStore { // Persist the parsed, migrated output, not the raw input: stored rows hold the latest version // with defaults filled in, so a later default change must not alter them. const config = migrateToLatest(trustedSourceConfigSchemaFor('admin').parse(input.config)); - const row = await this.trustedSourceRepository.insertRow({ - name: input.name, - issuer: input.issuer, - type: config.authentication.type, - managedBy: 'admin', - status: 'unchecked', - lastError: null, - lastCheckedAt: null, - configVersion: config.version, - config: await this.cipher.encryptV2(config), - metadata: null, - }); + const row = await this.trustedSourceRepository.insertRow( + { + name: input.name, + issuer: input.issuer, + type: config.authentication.type, + managedBy: 'admin', + status: 'unchecked', + lastError: null, + lastCheckedAt: null, + configVersion: config.version, + config: await this.cipher.encryptV2(config), + metadata: null, + }, + {}, + ); await this.invalidateCache(row); return this.toRuntime(row, config); } @@ -207,7 +219,7 @@ export class TrustedSourceDbStore extends TrustedSourceStore { async delete(id: string): Promise { const row = await this.loadAdminRow(id, 'delete'); - await this.trustedSourceRepository.deleteById(id); // identities go with it via FK cascade + await this.trustedSourceRepository.deleteById(id, {}); // identities go with it via FK cascade await this.invalidateCache(row); } @@ -218,25 +230,28 @@ export class TrustedSourceDbStore extends TrustedSourceStore { */ async seedSystemSource(input: SeedSystemSourceInput): Promise { const { id, name, issuer } = input; - const existing = await this.trustedSourceRepository.findById(id); + const existing = await this.trustedSourceRepository.findById(id, {}); if (!existing) { const config = migrateToLatest(trustedSourceConfigSchemaFor('system').parse(input.config)); - await this.trustedSourceRepository.insertIfAbsent({ - id, - name, - issuer, - type: config.authentication.type, - managedBy: 'system', - status: 'unchecked', - lastError: null, - lastCheckedAt: null, - configVersion: config.version, - config: await this.cipher.encryptV2(config), - metadata: null, - }); + await this.trustedSourceRepository.insertIfAbsent( + { + id, + name, + issuer, + type: config.authentication.type, + managedBy: 'system', + status: 'unchecked', + lastError: null, + lastCheckedAt: null, + configVersion: config.version, + config: await this.cipher.encryptV2(config), + metadata: null, + }, + {}, + ); // `orIgnore` also drops the insert when an admin row holds the name or the issuer. - if (!(await this.trustedSourceRepository.findById(id))) return 'conflict'; + if (!(await this.trustedSourceRepository.findById(id, {}))) return 'conflict'; await this.invalidateCache({ id, issuer }); return 'inserted'; } @@ -246,22 +261,26 @@ export class TrustedSourceDbStore extends TrustedSourceStore { if (existing.issuer === issuer) return 'unchanged'; // Another seeder can move the issuer first; then the row holding it is this one. - const holder = await this.trustedSourceRepository.findByIssuer(issuer); + const holder = await this.trustedSourceRepository.findByIssuer(issuer, {}); if (holder) return holder.id === id ? 'unchanged' : 'conflict'; try { // The keys do not change, so the source stays usable. Discovery fetches the metadata for the // new issuer on its next run. A run that started before the move loses its lease. - await this.trustedSourceRepository.updateById(id, { - issuer, - lastCheckedAt: null, - discoveryClaimToken: null, - discoveryClaimedAt: null, - }); + await this.trustedSourceRepository.updateById( + id, + { + issuer, + lastCheckedAt: null, + discoveryClaimToken: null, + discoveryClaimedAt: null, + }, + {}, + ); } catch (error) { // Another row can take the issuer between the check and the update. A conflict must not // stop the boot, so classify it by reading the issuer again. - const winner = await this.trustedSourceRepository.findByIssuer(issuer); + const winner = await this.trustedSourceRepository.findByIssuer(issuer, {}); if (!winner) throw error; return winner.id === id ? 'unchanged' : 'conflict'; } @@ -270,7 +289,7 @@ export class TrustedSourceDbStore extends TrustedSourceStore { } private async loadAdminRow(id: string, action: 'update' | 'delete') { - const row = await this.trustedSourceRepository.findById(id); + const row = await this.trustedSourceRepository.findById(id, {}); if (!row) throw new NotFoundError(`Trusted source "${id}" not found`); if (row.managedBy === 'system') throw new SystemTrustedSourceModificationError(action); return row; @@ -306,7 +325,7 @@ export class TrustedSourceDbStore extends TrustedSourceStore { } private async loadAll(): Promise { - const rows = await this.trustedSourceRepository.findAll(); + const rows = await this.trustedSourceRepository.findAll({}); const sources = await Promise.all(rows.map(async (row) => await this.load(row))); return sources.filter((source): source is TrustedSource => source !== undefined); } diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-flow.service.api.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-flow.service.api.test.ts index 42dc3087c21..2bcdfef885a 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-flow.service.api.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-flow.service.api.test.ts @@ -235,13 +235,16 @@ const createIdpSource = async ( }; const bindIdpSubject = async (sourceId: string, userId: string) => { - await Container.get(TrustedSourceIdentityRepository).insertIfAbsent({ - sourceId, - subject: IDP_SUBJECT, - userId, - provenance: 'admin', - status: 'active', - }); + await Container.get(TrustedSourceIdentityRepository).insertIfAbsent( + { + sourceId, + subject: IDP_SUBJECT, + userId, + provenance: 'admin', + status: 'active', + }, + {}, + ); }; /** An access token as the external issuer would mint it for the trigger. */ diff --git a/packages/cli/test/integration/inbound-auth-core/system-trusted-source.seeder.integration.test.ts b/packages/cli/test/integration/inbound-auth-core/system-trusted-source.seeder.integration.test.ts index c26d1957b8e..4ae86177f07 100644 --- a/packages/cli/test/integration/inbound-auth-core/system-trusted-source.seeder.integration.test.ts +++ b/packages/cli/test/integration/inbound-auth-core/system-trusted-source.seeder.integration.test.ts @@ -34,11 +34,14 @@ const systemRow = async () => await rows.findOneByOrFail({ id: ID }); const dueIds = async () => ( - await Container.get(TrustedSourceRepository).findDue({ - errorRetrySeconds: ERROR_RETRY_SECONDS, - healthyRefreshSeconds: HEALTHY_REFRESH_SECONDS, - limit: 10, - }) + await Container.get(TrustedSourceRepository).findDue( + { + errorRetrySeconds: ERROR_RETRY_SECONDS, + healthyRefreshSeconds: HEALTHY_REFRESH_SECONDS, + limit: 10, + }, + {}, + ) ).map((row) => row.id); const adminConfig: TrustedSourceConfigInput = { diff --git a/packages/cli/test/integration/inbound-auth-core/trusted-source-identity.service.integration.test.ts b/packages/cli/test/integration/inbound-auth-core/trusted-source-identity.service.integration.test.ts index 14e0fb5b7b0..3da032e86d1 100644 --- a/packages/cli/test/integration/inbound-auth-core/trusted-source-identity.service.integration.test.ts +++ b/packages/cli/test/integration/inbound-auth-core/trusted-source-identity.service.integration.test.ts @@ -477,9 +477,9 @@ describe('TrustedSourceIdentityService (integration)', () => { status: 'active', } as const; - await identities.insertIfAbsent({ ...row, userId: first.id }); + await identities.insertIfAbsent({ ...row, userId: first.id }, {}); await expect( - identities.insertIfAbsent({ ...row, userId: second.id }), + identities.insertIfAbsent({ ...row, userId: second.id }, {}), ).resolves.toBeUndefined(); expect(await bindings.count()).toBe(1); diff --git a/scripts/backend-module/backend-module-guide.md b/scripts/backend-module/backend-module-guide.md index 6428d66403e..e6cbddf712e 100644 --- a/scripts/backend-module/backend-module-guide.md +++ b/scripts/backend-module/backend-module-guide.md @@ -260,22 +260,44 @@ Service-level decorators to be aware of: Repositories handle database access using `typeorm`, operating on database models ("entities"). +A repository is the boundary that keeps TypeORM out of business logic. It therefore **composes** a `BaseRepository` and never extends one. If it extended `BaseRepository`, every service that injects it would also get `find`, `save`, `delete`, `createQueryBuilder` and `.manager` for free, and TypeORM would leak through inferred types that no lint rule can see. Instead the repository holds a file-private subclass (the table) and exposes only named use-case methods that call the table internally. + ```ts +import { BaseRepository, OperationContext, TransactionRunner } from '@n8n/db'; +import { Service } from '@n8n/di'; +import { DataSource } from '@n8n/typeorm'; + +import { MyFeatureEntity } from './my-feature.entity'; + +// File-private: never exported, never registered with DI. +class MyFeatureTable extends BaseRepository { + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + super(MyFeatureEntity, dataSource.manager, transactionRunner); + } +} + @Service() -export class MyFeatureRepository extends Repository { - constructor(dataSource: DataSource) { - super(MyFeatureEntity, dataSource.manager); +export class MyFeatureRepository { + private readonly table: MyFeatureTable; + + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + this.table = new MyFeatureTable(dataSource, transactionRunner); } - async getSummary() { - return await /* typeorm query on entities */; + async findById(id: string, ctx: OperationContext): Promise { + return await this.table.managerFor(ctx).findOneBy(MyFeatureEntity, { id }); } } ``` -Repository-level decorators to be aware of: +Rules: -- `@Service()` to make a repository usable by the dependency injection container +- Only the repository is `@Service()`. The table class is not exported. +- Every public method takes plain parameters, returns domain shapes, and takes `ctx: OperationContext` last. Do not give `ctx` a default: a forgotten `ctx` inside a transaction must be a compile error, not a second connection. Pass the entity class explicitly to the manager call (`findOneBy(MyFeatureEntity, …)`), because the repository is not a TypeORM repository for that entity. +- No generic `find`/`save`/`delete` passthrough and no getter for the table. +- A non-transactional caller passes `{}` as `ctx`. Inside `TransactionRunner.run`, thread the `ctx` you received. + +See `packages/cli/src/modules/inbound-auth-core/database/repositories/trusted-source.repository.ts` for a real example. Most existing repositories in `packages/cli` still extend `BaseRepository` or `Repository`. They are legacy: do not copy them. ## Entities @@ -345,6 +367,8 @@ Place tests with the module that owns them. A packaged module keeps its tests un Use `@n8n/backend-test-utils` for shared database and module helpers. Pass packaged module import thunks to `testModules.loadModules`. Use `tscDecoratorTransform` from `@n8n/vitest-config/tsc-decorator-transform` when a package owns TypeORM entities. +A composition repository has no `find`, `save`, or `delete`, so tests cannot seed or assert through `Container.get(MyFeatureRepository)` the way older tests do. Seed and assert through the repository's use-case methods, or through `Container.get(DataSource).getRepository(MyFeatureEntity)` as `packages/cli/src/encryption/__tests__/encryption-bootstrap.service.integration.test.ts` does. Do not add a `find()` passthrough for tests. `testDb.truncate` works unchanged. + ## Future work 1. A few aspects of modules continue to be defined outside a module's dir: diff --git a/scripts/backend-module/my-feature.repository.template b/scripts/backend-module/my-feature.repository.template index a6bb1596208..b5e9c0be1f6 100644 --- a/scripts/backend-module/my-feature.repository.template +++ b/scripts/backend-module/my-feature.repository.template @@ -1,15 +1,28 @@ +import { BaseRepository, OperationContext, TransactionRunner } from '@n8n/db'; import { Service } from '@n8n/di'; -import { DataSource, Repository } from '@n8n/typeorm'; +import { DataSource } from '@n8n/typeorm'; import { MyFeatureEntity } from './my-feature.entity'; -@Service() -export class MyFeatureRepository extends Repository { - constructor(dataSource: DataSource) { - super(MyFeatureEntity, dataSource.manager); - } - - async getSummary() { - return await Promise.resolve({}); +// File-private table: never exported, never registered with DI. Only the +// repository below reaches TypeORM, so nothing leaks into business logic. +class MyFeatureTable extends BaseRepository { + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + super(MyFeatureEntity, dataSource.manager, transactionRunner); + } +} + +@Service() +export class MyFeatureRepository { + private readonly table: MyFeatureTable; + + constructor(dataSource: DataSource, transactionRunner: TransactionRunner) { + this.table = new MyFeatureTable(dataSource, transactionRunner); + } + + // Every data access is a named use-case method: plain params in, domain + // shapes out, `ctx` last, manager resolved via `this.table.managerFor(ctx)`. + async getSummary(ctx: OperationContext) { + return await this.table.managerFor(ctx).count(MyFeatureEntity); } } diff --git a/scripts/backend-module/my-feature.service.template b/scripts/backend-module/my-feature.service.template index 7836036ec51..f86a4d188bb 100644 --- a/scripts/backend-module/my-feature.service.template +++ b/scripts/backend-module/my-feature.service.template @@ -38,6 +38,6 @@ export class MyFeatureService { } async getSummary() { - return await this.myFeatureRepository.getSummary(); + return await this.myFeatureRepository.getSummary({}); } }