From 09ab84011452cdb6bc3adfd7142feddf613d3266 Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Mon, 24 Feb 2025 20:07:16 -0800 Subject: [PATCH] fix(sql) fix binary detection + fix custom types (#17635) --- src/bun.js/bindings/ErrorCode.ts | 1 + src/js/bun/sql.ts | 17 +++- src/sql/postgres.zig | 21 +++-- src/sql/postgres/postgres_protocol.zig | 65 ++++++++------- test/js/sql/sql.test.ts | 111 +++++++++++++++++++++++++ 5 files changed, 173 insertions(+), 42 deletions(-) diff --git a/src/bun.js/bindings/ErrorCode.ts b/src/bun.js/bindings/ErrorCode.ts index 83868172f8..44f5bc0fb2 100644 --- a/src/bun.js/bindings/ErrorCode.ts +++ b/src/bun.js/bindings/ErrorCode.ts @@ -183,6 +183,7 @@ const errors: ErrorCodeMapping = [ ["ERR_POSTGRES_QUERY_CANCELLED", Error, "PostgresError"], ["ERR_POSTGRES_UNSAFE_TRANSACTION", Error, "PostgresError"], ["ERR_POSTGRES_NOT_TAGGED_CALL", Error, "PostgresError"], + ["ERR_POSTGRES_UNKNOWN_FORMAT_CODE", Error, "PostgresError"], // S3 ["ERR_S3_MISSING_CREDENTIALS", Error], ["ERR_S3_INVALID_METHOD", Error], diff --git a/src/js/bun/sql.ts b/src/js/bun/sql.ts index b7b8ba88d0..7f889e1229 100644 --- a/src/js/bun/sql.ts +++ b/src/js/bun/sql.ts @@ -1,4 +1,5 @@ const { hideFromStack } = require("internal/shared"); +const defineProperties = Object.defineProperties; const enum QueryStatus { active = 1 << 1, @@ -38,9 +39,19 @@ const escapeIdentifier = function escape(str) { class SQLResultArray extends PublicArray { static [Symbol.toStringTag] = "SQLResults"; - command; - count; + constructor() { + super(); + // match postgres's result array, in this way for in will not list the properties and .map will not return undefined command and count + Object.defineProperties(this, { + count: { value: null, writable: true }, + command: { value: null, writable: true }, + }); + } + static get [Symbol.species]() { + return Array; + } } + const _resolve = Symbol("resolve"); const _reject = Symbol("reject"); const _handle = Symbol("handle"); @@ -2361,7 +2372,7 @@ defaultSQLObject.flush = (...args) => { return lazyDefaultSQL.flush(...args); }; //define lazy properties -Object.defineProperties(defaultSQLObject, { +defineProperties(defaultSQLObject, { options: { get: () => { ensureDefaultSQL(); diff --git a/src/sql/postgres.zig b/src/sql/postgres.zig index 59792c26d2..7d7f204c2f 100644 --- a/src/sql/postgres.zig +++ b/src/sql/postgres.zig @@ -45,6 +45,7 @@ pub const AnyPostgresError = error{ UnsupportedIntegerSize, UnsupportedArrayFormat, UnsupportedNumericFormat, + UnknownFormatCode, }; pub fn postgresErrorToJS(globalObject: *JSC.JSGlobalObject, message: ?[]const u8, err: AnyPostgresError) JSValue { @@ -77,6 +78,7 @@ pub fn postgresErrorToJS(globalObject: *JSC.JSGlobalObject, message: ?[]const u8 error.UnsupportedArrayFormat => JSC.Error.ERR_POSTGRES_UNSUPPORTED_ARRAY_FORMAT, error.UnsupportedIntegerSize => JSC.Error.ERR_POSTGRES_UNSUPPORTED_INTEGER_SIZE, error.UnsupportedNumericFormat => JSC.Error.ERR_POSTGRES_UNSUPPORTED_NUMERIC_FORMAT, + error.UnknownFormatCode => JSC.Error.ERR_POSTGRES_UNKNOWN_FORMAT_CODE, error.JSError => { return globalObject.takeException(error.JSError); }, @@ -916,9 +918,11 @@ pub const PostgresRequest = struct { var iter = QueryBindingIterator.init(values_array, columns_value, globalObject); for (0..len) |i| { - const tag: types.Tag = @enumFromInt(@as(short, @intCast(parameter_fields[i]))); + const parameter_field = parameter_fields[i]; + const is_custom_type = std.math.maxInt(short) < parameter_field; + const tag: types.Tag = if (is_custom_type) .text else @enumFromInt(@as(short, @intCast(parameter_field))); - const force_text = tag.isBinaryFormatSupported() and brk: { + const force_text = is_custom_type or (tag.isBinaryFormatSupported() and brk: { iter.to(@truncate(i)); if (iter.next()) |value| { break :brk value.isString(); @@ -927,7 +931,7 @@ pub const PostgresRequest = struct { return error.InvalidQueryBinding; } break :brk false; - }; + }); if (force_text) { // If they pass a value as a string, let's avoid attempting to @@ -952,7 +956,9 @@ pub const PostgresRequest = struct { iter.to(0); var i: usize = 0; while (iter.next()) |value| : (i += 1) { - const tag: types.Tag = @enumFromInt(@as(short, @intCast(parameter_fields[i]))); + const parameter_field = parameter_fields[i]; + const is_custom_type = std.math.maxInt(short) < parameter_field; + const tag: types.Tag = if (is_custom_type) .text else @enumFromInt(@as(short, @intCast(parameter_field))); if (value.isEmptyOrUndefinedOrNull()) { debug(" -> NULL", .{}); // As a special case, -1 indicates a @@ -2835,8 +2841,8 @@ pub const PostgresSQLConnection = struct { return DataCell{ .tag = .array, .value = .{ .array = .{ .ptr = array.items.ptr, .len = @truncate(array.items.len), .cap = @truncate(array.capacity) } } }; } - pub fn fromBytes(binary: bool, bigint: bool, oid: int4, bytes: []const u8, globalObject: *JSC.JSGlobalObject) !DataCell { - switch (@as(types.Tag, @enumFromInt(@as(short, @intCast(oid))))) { + pub fn fromBytes(binary: bool, bigint: bool, oid: types.Tag, bytes: []const u8, globalObject: *JSC.JSGlobalObject) !DataCell { + switch (oid) { // TODO: .int2_array, .float8_array inline .int4_array, .float4_array => |tag| { if (binary) { @@ -3294,8 +3300,9 @@ pub const PostgresSQLConnection = struct { if (is_raw) { cell.* = DataCell.raw(optional_bytes); } else { + const tag = if (std.math.maxInt(short) < oid) .text else @as(types.Tag, @enumFromInt(@as(short, @intCast(oid)))); cell.* = if (optional_bytes) |data| - try DataCell.fromBytes(this.binary, this.bigint, oid, data.slice(), this.globalObject) + try DataCell.fromBytes((field.binary or this.binary) and tag.isBinaryFormatSupported(), this.bigint, tag, data.slice(), this.globalObject) else DataCell{ .tag = .null, diff --git a/src/sql/postgres/postgres_protocol.zig b/src/sql/postgres/postgres_protocol.zig index 6582a214c1..f20cd461dc 100644 --- a/src/sql/postgres/postgres_protocol.zig +++ b/src/sql/postgres/postgres_protocol.zig @@ -923,19 +923,6 @@ pub const ReadyForQuery = struct { pub const decode = decoderWrap(ReadyForQuery, decodeInternal).decode; }; -pub const FormatCode = enum { - text, - binary, - - pub fn from(value: short) !FormatCode { - return switch (value) { - 0 => .text, - 1 => .binary, - else => error.UnknownFormatCode, - }; - } -}; - pub const null_int4 = 4294967295; pub const DataRow = struct { @@ -1010,7 +997,7 @@ pub const FieldDescription = struct { table_oid: int4 = 0, column_index: short = 0, type_oid: int4 = 0, - + binary: bool = false, pub fn typeTag(this: @This()) types.Tag { return @enumFromInt(@as(short, @truncate(this.type_oid))); } @@ -1024,25 +1011,39 @@ pub const FieldDescription = struct { errdefer { name.deinit(); } - // If the field can be identified as a column of a specific table, the object ID of the table; otherwise zero. - // Int16 - // If the field can be identified as a column of a specific table, the attribute number of the column; otherwise zero. - // Int32 - // The object ID of the field's data type. - // Int16 - // The data type size (see pg_type.typlen). Note that negative values denote variable-width types. - // Int32 - // The type modifier (see pg_attribute.atttypmod). The meaning of the modifier is type-specific. - // Int16 - // The format code being used for the field. Currently will be zero (text) or one (binary). In a RowDescription returned from the statement variant of Describe, the format code is not yet known and will always be zero. - this.* = .{ - .table_oid = try reader.int4(), - .column_index = try reader.short(), - .type_oid = try reader.int4(), - .name_or_index = try ColumnIdentifier.init(name), - }; - try reader.skip(2 + 4 + 2); + // Field name (null-terminated string) + const field_name = try ColumnIdentifier.init(name); + // Table OID (4 bytes) + // If the field can be identified as a column of a specific table, the object ID of the table; otherwise zero. + const table_oid = try reader.int4(); + + // Column attribute number (2 bytes) + // If the field can be identified as a column of a specific table, the attribute number of the column; otherwise zero. + const column_index = try reader.short(); + + // Data type OID (4 bytes) + // The object ID of the field's data type. The type modifier (see pg_attribute.atttypmod). The meaning of the modifier is type-specific. + const type_oid = try reader.int4(); + + // Data type size (2 bytes) The data type size (see pg_type.typlen). Note that negative values denote variable-width types. + // Type modifier (4 bytes) The type modifier (see pg_attribute.atttypmod). The meaning of the modifier is type-specific. + try reader.skip(6); + + // Format code (2 bytes) + // The format code being used for the field. Currently will be zero (text) or one (binary). In a RowDescription returned from the statement variant of Describe, the format code is not yet known and will always be zero. + const binary = switch (try reader.short()) { + 0 => false, + 1 => true, + else => return error.UnknownFormatCode, + }; + this.* = .{ + .table_oid = table_oid, + .column_index = column_index, + .type_oid = type_oid, + .binary = binary, + .name_or_index = field_name, + }; } pub const decode = decoderWrap(FieldDescription, decodeInternal).decode; diff --git a/test/js/sql/sql.test.ts b/test/js/sql/sql.test.ts index 229b2c819e..b0c48a8191 100644 --- a/test/js/sql/sql.test.ts +++ b/test/js/sql/sql.test.ts @@ -3211,6 +3211,117 @@ if (isDockerEnabled()) { // return ['12233445566778', xs.sort().join('')] // }) + test("limits of types", async () => { + await sql + .transaction(async reserved => { + const table_name = sql(Bun.randomUUIDv7("hex").replaceAll("-", "_")); + // we need a lot of types + for (let i = 0; i < 1000; i++) { + const type_name = sql(`${table_name}${i}`); + // create a lot of custom types + await reserved`CREATE TYPE "public".${type_name} AS ENUM('active', 'inactive', 'deleted');`; + } + await reserved` +CREATE TABLE ${table_name} ( +"id" serial PRIMARY KEY NOT NULL, +"status" ${sql(`${table_name}999`)} DEFAULT 'active' NOT NULL +);`.simple(); + await reserved`insert into ${table_name} values (1, 'active'), (2, 'inactive'), (3, 'deleted')`; + const result = await reserved`select * from ${table_name}`; + expect(result).toBeDefined(); + expect(result.length).toBe(3); + expect(result[0].status).toBe("active"); + expect(result[1].status).toBe("inactive"); + expect(result[2].status).toBe("deleted"); + throw new Error("rollback"); // no need to commit all this + }) + .catch(e => { + expect(e.message || e).toBe("rollback"); + }); + }); + test("binary detection of unsupported types", async () => { + using reserved = await sql.reserve(); + // this test should return the same result in text and binary mode, using text mode for this types + { + const table_name = sql(Bun.randomUUIDv7("hex").replaceAll("-", "_")); + + await reserved` + CREATE TEMPORARY TABLE ${table_name} ( + a smallint NOT NULL, + b smallint NOT NULL, + c smallint NOT NULL + )`; + await reserved`insert into ${table_name} values (1, 23, 256)`; + const binary_mode = await reserved`select * from ${table_name} where a = ${1}`; + expect(binary_mode).toEqual([{ a: 1, b: 23, c: 256 }]); + const text_mode = await reserved`select * from ${table_name}`; + expect(text_mode).toEqual([{ a: 1, b: 23, c: 256 }]); + } + { + const table_name = sql(Bun.randomUUIDv7("hex").replaceAll("-", "_")); + + await reserved` + CREATE TEMPORARY TABLE ${table_name} ( + a numeric NOT NULL, + b numeric NOT NULL, + c numeric NOT NULL + )`; + await reserved`insert into ${table_name} values (1, 23, 256)`; + const binary_mode = await reserved`select * from ${table_name} where a = ${1}`; + expect(binary_mode).toEqual([{ a: "1", b: "23", c: "256" }]); + const text_mode = await reserved`select * from ${table_name}`; + expect(text_mode).toEqual([{ a: "1", b: "23", c: "256" }]); + } + + { + const table_name = sql(Bun.randomUUIDv7("hex").replaceAll("-", "_")); + + await reserved` + CREATE TEMPORARY TABLE ${table_name} ( + a bigint NOT NULL, + b bigint NOT NULL, + c bigint NOT NULL + )`; + await reserved`insert into ${table_name} values (1, 23, 256)`; + const binary_mode = await reserved`select * from ${table_name} where a = ${1}`; + expect(binary_mode).toEqual([{ a: "1", b: "23", c: "256" }]); + const text_mode = await reserved`select * from ${table_name}`; + expect(text_mode).toEqual([{ a: "1", b: "23", c: "256" }]); + } + + { + const table_name = sql(Bun.randomUUIDv7("hex").replaceAll("-", "_")); + + await reserved` + CREATE TEMPORARY TABLE ${table_name} ( + a date NOT NULL, + b date NOT NULL, + c date NOT NULL + )`; + await reserved`insert into ${table_name} values ('2025-01-01', '2025-01-02', '2025-01-03')`; + const binary_mode = await reserved`select * from ${table_name} where a >= ${"2025-01-01"}`; + expect(binary_mode).toEqual([ + { a: new Date("2025-01-01"), b: new Date("2025-01-02"), c: new Date("2025-01-03") }, + ]); + const text_mode = await reserved`select * from ${table_name}`; + expect(text_mode).toEqual([{ a: new Date("2025-01-01"), b: new Date("2025-01-02"), c: new Date("2025-01-03") }]); + } + // this is supported in binary mode and also in text mode + { + const table_name = sql(Bun.randomUUIDv7("hex").replaceAll("-", "_")); + await reserved`CREATE TEMPORARY TABLE ${table_name} (a integer[] null, b smallint not null)`; + await reserved`insert into ${table_name} values (null, 1), (array[1, 2, 3], 2), (array[4, 5, 6], 3)`; + const text_mode = await reserved`select * from ${table_name}`; + expect(text_mode.map(row => row)).toEqual([ + { a: null, b: 1 }, + { a: [1, 2, 3], b: 2 }, + { a: [4, 5, 6], b: 3 }, + ]); + const binary_mode = await reserved`select * from ${table_name} where b = ${2}`; + // for now we return a typed array with do not match postgres's array type (this need to accept nulls so will change in future) + expect(binary_mode.map(row => row)).toEqual([{ a: new Int32Array([1, 2, 3]), b: 2 }]); + } + }); test("reserve connection", async () => { const sql = postgres({ ...options, max: 1 }); const reserved = await sql.reserve();