sql(mysql): add utcDate option for UTC DATETIME/TIMESTAMP decoding#29216
sql(mysql): add utcDate option for UTC DATETIME/TIMESTAMP decoding#29216robobun wants to merge 6 commits into
Conversation
|
Updated 8:34 AM PT - May 1st, 2026
❌ @robobun, your commit b1740ee has 4 failures in
DetailsDetails🧪 To try this PR locally: bunx bun-pr 29216That installs a local version of the PR into your bun-29216 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughParses MySQL text-protocol date/time strings, enforces component validation, interprets components as UTC when converting to JS timestamps, updates result-set text-path to use the new parser, maps binary zero-date sentinels to NaN, and adds a regression test covering DATETIME/TIMESTAMP round-trips under a non-UTC process timezone. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the one-line binary-protocol swap and the new text-protocol parser both correctly interpret MySQL's naive datetime components as UTC.
Extended reasoning...
Overview
This PR fixes a timezone bug in MySQL DATETIME/TIMESTAMP decoding across two protocol paths in and , plus adds a regression test in .
Security risks
None. This is a pure data-decoding correctness fix with no authentication, permission, or injection surface changes.
Level of scrutiny
The changes are mechanical and focused: a one-function-name swap ( → ) for the binary protocol path, and a new ~50-line parser for the text protocol path. Both target the same clear root cause (naive datetime components being interpreted as local time instead of UTC). The binding was already present in JSGlobalObject.zig and only differs by passing . The parser is straightforward positional parsing with correct fractional-second right-padding logic. The regression test explicitly flips and round-trips through all four protocol×column-type combinations.
Other factors
The two bugs flagged as inline comments: (1) the pre-existing dead-code compile error predates this PR and has no callers, so it has zero runtime impact; (2) the unused import is a trivial nit. Neither warrants blocking the fix. Test coverage for the new behavior is solid.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29208.test.ts`:
- Around line 66-74: Test toggles process.env.TZ and is currently marked
concurrent: true which can cause race conditions; remove the concurrent: true
flag from the describeWithContainer invocation (the block using
describeWithContainer("issue `#29208` (containerized MySQL)", { image:
"mysql_plain", concurrent: true }, ...)) or replace it with concurrent: false to
ensure TZ isolation, or alternatively wrap runRoundTrip to acquire a global TZ
mutex before changing process.env.TZ; update the describeWithContainer options
or add a TZ-lock around runRoundTrip to prevent concurrent execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 38916f63-753a-4c13-9bd7-7aa2a45c2f94
📒 Files selected for processing (2)
src/sql/mysql/protocol/ResultSet.zigtest/regression/issue/29208.test.ts
robobun
force-pushed
the
farm/1208a173/mysql-datetime-utc
branch
from
c624d3a to
094d9c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sql/mysql/MySQLTypes.zig`:
- Around line 529-568: The fromText() parser currently only enforces month ∈
1..12 and day ∈ 1..31 so malformed dates like 2024-02-31 or times like 24:00:00
slip through and are later normalized by gregorianDateTimeToMSUTC(); after
parsing year/month/day/hour/minute/second (use the local variables year, month,
day, result.hour, result.minute, result.second) add strict validation: reject if
hour >= 24 or minute >= 60 or second >= 60, and validate day against the
month/Year (Apr/Jun/Sep/Nov max 30, Feb max 28 or 29 when leap year: leap =
(year % 4 == 0) and (year % 100 != 0 or year % 400 == 0)); return
error.InvalidDateTimeText on invalid values so invalid-but-storable MySQL rows
produce Invalid Date instead of being normalized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5ca67cef-c2dd-4234-b110-ffee671cea09
📒 Files selected for processing (3)
src/sql/mysql/MySQLTypes.zigsrc/sql/mysql/protocol/ResultSet.zigtest/regression/issue/29208.test.ts
There was a problem hiding this comment.
LGTM — all previously flagged issues resolved in 094d9c0.
Extended reasoning...
Overview
This PR fixes MySQL DATETIME/TIMESTAMP deserialization to use UTC instead of local time on both the binary protocol path (swap gregorianDateTimeToMS → gregorianDateTimeToMSUTC) and the text protocol path (new DateTime.fromText parser replacing the generic JS date parser). Three files changed: MySQLTypes.zig, ResultSet.zig, and a new regression test.
Security risks
None. This is a pure date-decoding correctness fix with no auth, crypto, or permission surface.
Level of scrutiny
Medium — touches production MySQL wire-protocol decoding on both binary and text paths, but the changes are narrow and mechanical. The fix is well-reasoned: the root cause (local-time constructor) is clearly identified, the fix (UTC constructor) is the right approach, and the regression test exercises all four affected scenarios.
Other factors
All three issues I raised in prior reviews were fixed in autofix.ci commit 094d9c0: (1) the zero-date regression (month < 1 or month > 12 / day < 1 or day > 31 guards added to fromText), (2) the dead-code DateTime.toJS signature mismatch, and (3) the unused afterAll import. The one remaining bug report (binary-protocol zero-date returning epoch 0 instead of NaN) is explicitly pre-existing — it predates this PR and lives in DecodeBinaryValue.zig which was not modified here.
There was a problem hiding this comment.
LGTM — all issues from prior rounds have been addressed and the fix is correct.
Extended reasoning...
Overview
This PR fixes MySQL DATETIME/TIMESTAMP decoding to interpret wire-format date components as UTC instead of local time (issue #29208). It modifies four files: MySQLTypes.zig (adds fromText, moves to gregorianDateTimeToMSUTC, fixes toJS signature, adds validation in toJSTimestamp), DecodeBinaryValue.zig (zero-length payload → NaN instead of epoch), ResultSet.zig (text path now uses DateTime.fromText + UTC conversion), and a new regression test.
Security risks
No security-sensitive code is touched. This is purely a data-decoding correctness fix with no auth, crypto, or permissions involvement.
Level of scrutiny
Medium — this is protocol-level code, but the changes are narrowly scoped, well-commented, and all edge cases (zero-dates, partial zero-dates, impossible calendar values) have been explicitly addressed through the review iteration. The fix is mechanically straightforward: swap local-time interpretation for UTC interpretation on both text and binary paths, with validation that mirrors the pre-existing behavior.
Other factors
The iterative review process was thorough: zero-date NaN consistency across protocols, partial zero-date validation in both fromText and toJSTimestamp, DateTime.toJS signature correctness, and test TZ isolation were all raised and fixed. The one pre-existing issue declined (pre-1970 date handling in gregorianDate) is genuinely out of scope for this PR. The bug hunting system found no new issues in the final state.
There was a problem hiding this comment.
LGTM — all review feedback has been addressed.
Extended reasoning...
Overview
The PR fixes MySQL DATETIME/TIMESTAMP decoding on both the binary (prepared-statement) and text (simple-query) protocol paths to interpret naive server-sent date components as UTC rather than local time. Changes span four files: MySQLTypes.zig (new fromText parser, UTC switch in toJSTimestamp, component validation, toJS compile fix), DecodeBinaryValue.zig (zero-date NaN), ResultSet.zig (text path routing through DateTime.fromText), and a new regression test.
Security risks
None. The change is confined to date/time decoding logic in the MySQL SQL driver. No auth, crypto, or permission paths are touched.
Level of scrutiny
Medium — the fix is targeted and well-motivated, touches only the MySQL date decoding path, and includes a round-trip regression test covering both protocols under a non-UTC timezone. The validation logic (month/day/hour/minute/second range checks, leap-year-aware daysInMonth) is straightforward and consistent with the pre-existing helpers already in the file.
Other factors
All substantive inline comments raised in earlier review iterations were addressed: zero-date NaN consistency across protocols, component range validation including partial zero-dates (2024-00-15), time-field bounds, the latent toJS compile error, and the concurrent: true TZ-mutation race in the test. The one declined item (gregorianDate() pre-1970 bug on the write path) is genuinely out of scope for a read-path fix and was correctly deferred. CI failures visible in the robobun comment are pre-existing infra flakes unrelated to this PR.
|
@robobun resume. Add an option defaulting to false for compatibility. Rebase off main |
MySQL's DATETIME/TIMESTAMP wire format carries no timezone — the server
returns naive year/month/day/hour/minute/second components. Bun decodes
these via JSC's local-time constructor, so under a non-UTC process TZ
the resulting JS Date is off by the client's UTC offset (the send path
already encodes UTC components, so a Date does not round-trip).
Add a `utcDate` connection option, defaulting to `false` for
compatibility with the existing local-time decoding. When `true`:
* Binary protocol: components are fed to gregorianDateTimeToMSUTC
instead of the local-time variant.
* Text protocol: the naive 'YYYY-MM-DD[ HH:MM:SS[.ffffff]]' string is
parsed directly by a new DateTime.fromText instead of the generic
JS date parser, and converted as UTC.
* Zero-date sentinels ('0000-00-00') and impossible calendar values
surface as Invalid Date instead of wrapping or the Unix epoch.
The option is threaded like `bigint`: parseOptions -> SQLQueryFlags ->
createQuery -> MySQLQuery.#flags -> ResultSet.Row / decodeBinaryValue.
Fixes #29208.
robobun
force-pushed
the
farm/1208a173/mysql-datetime-utc
branch
from
a570c95 to
d9524b0
Compare
robobun
changed the title
The mechanical gate runs the test without sourcing /opt/start-services.sh, so MariaDB is installed but not running. The local-path beforeAll would then fail to provision and early-return, making the test pass vacuously (0 expect() calls) on both with-fix and without-fix legs. Detect an installed-but-stopped server via `mysql -u root -e 'SELECT 1'`, launch `mysqld_safe` if present, and poll for readiness before provisioning. On environments without a local mysql/mysqld_safe (most CI lanes) this is a no-op and the Docker path is used instead.
|
Superseded by #31212 |
3 participants
Fixes #29208.
Repro
Under
TZ=Asia/Bangkok,d.toISOString()is2024-01-15T05:30:45.678Zbutrow.ts.toISOString()is2024-01-14T22:30:45.678Z— the client's UTC offset (−7h) subtracted.Root cause
MySQL's DATETIME/TIMESTAMP wire format carries no timezone — the server returns naive
year/month/day/hour/minute/second[/microsecond]components. Bun fed those components through JSC's local-time constructor on both protocol paths:src/sql/mysql/MySQLTypes.zigDateTime.toJSTimestampcalledgregorianDateTimeToMS(which passeslocalTime = true).src/sql/mysql/protocol/ResultSet.zigtext-protocol branch calledbun.String.parseDate, which runs the naive"2024-01-15 12:30:45"through JSC's date parser; without aZ/offset that's interpreted as local.The send path (
DateTime.fromUnixTimestamp) was already UTC-correct, so writes stored the right value and only the read path was wrong — matching the reporter'sSELECT DATE_FORMAT(ts,...)check.Fix
Add a
utcDate: booleanconnection option, defaulting tofalseso existing applications see no behaviour change. Whentrue:DateTime.toJSTimestampusesgregorianDateTimeToMSUTCinstead of the local-time variant.YYYY-MM-DD[ HH:MM:SS[.ffffff]]directly via a newDateTime.fromTextand route through the same UTC conversion (replacing the generic JS date parser for MySQL's text date/datetime rows).'0000-00-00','2024-00-15', etc.) surface asInvalid Dateinstead of the Unix epoch / wrapped months.The option is threaded exactly like
bigint:parseOptions→SQLQueryFlags.utcDate→createQuery→MySQLQuery.#flags.utc_date→ResultSet.Row.utc_date/decodeBinaryValue. It currently only affects the MySQL/MariaDB adapter.Verification
test/regression/issue/29208.test.tsflipsprocess.env.TZtoAsia/Bangkokand round-trips aDatethroughDATETIMEandTIMESTAMPcolumns on both binary (prepared) and text (simple) paths:utcDate: true2024-01-15T05:30:45.678Z(matches what was sent)utcDateomitted (default)2024-01-14T22:30:45.678Z(historical local-time decode)utcDate: false(explicit)2024-01-14T22:30:45.678Z(same as omitted)Fail-before (src/ reverted to
main, test file kept):Pass-after: all green, 5 expect() calls.
Notes
The report also mentions PostgreSQL
TIMESTAMP WITHOUT TIME ZONEbeing off by the client offset in the same scenario. The Bun-side PG binary path (src/sql/postgres/types/date.zigfromBinary) decodes microseconds-since-2000 directly into UTC ms, and bothtimestampandtimestamptztags go through that same function — so theutcDateoption is MySQL-only for now.