Repository navigation
Conversation
…tions
The hook examples in nestjs-core's and nestjs-repository's READMEs
treated WhereClause as a plain object and referenced APIs that don't
exist, so they didn't compile as written. Rewrite them against the real
OverlayRef/Where API and the new { replace: true } option, and add a
wiring checklist covering the CoreModule import step implementers were
missing. Document RS256/JWKS verification and the secretOrKeyProvider
exclusion in nestjs-authentication's README.
pretest already runs tsc -b before pnpm test so a clean clone doesn't fail on missing dist output, but test:e2e had no equivalent — pnpm's pre<script> hooks only match exact script names. Add pretest:e2e.
# Conflicts: # .github/workflows/ci-pr-test.yml
…tity re-reads update/replace on a versioned entity re-reads the row after the optimistic- lock guard to get its current state, and upsert re-reads the row it just wrote to return it — but both re-reads omitted withDeleted, so TypeORM's default soft-delete filter dropped a row the preceding write had just matched or created. update/replace threw "Entity not found after update" (#471); upsert threw "Upsert failed: entity not found after upsert". Both re-reads are identity lookups of a row whose existence the write already proved, so withDeleted: true is unconditionally correct here regardless of whether the row happens to be soft-deleted.
…rce escape hatch
update/replace/upsert reaching a soft-deleted row was never a deliberate
design choice, just incidental reuse of a shared lookup helper: crud's
includeDeleted query param is documented and exposed via Swagger for List
and Read only, no test anywhere mutated a soft-deleted row, and every
domain repository already treats a soft-deleted row as invisible (zero
occurrences of withDeleted outside this package). Left alone, that made
these writes invisible to every internal read path — the exact "ghost
data" a soft-delete filter exists to prevent.
Enforce immutability once, in RepositoryAdapter itself, rather than per
driver: update/replace/upsert now reject a soft-deleted target with the
new SoftDeletedImmutableException (409), restore() remains the sanctioned
way back, and { force: true } is available on update/replace/upsert
options for server-side carve-outs (pre-purge PII masking, admin
corrections) that HTTP callers can't reach. upsert has no existing row to
check against — its guard reads the target via the protected doFindOne
(bypassing the find permeator and its hooks) with withDeleted: true.
Cost of adopting this now, in v8 alpha, is close to zero: every entity
extending the documented Audit*/Common* base classes was already 100%
broken for this path (#471), so nothing depends on the old behavior for
those, and no existing test exercised it for any entity shape.
softDelete() on an already-soft-deleted row reached the driver unchanged, and TypeORM's soft-delete write unconditionally re-stamps the delete-date column — so a redundant soft-delete call silently overwrote the original deletion timestamp. Make it a no-op instead: idempotent, not rejected, since a retried HTTP DELETE must not fail.
…e, replace and delete paths The in-request optimistic lock added in #469 compares a version the server itself just read, so it only closes a race between concurrent in-flight writes — it cannot stop two separate requests that each re-read before writing, which is the case optimistic locking exists for (#472). Add an `expectedVersion` option to `update`, `replace`, `delete`, `softDelete`, and `restore`. `RepositoryAdapter` resolves whether a version guard applies and what value to check into a `RepositoryVersionGuardInterface` descriptor, so a driver never decides policy — it only executes the compare-and-swap it's handed. `update`/`replace` are guarded whether or not a caller supplies a version (matching #469's existing behavior); the three delete-path methods only take on the guard, and its transaction requirement, when a caller actually asks for one, keeping this non-breaking for every existing call site in the repo. A mismatch throws the existing `OptimisticLockException` (409). An `expectedVersion` against an entity with no version column throws a `fault: 'usage'` RuntimeException. `deleteMany` is excluded at the type level via a new `RepositoryDeleteOneOptions` — there is no single caller-held row for it to describe.
…te, soft-delete and restore Update/replace already ran the atomic version check inside a transaction (#469). Extend the same mechanism to delete, softDelete, and restore, but only when the caller supplies `expectedVersion` — `RepositoryAdapter` resolves that into a `versionGuard` descriptor, and this driver only ever executes it, never decides whether one applies. That conditional is what keeps this non-breaking: no existing caller passes `expectedVersion`, so none inherits a new transaction requirement. `saveWithVersionCheck` is split into a shared `withVersionGuard` (the transaction handling and the atomic compare-and-swap) and `saveWithVersionGuard` (update/replace's merge-and-save operation), so `doDelete`/`doSoftDelete`/`doRestore` can reuse the same guard without update/replace's re-read-and-merge logic. Also corrects two stale claims in the guard's doc comment: `save()` does not unconditionally bump the version column (TypeORM issues no SQL at all for a no-op change set), and a client-supplied version survives by suppressing the auto-increment, not by coexisting with it.
Closes #472. The in-request compare-and-swap (#469) only guards a read-then-write inside one request, so two people editing the same record from a screen still lose the earlier save silently: A and B both read version 1, B saves (-> 2), A saves and, without this change, still succeeds (-> 3). Clients now state the version they read via `If-Match`, threaded down to the repository's `expectedVersion` option (see the nestjs-repository/-typeorm commits). A mismatch returns 409 (`OptimisticLockException`, the same exception and error code the in-request guard already used) rather than RFC 9110's 412, so clients handle every version conflict on this API the same way. - `crud-precondition.parser.ts` accepts one strong entity-tag (`"3"`) or `*`; comma-separated lists and weak tags are RFC 9110 cache- revalidation features, not lost-update prevention, and 400 along with any malformed value. - `CrudContextOverlay` parses and validates `If-Match` only for Update/Replace/Delete/SoftDelete/Restore — a precondition has no meaning on a read, so a stale or malformed header on a GET is simply not evaluated. Resolving the entity's version column can legitimately fail for a hand-decorated `@CrudController` using a custom resolver (no adapter provider registered under the dynamic token) — treated the same as "no version column" (400), never a 500. - `CrudETagInterceptor` emits `ETag` on single-resource responses, registered per-controller like the serializer (not globally like `CrudContextOverlay`, which has no other way to learn a route is CRUD-decorated) — every route that could need it gets it, and no unrelated hand-written controller pays for it. Suppressed when the client narrowed the response with `?select=`, since the tag is a row validator that doesn't vary by representation. - `@CrudRequireVersion()` rejects with 428 when a route demands a precondition and none was sent — `If-Match: *` does not satisfy it, since it states no version at all. A dedicated `versioned` fixture backs the new e2e coverage rather than the `photo` fixtures: `photoSchema` doubles as a controller-level request body elsewhere, so a required `version` field there would have made it a mandatory POST field and broken that fixture's own tests. Also fixes a latent type-soundness gap in two `CrudContextInterface` mock helpers (nestjs-crud, nestjs-cache), surfaced by the interface gaining new fields: both handed a generically-typed value to a concretely-typed overlay slot.
The alpha.11 version bump (21c541c) updated only the 13 package version fields and missed the exact peerDependencies pins that every prior bump kept in lockstep, leaving eleven pins across seven v8 packages declaring a requirement on the now-superseded alpha.10.
… drivers SQLite (and 7 other TypeORM drivers: better-sqlite3, sqljs, expo, capacitor, cordova, nativescript, react-native) share one QueryRunner per DataSource, so two transactions started concurrently can both decide no transaction is open and both issue a real BEGIN — the second fails at the driver level with "cannot start a transaction within a transaction" instead of running the version guard (#476). TransactionFactoryInterface gains an optional supportsConcurrentTransactions flag (default true). TransactionFactoryRegistry owns a per-key TransactionQueue and TransactionManager now acquires it before starting a transaction and holds it for the whole BEGIN...COMMIT/ROLLBACK lifetime, so a second transaction on the same connection waits instead of racing. The queue is abort-aware, so a timed-out scope still parked in the queue rejects immediately rather than waiting for the holder. TypeOrmTransactionFactory is the only adapter-specific piece: it declares the flag based on the driver type, keeping the serialization mechanism itself in the abstraction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… it rests on
Row scope restricts which rows a caller may read and write. Enforcement sits at
every repository entry point, so layers above it are scoped without code of
their own — a nestjs-crud controller over a scoped entity is scoped with no
filter, guard or interceptor added.
RowScopeBase implements four rules and nothing else: reads inject the scope
columns into the WHERE; writes naming an existing row pre-check on the primary
key plus the scope columns; create — and an upsert whose key names no stored
row — is new data and gets stamped; and the scope columns take precedence over
any same-named payload value, refusing a mismatch by whichever route it
arrives.
Three repository defects were found underneath it and fixed there rather than
compensated for above. Each was reproduced on an entity with no row scope at
all:
- update/replace let `data` override the primary key supplied by the entity
argument, writing to a row the caller never named and leaving the named row
untouched. Refused by assertKeyUnchanged (PrimaryKeyImmutableException).
- A relation object on the entity argument decided which row was written, since
a driver resolves a column from its relation in preference to the column's
own scalar. withoutRelations strips them on all seven writes that name an
existing row, including softDelete's already-deleted early return.
- create read the raw payload to decide whether its key was taken, missing a
key supplied as `{ account: { id } }`. assertNotExisting now reads the
driver's transform() output.
Also included:
- core: an `immutable` option for context overlays, so a scope resolved before
hooks cannot be replaced downstream.
- repository: close a fail-open in the where-clause AST, where an empty AND/OR
collapsed to "no constraint" rather than matching nothing.
The load-bearing invariant throughout is that a column is read through the
repository's transform(), never off the caller's raw payload: a value can
arrive as its own scalar or through a relation object, and reading the payload
directly has defeated five separate guards.
…ial composite key
Four fixes found by auditing the READMEs against the code. Two were options
the docs promised and the implementation ignored, which is worse than an
unimplemented option because nothing tells the caller.
- core: `@UseHooks({ hook, spec })` accepted a specification that was never
evaluated, so the hook ran on every operation. The value was stored on the
resolved hook and read nowhere; gating comes from the hook-method argument, a
method-level `@Specification`, or the class-level `@Hook`/`@RepoHook`.
`spec` is removed from `HookWithSpec` — nothing used the object form.
- repository-typeorm: a `joinType: 'INNER'` clause was discarded, so
`Join.inner()` behaved exactly like `Join.left()` and returned the rows the
caller asked to exclude. TypeORM's `relations` find option is always a LEFT
JOIN, so the driver now refuses INNER rather than answering wrongly.
Federation is unaffected: it implements INNER itself and strips federated
joins before the driver sees them.
- repository: `create` with only part of a composite primary key failed as a
500 reporting that it could not *update* a row, naming no column. A partial
key also cannot be looked up, so the existence check never ran. Now a 400
(`PartialPrimaryKeyException`) naming the missing columns. Supplying none of
them is still the ordinary generated-key create.
- crud: export `createCrudAdapterProvider`, `createQueryHandler` and
`createCommandHandler`. The package demonstrates hand-written CRUD
controllers by example but reached these through deep relative imports, so an
external consumer could not reproduce the pattern.
Also corrects two TSDoc blocks that contradicted the code: `@AfterRead` is
wired only into `find` and `findOne`, and `RepositoryAdapter`'s `@example`
overrode the public operation methods its own documentation forbids.
An audit of every README overlapping `@concepta/nestjs-repository`, checking each claim against the implementation rather than against neighbouring comments. Snippets were compiled rather than re-read, which is what found the ones that could not have worked. Could not compile as written: - repository: the driver example used an unconstrained `<Entity>` against a base declared `<Entity extends PlainLiteralObject>`. - cache, user: wiring snippets named entity classes that do not exist (`UserCacheEntity`, `UserEntity`, `UserCredentialEntity`) instead of the exported `*SqliteEntity` classes their own tables list. - crud: `@CrudCommandHandler` takes an options object, not a bare class. Could not have worked: - otp: the HTTP wiring example never registered the entity with `RepositoryModule`, which the OTP repository provider injects — the module as documented fails dependency resolution at startup. - role: the entity example declared `version` and `dateDeleted` with plain `@Column()`. The repository finds them via TypeORM's `isVersion` / `isDeleteDate` flags, which only `@VersionColumn` / `@DeleteDateColumn` set, so the example booted with optimistic locking and soft-delete immutability both silently off. - cache: the repository-resolution snippet resolved by `ctx.entity`; handlers resolve by the operation's `namespace`. Claims the code contradicts: - crud: `includeDeleted` was documented as read-only and always 409. It reaches writes too — without it they 404, with it update/replace 409. - repository-typeorm: `expectedVersion` on an already-soft-deleted row was documented as voiding idempotency. The guard resolves before the no-op, so a stale version conflicts and a matching one still no-ops. - repository: `@AfterRead` covers `find` and `findOne` only; the two READMEs disagreed and the TypeORM one was right. `resolveJoinClauses()` validates relation names rather than resolving structural properties. - otp: three wrong entries in the repository table (`get` returns a nullable, and two options named `cutoffDate`). Plus: the exception tables gain the rows they were missing, `joinType` now documents the driver refusing INNER, and each consumer README says where `rowScope` is declared for entities carrying an owner column.
…overage with the code Follow-ups from the README accuracy audit, none of them behaviour changes. - crud README's hook example used `ctx.params` and `ctx.operation`, which are not on what a repository hook receives: the CRUD request is a `CrudCtx` overlay. Its parameters were also untyped (two TS7006 errors under a consumer's `strict` tsconfig), and `with()` throws when an overlay is absent, so the example would have broken any service reaching the same repository outside a CRUD route. Rewritten typed, through the overlay, and guarded on `supports()`; compiled verbatim to confirm. - Controller-level `transactional` was undocumented. It applies a class-level `@Transactional()` and opts read operations out; an operation's own setting overrides it. - The bare-id relation note claimed the driver rejects that form on a declared join column. Probed against a real driver: nothing is stored, but over HTTP the request schema refuses it first, and through the repository it surfaces as a query error. Reduced to what is true. - Row-scope fixtures named `PlainLiteralObject` instead of their entity, which discards the compile-time column check the README promises. The constraint accepts a TypeORM entity class, so the weaker typing was never necessary; a misspelled scope column is now a compile error rather than a boot failure. - `assertKeyUnchanged` runs after hooks and nothing covered it. The new cases pass no key at all, so a hook is the only source of the change.
The flakiness item is the one with a running cost: a red suite that goes green on retry trains everyone to re-run rather than look, so a real regression in a transactional path would be dismissed as it. The Join.inner note claimed implementing it meant routing joined reads through TypeORM's QueryBuilder. A NOT_NULL predicate on the related key would also do it, since a LEFT join plus a predicate on the joined column already excludes non-matching roots — cheaper than the recorded estimate, and never explored when the item was deferred.
…rwarded ctx
A hook on entity A that forwarded its ctx into entity B's repository ran none
of B's hooks, silently. `HookResolverService.execute` read the hook list off
the context, and a context carries the hook list of whatever created it — so a
nested call inherited the caller's hooks and found none of its own. Forwarding
the ctx is mandatory for the transaction, and forwarding it is what broke the
hooks. For a stamp or scope hook that meant a row written with no isolation
applied, with nothing thrown and nothing logged.
`execute` now takes the hook list as a parameter and no longer reads `ctx` for
it, so resolution cannot depend on provenance. Hooks are registered per entity
in `RepositoryModule.forFeature` and held on the adapter, which also makes them
run outside HTTP — a queue consumer, a seeder, a test — where `@UseHooks()`
had nothing to attach to.
A declared hook that could never run, or would run twice, is a failure rather
than a silent no-op: a missing CoreModule, a declaration nothing bound, a
class registered twice for one entity, a hook without `@RepoHook()`, one
decorated for another subsystem, and one the container cannot resolve. The
first two are also refused on the call itself, covering an app that never
reaches the startup checks, including the no-op softDelete that returns before
the permeator.
`@UseHooks()` keeps working. A class carried on the context that is also
registered runs once, warning per entity. The two registration sites differ in
reach, and that is the choice to make rather than reaching for a specification:
`@UseHooks` puts its list on the request's ctx, so it covers every entity that
ctx reaches and nothing outside HTTP, which is right for a cross-cutting hook;
entity registration reaches one entity by construction. Documented in
nestjs-core, which owns the decorator, along with removing a stale
`@UseHooks({ hook, spec })` example and table row for a capability that no
longer exists.
Two behavior changes: a repository call with no ctx now runs registered hooks,
so a hook reading an overlay must guard with `supports()`; and `execute`'s
signature gained the hook list.
Closes #477
…eading as a thenable `require(...refs)` narrowed types and checked nothing, so the name promised an assertion it did not make. It now throws `OverlayNotDefinedException` on the first absent ref — the same exception `with()` raises, so an absent overlay fails one way whichever accessor found it. Presence is tested with `name in this`, which sees an overlay inherited from a parent: every repository call builds its ambient context as a prototype child of the caller's, so checking own properties only would refuse every real request. This is narrower than it looks and worth stating plainly: the host's constructor proxy already threw for any absent `with*` property, so `require(ref).withRef()` was fail-closed before this change. What the assertion newly catches is `require(A, B)` where only `withA()` is read, and a narrowed context held without immediately reading through it. `optional()` answered every property with a callable, including `then`. A proxy with a callable `then` is a thenable, so awaiting one — `await ctx.optional()` — handed the runtime a `then` that never called back and hung forever. Only `with*` properties resolve to a callable now. Absence is also decided by looking the method up rather than by catching, so an error raised by an overlay cannot be reported as a missing one. `OverlayNotDefinedException` named only a missing route interceptor. A context reaches the same code from a nested write, a queue consumer, a cron job, a seeder or a test, where there is no route and no interceptor to apply, so the message now names both causes.
…ast its absence
The tenant-isolation example failed open in both halves. `addTenantFilter`
returned the caller's options unchanged when the tenant overlay was absent,
reading across every tenant; `stampTenant` returned the caller's payload, and
under `{ replace: true }` the hook's output wins — so a caller-supplied
`tenantId` survived the stamp it was supposed to be overwritten by. The three
copies of that example, and the user scope-hook fixture, now read the overlay
with `require()` and let an absent one refuse the call.
Which of the two a hook wants is the hook's decision, not something to
standardise: an ambient audit or metrics hook should run without a principal,
and a credentials lookup during login has to work before any authorized user
exists. The two genuinely optional reads are left on `supports()` with a line
each saying why — crud's fallback to a `'repository'` label, and core's read of
a hook list that is absent for a route declaring none.
`typeorm-nested-hook-overlay.spec.ts` covers the shape against a nested write:
a hook registered for one entity writing to another and forwarding its
context, where that context carries no tenant. Reverting the hook to the
branching form writes the row with `owner: 'caller-supplied'`. Note what the
refusal does not do — the outer row is already written by the time an
`after*` hook runs, so a refusal there leaves a partial write unless the
operation is wrapped in a `TransactionScope`.
The hooks spec's copy of the same example claimed to reproduce the README
verbatim and no longer did.
None of the thirteen published packages carried any of the three, so a consumer had no route from `node_modules` back here and `npm repo`/`npm bugs` both failed. The URL is `btwld/nestjs-modules`; the issue proposed `conceptadev`, which still redirects, which is why the stale path went unnoticed. The fourteen private pre-v8 packages are untouched. `repository.directory` differs per package and a wrong one is invisible until someone follows the npm link and lands on a sibling, so the smoke test now checks all three fields and that the directory names the package's own. `nestjs-federated` was also publishing with no `description`. Provenance is left out: it needs a CI publish job, and releases are manual.
1.x drops the sqlite3-backed `sqlite` driver, the string-array `select` form and `DataSource.name`. Fixtures move to better-sqlite3, a peer of both lines, and three sites translate rather than branch on version. Held below 1.1, which counts distinct values of the selected columns when find options carry a `select` (typeorm#11965): a projected paginated query then reports a distinct count instead of a total. Keeping the projection out of the count query is deferred. CI runs a leg per supported line — only one TypeORM instance can be installed at a time, so the 0.3 leg rewrites the workspace pin.
…acklog The flake is a faker collision against a unique constraint, not the parallel-load condition already listed — cross-referenced, since it may account for some of that item's sightings.
Brings in the repository link fix after the org transfer (#482). That commit is purely `conceptadev/rockets` -> `btwld/nestjs-modules`, so every content conflict resolved to the v8 README with the substitution reapplied — both sides' intent, nothing of either dropped. READMEs for the nine packages v8 removed stay deleted: six core auth modules consolidated in 6a0ef11, plus nestjs-jwt, nestjs-typeorm-ext and typeorm-common. Those directories hold no other file on this branch.
#482 fixed `conceptadev/rockets` on main. The v8 branch also carries `conceptadev/nestjs-modules` in its per-package NestJS Dep badges, and three v8-only READMEs main has no copy of, so the rename was incomplete here. The `@conceptadev/` npm scope in some v7 download badges is a separate typo and is left alone.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 3 minor 24 high 27 critical 20 medium |
🟢 Metrics 1942 complexity · 304 duplication
Metric Results Complexity 1942 Duplication 304
🟢 Coverage 91.69% diff coverage
Metric Results Coverage variation Report missing for 97c106c1 Diff coverage ✅ 91.69% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (97c106c) Report Missing Report Missing Report Missing Head commit (702b95b) 6944 6376 91.82% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#463) 6657 6104 91.69% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ctly The published peer range's `^0.3.0` lower bound admitted versions carrying known SQL-injection advisories: GHSA-q2pj-6v73-8rgj (high, via repository.save/update, patched 0.3.26) and GHSA-9ggv-8w38-r7pm (orderBy on MySQL/MariaDB, patched 0.3.29). Raise the floor to ^0.3.31, the top of the 0.3 line, across the seven published v8 packages. The seven private v7 packages keep ^0.3.0 — they are install-only and move when ported. The six dependents also now admit 1.0.x, matching the driver. They were left at 0.3-only when the driver gained ~1.0.0 support, so pairing them with typeorm@1.0.0 raised an unmet optional peer for the version the driver had just started supporting. The 0.3 CI leg wrote `^${matrix.typeorm}` into the override, so it installed whatever the newest 0.3.x was rather than the version the matrix named. Drop the caret so the matrix value is authoritative; a future 0.3 release now needs a deliberate bump instead of silently changing what CI covers. Codacy config: the `**/**.spec.js` excludes matched nothing in a TypeScript repo, leaving 71 spec files outside `__tests__/` under analysis — corrected to `.ts`. Excluded `scripts/**`, which holds only the private ESM smoke test and whose findings are all it importing the repo's own built output. Record CVE-2026-73651 on the TODO item as deliberately accepted: it affects >= 1.0.0, < 1.1.0, and 1.1.0 is the only patched 1.x, so the pin cannot clear it. The vector is template-literal injection in `migration:generate`, which nothing here invokes. Landing the deferred count fix is what resolves it.
This branch has not been deployed
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
2 participants
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.
No description provided.