feat(sql) add PostgreSQL COPY support (excluding CopyBoth)#23350
feat(sql) add PostgreSQL COPY support (excluding CopyBoth)#23350gohryt wants to merge 56 commits into
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds comprehensive PostgreSQL COPY protocol support across Zig bindings, connection state and protocol handlers, JS runtime COPY APIs and encoders, TypeScript typings, prototype wiring, error mappings, and extensive tests for text/CSV/binary streaming and non‑streaming flows. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/internal/sql/postgres.ts (2)
18-410: Fix COPY helper call sites to bind connection as thisArg.The Zig COPY helpers are declared to be invoked via
.call(connection, ...). Calling them as plain functions passesconnectionas an argument instead ofthis, so the native layer receives the wrong context and COPY will break. Bind the connection with.call(...)everywhere we delegate to these helpers.Apply this diff:
function copySendData(connection: $ZigGeneratedClasses.PostgresSQLConnection, data: string | Uint8Array) { // delegate to Zig binding with the connection as thisArg // Zig side currently expects strings; Uint8Array will be coerced by Bun // If binary mode is used later, we can pass bytes directly - (sendCopyData as any)(connection, data as any); + (sendCopyData as any).call(connection, data as any); } function copyDone(connection: $ZigGeneratedClasses.PostgresSQLConnection) { - (sendCopyDone as any)(connection); + (sendCopyDone as any).call(connection); } function copyFail(connection: $ZigGeneratedClasses.PostgresSQLConnection, message?: string) { - (sendCopyFail as any)(connection, message ?? ""); + (sendCopyFail as any).call(connection, message ?? ""); } @@ static copySendData(connection: $ZigGeneratedClasses.PostgresSQLConnection, data: string | Uint8Array) { // delegate to Zig binding with the connection as thisArg // Zig side currently expects strings; Uint8Array will be coerced by Bun // If binary mode is used later, we can pass bytes directly - (sendCopyData as any)(connection, data as any); + (sendCopyData as any).call(connection, data as any); } static copyDone(connection: $ZigGeneratedClasses.PostgresSQLConnection) { - (sendCopyDone as any)(connection); + (sendCopyDone as any).call(connection); } static copyFail(connection: $ZigGeneratedClasses.PostgresSQLConnection, message?: string) { - (sendCopyFail as any)(connection, message ?? ""); + (sendCopyFail as any).call(connection, message ?? ""); } static setCopyStreamingMode(connection: $ZigGeneratedClasses.PostgresSQLConnection, enable: boolean) { - (setCopyStreamingMode as any)(connection, !!enable); + (setCopyStreamingMode as any).call(connection, !!enable); } static setCopyTimeout(connection: $ZigGeneratedClasses.PostgresSQLConnection, ms: number) { - (setCopyTimeout as any)(connection, (ms | 0) >>> 0); + (setCopyTimeout as any).call(connection, (ms | 0) >>> 0); } static setMaxCopyBufferSize(connection: $ZigGeneratedClasses.PostgresSQLConnection, bytes: number) { - (setMaxCopyBufferSize as any)(connection, (bytes | 0) >>> 0); + (setMaxCopyBufferSize as any).call(connection, (bytes | 0) >>> 0); } static onWritable(connection: $ZigGeneratedClasses.PostgresSQLConnection, handler: () => void) { writableHandlers.set(connection, handler); } static awaitWritable(connection: $ZigGeneratedClasses.PostgresSQLConnection, handler?: () => void) { if (handler) { writableHandlers.set(connection, handler); } // Use the connection as thisArg; no explicit callback so the global dispatcher installed by init is used. - (awaitWritable as any)(connection); + (awaitWritable as any).call(connection); }
362-409: Update PostgresDotZig.init signature for onWritable.
initPostgresis now invoked with an extraonWritablecallback, but thePostgresDotZigtype still declares only five arguments. This breaks TypeScript checking and risks future refactors dropping the handler. Extend the signature to include the writable callback.Apply this diff:
init: ( onResolveQuery: ( query: Query<any, any>, result: SQLResultArray, commandTag: string, @@ - onCopyStart: (this: $ZigGeneratedClasses.PostgresSQLConnection) => void, - onCopyChunk: (this: $ZigGeneratedClasses.PostgresSQLConnection, chunk: any) => void, - onCopyEnd: (this: $ZigGeneratedClasses.PostgresSQLConnection) => void, + onCopyStart: (this: $ZigGeneratedClasses.PostgresSQLConnection) => void, + onCopyChunk: (this: $ZigGeneratedClasses.PostgresSQLConnection, chunk: any) => void, + onCopyEnd: (this: $ZigGeneratedClasses.PostgresSQLConnection) => void, + onWritable: (this: $ZigGeneratedClasses.PostgresSQLConnection) => void, ) => void;
🧹 Nitpick comments (1)
src/sql/postgres/protocol/CopyBothResponse.zig (1)
1-35: Consider refactoring to eliminate code duplication.
CopyBothResponse.zigandCopyOutResponse.zighave identical implementations (fields,deinit, anddecodeInternallogic). This duplication increases maintenance burden.Consider extracting shared logic into a common base type or generic function:
// shared/CopyResponseBase.zig pub fn CopyResponseBase(comptime name: []const u8) type { return struct { overall_format: u8 = 0, column_format_codes: []u16 = &[_]u16{}, pub fn deinit(this: *@This()) void { if (this.column_format_codes.len > 0) { bun.default_allocator.free(this.column_format_codes); } } pub fn decodeInternal(this: *@This(), comptime Container: type, reader: NewReader(Container)) !void { _ = try reader.length(); const overall_format = try reader.int(u8); const column_count: usize = @intCast(@max(try reader.short(), 0)); const column_format_codes = try bun.default_allocator.alloc(u16, column_count); errdefer bun.default_allocator.free(column_format_codes); for (column_format_codes) |*format_code| { format_code.* = @intCast(try reader.short()); } this.* = .{ .overall_format = overall_format, .column_format_codes = column_format_codes, }; } }; }Then use it in both files:
const CopyBothResponse = CopyResponseBase("CopyBothResponse");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
.gitignore(1 hunks).zed/settings.json(1 hunks)cmake/tools/SetupLLVM.cmake(2 hunks)packages/bun-types/sql.d.ts(1 hunks)src/bun.js/VirtualMachine.zig(1 hunks)src/bun.js/api/sql.classes.ts(2 hunks)src/codegen/class-definitions.ts(1 hunks)src/js/builtins.d.ts(1 hunks)src/js/bun/sql.ts(11 hunks)src/js/internal/sql/postgres.ts(6 hunks)src/sql/postgres.zig(1 hunks)src/sql/postgres/AnyPostgresError.zig(4 hunks)src/sql/postgres/PostgresProtocol.zig(1 hunks)src/sql/postgres/PostgresSQLConnection.zig(12 hunks)src/sql/postgres/PostgresSQLContext.zig(1 hunks)src/sql/postgres/protocol/CopyBothResponse.zig(1 hunks)src/sql/postgres/protocol/CopyData.zig(2 hunks)src/sql/postgres/protocol/CopyFail.zig(1 hunks)src/sql/postgres/protocol/CopyInResponse.zig(1 hunks)src/sql/postgres/protocol/CopyOutResponse.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);and invokinglog("...", .{})In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bun
Files:
src/sql/postgres/PostgresProtocol.zigsrc/sql/postgres/AnyPostgresError.zigsrc/sql/postgres/protocol/CopyBothResponse.zigsrc/sql/postgres/protocol/CopyData.zigsrc/sql/postgres/protocol/CopyFail.zigsrc/sql/postgres/protocol/CopyInResponse.zigsrc/sql/postgres/PostgresSQLContext.zigsrc/sql/postgres/protocol/CopyOutResponse.zigsrc/sql/postgres.zigsrc/bun.js/VirtualMachine.zigsrc/sql/postgres/PostgresSQLConnection.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/sql/postgres/PostgresProtocol.zigsrc/sql/postgres/AnyPostgresError.zigsrc/sql/postgres/protocol/CopyBothResponse.zigsrc/sql/postgres/protocol/CopyData.zigsrc/sql/postgres/protocol/CopyFail.zigsrc/sql/postgres/protocol/CopyInResponse.zigsrc/sql/postgres/PostgresSQLContext.zigsrc/sql/postgres/protocol/CopyOutResponse.zigsrc/sql/postgres.zigsrc/bun.js/VirtualMachine.zigsrc/sql/postgres/PostgresSQLConnection.zig
src/js/internal/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place internal modules not exposed to users under src/js/internal/
Files:
src/js/internal/sql/postgres.ts
src/js/internal/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place internal-only modules under internal/
Files:
src/js/internal/sql/postgres.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/internal/sql/postgres.tssrc/js/bun/sql.ts
src/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Files:
src/js/bun/sql.ts
src/js/bun/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/
Files:
src/js/bun/sql.ts
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/VirtualMachine.zig
**/*.classes.ts
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.classes.ts: Define JavaScript class interfaces in .classes.ts using define({ name, constructor, JSType, finalize, proto: { ... } })
Use cache: true on properties that should use JSC WriteBarrier-backed caching
Files:
src/bun.js/api/sql.classes.ts
src/**/*.classes.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Define JavaScriptCore class metadata in .classes.ts files to drive code generation
Files:
src/bun.js/api/sql.classes.ts
🧬 Code graph analysis (2)
src/js/builtins.d.ts (1)
src/js/internal-for-testing.ts (1)
SQL(17-17)
src/js/bun/sql.ts (3)
src/js/internal/sql/query.ts (3)
values(262-271)Query(23-23)resolve(183-194)src/js/internal/sql/postgres.ts (3)
err(614-643)err(645-674)escapeIdentifier(810-812)src/js/internal/sql/shared.ts (1)
SQLHelper(143-143)
🔇 Additional comments (9)
.gitignore (1)
21-21: Good call ignoring.ccls-cache.Prevents committing editor cache artifacts.
src/bun.js/VirtualMachine.zig (1)
222-225: LGTM! Good practice for debug builds.The addition of ASAN-conditional VM destruction ensures proper resource cleanup during testing, which helps detect memory leaks and use-after-free bugs more accurately. The OR logic is correct, and since
bun.Environment.enable_asanis compile-time, there's no runtime overhead in production builds.src/codegen/class-definitions.ts (1)
289-291: Approve conditional DOMJIT cleanupThe guard avoids polluting objects with unused
DOMJITproperties and downstream code checks its truthiness, so behavior is unchanged.cmake/tools/SetupLLVM.cmake (1)
1-146: LGTM!These are formatting and indentation improvements with no functional changes. The logic remains semantically unchanged.
src/sql/postgres/protocol/CopyOutResponse.zig (1)
3-29: LGTM!The implementation correctly handles:
- Memory allocation with proper cleanup via
deinit- Error handling with
errdeferto prevent leaks- Safe casting using
@max(try reader.short(), 0)to guard against negative column counts before the@intCasttousizeThe pattern matches PostgreSQL COPY protocol specifications for response messages.
src/sql/postgres/AnyPostgresError.zig (1)
3-4: LGTM!The new COPY-related error variants are properly defined and mapped to appropriate error codes following the existing
ERR_POSTGRES_*naming convention.Also applies to: 23-23, 30-30, 86-87, 104-104, 110-110
src/sql/postgres/protocol/CopyFail.zig (1)
22-22: LGTM!Removes unnecessary parentheses from
@sizeOf((u32))to@sizeOf(u32). The calculation logic remains unchanged; this is a code quality improvement.src/bun.js/api/sql.classes.ts (1)
6-70: LGTM!The refactoring improves code organization by:
- Extracting the
protoobject definition for clarity- Conditionally adding COPY-specific methods (
sendCopyData,sendCopyDone,sendCopyFail) only for PostgreSQL connections- Maintaining the same public API surface
The method length parameters are correctly specified (1 for data/fail, 0 for done).
src/sql/postgres/protocol/CopyData.zig (1)
8-8: Verify PostgreSQL COPY protocol adjustments
- decodeInternal: offset changed from
length -| 5tolength -| 4- writeInternal: switched from
writer.string(data)(null-terminated) towriter.write(data)(raw bytes)Ensure both conform to the COPY Data message format in the PostgreSQL specification.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bun-types/sql.d.ts (1)
602-677: LGTM! High-level COPY helpers are comprehensive and well-typed.The COPY method additions are complete:
copyFrom(lines 604-632): Bulk import helper with extensive options including the previously requestedmaxChunkSize,maxBytes, andtimeoutcontrols.
copyTo(lines 634-651): Streaming export helper returningAsyncIterablewith the previously requestedmaxBytes,stream, andtimeoutoptions.
copyToPipeTo(lines 653-677): Piping helper for direct sink integration with the same options ascopyTo.All options from the previous review have been successfully added to the type definitions.
Optional refinement: At line 671, consider explicitly adding
ArrayBufferto theWritableStreamgeneric for consistency with the custom write interface at line 673, though this is a minor point since the Web Streams API typically only supportsUint8Arraydirectly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/bun-types/sql.d.ts(2 hunks)
🔇 Additional comments (1)
packages/bun-types/sql.d.ts (1)
13-74: LGTM! Comprehensive low-level COPY protocol controls.The additions provide fine-grained control over PostgreSQL COPY operations:
- Event callbacks (
onCopyStart,onCopyChunk,onCopyEnd) for monitoring- Control methods (
copySendData,copyDone,copyFail) for managing data flow- Configuration methods for timeouts, buffer sizes, and streaming modes
- Defaults management for reusable settings
These low-level methods complement the high-level helpers and enable advanced use cases where users need direct protocol control.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/bun/sql.ts (1)
2173-2196: [Duplicate] Fix schema-qualified table quoting in copyTo().Line 2196 still wraps the entire table identifier in a single pair of quotes (
"schema.table"), which fails for schema-qualified names. The server will look for a literal relation namedschema.tableinstead ofschema."table"`.The
copyFromhelper at line 1063-1066 already implements correct identifier escaping that splits on dots. ThecopyTohelper should use the same pattern.Apply this diff to fix the table identifier escaping:
+ const escapeIdentifier = (value: string) => + '"' + value.replaceAll('"', '""').replaceAll(".", '"."') + '"'; const table = queryOrOptions.table; const cols = (queryOrOptions.columns ?? []) - .map(c => '"' + String(c).replaceAll('"', '""').replaceAll(".", '"."') + '"') + .map(c => escapeIdentifier(String(c))) .join(", "); const fmt = queryOrOptions.format === "csv" ? " (FORMAT CSV)" : queryOrOptions.format === "binary" ? " (FORMAT BINARY)" : ""; - return `COPY "${String(table).replaceAll('"', '""')}"${cols ? ` (${cols})` : ""} TO STDOUT${fmt}`; + return `COPY ${escapeIdentifier(String(table))}${cols ? ` (${cols})` : ""} TO STDOUT${fmt}`;Based on learnings
🧹 Nitpick comments (1)
src/js/bun/sql.ts (1)
418-504: Consider tightening adapter interface types to reduce casts.The COPY control methods rely heavily on
(pool as any)and(reserved as any)type assertions, suggesting the adapter interfaces don't fully expose these methods. While the runtime fallback logic is sound, consider:
- Extending the adapter base interface to declare COPY-related methods as optional
- Using proper type guards instead of runtime
typeofchecks after casting toanyThis would improve type safety and make the codebase more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/bun-types/sql.d.ts(4 hunks)src/js/bun/sql.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Files:
src/js/bun/sql.ts
src/js/bun/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/
Files:
src/js/bun/sql.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/bun/sql.ts
🧬 Code graph analysis (1)
src/js/bun/sql.ts (2)
src/js/thirdparty/undici.js (1)
AbortSignal(17-17)src/js/internal/sql/postgres.ts (1)
escapeIdentifier(810-812)
🔇 Additional comments (5)
src/js/bun/sql.ts (2)
16-94: LGTM! Comprehensive COPY option types.The type definitions for COPY operations are well-structured and cover the necessary configuration surface including formats, delimiters, binary types, progress tracking, timeouts, and resource limits.
2047-2112: Validate quoted identifiers in binary COPY OID check
ThereplaceAll('"', "")at line 2059 may strip necessary quotes for case-sensitive or schema-qualified names, causing OID validation errors. Add tests (e.g."MyTable","MySchema"."MyTable") or adjust logic to preserve quoted identifiers.packages/bun-types/sql.d.ts (3)
14-74: LGTM! ReservedSQL COPY interface is well-documented.The COPY control methods on ReservedSQL are clearly documented with helpful JSDoc comments explaining their purpose. The signatures align with the implementation in
src/js/bun/sql.ts.
125-157: LGTM! CopyBinary types are comprehensive.The
CopyBinaryBaseType,CopyBinaryArrayType, andCopyBinaryTypedefinitions cover the full range of PostgreSQL binary COPY types implemented in the encoder (src/js/bun/sql.ts lines 1156-1196).
637-711: LGTM! COPY method signatures are complete and match implementation.The
copyFrom,copyTo, andcopyToPipeTomethod signatures now include all the options that were flagged in the previous review (maxChunkSize, maxBytes, timeout, stream). The types align with the implementation insrc/js/bun/sql.ts.Based on learnings
Also applies to: 1042-1116
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
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.
📒 Files selected for processing (6)
src/js/bun/sql.ts(11 hunks)src/js/internal/sql/postgres.ts(6 hunks)src/sql/postgres/protocol/CopyBothResponse.zig(1 hunks)src/sql/postgres/protocol/CopyInResponse.zig(1 hunks)src/sql/postgres/protocol/CopyOutResponse.zig(1 hunks)test/js/sql/sql-postgres-copy.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/sql/postgres/protocol/CopyOutResponse.zigsrc/sql/postgres/protocol/CopyBothResponse.zigsrc/sql/postgres/protocol/CopyInResponse.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/sql/postgres/protocol/CopyOutResponse.zigsrc/sql/postgres/protocol/CopyBothResponse.zigsrc/sql/postgres/protocol/CopyInResponse.zig
src/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Files:
src/js/bun/sql.ts
src/js/bun/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/
Files:
src/js/bun/sql.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/bun/sql.tssrc/js/internal/sql/postgres.ts
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/sql/sql-postgres-copy.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/sql/sql-postgres-copy.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/sql/sql-postgres-copy.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/sql/sql-postgres-copy.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/sql/sql-postgres-copy.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/sql/sql-postgres-copy.test.ts
src/js/internal/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place internal modules not exposed to users under src/js/internal/
Files:
src/js/internal/sql/postgres.ts
src/js/internal/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place internal-only modules under internal/
Files:
src/js/internal/sql/postgres.ts
🧬 Code graph analysis (2)
src/js/bun/sql.ts (4)
src/js/thirdparty/undici.js (1)
AbortSignal(17-17)src/js/internal/sql/postgres.ts (1)
escapeIdentifier(810-812)src/js/internal/sql/shared.ts (1)
SQLHelper(143-143)src/js/internal/sql/sqlite.ts (1)
escapeIdentifier(352-354)
test/js/sql/sql-postgres-copy.test.ts (2)
test/harness.ts (1)
isDockerEnabled(856-879)src/js/internal-for-testing.ts (1)
SQL(17-17)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
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.
📒 Files selected for processing (2)
src/js/bun/sql.ts(11 hunks)src/sql/postgres/PostgresSQLConnection.zig(12 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Files:
src/js/bun/sql.ts
src/js/bun/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/
Files:
src/js/bun/sql.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/bun/sql.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/sql/postgres/PostgresSQLConnection.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
🧬 Code graph analysis (1)
src/js/bun/sql.ts (1)
src/js/internal/sql/postgres.ts (3)
err(614-643)err(645-674)escapeIdentifier(810-812)
Fix async flush must be awaited before mutating batch. Fix update counters when piping raw byte chunks. Fix binary COPY signature check skips fragmented headers.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
.zed/settings.json (1)
1-14: Remove personal Zed config from the repoThis still hardcodes your local
/home/gohryt/...toolchain paths, so it’s non-portable and leaks workstation details. Please drop.zed/settings.json(and ignore.zed/), or replace these with project-relative paths and document the shared setup instead.src/sql/postgres/PostgresSQLConnection.zig (1)
1980-2078: Binary COPY signature: fragmented header handling looks correct.The logic now validates across buffered+incoming bytes in both streaming and non‑streaming paths, addressing the earlier gap.
src/js/bun/sql.ts (2)
2236-2244: Clamp copyTo maxBytes; avoid| 0.Prevent modulo wrap when enforcing receive limit.
Apply this diff:
- const toMax = - typeof queryOrOptions === "string" - ? __toDefaults__.maxBytes | 0 - : typeof (queryOrOptions as any)?.maxBytes === "number" && (queryOrOptions as any).maxBytes > 0 - ? Number((queryOrOptions as any).maxBytes) - : __toDefaults__.maxBytes | 0; + const toMax = (() => { + const v = + typeof queryOrOptions === "string" + ? __toDefaults__.maxBytes + : typeof (queryOrOptions as any)?.maxBytes === "number" && (queryOrOptions as any).maxBytes > 0 + ? Number((queryOrOptions as any).maxBytes) + : __toDefaults__.maxBytes; + return Math.max(0, Math.trunc(Number(v) || 0)); + })();
452-475: Avoid| 0/>>> 0for limits; clamp instead.Bitwise coercion modulo‑wraps large values. Clamp to the supported range.
Apply this diff:
- reserved_sql.setCopyTimeout = (ms: number) => { + reserved_sql.setCopyTimeout = (ms: number) => { if (typeof (pool as any).setCopyTimeoutFor === "function") { - (pool as any).setCopyTimeoutFor(pooledConnection, (ms | 0) >>> 0); + const v = Number(ms); + const clamped = Math.max(0, Math.min(v, 0xFFFFFFFF)); + (pool as any).setCopyTimeoutFor(pooledConnection, clamped); } else { const underlying = pool.getConnectionForQuery ? pool.getConnectionForQuery(pooledConnection) : pooledConnection?.connection; if (underlying && (PostgresAdapter as any).setCopyTimeout) { - (PostgresAdapter as any).setCopyTimeout(underlying, (ms | 0) >>> 0); + const v = Number(ms); + const clamped = Math.max(0, Math.min(v, 0xFFFFFFFF)); + (PostgresAdapter as any).setCopyTimeout(underlying, clamped); } } }; /** @type {(bytes: number) => void} */ reserved_sql.setMaxCopyBufferSize = (bytes: number) => { if (typeof (pool as any).setMaxCopyBufferSizeFor === "function") { - (pool as any).setMaxCopyBufferSizeFor(pooledConnection, (bytes | 0) >>> 0); + const v = Number(bytes); + const clamped = Math.max(0, Math.min(v, 0xFFFFFFFF)); + (pool as any).setMaxCopyBufferSizeFor(pooledConnection, clamped); } else { const underlying = pool.getConnectionForQuery ? pool.getConnectionForQuery(pooledConnection) : pooledConnection?.connection; if (underlying && (PostgresAdapter as any).setMaxCopyBufferSize) { - (PostgresAdapter as any).setMaxCopyBufferSize(underlying, (bytes | 0) >>> 0); + const v = Number(bytes); + const clamped = Math.max(0, Math.min(v, 0xFFFFFFFF)); + (PostgresAdapter as any).setMaxCopyBufferSize(underlying, clamped); } } };
📜 Review details
Configuration used: CodeRabbit UI
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.
📒 Files selected for processing (4)
.zed/settings.json(1 hunks)src/js/bun/sql.ts(11 hunks)src/sql/postgres/PostgresSQLConnection.zig(12 hunks)test/js/sql/sql-postgres-copy.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
src/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Files:
src/js/bun/sql.ts
src/js/bun/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/
Files:
src/js/bun/sql.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/bun/sql.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/sql/postgres/PostgresSQLConnection.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/sql/sql-postgres-copy.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/sql/sql-postgres-copy.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/sql/sql-postgres-copy.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/sql/sql-postgres-copy.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/sql/sql-postgres-copy.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/sql/sql-postgres-copy.test.ts
🧬 Code graph analysis (2)
src/js/bun/sql.ts (3)
src/js/internal/sql/query.ts (3)
values(262-271)Query(23-23)resolve(183-194)src/js/internal/sql/postgres.ts (3)
err(614-643)err(645-674)escapeIdentifier(810-812)src/js/internal/sql/shared.ts (1)
SQLHelper(143-143)
test/js/sql/sql-postgres-copy.test.ts (2)
test/harness.ts (1)
isDockerEnabled(856-879)src/js/internal-for-testing.ts (1)
SQL(17-17)
🔇 Additional comments (3)
test/js/sql/sql-postgres-copy.test.ts (2)
6-21: Solid docker gating and connection lifecycle.Good skip behavior on non‑docker envs and cleanup via afterAll.
268-379: Binary COPY scenarios are well covered.Valid header/rows and streaming checks help guard regressions.
src/js/bun/sql.ts (1)
2167-2183: Table/column identifier escaping in copyTo() looks correct.Schema‑qualified names are handled by quoting each part.
Fix await flushBatch before trailer/done. Fix type mismatch and redundant clamp in progress counters.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/js/bun/sql.ts (1)
2170-2175: DRY up identifier escaping in copyTo()You re-implemented the quoting logic inline. Use a single escapeIdentifier helper (same as copyFrom) to avoid drift and subtle inconsistencies.
- const table = queryOrOptions.table; - // Escape table identifier with same logic as copyFrom to handle schema-qualified names - const tableName = '"' + String(table).replaceAll('"', '""').replaceAll(".", '"."') + '"'; - const cols = (queryOrOptions.columns ?? []) - .map(c => '"' + String(c).replaceAll('"', '""').replaceAll(".", '"."') + '"') - .join(", "); + const escapeIdentifier = (value: string) => + '"' + String(value).replaceAll('"', '""').replaceAll(".", '"."') + '"'; + const tableName = escapeIdentifier(queryOrOptions.table); + const cols = (queryOrOptions.columns ?? []).map(c => escapeIdentifier(c)).join(", ");
📜 Review details
Configuration used: CodeRabbit UI
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.
📒 Files selected for processing (2)
src/js/bun/sql.ts(11 hunks)src/sql/postgres/PostgresSQLConnection.zig(12 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Files:
src/js/bun/sql.ts
src/js/bun/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/
Files:
src/js/bun/sql.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/bun/sql.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/sql/postgres/PostgresSQLConnection.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
🧬 Code graph analysis (1)
src/js/bun/sql.ts (4)
src/js/thirdparty/undici.js (1)
AbortSignal(17-17)src/js/internal/sql/postgres.ts (3)
err(614-643)err(645-674)escapeIdentifier(810-812)src/js/internal/sql/mysql.ts (3)
err(321-353)err(355-384)escapeIdentifier(480-482)src/js/internal/sql/sqlite.ts (1)
escapeIdentifier(352-354)
🔇 Additional comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
1980-2077: Good fix: validate binary COPY signature across fragmented framesThe new logic correctly accumulates header bytes (both streaming and non‑streaming) and validates the signature once enough data is present, closing the gap where split frames skipped validation. Looks solid.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
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.
📒 Files selected for processing (2)
src/js/bun/sql.ts(11 hunks)src/sql/postgres/PostgresSQLConnection.zig(12 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/sql/postgres/PostgresSQLConnection.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
src/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Files:
src/js/bun/sql.ts
src/js/bun/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/
Files:
src/js/bun/sql.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/bun/sql.ts
🧬 Code graph analysis (1)
src/js/bun/sql.ts (3)
src/js/internal/sql/postgres.ts (3)
err(614-643)err(645-674)escapeIdentifier(810-812)src/js/internal/sql/mysql.ts (3)
err(321-353)err(355-384)escapeIdentifier(480-482)src/js/internal/sql/sqlite.ts (1)
escapeIdentifier(352-354)
🔇 Additional comments (8)
src/sql/postgres/PostgresSQLConnection.zig (8)
4-108: LGTM! Well-designed COPY state management.The constants and state fields are comprehensive and well-documented:
- Sensible limits: 256MB max buffer, 64MB shrink threshold, 5-minute default timeout
- Complete state tracking: progress counters, streaming mode, timeout tracking, header validation
- Proper PostgreSQL binary COPY signature constant
112-165: LGTM! JS bindings have proper validation.The JS-exposed methods are well-implemented:
- Proper bounds checking with
@minto prevent overflow (lines 131, 147)awaitWritableprovides useful backpressure management for JS callers- Clear error messages for insufficient arguments
995-1089: LGTM! Comprehensive COPY data sending implementation.The COPY data sending functions are well-designed:
- Proper state validation before operations
- Clear, detailed error messages for debugging
- Safe handling of both ArrayBuffer and string data
- Progress tracking with saturating adds to prevent overflow
- Resource cleanup on error paths
1115-1172: LGTM! Thorough cleanup with optimization.The
cleanupCopyStatefunction is comprehensive:
- Early exit optimization for already-clean state
- Frees all allocated resources
- Smart buffer shrinking when capacity exceeds threshold
- Resets all state fields
- Good debug logging
1174-1218: LGTM! Robust COPY initialization.The
startCopyfunction handles initialization well:
- Prevents concurrent COPY operations
- Duplicates column formats with proper error handling (errdefer)
- Fires onCopyStart callback with exception handling
- Cleans up state if callback throws
1958-2156: LGTM! Complex but well-structured CopyData handling.This complex message handling is well-implemented and addresses past review concerns:
Strengths:
- ✅ Fragmented header handling fixed: Lines 2059-2075 correctly handle binary COPY signatures split across frames using a scratch buffer (addresses previous review comment)
- ✅ Progress tracking fixed: Lines 2139-2140 use proper
@intCastwith correct types (addresses previous review comment)- Multiple buffer size checks prevent DoS attacks (lines 1984, 1996, 2082, 2105, 2118)
- Reentrancy protection via
copy_callback_in_progressflag prevents callback issues- Timeout validation prevents hung operations
- Clear separation between streaming and non-streaming paths
Implementation details:
- Binary header validation supports fragmentation in both streaming (lines 1982-2042) and non-streaming (lines 2043-2077) modes
- Streaming mode emits chunks immediately while non-streaming accumulates
- Progress tracking occurs exactly once per chunk across all code paths
Based on learnings
2198-2204: LGTM! Correct protocol message handling.The COPY protocol message handlers are properly implemented:
CommandCompletecorrectly delegates tofinishCopyfor COPY operationsErrorResponsecleans up COPY state on errors during COPY operationsCopyInResponseandCopyOutResponseproperly initialize COPY state with correct direction flagsCopyDonevalidates state and buffer size with safety guardsCopyBothResponsecorrectly rejects unimplemented streaming replication with clear error messageAlso applies to: 2443-2447, 2497-2573
455-456: LGTM! Comprehensive resource cleanup.COPY state cleanup is properly integrated into all teardown paths:
onClosecleans up when connection closes during COPY (line 456)deinitfrees COPY column formats and buffer (lines 1357-1360)cleanUpRequestsensures cleanup when clearing all requests (line 1370)This ensures no resource leaks regardless of how the connection terminates.
Also applies to: 1356-1360, 1369-1370
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/js/bun/sql.ts (5)
93-94: Fix SQLTemplateFn to accept tagged-template and helper/query callsType is too narrow; tagged templates and SQLHelper/Query calls won’t type-check in callbacks.
-type SQLTemplateFn = (strings: TemplateStringsArray | string, ...values: unknown[]) => Query<unknown, unknown>; +type SQLTemplateFn = ( + strings: TemplateStringsArray | string | import("internal/sql/shared.ts").SQLHelper<unknown> | Query<unknown, unknown>, + ...values: unknown[] +) => Query<unknown, unknown>;
1123-1144: CSV empty string serialized as NULL (must quote empty string)Empty string fields are emitted unquoted and become NULL when nullToken === "". Quote empty strings.
- const needsCsvQuoting = (s: string) => - s.includes('"') || s.includes("\n") || s.includes("\r") || s.includes(delimiter); + const needsCsvQuoting = (s: string) => + s === "" || s.includes('"') || s.includes("\n") || s.includes("\r") || s.includes(delimiter); const csvQuote = (s: string) => `"${s.replaceAll('"', '""')}"`; @@ const parts = row.map(v => { const s = serializeValue(v); if (s === nullToken) return ""; - return needsCsvQuoting(s) ? csvQuote(s) : s; + // empty string must be quoted to distinguish from NULL when nullToken === "" + return needsCsvQuoting(s) ? csvQuote(s) : s; });
2052-2096: binaryTypes OID validation: wrong matching without columns + unstable orderingAdd ORDER BY attnum and validate by name only when columns provided; otherwise validate by ordinal.
- const q = ` - SELECT a.attname::text AS name, a.atttypid::oid AS oid + const q = ` + SELECT a.attname::text AS name, a.atttypid::oid AS oid, a.attnum::int AS attnum FROM pg_catalog.pg_attribute a JOIN pg_catalog.pg_class c ON c.oid = a.attrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace WHERE c.relname = $1 AND ($2::text IS NULL OR n.nspname = $2) AND a.attnum > 0 AND NOT a.attisdropped - `; + ORDER BY a.attnum + `; const rows = await (reserved as any).unsafe(q, [relName, schemaName]); @@ - const oidByName = new Map<string, number>(); - for (const r of rows) { - if (typeof r?.name === "string" && typeof r?.oid === "number") { - oidByName.set(r.name, r.oid); - } - } - for (let i = 0; i < expectedOids.length; i++) { - const colName = String(colNames[i] ?? `col${i + 1}`); - const got = oidByName.get(colName); - const want = expectedOids[i]; - if (typeof got !== "number" || got !== want) { - throw new Error( - `COPY binaryTypes validation failed for column "${colName}": expected OID ${want}, got ${got}`, - ); - } - } + if (colNames.length > 0) { + const oidByName = new Map<string, number>(); + for (const r of rows) if (typeof r?.name === "string" && typeof r?.oid === "number") oidByName.set(r.name, r.oid); + for (let i = 0; i < expectedOids.length; i++) { + const colName = String(colNames[i]); + const got = oidByName.get(colName); + const want = expectedOids[i]; + if (typeof got !== "number" || got !== want) { + throw new Error(`COPY binaryTypes validation failed for column "${colName}": expected OID ${want}, got ${got}`); + } + } + } else { + if (expectedOids.length !== rows.length) { + throw new Error(`COPY binaryTypes length (${expectedOids.length}) must match table column count (${rows.length}) when columns are not specified.`); + } + for (let i = 0; i < expectedOids.length; i++) { + const got = rows[i]?.oid; + const want = expectedOids[i]; + if (typeof got !== "number" || got !== want) { + throw new Error(`COPY binaryTypes validation failed at column ${i + 1}: expected OID ${want}, got ${got}`); + } + } + }
452-476: Avoid 32‑bit bitwise coercion for COPY timeout/buffer setters|0/>>>0 can wrap/disable limits. Use a clamp helper.
+ const clampUint32 = (value: number) => { + const n = Number(value); + if (!Number.isFinite(n) || n <= 0) return 0; + return Math.min(0xffffffff, Math.trunc(n)); + }; /** @type {(ms: number) => void} */ reserved_sql.setCopyTimeout = (ms: number) => { if (typeof (pool as any).setCopyTimeoutFor === "function") { - (pool as any).setCopyTimeoutFor(pooledConnection, (ms | 0) >>> 0); + (pool as any).setCopyTimeoutFor(pooledConnection, clampUint32(ms)); } else { @@ - if (underlying && (PostgresAdapter as any).setCopyTimeout) { - (PostgresAdapter as any).setCopyTimeout(underlying, (ms | 0) >>> 0); + if (underlying && (PostgresAdapter as any).setCopyTimeout) { + (PostgresAdapter as any).setCopyTimeout(underlying, clampUint32(ms)); } } }; /** @type {(bytes: number) => void} */ reserved_sql.setMaxCopyBufferSize = (bytes: number) => { if (typeof (pool as any).setMaxCopyBufferSizeFor === "function") { - (pool as any).setMaxCopyBufferSizeFor(pooledConnection, (bytes | 0) >>> 0); + (pool as any).setMaxCopyBufferSizeFor(pooledConnection, clampUint32(bytes)); } else { @@ - if (underlying && (PostgresAdapter as any).setMaxCopyBufferSize) { - (PostgresAdapter as any).setMaxCopyBufferSize(underlying, (bytes | 0) >>> 0); + if (underlying && (PostgresAdapter as any).setMaxCopyBufferSize) { + (PostgresAdapter as any).setMaxCopyBufferSize(underlying, clampUint32(bytes)); } } };
2121-2127: Avoid| 0truncation for COPY FROM timeoutUse safe clamp to keep large timeouts valid.
- const timeout = - options && typeof (options as any).timeout === "number" && (options as any).timeout >= 0 - ? (options as any).timeout | 0 - : (__fromDefaults__.timeout ?? 0) | 0; + const timeout = + options && typeof (options as any).timeout === "number" && (options as any).timeout >= 0 + ? Math.max(0, Math.trunc(Number((options as any).timeout) || 0)) + : Math.max(0, Math.trunc(Number(__fromDefaults__.timeout ?? 0) || 0));
📜 Review details
Configuration used: CodeRabbit UI
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.
📒 Files selected for processing (3)
src/js/bun/sql.ts(11 hunks)src/sql/postgres/PostgresSQLConnection.zig(12 hunks)test/js/sql/sql-postgres-copy.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
src/js/bun/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Bun-specific built-in modules under src/js/bun/ (e.g., bun:ffi, bun:sqlite)
Files:
src/js/bun/sql.ts
src/js/bun/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Bun-specific modules (e.g., bun:ffi, bun:sqlite) under bun/
Files:
src/js/bun/sql.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/bun/sql.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/sql/postgres/PostgresSQLConnection.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/sql/postgres/PostgresSQLConnection.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/sql/sql-postgres-copy.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/sql/sql-postgres-copy.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/sql/sql-postgres-copy.test.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.ts: Name test files*.test.tsand usebun:test
Do not write flaky tests: never wait for arbitrary time; wait for conditions instead
Never hardcode port numbers in tests; useport: 0to get a random port
When spawning Bun in tests, usebunExe()andbunEnvfromharness
Preferasync/awaitin tests; for a single callback, usePromise.withResolvers()
Do not set explicit test timeouts; rely on Bun’s built-in timeouts
UsetempDir/tempDirWithFilesfromharnessfor temporary files and directories in tests
For large/repetitive strings in tests, preferBuffer.alloc(count, fill).toString()over"A".repeat(count)
Import common test utilities fromharness(e.g.,bunExe,bunEnv,tempDirWithFiles,tmpdirSync, platform checks, GC helpers)
In error tests, assert non-zero exit codes for failing processes and usetoThrowfor synchronous errors
Usedescribeblocks for grouping,describe.eachfor parameterized tests, snapshots withtoMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach); track resources for cleanup inafterEach
Useusing/await usingwith Bun resources (e.g., Bun.listen/connect/spawn/serve) to ensure cleanup in tests
Files:
test/js/sql/sql-postgres-copy.test.ts
test/js/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests for specific features under
test/js/by module
Files:
test/js/sql/sql-postgres-copy.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or roll your own random port
Prefer normalizeBunSnapshot for snapshotting test output instead of asserting raw strings
Do not write tests that assert absence of crashes (e.g., 'no panic' or 'no uncaught exception')
Use Bun’s Jest-compatible runner (import { test, expect } from "bun:test") for tests
Avoid shell commands like find or grep in tests; use Bun’s Glob and built-in tools instead
Prefer running tests via bun bd test and use provided harness utilities (bunEnv, bunExe, tempDir)
Use Bun.spawn with proper stdio handling and await proc.exited in process-spawning tests
Files:
test/js/sql/sql-postgres-copy.test.ts
🧬 Code graph analysis (2)
src/js/bun/sql.ts (1)
src/js/internal/sql/postgres.ts (3)
err(614-643)err(645-674)escapeIdentifier(810-812)
test/js/sql/sql-postgres-copy.test.ts (2)
test/harness.ts (1)
isDockerEnabled(856-879)src/js/internal-for-testing.ts (1)
SQL(17-17)
🔇 Additional comments (3)
src/sql/postgres/PostgresSQLConnection.zig (3)
1311-1317: Good guard: ensure binary COPY header validated before returning dataPrevents incomplete/invalid binary streams from succeeding.
1988-2051: Good: fragmented binary COPY header handlingCorrectly buffers and validates split signatures; safe buffering with limits.
2146-2149: Good: saturating progress counters with proper castsAvoids overflow/type mismatch. Clean.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 1439-1450: Binary rows encoded by encodeBinaryRow are being
written with reserved.copySendData(payload) which bypasses sendChunkedData and
the maxChunkSize enforcement; update the binary-path (inside the block that
calls validateBinaryTypes and encodeBinaryRow) to route the payload through
sendChunkedData instead of copySendData so large binary rows get chunked
consistently. Use sendChunkedData(reserved, payload, maxChunkSize, pool) (or the
existing sendChunkedData signature in this module), ensure bytesSent and
chunksSent are incremented correctly if sendChunkedData does not already update
them, and remove or adapt the separate notifyProgress/awaitWritableWithFallback
calls if sendChunkedData handles flow control; keep sendBinaryHeader() and the
validateBinaryTypes/encodeBinaryRow usage intact.
♻️ Duplicate comments (2)
src/js/internal/sql/postgres.ts (1)
349-359: Verify Zig binding invocation pattern for COPY helpers.The past review flagged that COPY bindings imported from
$zigexpectthisto be the connection. The current implementation passesconnectionas the first argument:function copySendData(connection, data) { sendCopyData(connection, data); }If the Zig binding signature truly expects
connectionasthis, you need.$call(connection, data). However, the comment on line 350-351 states "expects explicit thisArg as first parameter", suggesting the binding was designed to acceptconnectionas a regular argument rather thanthis.Please confirm the Zig binding contract matches this invocation pattern. If the binding expects
this, apply the fix from the past review.#!/bin/bash # Check how the Zig binding is defined to confirm parameter expectations rg -A 20 "sendCopyData|copySendDataFromJSValue" --type zig | head -60src/js/bun/sql.ts (1)
1766-1768: Streaming capability detection checks wrapper existence, not registration.The variables
hasCopyChunkHandlerandhasCopyEndHandlercheck whether the wrapper functions exist (typeof reserved.onCopyChunk === "function"), but these wrappers are always defined on reserved connections. The actual capability depends on whetheronCopyChunk()was called to register a handler.The fallback logic at line 1837 (
if (desiredStream && !hasCopyChunkHandler)) will never trigger becausehasCopyChunkHandleris alwaystruefor PostgreSQL reserved connections.Consider checking if the underlying connection has
copy_chunk_handler_registeredflag set, or haveonCopyChunkreturn a boolean indicating successful registration.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
537-663: Reject pendingawaitWritable()on remote close.
onClosecan be invoked by the server closing the socket, but pendingawaitWritable()promises are only rejected inclose()/disconnect(). That leaves a hanging promise when the remote side closes unexpectedly.🩹 Proposed fix
pub fn onClose(this: *PostgresSQLConnection) void { this.unregisterAutoFlusher(); // Clean up COPY state if connection closes during COPY operation this.cleanupCopyState(); + this.rejectAwaitWritable(this.globalObject, "Connection closed"); if (this.vm.isShuttingDown()) {
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 1266-1277: Add input validation to the new COPY APIs: in
sql.copyFrom validate that the table parameter is a non-empty string and that
columns is an array (and each column a string) using the helpers from
internal/validators and throw $ERR_INVALID_ARG_VALUE on bad inputs; also
validate options.format against the allowed formats before building SQL;
similarly, in copyToPipeTo validate that the provided sink is a Writable stream
(or otherwise allowed) using the validators and throw $ERR_INVALID_ARG_VALUE for
invalid writable sinks. Target the sql.copyFrom function and the copyToPipeTo
routine, use internal/validators helper functions for checks, and ensure all
thrown errors use $ERR_INVALID_ARG_VALUE to match public API conventions.
- Around line 1366-1370: The text COPY branch currently checks the serialized
string (s) against nullToken, causing literal values like "\N" to be returned
unescaped and treated as NULL; update the row.map handler so it first checks the
original value (v) for v === null || v === undefined and returns nullToken in
that case, otherwise call serializeValue(v) and then pass the result to
copyTextEscape; change references in this block to use v === null || v ===
undefined (not s === nullToken) and ensure copyTextEscape(serialized) is used
for all non-null values (symbols: serializeValue, nullToken, copyTextEscape, the
row.map callback).
- Around line 1266-1279: Add an adapter guard at the start of the COPY-related
methods (e.g., sql.copyFrom, sql.copyTo, sql.copyToPipeTo) to check
reserved/connectionInfo.adapter and fail fast for non-Postgres adapters:
validate connectionInfo.adapter === "postgres" (or the postgres adapter
identifier used) and throw a clear Error before calling sql.reserve() or any
pool.* copy* methods; apply the same guard to the other COPY implementations
referenced (the methods around the other ranges) so pool.onCopyStartFor /
pool.copySendDataFor / pool.copyDoneFor / pool.copyFailFor /
pool.setCopyStreamingModeFor are only invoked for the Postgres adapter.
- Around line 1390-1400: The COPY BINARY header/trailer must always be emitted
even for empty iterables: move binary mode detection and call to
sendBinaryHeader (which uses binaryHeaderSent, reserved.copySendData,
createBinaryCopyHeader) to before any iteration begins, remove the current "only
on first row" trigger, and after each iteration path unconditionally call
sendBinaryTrailer (which uses createBinaryCopyTrailer) regardless of
binaryHeaderSent. Update the async iterable loop (around
sendBinaryHeader/sendBinaryTrailer usage near the async iterable block), the
sync iterable loop, and the other related paths so header is written before
iterating and trailer is always written after the loop completes.
- Around line 1833-1897: The current COPY TO branch discards the result of
reserved.unsafe() when streaming is disabled and coerces everything to String
which corrupts binary format; update the branch that runs when desiredStream &&
!hasCopyChunkHandler (and the non-streaming path) to use the accumulated value
returned from reserved.unsafe() instead of ignoring it, and when building the
single payload preserve binary data by detecting ArrayBuffer/Uint8Array results
and concatenating buffers (or returning a single buffer) rather than calling
String(...)—adjust handling around the accumulated variable and where payload is
produced (functions/symbols to change: makeQuery(), reserved.unsafe(),
setCopyStreamingMode, setCopyTimeout, hasCopyChunkHandler, desiredStream,
accumulated) so COPY TO with format:"binary" and stream:false returns the exact
accumulated bytes.
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 1231-1276: The cleanupCopyState function is clearing the
connection-level flag copy_chunk_handler_registered which disables future COPY
streaming; remove the line that sets this.copy_chunk_handler_registered = false
from cleanupCopyState (leave other resets like copy_streaming_mode and buffers
intact) and instead only clear copy_chunk_handler_registered when the connection
is explicitly closed or when the JS handler is explicitly unregistered (e.g., in
the connection teardown or an unregister handler function) so handler
registration persists across successive COPY operations; update references in
cleanupCopyState to no longer touch copy_chunk_handler_registered.
♻️ Duplicate comments (1)
src/js/bun/sql.ts (1)
253-256: Use$isArrayinstead ofArray.isArrayin builtins.These spots still use the public global, which is user-tamperable. Replace with the intrinsic
$isArray.♻️ Proposed edits
- if (!types || !Array.isArray(types)) { + if (!types || !$isArray(types)) { ... - if (fmt === "binary" && options && Array.isArray((options as any).binaryTypes)) { + if (fmt === "binary" && options && $isArray((options as any).binaryTypes)) { ... - if (Array.isArray(accumulated)) { + if ($isArray(accumulated)) {As per coding guidelines, use intrinsic
$isArrayin builtins.Also applies to: 1579-1580, 1872-1873
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
65-118: Prefix these COPY/await‑writable fields with#to align with codebase conventions.
These fields are documented as internal but lack the private field prefix used throughout the codebase (e.g.,src/sql/mysql/js/JSMySQLConnection.ziguses#js_value,#globalObject, etc., andsrc/sql/postgres/protocol/CopyResponse.ziguses#overall_format). Apply the#prefix to:await_writable_promise,copy_state,copy_format,copy_column_formats,copy_data_buffer,copy_owner,copy_bytes_transferred,copy_chunks_processed,copy_streaming_mode,copy_chunk_handler_registered,copy_callback_in_progress,copy_timeout_ms,copy_start_timestamp_ms, andcopy_binary_header_validated.
🤖 Fix all issues with AI agents
In `@packages/bun-types/sql.d.ts`:
- Around line 14-78: The type declaration for copySendData is missing
ArrayBuffer support; update the signature of copySendData in
packages/bun-types/sql.d.ts to accept string | Uint8Array | ArrayBuffer
(matching the runtime Zig implementation used by copySendDataFromJSValue and
aligning with onCopyChunk which already allows ArrayBuffer), then run a quick
type-check to ensure callers accepting ArrayBuffer compile without errors.
In `@src/js/bun/sql.ts`:
- Around line 1569-1581: The code currently calls validateBinaryTypes(options,
columns) inside each iteration for async and sync iterables, causing redundant
work for large COPY streams; instead compute and cache the result once (lazily
if needed) before entering the loops and reuse it. Locate the places where
validateBinaryTypes is invoked inside the iteration blocks (references:
validateBinaryTypes, maybeIter, encodeBinaryRow, sendChunkedData, flushBatch,
fmt, columns, options) and introduce a local cached variable (e.g.,
cachedBinaryTypes or types) initialized to undefined and set to
validateBinaryTypes(options, columns) only the first time fmt === "binary" is
encountered; then use that cached value for subsequent rows in both the async
and sync iteration paths. Ensure behavior unchanged when fmt !== "binary" and
preserve existing calls to flushBatch and sendChunkedData.
- Around line 150-152: Replace the user‑tamperable global Array.isArray with the
intrinsic $isArray in the type guard function isCopyFromBinaryOptions (and other
similar builtins that check arrays) so the runtime uses the secure intrinsic;
keep the guard logic the same (check options, options.format === "binary",
"binaryTypes" in options) but call $isArray(options.binaryTypes) instead of
Array.isArray(options.binaryTypes).
- Around line 1405-1486: The serialization uses options?.delimiter and
options?.null (delimiter, nullToken, fmt, serializeRow/serializeValue) but the
SQL builder only emits DELIMITER/NULL for CSV and truncates multi-char
delimiters; validate options.delimiter is either undefined or exactly one
character (throw/raise a user-facing error when it's longer) and normalize
delimiter to that single character at the top where delimiter is computed, then
ensure the SQL COPY generation emits DELIMITER '<char>' and NULL '<token>' for
both FORMAT 'csv' and FORMAT 'text' cases so client serialization and server
COPY settings match.
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 537-542: on remote closes (onClose) we currently unregister
flusher and cleanup copy state but do not notify any pending awaitWritable
callers, leaving promises hanging; update PostgresSQLConnection.onClose to
detect and reject/complete any pending awaitWritable waiter (the internal
awaitWritable promise/waker/future on the connection) with an error indicating
the remote closed so callers unblock, and ensure this is done before or
alongside unregisterAutoFlusher()/cleanupCopyState to avoid races; reference the
awaitWritable waiter field/function used by awaitWritable and clear it after
rejecting to avoid double-completion or leaks.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 2031-2059: Extract the duplicated helper functions to a shared
scope (module-level or enclosing function) by moving toUint8Array,
toRealArrayBuffer, and joinUint8Arrays out of the conditional branches and into
the surrounding scope, then remove their duplicate definitions from both the
desiredStream && !hasCopyChunkHandler branch and the !desiredStream branch;
ensure any references to these functions (calls within joinUint8Arrays usage
sites) still resolve after moving them to the new shared scope and keep their
exact signatures (toUint8Array(value: unknown): Uint8Array | null,
toRealArrayBuffer(u8: Uint8Array): ArrayBuffer, joinUint8Arrays(parts:
Uint8Array[]): ArrayBuffer).
- Around line 2061-2096: The binary/text accumulation logic duplicated around
the `isBinary` handling (using `accumulated`, `toUint8Array`, `joinUint8Arrays`,
`toRealArrayBuffer`, and `$ERR_INVALID_ARG_VALUE`) should be extracted into a
single generator helper (e.g., `yieldAccumulated(accumulated, isBinary)`) that
encapsulates the Array.isArray checks, error throws, joining of Uint8Array
parts, and string joining; replace both occurrences (the `desiredStream` and
`!desiredStream` branches) with calls to this helper so the same behavior is
reused and tests/signatures remain unchanged.
♻️ Duplicate comments (3)
src/js/bun/sql.ts (3)
150-152: Use$isArrayinstead ofArray.isArrayin builtins.Multiple locations in this file use the public
Array.isArrayglobal which is user-tamperable. Replace all occurrences with the intrinsic$isArrayper coding guidelines.Affected locations:
- Line 151:
Array.isArray(options.binaryTypes)- Line 332:
Array.isArray(types)invalidateBinaryTypes- Line 1377:
Array.isArray(columns)- Line 1771:
Array.isArray(rows)- Lines 2062, 2077, 2090, 2093, 2149, 2164, 2177, 2180:
Array.isArray(accumulated)♻️ Example fix for line 151
const isCopyFromBinaryOptions = (options: CopyFromOptions | undefined): options is CopyFromBinaryOptions => { - return !!options && options.format === "binary" && "binaryTypes" in options && Array.isArray(options.binaryTypes); + return !!options && options.format === "binary" && "binaryTypes" in options && $isArray(options.binaryTypes); };As per coding guidelines, use intrinsic
$isArrayin builtins.
1573-1585: Avoid re-validating binary types on every row.
validateBinaryTypes()is called inside both the async (line 1578) and sync (line 1621) iterable loops, invoking validation for each row. Cache the result before iteration to reduce overhead for large COPY streams.♻️ Suggested refactor
+ let cachedBinaryTypes: string[] | null = null; + const getBinaryTypes = () => (cachedBinaryTypes ??= validateBinaryTypes(options, columns)); + // Async iterable (rows or raw string/Uint8Array chunks) if (isAsyncIterable(maybeIter)) { for await (const item of maybeIter as AsyncIterable<unknown>) { ... if ($isArray(item)) { if (fmt === "binary") { - const types = validateBinaryTypes(options, columns); + const types = getBinaryTypes(); await flushBatch();
1409-1411: Validate delimiter is a single character.The code accepts any string as
options.delimiter(line 1410), but the SQL generation at line 1811 truncates toString(delim)[0]. This causes a mismatch: serialization uses the full delimiter while PostgreSQL receives only the first character.🔧 Proposed fix
+ if (options?.delimiter !== undefined) { + const d = String(options.delimiter); + if (d.length !== 1) { + throw $ERR_INVALID_ARG_VALUE("options.delimiter", options.delimiter, "must be a single character"); + } + } const fmt = options?.format === "csv" ? "csv" : options?.format === "binary" ? "binary" : "text"; const delimiter = options?.delimiter ?? (fmt === "csv" ? "," : "\t");
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
65-118: Use#prefix for private fields in struct definition and all references.Per the project's Zig coding guidelines, internal implementation fields like
copy_state,copy_format,copy_data_buffer,await_writable_promise,copy_timeout_ms,copy_streaming_mode,copy_chunk_handler_registered,copy_callback_in_progress,copy_start_timestamp_ms, andcopy_binary_header_validatedshould be declared and accessed with the#prefix (e.g.,#copy_state,this.#copy_state). This matches the pattern used in similar structures likeMySQLConnectionand keeps the codebase consistent.Note: The struct currently has other internal fields without
#prefix. Consider whether a broader refactor of all internal fields would be appropriate to maintain consistency across the entire struct.
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 1712-1720: The fallback path currently bypasses
maxBytes/backpressure by calling reserved.copySendData(fallback) directly;
instead, route the sanitized string through the same chunking/backpressure logic
used elsewhere by calling sendChunkedData(fallback) so bytesSent, chunksSent,
notifyProgress and maxBytes enforcement are applied; ensure you keep the aborted
check, call sendBinaryTrailer only after sendChunkedData completes, and still
call reserved.copyDone() at the end (remove the direct reserved.copySendData
usage and update bytesSent/chunksSent via the existing sendChunkedData flow).
- Around line 1895-1913: Validate the copyTo option object before building SQL
in makeQuery: ensure queryOrOptions.table is a non-empty string or symbol,
queryOrOptions.columns (if present) is an array of valid identifier strings, and
queryOrOptions.format (if present) is one of "csv" or "binary"; if any check
fails throw the standard $ERR_INVALID_ARG_VALUE using internal/validators
(mirror copyFrom behavior). Do this at the start of makeQuery (before using
escapeIdentifier/pool) and sanitize inputs so tableName, cols and fmt are only
built from validated values referenced by queryOrOptions.table,
queryOrOptions.columns and queryOrOptions.format.
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 2084-2249: The streaming logic currently only flushes buffered
data when copy_binary_header_validated is true and always calls emitChunkToJS
regardless of streaming mode; update the conditions so flushBufferedChunkToJS()
is invoked when copy_streaming_mode is true and (copy_format == 0 or
copy_binary_header_validated) to handle text COPY correctly, and ensure
emitChunkToJS(data_slice) is only called when copy_streaming_mode is true
(non‑streaming should only append to copy_data_buffer and not call
emitChunkToJS); adjust checks around copy_streaming_mode, copy_format,
copy_binary_header_validated, flushBufferedChunkToJS, and emitChunkToJS to
enforce correct ordering and avoid duplicate emissions.
♻️ Duplicate comments (1)
src/js/bun/sql.ts (1)
332-333: Use$isArrayinstead ofArray.isArrayin builtins.Line 332-333, Line 1377-1378, Line 1786-1787, and Lines 2035-2071 still use the public global
Array.isArray, which is user‑tamperable in builtins. Please switch these checks to$isArray.♻️ Suggested change
- if (!types || !Array.isArray(types)) { + if (!types || !$isArray(types)) {- if (!Array.isArray(columns)) { + if (!$isArray(columns)) {- if (!Array.isArray(rows) || rows.length === 0) { + if (!$isArray(rows) || rows.length === 0) {- if (Array.isArray(accumulated)) { + if ($isArray(accumulated)) { ... - const value = Array.isArray(accumulated) ? accumulated[0] : (accumulated ?? null); + const value = $isArray(accumulated) ? accumulated[0] : (accumulated ?? null); ... - if (Array.isArray(accumulated)) { + if ($isArray(accumulated)) { ... - yield String(Array.isArray(accumulated) ? (accumulated[0] ?? "") : (accumulated ?? "")); + yield String($isArray(accumulated) ? (accumulated[0] ?? "") : (accumulated ?? ""));As per coding guidelines, use intrinsics/private globals in builtins.
Also applies to: 1377-1378, 1786-1787, 2035-2071
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 1453-1467: serializeValue currently calls
sanitizeString(JSON.stringify(v)) and doesn't handle JSON.stringify returning
undefined for functions/symbols; update serializeValue (and its fallback
stringify branch) to capture the result of JSON.stringify into a variable, check
if that result is undefined, and if so use String(v) before passing into
sanitizeString (so sanitizeString never receives undefined); reference function
serializeValue, helper sanitizeString, and variables nullToken and fmt when
making the change.
- Around line 2367-2374: sql.getCopyDefaults and sql.setCopyDefaults are being
attached unconditionally which breaks non-Postgres adapters
(pool.getCopyDefaults is not a function); guard these attachments the same way
other COPY APIs are guarded — only attach sql.getCopyDefaults and
sql.setCopyDefaults when the adapter supports COPY (e.g., check the adapter type
or the existence of pool.getCopyDefaults/pool.setCopyDefaults) and otherwise
skip adding them so MySQL/SQLite won't call undefined pool methods; update the
block that currently calls pool.getCopyDefaults/pool.setCopyDefaults to perform
that guard before assigning to sql.
- Around line 1359-1370: The SQL builtin methods copyFrom, copyTo, and
copyToPipeTo are missing an explicit this parameter and must be declared as
methods that receive this: Bun.SQL so they bind correctly from C++; update the
function signatures for copyFrom, copyTo (which uses const self = this), and
copyToPipeTo to include the explicit this: Bun.SQL parameter (e.g.,
function(this: Bun.SQL, ...)) and ensure any internal references to this remain
unchanged so existing logic continues to work.
- Around line 1574-1623: The issue is that raw byte buffers (Uint8Array/Buffer)
are iterable and will be misrouted into the isIterable branch; to fix, add a
top-level hasByteLength(maybeIter) check before the isIterable(maybeIter) branch
(same logic as in the async branch): flushBatch(), convert with
toUint8ArrayView(maybeIter) and error if null, choose src = fmt === "binary" ?
view : sanitizeBytes(view), await sendChunkedData(src, reserved, pool,
resolvedLimits, counters, notifyProgress), then call sendBinaryTrailer(),
reserved.copyDone(), and return; update the sync-path to handle bytes
identically to the async-path (use validateBinaryTypes/getBinaryTypes only where
rows are handled).
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 1166-1181: msg_slice currently points into the temporary UTF-8
buffer `msg` which is deallocated by `defer msg.deinit()` before
`CopyFail.writeInternal` uses it, causing a use-after-free; fix this by moving
the creation of `fail_msg` and the call to
`fail_msg.writeInternal(PostgresSQLConnection.Writer, this.writer())` (and its
error handling) inside the block that creates `msg` (i.e., the block that
defines `msg_str` and `msg`) so the write happens while `msg` is still
allocated, or alternatively copy the slice into a live buffer before the `defer`
runs; adjust references to `msg_slice`, `message_value`, `msg_str`, `msg`,
`fail_msg`, `CopyFail.writeInternal`, `PostgresSQLConnection.Writer`,
`this.writer()`, `this.abortCopyAndFailConnection`, and `this.flushData`
accordingly.
- Around line 1106-1132: The slice returned from data_str.toUTF8()
(data_utf8.slice()) is deallocated by the deferred data_utf8.deinit(), so move
the CopyData.writeInternal/flushData work into the same branch/scope that owns
data_utf8 (or copy the bytes to a temporary buffer) so the write happens before
data_utf8.deinit() runs; specifically, in the else branch where
data_str/data_utf8 are created, call protocol.CopyData{ .data = .{ .temporary =
data_utf8.slice() } } and invoke
copy_data.writeInternal(PostgresSQLConnection.Writer, this.writer()) and
this.flushData() before deferring/deinitializing data_utf8 (or remove the defer
and explicitly deinit after the write), while keeping the existing size check
via this.effectiveMaxCopyBufferSize() and the abortCopyAndFailConnection/error
handling intact.
♻️ Duplicate comments (1)
src/js/bun/sql.ts (1)
337-339: Binary type count should match table columns whencolumnsis omittedIf
columnsis an empty array,validateBinaryTypesthrows (length mismatch), and later ordinal validation only rejects whenexpectedOidsexceeds the table length. That allows mismatched field counts and fails at COPY runtime. Treat emptycolumnsas “all columns” and enforce exact length.🐛 Suggested fix
- if (types.length !== (columns?.length ?? types.length)) { + if ((columns?.length ?? 0) > 0 && types.length !== columns!.length) { throw new Error("binaryTypes length must match number of columns for COPY FROM."); }- if (rows.length < expectedOids.length) { - throw new Error("Could not resolve column OIDs for validation."); - } + if (rows.length !== expectedOids.length) { + throw new Error( + `COPY binaryTypes length (${expectedOids.length}) must match table column count (${rows.length}) when columns are not specified.`, + ); + }Also applies to: 1784-1791
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sql/postgres/PostgresSQLConnection.zig (1)
65-118: Consider#-prefix for new private fields.
Per repo Zig guidelines, private struct fields should use#(e.g.,#copy_state). These look internal; please confirm and update if they aren’t meant to be accessed externally. As per coding guidelines, ...
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 2161-2163: The call to yieldAccumulated is missing the third
`format` argument causing error messages to show undefined; update the
invocation inside the for-await loop (where yieldAccumulated(accumulated,
isBinary) is used) to pass the correct `format` variable (the same `format` used
earlier in the surrounding function) so the function signature
yieldAccumulated(accumulated, isBinary, format) is satisfied and error messages
show the real format value.
- Around line 2070-2080: The ternary using $isArray(accumulated) is redundant
because this branch only runs when previous $isArray(accumulated) handling has
returned; update the value calculation in this branch to simply use accumulated
?? null (e.g., const value = accumulated ?? null), then proceed to call
toUint8Array(value), keep the same error throw via
$ERR_INVALID_ARG_VALUE(format, ...) and yield toRealArrayBuffer(u8) as before;
ensure references remain to accumulated, toUint8Array, $ERR_INVALID_ARG_VALUE,
format and toRealArrayBuffer.
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 1417-1447: finishCopy can return early in streaming mode before
validating a binary COPY header; add a streaming-specific guard near the top of
the copy_out_progress branch that checks if this.copy_format == 1 and
!this.copy_binary_header_validated and, if true, call this.cleanupCopyState(),
this.fail(...) with an appropriate error (e.g. error.InvalidBinaryData) and
return the error instead of flushing or invoking request.onResult; ensure this
check happens before flushBufferedChunkToJS(), the onCopyEnd callback
invocation, and the streaming-mode early-return so truncated/invalid binary
streams are rejected.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 2108-2113: The timeout selection allows NaN/negative values
because Math.trunc is applied without validating the input; update the branch
that reads queryOrOptions.timeout to validate it is a finite, non-negative
number (use the internal/validators helpers and the $ERR_* constants) before
truncating and using it, and if invalid either fallback to
(__toDefaults__.timeout ?? 0) or throw an appropriate $ERR_*; ensure the
validated integer is what gets passed to setCopyTimeout (and replace the current
direct Math.trunc usage on queryOrOptions.timeout with this validated value).
- Around line 1626-1666: The sync-iterable and raw byte-buffer COPY FROM paths
do not check the AbortSignal, so add abort checks using the existing aborted
flag (or the AbortSignal) in the hasByteLength branch and inside the isIterable
loop to stop work as soon as cancellation occurs: before flushing/sending
(before calls to flushBatch and sendChunkedData), before calling
addToBatch/serializeRow, and at the top of each iteration of the for...of over
maybeIter; ensure the toUint8ArrayView/result is not processed if aborted and
return/throw immediately to prevent further
sendChunkedData/sendBinaryTrailer/reserved.copyDone actions. Use the existing
functions names (hasByteLength, isIterable, flushBatch, sendChunkedData,
sendBinaryTrailer, reserved.copyDone, addToBatch, serializeRow,
toUint8ArrayView) to locate where to insert these checks.
In `@src/sql/postgres/PostgresSQLConnection.zig`:
- Around line 1319-1365: The errdefer freeing new_column_formats in startCopy
causes a double-free after assigning new_column_formats to
this.copy_column_formats and later calling cleanupCopyState on error; remove the
errdefer and instead transfer ownership explicitly: allocate new_column_formats
without errdefer, and if allocation fails return the error; after assigning
this.copy_column_formats = new_column_formats, ensure error paths call
cleanupCopyState() (which frees copy_column_formats) and do not free
new_column_formats again. Reference: function startCopy, local
new_column_formats, field copy_column_formats, and cleanupCopyState.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 2093-2099: The final yield contains a redundant
$isArray(accumulated) ternary because the preceding if ($isArray(accumulated))
branch returns for arrays; update the accumulator fallback to only handle
non-array values by removing the ternary and simply yielding String(accumulated
?? "") (reference the $isArray check and the accumulated variable in the
surrounding generator function). Ensure you leave the early-return array branch
(the accumulated.map(...) branch) intact and replace the final yield expression
to the simplified non-array form.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 1961-1976: The iterator can hang if the AbortSignal fires while
waitForChunk() is pending because chunkResolve remains unresolved; update the
onAbort handler inside the same scope (where signal, aborted, chunkResolve,
waitForChunk are defined) to, in addition to setting aborted = true, call
chunkResolve() if it's non-null (and then clear chunkResolve) so any pending
waitForChunk promise is resolved and the read loop can observe aborted and exit;
ensure you still add the "abort" listener on signal and keep the { once: true }
behavior.
- Around line 1569-1576: The handler that accepts string payloads (the branch
that calls sanitizeString(...) and sendChunkedData(...)) must reject plain
string input when a binary COPY is requested: check the COPY mode/format flag
(e.g., when format === "binary" or similar in the current request/options) and
throw a clear error explaining that string payloads are not allowed in binary
COPY mode and that callers must provide row arrays with binaryTypes or raw byte
Buffers; update the branch around sanitizeString/sendChunkedData to perform this
guard before encoding and avoid producing an invalid COPY BINARY stream
(reference symbols: sanitizeString, sendChunkedData, binaryTypes, and the
string-handling branch shown).
- Around line 1544-1558: flushBatch currently sends the whole batch via
reserved.copySendData(batch) which can exceed maxChunkSize; change flushBatch
(used alongside batchSize and maxChunkSize) to route the assembled batch through
the existing sendChunkedData(reserved, batch, maxChunkSize) helper (or otherwise
clamp batchSize to not exceed maxChunkSize) before calling
awaitWritableWithFallback, and remove the direct reserved.copySendData(batch)
call so all batched text/CSV rows are chunked to respect maxChunkSize.
gohryt
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 213-234: resolveCopyFromLimits can return maxChunkSize <= 0 which
causes sendChunkedData to loop with a zero step; clamp maxChunkSize to a safe
positive value before returning. In resolveCopyFromLimits, after computing
maxChunkSize, enforce a minimum (e.g., Math.max(1, maxChunkSize) or
DEFAULT_COPY_MAX_CHUNK_SIZE) so the returned maxChunkSize is never 0 or
negative; reference the function name resolveCopyFromLimits, the local variable
maxChunkSize, and the constant DEFAULT_COPY_MAX_CHUNK_SIZE when making the
change.
- Around line 2209-2222: The streaming loop yields raw Uint8Array chunks but the
public API promises AsyncIterable<string | ArrayBuffer>, so normalize any
Uint8Array before yielding: when pulling the next piece from chunks (the value
from chunks.shift() inside the while loop that uses done, aborted and
waitForChunk), detect if it's a Uint8Array and convert it to an ArrayBuffer that
contains only the bytes for that view (use the byteOffset/byteLength to slice
the underlying buffer), then yield that ArrayBuffer; otherwise yield the value
unchanged. Ensure you still respect the aborted/rejectErr semantics and preserve
existing control flow.
- Around line 1590-1676: The iterable branches currently accept strings and
fallback-serialized items even when fmt === "binary", producing invalid COPY
BINARY payloads; update both the async iterable loop and the sync iterable loop
so that: when fmt === "binary" you only accept array rows (handled via
encodeBinaryRow) or byte sources (hasByteLength -> toUint8ArrayView), and for
any string or other non-array/non-byte item throw the same argument error used
elsewhere (e.g. $ERR_INVALID_ARG_VALUE("data", item, "must be a string, an array
row, or a byte source") or a stricter message like "must be a row or byte
source"); specifically change the branches that currently call
sanitizeString(item) or serializeRow(item) to instead throw when fmt ===
"binary", while keeping existing behavior for text/csv and raw byte paths
(functions to inspect: isAsyncIterable, isIterable, $isArray, hasByteLength,
encodeBinaryRow, sanitizeString, serializeRow, toUint8ArrayView,
sendChunkedData).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/js/bun/sql.ts`:
- Around line 228-234: The computed maxChunkSize can be 0 which will cause
sendChunkedData's loop (for (let i = 0; i < dataLength; i += maxChunkSize)) to
never advance; in the code that sets maxChunkSize (the variable returned at the
end of the function), clamp the value to a safe positive minimum (e.g.
Math.max(1, Math.trunc(Number(...)))) after converting to Number so that
maxChunkSize is always >= 1 before returning; update the logic that computes
maxChunkSize to enforce this invariant (referencing the maxChunkSize variable
and the caller sendChunkedData loop).
|
@nektro maybe you can check this? |
|
any progress on this? |
|
any progress on this @alii? |
2 participants
Summary
Add PostgreSQL COPY support (excluding CopyBoth)
What does this PR do?
Same as summary
How did you verify your code works?
✅ Build: Successfully compiles on Linux (amd64)
✅ Tests: tests added and succesfully runs on same machine
Motivation
Related to #15088
Additional
There is also files for basic zed support
There is gitignore change to ignore ccls (clangd alternative) cache