Repository navigation
fix(sql): MySQL VARCHAR with binary collations returns string instead of Buffer - #26064
Conversation
|
Updated 3:09 AM PT - Jan 14th, 2026
❌ @autofix-ci[bot], your commit 3e66e47 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 26064That installs a local version of the PR into your bun-26064 --bun |
WalkthroughAdds Changes
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. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📥 CommitsReviewing files that changed from the base of the PR and between eb2ea00057b5fe76035221e72c46a3d16381cdf8 and 3e66e47. 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (6)**/*.zig📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.zig📄 CodeRabbit inference engine (src/CLAUDE.md)
Files:
**/*.test.ts?(x)📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/regression/issue/*.test.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.test.ts?(x)📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
🧠 Learnings (21)📓 Common learnings📚 Learning: 2025-12-11T02:11:47.024ZApplied to files:
📚 Learning: 2026-01-05T16:32:07.551ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-09-30T02:56:30.615ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2025-10-30T03:48:10.513ZApplied to files:
📚 Learning: 2025-11-06T00:58:23.965ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-09-25T22:07:13.851ZApplied to files:
📚 Learning: 2025-09-25T22:51:15.539ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-10-08T13:48:02.430ZApplied to files:
📚 Learning: 2025-09-26T01:36:48.705ZApplied to files:
🧬 Code graph analysis (1)test/regression/issue/26063.test.ts (3)
🔇 Additional comments (8)
✏️ Tip: You can disable this entire section by setting 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: 2
🤖 Fix all issues with AI agents
In `@src/sql/mysql/protocol/DecodeBinaryValue.zig`:
- Around line 154-158: The literal 63 used in the binary-collation check should
be replaced with a descriptive named constant to improve readability; define a
constant (e.g., BINARY_CHARACTER_SET = 63) near the top of the module or in the
same scope as DecodeBinaryValue, then update the condition from `binary and
character_set == 63` to use that constant (`binary and character_set ==
BINARY_CHARACTER_SET`), and adjust the comment to reference the constant name
instead of the magic number.
In `@test/regression/issue/26063.test.ts`:
- Around line 19-25: Add an afterAll hook that closes the test database
connection created in beforeAll: call the SQL instance's close method (await
sql.close()) to ensure the connection is properly terminated after tests finish;
if your SQL class uses a different API, use the equivalent (e.g., await
sql.end()) inside afterAll to avoid leaking connections.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 967a6a2 and 070a7c2d35eb5f2ad0af09e65abde7b2fa699120.
📒 Files selected for processing (3)
src/sql/mysql/protocol/DecodeBinaryValue.zigsrc/sql/mysql/protocol/ResultSet.zigtest/regression/issue/26063.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/sql/mysql/protocol/DecodeBinaryValue.zigsrc/sql/mysql/protocol/ResultSet.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct {#foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/sql/mysql/protocol/DecodeBinaryValue.zigsrc/sql/mysql/protocol/ResultSet.zig
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/regression/issue/26063.test.ts
test/regression/issue/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place regression tests for specific GitHub issues in
test/regression/issue/${issueNumber}.test.tswith real issue numbers only
Files:
test/regression/issue/26063.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/regression/issue/26063.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/regression/issue/26063.test.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:138-143
Timestamp: 2025-09-25T22:51:15.539Z
Learning: In PostgreSQL array serialization for Bun.SQL, typed arrays (Uint8Array, Int32Array, etc.) should always be treated as arrays and expand their elements, not as binary data. BYTEA binary serialization should only use Buffer objects, maintaining consistency with postgres.js behavior. Typed arrays and Buffer have different semantic purposes.
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/sql/mysql/protocol/DecodeBinaryValue.zigsrc/sql/mysql/protocol/ResultSet.zig
📚 Learning: 2025-09-26T01:36:48.705Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:1326-1332
Timestamp: 2025-09-26T01:36:48.705Z
Learning: In PostgreSQL single-object INSERT path (src/js/internal/sql/postgres.ts), when handling array values in columnValue, the user prefers to rely on PostgreSQL's native type conversion rather than explicitly checking for typed arrays with isTypedArray(). The current $isArray(columnValue) check with serializeArray(columnValue, "JSON") is sufficient, and native PostgreSQL handling is preferred over explicit typed array detection in this specific INSERT context.
Applied to files:
src/sql/mysql/protocol/ResultSet.zig
📚 Learning: 2025-09-25T22:51:15.539Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:138-143
Timestamp: 2025-09-25T22:51:15.539Z
Learning: In PostgreSQL array serialization for Bun.SQL, typed arrays (Uint8Array, Int32Array, etc.) should always be treated as arrays and expand their elements, not as binary data. BYTEA binary serialization should only use Buffer objects, maintaining consistency with postgres.js behavior. Typed arrays and Buffer have different semantic purposes.
Applied to files:
src/sql/mysql/protocol/ResultSet.zigtest/regression/issue/26063.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-09-30T02:56:30.615Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22700
File: test/js/sql/sql.test.ts:11673-11710
Timestamp: 2025-09-30T02:56:30.615Z
Learning: Repository oven-sh/bun tests run in a Docker image where the pgcrypto extension is pre-installed, so gen_random_uuid() is available without explicitly running CREATE EXTENSION in tests (e.g., in test/js/sql/sql.test.ts “upsert helper”).
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize regression tests for specific issues in `/test/regression/issue/${issueNumber}.test.ts`. Do not place regression tests in the regression directory if there is no associated issue number.
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Use `normalizeBunSnapshot` to normalize snapshot output of tests
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*-fixture.ts : Test files that spawn Bun processes should end in `*-fixture.ts` to identify them as test fixtures rather than tests themselves.
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-09-25T22:07:13.851Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/regression/issue/26063.test.ts
🧬 Code graph analysis (1)
test/regression/issue/26063.test.ts (3)
test/harness.ts (2)
isDockerEnabled(875-898)describeWithContainer(927-1027)src/js/internal-for-testing.ts (1)
SQL(17-17)src/node-fallbacks/buffer.js (1)
Buffer(136-145)
🔇 Additional comments (6)
src/sql/mysql/protocol/DecodeBinaryValue.zig (1)
1-1: LGTM: Function signature correctly extended withcharacter_setparameter.The addition of
character_set: u16enables proper distinction between true binary columns and string columns with binary collations.src/sql/mysql/protocol/ResultSet.zig (2)
143-147: Correct fix for text protocol binary detection.The condition now properly matches the binary protocol logic, ensuring consistent behavior between text and binary protocols. The comment accurately explains the fix.
237-237: LGTM: Binary protocol correctly passescharacter_setto decoder.The
column.character_setis now passed todecodeBinaryValue, enabling the updated binary detection logic in the decoder.test/regression/issue/26063.test.ts (3)
27-46: Good test coverage for binary protocol path.The test correctly:
- Uses a unique temporary table name
- Tests VARCHAR with utf8mb4_bin collation
- Verifies both the type (
typeof) and actual values- Uses the default binary protocol path
48-68: Good test coverage for text protocol path.The test correctly uses
.simple()to force the text protocol, ensuring both code paths are validated.
110-133: Important: Verifies that true binary types still return Buffers.This test ensures the fix doesn't break the expected behavior for actual BINARY/VARBINARY/BLOB columns, which should continue returning Buffers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
070a7c2 to
e8e1366
Compare
|
Addressed the code review feedback:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/regression/issue/26063.test.ts`:
- Line 2: The test uses afterAll but it is not imported from bun:test; update
the import declaration that currently imports beforeAll, expect, test to also
include afterAll (i.e., add the afterAll symbol to the import list from
"bun:test") so the runtime has the afterAll function available for the teardown
on line 27.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 070a7c2d35eb5f2ad0af09e65abde7b2fa699120 and eb2ea00057b5fe76035221e72c46a3d16381cdf8.
📒 Files selected for processing (3)
src/sql/mysql/protocol/DecodeBinaryValue.zigsrc/sql/mysql/protocol/ResultSet.zigtest/regression/issue/26063.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/regression/issue/26063.test.ts
test/regression/issue/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place regression tests for specific GitHub issues in
test/regression/issue/${issueNumber}.test.tswith real issue numbers only
Files:
test/regression/issue/26063.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/regression/issue/26063.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/regression/issue/26063.test.ts
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/sql/mysql/protocol/ResultSet.zigsrc/sql/mysql/protocol/DecodeBinaryValue.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct {#foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/sql/mysql/protocol/ResultSet.zigsrc/sql/mysql/protocol/DecodeBinaryValue.zig
🧠 Learnings (16)
📓 Common learnings
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25462
File: src/ast/visitExpr.zig:1644-1695
Timestamp: 2025-12-11T02:11:47.024Z
Learning: In Bun's bundler feature flag implementation (src/ast/visitExpr.zig), the validation for feature() flag names intentionally only rejects UTF-16 strings (checking `is_utf16`) while allowing UTF-8 strings, even though the error message says "must be an ASCII string". This is the intended behavior and should not be changed to enforce strict ASCII validation.
📚 Learning: 2025-09-30T02:56:30.615Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22700
File: test/js/sql/sql.test.ts:11673-11710
Timestamp: 2025-09-30T02:56:30.615Z
Learning: Repository oven-sh/bun tests run in a Docker image where the pgcrypto extension is pre-installed, so gen_random_uuid() is available without explicitly running CREATE EXTENSION in tests (e.g., in test/js/sql/sql.test.ts “upsert helper”).
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize regression tests for specific issues in `/test/regression/issue/${issueNumber}.test.ts`. Do not place regression tests in the regression directory if there is no associated issue number.
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-09-25T22:07:13.851Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: test/js/sql/sql.test.ts:195-202
Timestamp: 2025-09-25T22:07:13.851Z
Learning: PR oven-sh/bun#22946: JSON/JSONB result parsing updates (e.g., returning parsed arrays instead of legacy strings) are out of scope for this PR; tests keep current expectations with a TODO. Handle parsing fixes in a separate PR.
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-09-25T22:51:15.539Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:138-143
Timestamp: 2025-09-25T22:51:15.539Z
Learning: In PostgreSQL array serialization for Bun.SQL, typed arrays (Uint8Array, Int32Array, etc.) should always be treated as arrays and expand their elements, not as binary data. BYTEA binary serialization should only use Buffer objects, maintaining consistency with postgres.js behavior. Typed arrays and Buffer have different semantic purposes.
Applied to files:
test/regression/issue/26063.test.tssrc/sql/mysql/protocol/ResultSet.zig
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Ensure V8 API tests compare identical C++ code output between Node.js and Bun through the test suite validation process
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/regression/issue/26063.test.ts
📚 Learning: 2025-09-26T01:36:48.705Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22946
File: src/js/internal/sql/postgres.ts:1326-1332
Timestamp: 2025-09-26T01:36:48.705Z
Learning: In PostgreSQL single-object INSERT path (src/js/internal/sql/postgres.ts), when handling array values in columnValue, the user prefers to rely on PostgreSQL's native type conversion rather than explicitly checking for typed arrays with isTypedArray(). The current $isArray(columnValue) check with serializeArray(columnValue, "JSON") is sufficient, and native PostgreSQL handling is preferred over explicit typed array detection in this specific INSERT context.
Applied to files:
src/sql/mysql/protocol/ResultSet.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/sql/mysql/protocol/ResultSet.zigsrc/sql/mysql/protocol/DecodeBinaryValue.zig
📚 Learning: 2025-12-11T02:11:47.024Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25462
File: src/ast/visitExpr.zig:1644-1695
Timestamp: 2025-12-11T02:11:47.024Z
Learning: In Bun's bundler feature flag implementation (src/ast/visitExpr.zig), the validation for feature() flag names intentionally only rejects UTF-16 strings (checking `is_utf16`) while allowing UTF-8 strings, even though the error message says "must be an ASCII string". This is the intended behavior and should not be changed to enforce strict ASCII validation.
Applied to files:
src/sql/mysql/protocol/DecodeBinaryValue.zig
🧬 Code graph analysis (1)
test/regression/issue/26063.test.ts (3)
test/harness.ts (2)
isDockerEnabled(875-898)describeWithContainer(927-1027)src/js/internal-for-testing.ts (1)
SQL(17-17)src/node-fallbacks/buffer.js (1)
Buffer(136-145)
🔇 Additional comments (7)
src/sql/mysql/protocol/DecodeBinaryValue.zig (2)
1-6: Well-documented constant and correct fix for binary collation detection.The
binary_charsetconstant is properly documented, and the updated function signature correctly passescharacter_setfor the refined binary detection logic. This addresses the root cause where VARCHAR/CHAR/TEXT columns with binary collations (likeutf8mb4_bin) were incorrectly treated as binary.
159-167: Correct binary detection logic.The condition
binary and character_set == binary_charsetcorrectly identifies true binary columns while allowing string columns with binary collations (e.g.,utf8mb4_bin) to return strings. The inline comment clearly explains the rationale.src/sql/mysql/protocol/ResultSet.zig (3)
143-152: Consistent binary detection in text protocol path.The text protocol (parseValueAndSetCell) now uses the same binary detection logic as the binary protocol, referencing the shared
DecodeBinaryValue.binary_charsetconstant. This ensures consistent behavior across both protocol paths.
237-237: Binary protocol correctly passes character_set to decoder.The
column.character_setis now passed todecodeBinaryValue, enabling the refined binary detection logic that distinguishes true binary types from string columns with binary collations.
269-271: Import structure follows Zig conventions.The imports are placed at the bottom of the file as per coding guidelines. The module import allows access to both the
binary_charsetconstant and thedecodeBinaryValuefunction.test/regression/issue/26063.test.ts (2)
19-29: Good test setup with proper connection cleanup.The connection is correctly awaited for readiness in
beforeAlland properly closed inafterAll. This addresses connection cleanup hygiene.
31-137: Comprehensive regression test coverage.The tests thoroughly cover:
- All affected string types (VARCHAR, CHAR, TEXT) with
utf8mb4_bincollation- Both binary protocol (default) and text protocol (
.simple())- Non-regression verification for true binary types (BINARY, VARBINARY, BLOB)
The use of random table names via
randomUUIDv7ensures test isolation in concurrent runs.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
… of Buffer VARCHAR/CHAR/TEXT columns with binary collations like `utf8mb4_bin` have the BINARY flag set but should return strings, not Buffers. The fix checks for character_set == binary_charset (63) in addition to the BINARY flag to properly distinguish true binary types (BINARY, VARBINARY, BLOB) from string types with binary collations. Fixes #26063 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
eb2ea00 to
429f051
Compare
|
Download the code for other MySQL clients and double check that this is what they do. |
Research on Other MySQL ClientsI've verified that checking 1. node-mysql2 (Node.js)From charsets.js: exports.BINARY = 63;From text_parser.js: if (charset === Charsets.BINARY) {
return 'packet.readLengthCodedBuffer()';
}
return \`packet.readLengthCodedString(${encodingExpr})\`;2. go-sql-driver/mysql (Go)From collations.go: const binaryCollationID = 63From fields.go: // For scan type resolution:
if mf.charSet == binaryCollationID {
return scanTypeBytes // Returns []byte
}
return scanTypeString // Returns string3. MySQL Official DocumentationFrom The binary Collation Compared to _bin Collations:
The key distinction: ConclusionThe fix is correct: checking |
3 participants
Summary
utf8mb4_bin) being incorrectly returned asBufferinstead ofstringcharacter_set == 63(binary collation) in addition to the BINARY flag to properly distinguish true binary typesFixes #26063
Root Cause
PR #26011 introduced a fix for binary column handling that checked
column.flags.BINARYto determine if data should be returned asBuffer. However, MySQL sets the BINARY flag on VARCHAR/CHAR/TEXT columns with binary collations (likeutf8mb4_bin) even though they should return strings.The proper way to detect true binary types (BINARY, VARBINARY, BLOB) is to check if
character_set == 63(the "binary" collation), not just the BINARY flag.Changes
ResultSet.zig:143-148): Updated binary check tocolumn.flags.BINARY and column.character_set == 63DecodeBinaryValue.zig:154-156): Addedcharacter_setparameter and updated binary checkTest plan
test/regression/issue/26063.test.tsthat tests VARCHAR, CHAR, and TEXT columns withutf8mb4_bincollation return strings🤖 Generated with Claude Code