mysql: decode DATETIME/TIMESTAMP as UTC to match the UTC-based encode#31212
Conversation
|
@robobun adopt |
|
Updated 1:29 PM PT - May 30th, 2026
❌ @robobun, your commit bfeb3cb has 3 failures in
🧪 To try this PR locally: bunx bun-pr 31212That installs a local version of the PR into your bun-31212 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR fixes a bug where MySQL ChangesDateTime UTC Round-trip Fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
✅ Fixes naive
Verified against real MySQL and Postgres servers (docker-compose in CI, local otherwise) across Fixes the MySQL The SQL diff is green — all ASAN test lanes (which run these tests) pass. The only red CI lanes are unrelated to this change: a macOS-x64 |
|
Reworked to a real MySQL server — spawns a fixture under TZ=UTC/America-New_York/Asia-Tokyo and asserts the DATETIME round-trip is identity in each. @robobun adopt |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/sql/sql-mysql.test.ts`:
- Around line 67-78: Replace the parameterized test with describe.each instead
of test.each and make the assertion verify the actual timezone token;
specifically, change the table-runner from test.each to describe.each and update
the inner test that calls bunRun(path.join(import.meta.dir,
"sql-mysql-datetime-tz-fixture.ts"), {..., TZ}) so it expects the child stdout
to include the TZ marker (e.g. expect(stdout).toContain(`OK TZ=${TZ}`)) rather
than just "OK", ensuring the fixture/sql-mysql-datetime-tz-fixture.ts emits the
`OK TZ=${TZ}` token.
🪄 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: d689445a-16df-4814-a897-bdf7ef81882e
📒 Files selected for processing (2)
test/js/sql/sql-mysql-datetime-tz-fixture.tstest/js/sql/sql-mysql.test.ts
3208488 to
ec612d3
Compare
|
@robobun use the real mysql instance in your container and also in Ci rather than a mock server. |
bc8c311 to
ac7238f
Compare
ac7238f to
f97f7d8
Compare
|
@robobun, #29216 implemented a flag to not break behaviour. Since 1.4 is coming up, do you think it would be acceptable to make this slightly breaking change without the flag? What else was in the previous pull request that we might want to add here? Just do research and leave a comment first, before we commit to any further implementation. |
|
Research only — no code changes yet. Here's where things stand and what #29216 (closed) covered that this PR doesn't. Is dropping the flag acceptable for 1.4?I think yes, unconditional is the right call —
What #29216 did that this PR does NOT (all still gaps on the live Rust path)#29216 targeted the old Zig tree (
Suggested scope for the flagless version
Open question for you: do we also want to fix the Postgres text path ( |
|
Let's do it. PG can also go in this PR for correctness. |
f97f7d8 to
36cfe27
Compare
|
Done — implemented the full scope in MySQL
Postgres
Tests (real servers — docker-compose in CI, local instance otherwise; no mock): binary-vs-text consistency and zero-dates across I'll add a changelog/docs note about the decode change. Left |
…write path
A JS Date bound to a MySQL DATETIME/TIMESTAMP (or Postgres timestamp) and read
back was silently shifted by the machine's UTC offset on non-UTC machines.
Encode breaks the Date's UTC epoch into Y/M/D h:m:s with pure-UTC arithmetic,
but decode used the local-time conversion. Decode the stored wall-clock as UTC
so both sides agree, on every protocol:
- MySQL binary DATETIME/TIMESTAMP: gregorian_date_time_to_ms_utc.
- MySQL text (.simple()): parse components via DateTime::from_text and convert
as UTC, instead of JS Date.parse (which read them as local time).
- MySQL zero / partial-zero dates ('0000-00-00', month/day 0) surface as
Invalid Date on both paths instead of the Unix epoch / a wrapped Date.
- Postgres 'timestamp' (WITHOUT TIME ZONE) text: decode as UTC to match its
binary path; 'timestamptz' (explicit offset) and 'date' keep using Date.parse.
Tested against real MySQL and Postgres servers (docker-compose in CI, or a
local instance) — binary vs text consistency and zero-dates across
UTC / America/New_York / Asia/Tokyo.
36cfe27 to
bfeb3cb
Compare
…gres (#31663) Follow-up to #31212. ## What this does #31212 fixed naive `DATETIME`/`TIMESTAMP` text decoding to be UTC, but introduced two near-identical hand-rolled parsers for the `YYYY-MM-DD HH:MM:SS[.ffffff]` wall-clock format: - `DateTime::from_text` in `src/sql_jsc/mysql/MySQLValue.rs` - `timestamp_text_to_ms_utc` in `src/sql_jsc/postgres/types/date.rs` Both had the same digit accumulator, the same separator validation, and the same fractional-second padding. This PR extracts the structural parsing into one shared module, `src/sql_jsc/shared/datetime_text.rs`, and reduces both call sites to thin wrappers. No existing helper could be reused instead: the workspace has no date crates, and the only WTF date function exposed to Rust (`parse_date`) is JS `Date.parse` — the local-time behavior #31212 exists to avoid. ## What stays driver-specific (deliberately) - **MySQL** (`parse_mysql`): accepts the 10-byte date-only form and a `T` separator, validates calendar dates (`days_in_month`) and h/m/s ranges, keeps microsecond precision. - **Postgres** (`parse_postgres_timestamp`): requires the full `YYYY-MM-DD HH:MM:SS` shape — anything else returns `None` so the caller falls back to `Date.parse` (`infinity`, BC dates, 5+ digit years); no range validation (delegated to `gregorianDateTimeToMS`); millisecond precision. Public signatures of both wrapped functions are unchanged, so no call sites move. ## New test coverage The existing roundtrip tests skip their assertions when no real MySQL/Postgres server is reachable, which left the text-protocol date decode path untested without Docker. This PR adds a mock-MySQL-server test (same approach as `sql-mysql-raw-length-prefix.test.ts`) that exercises text-protocol `DATE`/`DATETIME` decode across `Etc/UTC` / `America/New_York` / `Asia/Tokyo`, including zero dates and impossible calendar dates. ## How we know it works - `USE_SYSTEM_BUN=1 bun test test/js/sql/sql-mysql-datetime-roundtrip.test.ts` → **3 fail** on Bun 1.4.0 (text dates read as local time; `2024-02-31` wraps to March 2 instead of Invalid Date) - `bun bd test test/js/sql/sql-mysql-datetime-roundtrip.test.ts` → **6 pass** - Full SQL suite (`sql-mysql-mediumint`, `sql-mysql-raw-length-prefix`, `sql-mysql-bind-oob`, both datetime roundtrip files) passes with this change. - The CI docker lanes run the real-server roundtrip tests, which assert binary/text agreement for both drivers across the same three timezones.
Merging main brought #31212's decode-side calendar validation (DateTime::from_text / to_js_timestamp), which calls days_in_month — a helper this branch had removed together with the loop-based gregorian_date. Restore is_leap_year/days_in_month for the decoder; the Hinnant-based encode path still doesn't need them. Drop the file-level process.env.TZ pin from sql-mysql.test.ts: its own comment said it could go once the decoder moved to UTC, which #31212 did. Main's TZ-parametrized round-trip tests (UTC / New_York / Tokyo) pass without it.
Fixes #29208
A JS
Datebound to a MySQLDATETIME/TIMESTAMPand read back was silently shifted by the machine's UTC offset on non-UTC machines (e.g. +240 min underTZ=America/New_York). Encode and decode used different timezone conventions:from_unix_timestampbreaks the Date's UTC epoch into Y/M/D h:m:s with pure-UTC arithmetic (writing the UTC wall-clock), butto_js_timestampdecoded via the local-time conversion, interpreting the stored wall-clock as the client's local time.Decode the stored wall-clock as UTC (
gregorian_date_time_to_ms_utc) so both sides agree, giving a timezone-independent identity round-trip — consistent with how Bun's Postgres driver already handles dates.Adds a mock-MySQL-server round-trip test run under UTC / America/New_York / Asia/Tokyo. (Not a port regression — the Zig path has the same asymmetry.)