mysql: compute civil date from day count so pre-1970 Date params don't abort#31209
mysql: compute civil date from day count so pre-1970 Date params don't abort#31209alii wants to merge 13 commits into
Conversation
|
@robobun adopt |
|
Updated to latest
All 69 non-darwin lanes are green (including Linux ASAN, which runs this PR's tests under sanitizers). Everything red is macOS docker-infra or container-timing flake. I've spent my one re-roll already, so I'll leave CI as-is rather than spam retriggers — the diff itself is green and ready to merge. |
|
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 (2)
WalkthroughThis PR fixes MySQL ChangesPre-1970 Date Support with Range Validation
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
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 `@src/sql_jsc/mysql/MySQLValue.rs`:
- Around line 849-853: The code currently clamps proleptic years to 0..=65535
after using civil_from_days (silently collapsing distinct timestamps); instead,
detect when the computed year is outside the valid u16 range and return an error
rather than saturating: in src/sql_jsc/mysql/MySQLValue.rs (the Date/DateTime ->
MySQL wire conversion using civil_from_days) remove the collapse-to-0000/65535
logic and propagate a failure (Err) from the DateTime conversion path with a
descriptive InvalidDate/OutOfRange error when year < 0 or year > 65535; ensure
callers of the conversion (the DateTime conversion path) handle/propagate that
error accordingly.
🪄 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: e60b66d9-e7e3-4802-86be-45952ab1fbc6
📒 Files selected for processing (2)
src/sql_jsc/mysql/MySQLValue.rstest/js/sql/sql-mysql-pre1970-date.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — faithful port of Hinnant's civil_from_days fixing a process abort; the inline nit about test TZ-dependence is non-blocking since bun test defaults to UTC.
Extended reasoning...
Overview
This PR replaces the gregorian_date(days: i32) helper in src/sql_jsc/mysql/MySQLValue.rs with Howard Hinnant's well-known civil_from_days algorithm, and adds a ~220-line mock-MySQL-server test that round-trips several pre-1970 Date parameters. The old implementation walked forward from 1970 with loops that assumed a non-negative day count, so any pre-1970 Date bound as a prepared-statement parameter fell through to u8::try_from(negative).expect(...) and aborted the process.
Security risks
None. This is pure integer date arithmetic on a value derived from a JS Date's epoch milliseconds. There is no parsing of untrusted network input, no auth/crypto/permissions, and the only behavioral change beyond the bug fix is replacing a panic on out-of-u16-range proleptic years with a clamp — strictly safer than the previous expect.
Level of scrutiny
Low-to-moderate. The production change is ~25 lines in a single self-contained leaf function, and it is a line-for-line transcription of a published, widely-cited algorithm (era = z.div_euclid(146097) for correct floor division on negatives; all subsequent divisions are on u32 so truncating / is correct). I spot-checked the constants and structure against the reference and they match. The removed is_leap_year/days_in_month helpers had no other callers in this file. The test file is larger but is test-only infrastructure exercising the encode path that previously crashed.
Other factors
The bug-hunting system flagged one nit: the test's round-trip assertion relies on encode (UTC) and decode (local time via gregorian_date_time_to_ms → WTF::LocalTime) agreeing, which only holds when the process TZ is UTC. Since bun test defaults JSC's timezone to Etc/UTC when TZ is unset, CI is unaffected; this is a developer-environment robustness suggestion, not a correctness issue with the fix. No CODEOWNERS cover these paths and there are no outstanding reviewer comments. Given the fix turns a hard process abort into correct behavior using a textbook algorithm, I'm comfortable approving.
|
We should use a real mysql server. CI has one and @robobun your docker container should also have one. Look for the other mysql tests to see how that works. |
…t out-of-range years
# Conflicts: # src/sql_jsc/mysql/MySQLValue.rs
… tests A bind-time encoder error (e.g. a DATETIME whose year doesn't fit the wire u16) on the *first* execution of a prepared statement hung the query's promise forever. The error travels the prepare-then-execute path through the request queue's on_error, but run()'s error guard had already marked the query Fail, so reject_with_js_value's 'already failed' guard dropped the rejection. The guard now only rolls back the this_value upgrade; both callers settle the promise themselves. A Bun SQL query is a single-consumption thenable, so passing the query object to expect().rejects (which .then()s it twice) hangs. Await it in a wrapper promise instead — this also fixes the existing out-of-range-year test, which had the same latent hang. Adds a real-server regression test (docker mysql_plain in CI, or a local MYSQL_URL / 127.0.0.1 fallback) that binds an out-of-range DATETIME to a not-yet-prepared statement and asserts it rejects — and the connection stays usable — instead of hanging.
|
Reworked the test to use a real MySQL server (per review) — it now lives in |
…ting
Removing DateTime::check_range left `i32::try_from(days).expect("int cast")`
in from_unix_timestamp unguarded. A numeric timestamp (the is_number path
in from_js) can carry a day count past i32::MAX without saturating i64 —
e.g. 1e20 ms yields days ~= 1.16e12 — so that .expect() aborts the process
where check_range previously returned a catchable error. Reachable via an
array-index getter that returns a Date during Signature::generate (binding
the column as DATETIME) and a number during bind, the same getter-mutation
scenario already guarded for the length dimension.
The function already returns Result, so route the days->i32 cast through
the same out-of-range error. Adds a regression test driving that getter
path; the connection stays usable after the rejected bind.
The helper takes the SQL client as a parameter and returns a boolean (true on connect, false on soft-skip), not a client-or-null as the leftover comment claimed.
…ring 1970-01-01
An Invalid Date (new Date(NaN), new Date('garbage')) is still a Date, so
it binds as DATETIME. Its timestamp is NaN; the float->int casts in
from_js laundered it into 0, silently storing 1970-01-01 00:00:00 on the
wire. Since from_unix_timestamp was already made fallible to reject
unrepresentable inputs, guard the casts with a finiteness check and reject
non-finite timestamps (NaN/Inf) with a catchable error. Applies to both
DateTime and Time, and both the is_date() and is_number() branches.
Adds a regression test; the connection stays usable after the rejected binds.
…ed ts On the is_number() path, `ms * 1000` is evaluated as a call argument before from_unix_timestamp's i32::try_from(days) guard runs. A numeric timestamp like 1e22 saturates ts to i64::MAX and the residual cast to u32::MAX, so ms*1000 overflowed u32 and aborted a debug build (overflow-checks + panic=abort) before the day guard could reject it. ms is the sub-second millisecond residual, always in [0,1000), so clamp it with .min(999) — the day-range check then rejects the saturated ts with the intended catchable error in debug as well as release. Applied to DateTime and Time. Extends the getter-mutation regression test to cover 1e22 (ms overflow) in addition to 1e20 (days overflow).
There was a problem hiding this comment.
All earlier review feedback has been addressed and I didn't find new issues, but this has grown beyond a localized date-math fix — the run() errguard change in JSMySQLQuery.rs alters error-path state transitions, and process.env.TZ is now pinned file-wide in sql-mysql.test.ts — so it's worth a human pass (CI is also showing failures on the latest build).
Extended reasoning...
Overview
The PR replaces the loop-based gregorian_date with Howard Hinnant's civil_from_days so pre-1970 Date parameters no longer abort, and over several review rounds has accumulated: a fallible DateTime::from_unix_timestamp with u16 year / i32 day range rejection, a check_finite_ms guard for Invalid Date, a .min(999) clamp on the sub-second residual, removal of the q.fail() call from JSMySQLQuery::run's errguard (to fix a swallowed-rejection hang on the prepare-then-execute path), a new real-server regression test file, three new tests in sql-mysql.test.ts, and a file-level process.env.TZ = "Etc/UTC" pin in that shared test file.
Security risks
None identified. Inputs are JS Date/number values flowing into the MySQL binary protocol encoder; the changes replace panics with catchable errors and tighten input validation. No auth, crypto, permissions, or untrusted-string handling is touched.
Level of scrutiny
Medium-high. The Hinnant algorithm itself is well-known and the transcription looks correct, but two pieces reach beyond pure date math: (1) the JSMySQLQuery::run errguard no longer flips the query to Fail on the error path — the in-code comment argues both callers (do_run and the request-queue advance() → on_error → reject_with_js_value) settle the promise themselves, and that checks out against the surrounding code, but it's a subtle state-machine change worth a maintainer's eye; (2) pinning TZ at the top of sql-mysql.test.ts affects every test in that ~900-line file, not just the new ones.
Other factors
All four of my earlier inline comments (TZ-dependent assertion, unguarded i32::try_from(days).expect(), Invalid-Date-as-epoch, ms * 1000 debug overflow) were addressed in follow-up commits, and robobun reports the new tests were verified fail-before/pass-after against a real MySQL. The bug-hunting system found nothing new this round. However, robobun's status comment shows Build #58415 failing on the latest pushed commits, which on its own is reason not to auto-approve.
Diff is green on all build lanes and the date-encoding fixes are verified fail-before/pass-after against a real MySQL. Re-rolling flaky test-bun lanes (darwin-14-aarch64 recovered on retry; ubuntu-25-x64 passed twice then flaked) and a transient build-binary corruption in the QA build env.
…e-abort # Conflicts: # test/js/sql/sql-mysql.test.ts
|
@robobun this was green earlier but CI went red after main moved (a single sub-job failing — looks flaky/infra). Please rebase onto main and re-run CI; if a test genuinely fails, investigate and fix it. |
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.
2 participants
Binding a pre-1970
Dateto a MySQL prepared-statement parameter aborted the whole process.gregorian_date(days)walked year/month forward with loops that assumedays >= 0, so for a negative day count it fell through tou8::try_from(d + 1).expect(...)with a negative value and panicked (uncatchable in release).Replace the loop with Howard Hinnant's branch-free
civil_from_days(era arithmetic,div_euclidfor correct floor division on negative counts). It's correct for any day count, so pre-1970 dates now produce the right calendar date instead of crashing — and it also fixes the latent wrong-date behavior the old code shared with the Zig version (e.g. 1969-12-31 → day 31, not day 0). The year is clamped into the wire format'su16instead of panicking on an out-of-range value.Adds a mock-MySQL-server test that round-trips several pre-1970 dates.