Skip to content

mysql: normalize ALLOW_INVALID_DATES binary DATETIME instead of returning Invalid Date#32283

Open
robobun wants to merge 2 commits into
mainfrom
farm/c4011898/mysql-allow-invalid-dates
Open

mysql: normalize ALLOW_INVALID_DATES binary DATETIME instead of returning Invalid Date#32283
robobun wants to merge 2 commits into
mainfrom
farm/c4011898/mysql-allow-invalid-dates

Conversation

@robobun

@robobun robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

What

DateTime::to_js_timestamp() rejected over-range DATETIME components (day > days-in-month, hour > 23, etc.) with f64::NAN. The Zig reference at MySQLValue.zig:373-386 passes raw components straight to WTF::GregorianDateTime, which normalizes them the same way Date.UTC does (Feb 30 → Mar 1, Apr 31 → May 1).

With ALLOW_INVALID_DATES, MySQL stores day-of-month 1..31 regardless of the actual month length and returns those components as-is over the binary prepared-statement protocol. 1.3.x produced a normalized valid Date; current main produces Invalid Date.

Repro

SET SESSION sql_mode='ALLOW_INVALID_DATES';
CREATE TEMPORARY TABLE t (dt DATETIME);
INSERT INTO t VALUES ('2024-02-30 12:00:00'), ('2024-04-31 12:00:00');
await sql`SELECT dt FROM t`
// 1.3.x: [2024-03-01T12:00:00.000Z, 2024-05-01T12:00:00.000Z]
// main:  [Invalid Date, Invalid Date]

Cause

#31212 added a bounds gate to to_js_timestamp() intended for zero-date sentinels (0000-00-00, 2024-00-15), but it also included upper-bound checks (month > 12, day > days_in_month(...), hour > 23, minute > 59, second > 59) that the Zig path never had.

Fix

Keep only month < 1 || day < 1 so NO_ZERO_IN_DATE-off sentinels still surface as Invalid Date, and let GregorianDateTime normalize over-range components. The text-protocol path (from_text) is unchanged: it previously went through Date.parse, which rejects 2024-02-30, and the existing impossible_date → NaN assertion in sql-mysql-datetime-text-mock-fixture.ts still holds.

Verification

test/js/sql/sql-mysql-datetime-allow-invalid.test.ts drives a mock MySQL server over the binary prepared-statement protocol and asserts Feb 30 / Apr 31 / non-leap Feb 29 normalize via Date.UTC, zero-month / zero-day stay Invalid Date, and a trailing INT column stays in sync.

fail-before / pass-after
# USE_SYSTEM_BUN=1 bun test sql-mysql-datetime-allow-invalid.test.ts
-   "apr31": 1714564800000,
-   "feb29_nonleap": 1677628800000,
-   "feb30": 1709294400000,
+   "apr31": NaN,
+   "feb29_nonleap": NaN,
+   "feb30": NaN,
(fail)

# bun bd test sql-mysql-datetime-allow-invalid.test.ts
(pass) binary DATETIME with ALLOW_INVALID_DATES day-of-month normalizes, zero month/day stays Invalid Date

Live MariaDB 11.8 via the extended sql-mysql-datetime-tz-fixture.ts:

# system bun
FAIL TZ=America/New_York offsetMin=240
  binary ALLOW_INVALID_DATES id=1: expected 2024-03-01T12:00:00.000Z, got Invalid Date
  binary ALLOW_INVALID_DATES id=2: expected 2024-05-01T12:00:00.000Z, got Invalid Date

# debug build
OK TZ=America/New_York offsetMin=240

…ning Invalid Date

With ALLOW_INVALID_DATES, MySQL stores day-of-month 1..31 regardless of the
actual month length (2024-02-30, 2024-04-31) and returns those components
as-is over the binary prepared-statement protocol. The bounds gate added to
DateTime::to_js_timestamp in #31212 rejected them as NaN. The Zig reference
(and 1.3.x) passed the raw components through to WTF::GregorianDateTime,
which normalizes them the same way Date.UTC does (Feb 30 -> Mar 1).

Keep only the month<1 / day<1 check so zero-date sentinels (NO_ZERO_IN_DATE
off) still surface as Invalid Date, and let GregorianDateTime handle
over-range components.
@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 2 minutes and 40 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7e7ca634-735c-454e-b115-80cef110bc57

📥 Commits

Reviewing files that changed from the base of the PR and between e0acad3 and e14135f.

📒 Files selected for processing (3)
  • src/sql_jsc/mysql/MySQLValue.rs
  • test/js/sql/sql-mysql-datetime-allow-invalid.test.ts
  • test/js/sql/sql-mysql-datetime-tz-fixture.ts

Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 8:33 AM PT - Jun 15th, 2026

@robobun, your commit e14135f has 1 failures in Build #62508 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 32283

That installs a local version of the PR into your bun-32283 executable, so you can run:

bun-32283 --bun

@robobun

robobun commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator Author

Status: fix applied and verified; diff is green, CI red is unrelated infra.

Reproduced with the mock binary-protocol server in test/js/sql/sql-mysql-datetime-allow-invalid.test.ts:

  • USE_SYSTEM_BUN=1 bun test test/js/sql/sql-mysql-datetime-allow-invalid.test.ts → fails (feb30/apr31/feb29-nonleap decode to NaN)
  • bun bd test test/js/sql/sql-mysql-datetime-allow-invalid.test.ts → passes

Also verified against live MariaDB 11.8 via the extended sql-mysql-datetime-tz-fixture.ts (see PR body).

CI: builds #62406 and #62508 both show 72+ lanes passing with the only hard failures being the darwin-14-aarch64 / darwin-14-x64 / darwin-26-aarch64 test lanes reporting Expired (no agent picked up the job). The remaining annotations are flaky-context retries (serve-body-leak, spawn-pipe-leak, update_interactive_install, streams-leak, napi 30205) on unrelated subsystems; none mention mysql/sql/datetime. Ready for a maintainer to merge past the expired darwin lanes.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — surgical regression fix restoring 1.3.x/Zig normalization behavior for ALLOW_INVALID_DATES, with thorough mock + live test coverage.

Extended reasoning...

Overview

This PR relaxes the bounds gate in DateTime::to_js_timestamp() (src/sql_jsc/mysql/MySQLValue.rs) from a full component-range check down to month < 1 || day < 1. The removed upper-bound checks (month > 12, day > days_in_month, hour > 23, etc.) were a regression introduced by #31212 — they were intended only to catch MySQL zero-date sentinels but over-reached and broke ALLOW_INVALID_DATES rows (Feb 30, Apr 31), which 1.3.x and the Zig reference path normalized via WTF::GregorianDateTime exactly like Date.UTC does. The text-protocol path (from_text) is intentionally left strict to preserve its prior Date.parse-compatible behavior. Two test files are added/extended: a self-contained mock-MySQL binary-protocol test and an extension to the live MariaDB TZ fixture.

Security risks

None. This is pure decode-side date normalization. The wire-protocol parser still reads fixed-width fields; the only difference is that out-of-calendar components now flow into gregorian_date_time_to_ms_utc (which normalizes) instead of returning NaN. No injection, auth, or memory-safety surface is touched.

Level of scrutiny

Moderate — this is a runtime behavior change in the MySQL driver, but it is a narrowly scoped regression fix that restores documented prior behavior (1.3.x / Zig parity) rather than introducing new semantics. The 8-line Rust diff is mostly comments; the actual logic change is reducing one if condition. The PR description includes a clear repro, root-cause analysis, and fail-before/pass-after output for both the mock test and a live MariaDB 11.8 run.

Other factors

  • The new mock-server test asserts both the positive cases (Feb 30 / Apr 31 / non-leap Feb 29 normalize to Date.UTC equivalents) and the negative cases (zero month/day still yield Invalid Date), plus a trailing INT sentinel to confirm the binary cursor stays aligned.
  • days_in_month remains used by from_text and gregorian_date, so no dead code is introduced.
  • No CODEOWNERS apply to the touched paths.
  • No outstanding reviewer comments; only bot build/ratelimit notices in the timeline.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any issues, but this changes user-visible decode semantics (over-range DATETIME components now normalize instead of yielding Invalid Date), so it's worth a quick human sanity-check that this is the intended product behavior.

Extended reasoning...

Overview

The PR relaxes the bounds gate in DateTime::to_js_timestamp() (src/sql_jsc/mysql/MySQLValue.rs) from a full range check (month/day/hour/minute/second upper bounds + days_in_month) down to just month < 1 || day < 1. Over-range components now flow through to GregorianDateTime, which normalizes them the same way Date.UTC does (Feb 30 → Mar 1). It adds a mock binary-protocol test and extends the live-server TZ fixture to cover ALLOW_INVALID_DATES.

Security risks

None. This is pure value-decoding arithmetic on integer date components received from a MySQL server; no auth, crypto, parsing of untrusted input beyond fixed-width binary fields, or memory-safety concerns. Removing the upper-bound checks cannot panic — the values are passed as i32 to a normalizing C++ helper, and the now-unused days_in_month indexing (which could panic on month=0/13) is no longer reached on this path.

Level of scrutiny

Moderate. The code change is mechanically tiny (one if condition simplified, with an explanatory comment), and it restores 1.3.x / Zig-reference behavior, so it's framed as a regression fix. However, it is a deliberate semantic choice about how an edge-case server value surfaces to JS — "normalize like Date.UTC" vs. "Invalid Date". The PR argues convincingly for normalization (matches prior shipped behavior, matches the Zig reference, matches JS Date.UTC conventions), and the text-protocol path is intentionally left strict to preserve its existing Date.parse-equivalent contract. That asymmetry between binary and text paths is justified in the description but is the kind of product/compat call a maintainer should ratify.

Other factors

Test coverage is strong: the new mock-server test asserts both the positive (Feb 30 / Apr 31 / non-leap Feb 29 normalize) and negative (zero month/day stay Invalid Date) cases, plus a trailing INT sentinel proving the binary cursor stays aligned. Fail-before/pass-after is demonstrated against both system bun and a live MariaDB. No CODEOWNERS match, no outstanding reviewer comments, and the bug-hunting pass found nothing. I'm deferring only because the change encodes a user-visible behavioral decision rather than a purely mechanical fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant