Conversation
updateBy and updateManyById built their SET clause with the raw column name updated_at. Drizzle's buildUpdateSet keeps only keys that exist in the table's JS column map, where the property is updatedAt, and mapUpdateSet forwards an SQL value without checking the key exists, so the stamp was dropped with no error and no update routed through the base class ever moved the column. Both methods now go through one toUpdateSet helper that sets the schema-typed key. The stamp is spread first so a caller that passes its own updatedAt still wins, which is what the trial abuse enforcement writes rely on today and matches how drizzle resolves a set value against an onUpdate function. A payload that reduces to nothing used to build a SET clause with no assignments at all, which Postgres rejects; it now emits a bare updated_at touch. The spec drives the real repositories against a driverless drizzle instance and asserts the statement that reaches the driver, since the defect lives in the SQL drizzle emits rather than in the object handed to set(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… does toInput mutated the caller's payload to set updatedAt on every update, which was only ever needed because the base class's own stamp never reached the database. With that fixed the timestamp comes from now() in the same statement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two lint errors, both from this branch. TxService is only used as a type argument to mock(), so the import has to be a type import. The larger one: the spec built its database with drizzle.mock(), and the api's no-restricted-syntax rule matches any call to a member named mock, since it is there to force module mocks into the isolated project. Drizzle's factory is not a module mock, but the selector cannot tell the difference, and renaming the import to slip past it would hide the rule from the next reader. A real postgres client works instead: it opens nothing until a query runs, and its unsafe method is stubbed out before one can. The stub records the statement and throws, which is also what lets it type check, as a body that ends in a throw returns never and never satisfies PendingQuery. Capturing the driver call itself is closer to the defect than the query logger was. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesRepository update timestamp handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The timestamp update behavior is covered across repository update paths, including caller overrides and tables without timestamp columns. No merge-blocking risk remains. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
apps/api/src/core/repositories/base.repository.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. apps/api/src/core/repositories/base.repository.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3923 +/- ##
==========================================
- Coverage 82.28% 81.70% -0.58%
==========================================
Files 1276 1176 -100
Lines 35230 32617 -2613
Branches 8542 8019 -523
==========================================
- Hits 28989 26651 -2338
+ Misses 5516 5260 -256
+ Partials 725 706 -19
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — a small, well-isolated fix with solid pinned-SQL test coverage.
What was reviewed: the toUpdateSet helper and its use in updateBy/updateManyById (spread order lets a caller-supplied updatedAt win over the now() default); removal of DeploymentSettingRepository.toInput — checked that every other write path in that repository already sets updatedAt: sql\now()`explicitly, so it's unaffected; checked the other two repositories withtoInput overrides (UserRepository, UserWalletRepository) and confirmed neither touches updatedAt, so they're unaffected too; new base.repository.spec.tsfollows thesetup()-function convention and pins the actual emitted SQL for tables with and without an updatedAt` column.
Extended reasoning...
Overview
The diff touches three files: apps/api/src/core/repositories/base.repository.ts (extracts a toUpdateSet helper used by updateBy/updateManyById to correctly stamp updatedAt using the Drizzle schema property name instead of the raw, silently-ignored updated_at column key), apps/api/src/deployment/repositories/deployment-setting/deployment-setting.repository.ts (deletes a now-redundant toInput override that was papering over the bug with a JS-side new Date()), and a new base.repository.spec.ts unit test that pins the exact SQL emitted for four scenarios (stamp on a table with the column, caller-supplied override wins, empty payload still stamps, table without the column is unaffected).
Security risks
None. This is a data-integrity/audit-trail fix (ensuring updated_at timestamps are actually persisted); it does not touch authentication, authorization, or any user-controlled input in a way that introduces injection or exposure risk. All values are still parameterized through Drizzle's query builder.
Level of scrutiny
This warranted a careful read since BaseRepository is shared across many repositories in apps/api, so a mistake here could have wide blast radius. I traced every other consumer of the removed toInput override pattern and every call site in DeploymentSettingRepository that mutates updatedAt to confirm none of them depended on the deleted workaround. I also verified the spread order in the new toUpdateSet helper preserves the "caller override wins" semantics claimed in the PR description, and cross-checked the new test's expected table/column names against the actual DataKeys and Users/userSetting schemas and repositories.
Other factors
The new test file follows the repo's setup()-instead-of-beforeEach convention (no shared state, setup at the bottom, though it takes no parameters here — consistent with existing precedent elsewhere in the codebase where no parameterization is needed), uses mock<T>() from vitest-mock-extended rather than as unknown as, and is correctly named/colocated as *.spec.ts. The change is small, mechanical, and the test suite directly demonstrates the fix by pinning the actual generated SQL, which gives strong confidence. No outstanding review threads or CHANGES_REQUESTED reviews are visible in the timeline.
1 participant
Why
Fixes bug
BaseRepository.updateBy,updateByIdandupdateManyByIdrefreshed a row's modificationtimestamp by putting the raw column key
updated_atinto Drizzle's.set(). Drizzle builds anUPDATE's SET clause from schema property names (
updatedAt), and silently ignores a key it doesnot recognise — so the stamp never reached the database and no error was raised.
Every row updated through those methods kept its creation-time timestamp. That matters most for
data_keys: a KMS rotation re-wraps thousands of rows throughupdateById, and each one kept itspre-rotation
updated_at, leaving operators no way to audit when a rotation actually touched a row.What
BaseRepositorynow routes both update paths through onetoUpdateSethelper that uses theschema key
updatedAt, so the stamp is emitted as"updated_at" = now().updatedAtstill wins — the spread order makes the stamp a default, not anoverride.
updatedAtcolumn (e.g.userSetting) are unaffected: no clause, no crash.DeploymentSettingRepository.toInputis deleted. It was a per-repository workaround that injecteda JavaScript
new Date()to paper over the gap; the base class now covers it, and the stamp comesfrom the database clock instead of the API process's.
base.repository.spec.tspins the emitted SQL for all four cases by stubbing the driver.No migration, no API contract change.
apps/apiis green onnpm test,npm run lint -- --quietand
npx tsc --noEmit.Demo
BaseRepository stamps updated_at with the schema key
2026-09-11T14:45:47Z by Showboat 0.6.1
BaseRepository.updateByandupdateManyByIdrefreshed a row's modification timestamp by puttingthe raw column key
updated_atinto Drizzle's.set()call. Drizzle builds an UPDATE's SET clausefrom schema property names, not from database column names, and it silently ignores a key it
does not recognise. The column is
updated_at; the schema property isupdatedAt. So the stamp wasdropped on the floor, and the column kept whatever value it was given when the row was created.
This branch routes both methods through one
toUpdateSethelper that uses the schema key, and dropsthe per-repository workaround
DeploymentSettingRepositoryhad grown to paper over the gap.What follows compiles the SQL that the real repository classes emit — first on this branch, then on
main— so the difference is visible as a statement rather than as a test result.The probe
There is no Postgres in this sandbox, so instead of asserting on a stored row the demo hands the
real repository classes a Drizzle instance whose driver is replaced by a recorder: every statement
Drizzle hands to the driver is captured verbatim and the call is then aborted. What gets printed is
the exact SQL that would have reached the database.
It exercises three tables on purpose —
data_keys(has anupdatedAtcolumn),userSetting(hasnone), and
deployment_settings(the table whose repository carried the workaround).Every
data_keysupdate carries"updated_at" = now(). A caller who passes an explicitupdatedAtstill wins — the stamp is a default, not an override.
userSetting, which has no such column, isleft alone rather than being handed a column it does not have.
The same probe against
mainNow the two files are replaced with their
maincontents and the identical probe is run again. Theprobe itself is untouched, so any difference in output comes from the repository code.
Three statements changed, and they are the bug:
data_keys / updateBy,updateByIdandupdateManyByIdemit noupdated_atat all onmain. Drizzle accepted theupdated_atkey, found no schema property by that name, and droppedit. Every row updated through these methods kept a stale modification timestamp, with no error
anywhere to say so.
deployment_settings / updateBydoes carry a stamp onmain, but as a bound parameter$2rather than
now()— that is thetoInputworkaround inserting a JavaScriptnew Date(), so thevalue came from the API process's clock instead of the database's. On this branch it is
now(),consistent with every other table, and the workaround is gone.
updatedAtcase is identical on both sides, which is the point: the fix addsa default without taking the override away.
Finally, the probe is removed and the working tree is shown to be back where it started, so nothing
in this demo leaves a trace.
Summary by CodeRabbit
Bug Fixes
Tests