Conversation
Webiny stores timestamps as UTC ISO strings and expects to read the same thing back, but SQL drivers disagree about that. node-postgres and mysql2 parse date and timestamp columns into `Date` objects, while better-sqlite3 returns the text verbatim. Storage code then has to guess, and the guesses have diverged: the key-value store parses through `new Date(...)` and types its row `Date`, audit logs converts to `Date` on read while typing the row `string`, and the websockets registry compared the value as a string and silently dropped every row on Postgres. A `Date` escaping the database is a problem past the inconsistency, because storage code shared with DynamoDB writes these values straight into the document client, where a `Date` is stored as an empty object. Add `withKnexDefaults`, which installs a `postProcessResponse` hook converting any `Date` in a response to its UTC ISO string. Rows are edited in place and the original response is returned, so the extra properties a driver puts on a raw result survive. This is one dialect-agnostic place rather than per-driver settings: mysql2's `dateStrings` and pg's type parsers each need their own code, and pg's is process-wide. Wire it into both connection factories the server hosting type ships, and into the headless-cms SQL test clients so tests see what production sees. Note this is a safety net, not a licence to keep using date columns. A real date or timestamp column also reformats on the way in, dropping milliseconds on MySQL, so a column Webiny reads as an ISO string should be text. That rule is now written down in ai-context/code-style. api-core-sql had no vitest setup and so ran no tests; it has one now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🚓 Slop Cop ✅ Nothing worth flagging. The diff looks consistent with the PR's stated intent and the code-style rules. The PR is a coherent, well-scoped change matching its stated intent (adding withKnexDefaults to normalize SQL date reads to ISO strings), with no integrity or style issues found. Automated, non-blocking heads-up from an LLM. It can be wrong — use your judgment. Regenerates on every push. |
Per review, timestamps stay in normal `datetime` columns rather than moving to text. The column keeps its native semantics that way, and no migration is needed. `withKnexDefaults` is what makes the read shape consistent, so the rule now describes that as the mechanism rather than as a fallback for columns that were missed. Notes the MySQL `DATETIME` precision-0 truncation, since that is the one case where the column type still loses something. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The key-value store reads `expiresAt` back out of a `datetime` column and compares it to decide whether a record has lapsed, which is exactly the value the new Knex hook rewrites, and nothing covered it. Adds the two cases either side of the boundary: an expiry in the future still returns the value, one in the past reads as missing. Passes on both lanes, so the comparison holds whether the driver hands back a `Date` that the hook converts (Postgres) or the text it was given (SQLite). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The key-value store was the one place still handing a raw `Date` to Knex and typing its row `Date | null`, which described the write direction only. Convert to a UTC ISO string when writing and let the row type say `string`, matching how the audit-log and websockets registries already treat their timestamps. `IKeyValueStoreSetOptions.expiresAt` stays a `Date`, so callers are unaffected; the conversion happens at the storage boundary where it belongs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
/vitest |
|
Vitest tests have been initiated (for more information, click here). ✨
|
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
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.
Why
Webiny stores timestamps as UTC ISO strings and expects to read the same thing back. SQL drivers disagree about that: node-postgres and mysql2 parse date and timestamp columns into
Dateobjects, while better-sqlite3 returns the text verbatim.Storage code has been left to guess, and the guesses have diverged:
api-core-sql/src/keyValueStore/index.ts:59new Date(row.expiresAt), types the rowDate | nullapi-audit-logs-sql/src/SqliteStorage.ts:55Dateon read, types the rowstringapi-websockets-sqlThat third one shipped as a real bug: server→client WebSocket pushes matched zero connections on self-hosted SQL deployments, with no error and no failed task.
A
Dateescaping the database is a problem past the inconsistency. Storage code shared with DynamoDB writes these values straight into the document client, where aDateis stored as an empty object.What this does
Adds
withKnexDefaultsto@webiny/api-core-sql. It installs apostProcessResponsehook that converts anyDatein a response to its UTC ISO string, so adatetimecolumn has the same read shape on every driver and callers can treat a timestamp as a string throughout.Rows are edited in place and the original response is returned, so the extra properties a driver puts on a raw result (
rowCount,command,rows) survive untouched. SeveraldropAllTablestest helpers readresult.rowsoff a raw query, and they keep working.It is wired into both connection factories the server hosting type ships (
createPostgresConnection,createSqliteConnection) and into the headless-cms SQL test clients, so tests see what production sees.Columns stay as
datetime. This deliberately replaces the earlier approach in #5553, which moved the websockets columns to text and normalized per read site: one hook covers every table without a migration, and the column keeps its native semantics.Why a Knex hook rather than driver options
Per-driver settings exist, but each needs its own code and none of them give an ISO string:
"2026-08-04 17:02:06.028+00"dateStrings: true"2026-08-04 17:02:06"Those are strings, not ISO strings, so a lexicographic comparison against an ISO cutoff still breaks (a space sorts before
T). pg'ssetTypeParseris also process-wide. One Knex hook covers every dialect in a single place.Verification
yarn test:pglite packages/api-websockets: 25/25 pass. This is the load-bearing check. This branch carries no other change:WebsocketsConnectionRegistry.ts:222still declaresdatetimecolumns andListConnectionsUseCase.ts:31still does the raw lexicographic compare. The testshould properly list connectionsfailed on this lane before (expect.any(String)received aDate) and passes now purely because of the hook, which makes this the end-to-end proof that the conversion works against a real Postgres driver.yarn test:pglite packages/api-headless-cms: 295 test files, 920 passed, 16 skipped. The largest Postgres-backed suite in the repo, with its test client routed through the hook, so every row those tests read has been through the conversion.yarn test packages/api-core-sql: 11/11 pass, covering row lists, single rows from.first(), raw Postgres results, object identity, nulls/numbers/buffers, scalars, hook composition with a caller's ownpostProcessResponse, and a round trip through a real better-sqlite3 connection.api-core-sqlhad no vitest setup and so ran no tests at all; it has one now.Known limits
postProcessResponsecan't be retrofitted onto an existing instance. That covers every deployment today:GenerateApiDbConnectionsubstitutes the generated api entry's{DB_FACTORY}placeholder withcreateSqliteConnectionorcreatePostgresConnection, so those two factories are the only way a self-hosted client is built. The stale docstring oncreateSqliteConnectionsuggesting a deployment can supply its own client should be corrected separately.DATETIMEdefaults to precision 0 and truncates milliseconds on write, before any read-side setting matters. Columns that need sub-second precision should ask for it explicitly. Postgres and SQLite keep the value as written.The resulting rule
With the hook in place there is one rule, and it only governs the storage boundary:
datetime.toISOString()in the storage layer; never hand Knex aDatestringMeasured against that,
api-audit-logs-sqlwas already compliant:AuditLogRow.createdOnis declaredstring,toRowwrites.toISOString(), andfromRowrebuilding aDateis a domain choice made after the boundary. Its row type was previously a lie on Postgres and the hook makes it true, with no change to the file. Its cursor pagination readsafterRow.createdOnback into awhereclause, which now sends an ISO string that the column coerces.api-core-sql's key-value store was the only deviation, handing Knex a rawDateand typing its rowDate | null. This PR converts on write and lets the row saystring.IKeyValueStoreSetOptions.expiresAtstays aDate, so callers are unaffected.Follow-ups, not in this PR
api-headless-cms-pg-oshas its own test client factories that could route throughwithKnexDefaultstoo.self-hosted-auth-sql,api-aco-sqlandapi-audit-logs-sqlhave no vitest config, so they currently run no tests.🤖 Generated with Claude Code