sql: make result array metadata properties non-enumerable

[robobun]

Problem

• The arrays returned by Bun.SQL queries carry count, command, lastInsertRowid and affectedRows as enumerable own properties, so Object.keys(rows), for (const k in rows) and { ...rows } list them next to the row indexes:

  const rows = await new SQL(":memory:")`select 1 as a union all select 2 as a`;
  Object.keys(rows); // [ "0", "1", "count", "command", "lastInsertRowid", "affectedRows" ]

• Same on every adapter (sqlite, postgres, mysql), since the properties are created in the shared SQLResultArray constructor. Reproduces on bun 1.4.0 and on main.
• Cause: src/js/internal/sql/shared.ts:81-84 declares the four names as class fields (public count!: ...). The builtins bundle emits them as real class fields, so every instance gets them as enumerable, configurable properties as soon as super() returns. The Object.defineProperties call right below (shared.ts:93), whose comment says it exists so that for in does not list them, then finds properties that already exist, and a descriptor applied to an existing property only changes the attributes it names (value, writable); enumerable stays true.
• This is a regression. 09ab840 (fix(sql) fix binary detection + fix custom types #17635, bun 1.2.4) fixed exactly this by removing the command; count; class fields and adding the defineProperties call. 784271f (SQLite in Bun.sql #21640, bun 1.2.21) moved the class into shared.ts and re-added the names as typed class fields, which undid the fix. Nothing caught it because toEqual on arrays ignores non-index properties, and no test looked at Object.keys of a result.

Fix

• The four fields become declare fields, so nothing is emitted for them and Object.defineProperties is what creates the properties, as in postgres.js's Result (the model for this class) and as in the 1.2.4 fix. This also removes the define-then-redefine of every property on every result array.
• The descriptors now spell out enumerable: false. With declare fields the creation default already gives that, but the 1.2.21 regression happened precisely because the intent lived only in that default; stated explicitly, the descriptor map is correct even if a real class field is ever reintroduced (see sql: expose column type metadata on query results #30037 below).
• The descriptors also pass configurable: true. Creating the properties would otherwise make them non-configurable; the bug is the enumerable flag, and delete rows.count / redefining a property has worked in every shipped release, so that is left as is. The final descriptor is { writable: true, enumerable: false, configurable: true }, i.e. today's descriptor with only enumerable changed.
• Reading and assigning the metadata is unchanged: the adapters set it with plain assignment (sqlite.ts:263, postgres.ts:270, mysql.ts:33), which keeps the attributes of an existing writable property. JSON.stringify, .map() (Symbol.species is Array) and expect().toEqual() against a plain array behave as before. structuredClone(rows) and object spread now drop the metadata, since they only copy enumerable properties; console.log(rows) still shows it because Bun's inspect currently prints non-enumerable properties too.
• Tests (each fails on the unfixed build at its Object.keys assertion, passes with the fix):
  • test/js/sql/sqlite-sql.test.ts "result metadata properties are not enumerable": both constructor paths (a SELECT with rows and an INSERT without), Object.keys, for...in, spread, and the full descriptors of all four properties. Whole file: 240 pass. The Object.keys pin also catches any metadata name added enumerably in the future.
  • test/js/sql/sql.test.ts "Result metadata is not enumerable": postgres, after the resolve callback has filled in command/count.
  • test/js/sql/sql-mysql.test.ts "result metadata properties are not enumerable": mysql, after the resolve callback has filled in count/lastInsertRowid/affectedRows. Both server tests were also run against local postgres and MariaDB services with a copy of the test bodies (the files themselves are docker-gated locally).
• Related: Bun.deepEquals: fix four false-positive equality classes #32872 carries the same descriptor change as a side effect of making toEqual stricter and needs it to land. sql: expose column type metadata on query results #30037 adds columns/statement to this class with the pre-fix public x!: pattern; after this lands they should be declare fields too (its copied descriptors will now carry enumerable: false either way). sql(sqlite): lift the ~640k row ceiling on result sets #38438 changes the super(...values) line of the same constructor and does not overlap with these hunks.

Background

• SQLResultArray (src/js/internal/sql/shared.ts) is the Array subclass every Bun.SQL query resolves to. Rows are the array elements; query metadata hangs off the array as extra own properties, mirroring postgres.js's Result class, whose constructor is super(); Object.defineProperties(this, { count: { value: null, writable: true }, ... }).
• A TypeScript class field (count!: T;) is not type-only: it compiles to an ES class field, which runs [[DefineOwnProperty]] on the instance with enumerable: true, configurable: true, writable: true right after super() returns. A declare field is the type-only form and emits nothing.
• Object.defineProperty/defineProperties on a property that does not exist yet creates it with every unspecified attribute set to false; on a property that already exists, it only changes the attributes that the descriptor mentions.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 36 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

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

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c57bd434-26ba-4a40-b543-e9e0c2c4fcaa

📥 Commits

Reviewing files that changed from the base of the PR and between 840eac2 and 1461e34.

📒 Files selected for processing (4)

• src/js/internal/sql/shared.ts
• test/js/sql/sql-mysql.test.ts
• test/js/sql/sql.test.ts
• test/js/sql/sqlite-sql.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: ready for review.

Reproduced on bun 1.4.0 and on a debug build of main: Object.keys(await sqlselect 1 as a) returns ["0", "count", "command", "lastInsertRowid", "affectedRows"] on sqlite, postgres and mysql. With this branch it returns ["0"], and rows.count / rows.command / rows.lastInsertRowid / rows.affectedRows still read back the same values. This is a regression from 1.2.21 (784271f re-added the class fields that 09ab840 had removed in 1.2.4); details in the description.

Verification (at 1461e34):

• bun bd test test/js/sql/sqlite-sql.test.ts: 240 pass (the new test fails on the unfixed build at its Object.keys assertion).
• The postgres and mysql tests added to sql.test.ts / sql-mysql.test.ts were run against local postgres and MariaDB services via a copy of the test bodies: pass with the fix, fail without it.

[claude]

LGTM — focused fix that makes SQLResultArray metadata non-enumerable, matching the existing comment's intent and postgres.js.

What was reviewed:

• Confirmed the declare fields emit nothing, so Object.defineProperties now creates the properties (defaulting enumerable to false) instead of updating already-enumerable class fields.
• Checked that adding configurable: true preserves the prior descriptor shape — only enumerable flips.
• Verified adapter assignment sites (sqlite.ts:263-274, postgres, mysql) use plain assignment on writable properties, so attributes are preserved; test assertions for sqlite INSERT/SELECT match what the adapter actually sets.
• Tests cover all three adapters plus Object.keys, for...in, spread, and full descriptors.

Extended reasoning...

Overview

This PR changes 8 lines in src/js/internal/sql/shared.ts's SQLResultArray constructor: four public count!: ... class fields become declare fields, and the four Object.defineProperties descriptors gain configurable: true. Three test files each get one new test asserting the metadata properties are non-enumerable and have the expected descriptor.

Root cause and fix

The analysis is correct per the ECMAScript spec: a class field runs [[DefineOwnProperty]] with {enumerable: true, configurable: true, writable: true} after super(), and a subsequent Object.defineProperty with a partial descriptor on an existing property only updates the named attributes — so enumerable stayed true. Switching to declare (type-only, emits nothing) means defineProperties creates the properties, defaulting unspecified attributes to false. Adding configurable: true explicitly keeps that attribute at its previous value, so the only observable change is enumerable: true → false. This is what the existing code comment ("for in will not list the properties") already intended, and it matches postgres.js's Result.

I checked the sqlite adapter (src/js/internal/sql/sqlite.ts:261-276): the SELECT path sets command/count and leaves lastInsertRowid/affectedRows as null; the run path sets command/count/lastInsertRowid and leaves affectedRows as null. The new sqlite test's descriptor assertions match exactly. All adapters write via plain assignment to a writable own property, which preserves the descriptor's attributes.

Security risks

None. This changes property enumerability on a result array in a built-in JS module; no parsing of untrusted input, no auth/crypto/permissions.

Level of scrutiny

Low-to-medium. The source change is 8 lines in one class constructor with a well-understood spec-level mechanism. The behavioral change (metadata dropped from Object.keys/spread/for...in) is the intended fix and aligns with the reference implementation (postgres.js). .map() was already unaffected because Symbol.species returns Array.

Other factors

Tests cover the variant matrix per REVIEW.md: all three adapters (sqlite/postgres/mysql), both constructor entry points (SELECT with rows, INSERT/run without), and multiple enumeration surfaces (Object.keys, for...in, spread, full getOwnPropertyDescriptor). The PR description confirms each test fails on the unfixed build at its Object.keys assertion. No prior reviewer comments to address. The bug hunting system found no issues.

[robobun]

^{Updated 11:48 AM PT - Aug 14th, 2026}

✅ @robobun, your commit 1461e34e90c84a78b466769aca4d5b6bbe1b8434 passed in Build #96063! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 38443

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

bun-38443 --bun

[claude]

LGTM — restores the 1.2.4 non-enumerable metadata behavior that #21640 accidentally undid.

What was reviewed:

• declare vs. emitted class field semantics and how Object.defineProperties merges with an existing descriptor — the root-cause analysis holds.
• configurable: true preserves today's shipped descriptor (class fields were configurable); only enumerable actually changes.
• Adapters (sqlite.ts, postgres.ts, mysql.ts) write metadata via plain assignment, which keeps the non-enumerable descriptor intact.
• Tests pin Object.keys, for...in, spread, and full descriptors on all three adapters; the comment-cop feedback was addressed in 1461e34.

Extended reasoning...

Overview

Changes SQLResultArray in src/js/internal/sql/shared.ts: the four metadata fields (count, command, lastInsertRowid, affectedRows) become declare (type-only) fields instead of emitted class fields, and their Object.defineProperties descriptors now spell out enumerable: false, configurable: true. Three test files gain descriptor/enumeration assertions covering sqlite, postgres, and mysql.

Security risks

None. This is a property-descriptor attribute change on a result array in built-in JS; no parsing, auth, crypto, or native code touched.

Level of scrutiny

Low-to-medium. The runtime change is ~10 lines in one constructor and restores previously-shipped behavior (1.2.4–1.2.20). The mechanism is standard ES/TS semantics: emitted class fields run [[DefineOwnProperty]] with enumerable: true before defineProperties runs, and a partial descriptor applied to an existing property leaves unspecified attributes unchanged. Switching to declare makes defineProperties the sole creator; configurable: true is added so the net descriptor differs from today's only in enumerable. I verified all three adapters write metadata via plain assignment to the already-defined writable property, so attributes are preserved after the resolve callback runs.

Other factors

The PR description traces the regression to specific commits (#17635 fixed it, #21640 reintroduced it), the sqlite test suite (240 tests) passes with the change, and each new test's Object.keys assertion fails on the unfixed build. The github-actions comment-cop feedback about a long comment was addressed in 1461e34 (now one line). No outstanding human reviewer comments. The observable behavior change — object spread and structuredClone now drop metadata — is the intended fix and matches postgres.js's Result.