sql: share the wall-clock datetime text parser between MySQL and Postgres#31663
Conversation
…gres PR #31212 fixed naive DATETIME/TIMESTAMP text decoding to be UTC, but did so with two near-identical hand-rolled parsers: DateTime::from_text in mysql/MySQLValue.rs and timestamp_text_to_ms_utc in postgres/types/date.rs. Both parsed "YYYY-MM-DD HH:MM:SS[.ffffff]" with the same digit accumulator, separator checks, and fractional-second padding. Extract the structural parsing into shared/datetime_text.rs and reduce both call sites to thin wrappers. Driver-specific semantics stay at the call sites: - MySQL accepts the date-only form and a 'T' separator, validates calendar dates (days_in_month) and h/m/s ranges, and keeps microsecond precision. - Postgres requires the full "YYYY-MM-DD HH:MM:SS" shape (anything else returns None so the caller falls back to Date.parse), does not range-check, and truncates to milliseconds. Also adds a mock-MySQL-server test for the text-protocol DATE/DATETIME decode path so it is covered without Docker or a live server: the existing roundtrip tests skip their assertions when no real MySQL is reachable, which left this path untested locally. The test fails on Bun 1.4.0 (text dates were read as local time; impossible dates wrapped instead of producing Invalid Date) and passes with this change.
|
Updated 3:26 PM PT - Jun 1st, 2026
❌ @robobun, your commit c133ed5 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31663That installs a local version of the PR into your bun-31663 --bun |
|
Warning Review limit reached
More reviews will be available in 15 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR extracts datetime text parsing logic common to MySQL and Postgres SQL text protocols into a shared ChangesSQL datetime text parsing consolidation
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 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-datetime-roundtrip.test.ts`:
- Around line 64-68: Remove the ASAN stderr filtering introduced around the
diagnostics variable: stop splitting and filtering stderr for "WARNING: ASAN
interferes" and instead assert stderr directly; locate the code that builds
diagnostics (using stderr, diagnostics) in the test and replace the filtered
diagnostics assertion with a direct expect(stderr).toBe("") to ensure any stderr
output is caught.
🪄 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: a0840486-9ce2-4764-84cb-8ed44386e686
📒 Files selected for processing (6)
src/sql_jsc/lib.rssrc/sql_jsc/mysql/MySQLValue.rssrc/sql_jsc/postgres/types/date.rssrc/sql_jsc/shared/datetime_text.rstest/js/sql/sql-mysql-datetime-roundtrip.test.tstest/js/sql/sql-mysql-datetime-text-mock-fixture.ts
|
@robobun fix review comments |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the shared parser is a faithful union of the two originals and the wrappers preserve each driver's prior behavior — but since this touches the production datetime decode path for both SQL drivers and adds a ~200-line mock wire-protocol fixture, it's worth a quick human pass.
Extended reasoning...
Overview
This PR deduplicates two near-identical hand-rolled YYYY-MM-DD HH:MM:SS[.ffffff] parsers (one in the MySQL driver's DateTime::from_text, one in the Postgres driver's timestamp_text_to_ms_utc) into a single shared module src/sql_jsc/shared/datetime_text.rs. The shared parse() takes two flags (allow_date_only, allow_t_separator) to express the driver-specific differences; each call site becomes a thin wrapper that adds its own range/calendar validation (MySQL) or delegates to gregorian_date_time_to_ms_utc (Postgres). It also adds a mock-MySQL-server test fixture so the text-protocol decode path is exercised without Docker, across three TZ values, including zero/impossible dates.
I traced both before/after paths line-by-line: the digit accumulator, separator checks, length gates, fractional-second right-padding, and the Postgres-specific "space-only separator, no date-only, no range validation" constraints are all preserved. The Postgres path's switch from i32 intermediates to u16/u8 is safe (4-digit year ≤ 9999, 2-digit fields ≤ 99), and the previously explicit is_ascii_digit check on the fraction was redundant with parse_u's per-byte check.
Security risks
None identified. This is pure parsing of server-supplied datetime text into integer components; no auth, crypto, file I/O, or unsafe blocks are introduced. Bounds are guarded by length checks before every index, and integer accumulation uses checked arithmetic.
Level of scrutiny
Moderate. The refactor itself is mechanical and behavior-preserving, but it sits on the production decode path for DATE/DATETIME/TIMESTAMP in both SQL drivers — code where a subtle slip would silently shift or corrupt timestamps for users. That, plus a non-trivial new ~200-line mock MySQL wire-protocol fixture (handshake, OK packet, column defs, text result set), is enough that a maintainer familiar with this area should glance at it rather than rely solely on automated review.
Other factors
- No CODEOWNERS apply to the touched paths.
- No prior reviewer comments; only the build-bot has posted.
- Test coverage is good: the new mock-server tests run unconditionally, and the existing Docker-gated real-server roundtrip tests still cover binary/text agreement for both drivers.
- Public signatures of both wrapped functions are unchanged, so no call-site churn.
|
✅ Merged — all review comments addressed:
Verified with the ASAN debug build: all 6 tests pass in all three timezones, including the live round-trip path against a real MySQL-compatible server. |
The "WARNING: ASAN interferes" string no longer exists in JSC, so the stderr filter is dead code that could hide real stderr regressions.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/sql/sql-mysql-datetime-roundtrip.test.ts (1)
26-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResidual ASAN stderr filter left in
assertRoundTrip.The mock-test path was fixed at line 63, but
assertRoundTripstill splits/filters stderr forWARNING: ASAN interferes(lines 31-35) and carries the now-stale note at line 30. Subprocesses no longer emit that warning across build variants, so this filter can only mask real stderr regressions on the docker/local round-trip paths. Assert stderr directly here too.Suggested change
function assertRoundTrip(stdout: string, stderr: string, TZ: string) { // On a round-trip mismatch the fixture writes `FAIL TZ=… offsetMin=…` plus the // per-row `diffMin` breakdown to stderr, then exits 1. Assert it's empty so a // CI failure surfaces *which* dates drifted and by how much, not just a bare - // "CONNECTED" vs "OK" mismatch. (ASAN emits a harmless interposition warning.) - const diagnostics = stderr - .split(/\r?\n/) - .filter(l => l && !l.startsWith("WARNING: ASAN interferes")) - .join("\n"); - expect(diagnostics).toBe(""); + // "CONNECTED" vs "OK" mismatch. + expect(stderr).toBe("");Based on learnings: after PR
#30412, Bun subprocesses no longer emit theWARNING: ASAN interferesstartup warning across build variants, so the plaintoBe("")assertion is correct for all CI configurations.🤖 Prompt for 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. In `@test/js/sql/sql-mysql-datetime-roundtrip.test.ts` around lines 26 - 43, The test's assertRoundTrip function still filters out "WARNING: ASAN interferes" from stderr which is now stale; remove the split/filter/join block that removes that warning and the accompanying comment, and change/assert stderr directly (keep expect(diagnostics).toBe("") but applied to stderr without the ASAN filter) in function assertRoundTrip so real stderr regressions are not masked and ensure the surrounding comment about ASAN is deleted or updated.
🤖 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.
Outside diff comments:
In `@test/js/sql/sql-mysql-datetime-roundtrip.test.ts`:
- Around line 26-43: The test's assertRoundTrip function still filters out
"WARNING: ASAN interferes" from stderr which is now stale; remove the
split/filter/join block that removes that warning and the accompanying comment,
and change/assert stderr directly (keep expect(diagnostics).toBe("") but applied
to stderr without the ASAN filter) in function assertRoundTrip so real stderr
regressions are not masked and ensure the surrounding comment about ASAN is
deleted or updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0294b776-b706-4e2e-a06e-986d2ac277a4
📒 Files selected for processing (1)
test/js/sql/sql-mysql-datetime-roundtrip.test.ts
Same dead filter as the mock-server path — the warning string no longer exists, so the filter could only mask real stderr regressions on the docker/local round-trip paths.
3 participants
Follow-up to #31212.
What this does
#31212 fixed naive
DATETIME/TIMESTAMPtext decoding to be UTC, but introduced two near-identical hand-rolled parsers for theYYYY-MM-DD HH:MM:SS[.ffffff]wall-clock format:DateTime::from_textinsrc/sql_jsc/mysql/MySQLValue.rstimestamp_text_to_ms_utcinsrc/sql_jsc/postgres/types/date.rsBoth 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 JSDate.parse— the local-time behavior #31212 exists to avoid.What stays driver-specific (deliberately)
parse_mysql): accepts the 10-byte date-only form and aTseparator, validates calendar dates (days_in_month) and h/m/s ranges, keeps microsecond precision.parse_postgres_timestamp): requires the fullYYYY-MM-DD HH:MM:SSshape — anything else returnsNoneso the caller falls back toDate.parse(infinity, BC dates, 5+ digit years); no range validation (delegated togregorianDateTimeToMS); 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-protocolDATE/DATETIMEdecode acrossEtc/UTC/America/New_York/Asia/Tokyo, including zero dates and impossible calendar dates.How we know it works
This is a behavior-preserving refactor, so there is deliberately no test that fails without it on a current build: any Bun that already includes #31212 decodes these dates identically before and after this change. The tests pin the #31212 semantics as regression coverage that now runs without Docker. (On builds predating #31212 — e.g. the 1.4.0 release — the mock-server tests fail: text dates are read as local time, and
2024-02-31wraps to March 2 instead of Invalid Date.)bun bd test test/js/sql/sql-mysql-datetime-roundtrip.test.ts→ 6 pass in all three timezones, including the live-server round-trip path against a local MySQL-compatible server (which exercisesassertRoundTrip, not just the mock path).sql-mysql-mediumint,sql-mysql-raw-length-prefix,sql-mysql-bind-oob, both datetime roundtrip files) passes with this change.