Skip to content

fix(drizzle): remap object rows to TS field names for all dialects#227

Open
meyer1994 wants to merge 3 commits into
unjs:mainfrom
meyer1994:main
Open

fix(drizzle): remap object rows to TS field names for all dialects#227
meyer1994 wants to merge 3 commits into
unjs:mainfrom
meyer1994:main

Conversation

@meyer1994

@meyer1994 meyer1994 commented May 21, 2026

Copy link
Copy Markdown

Fixes #196

I encountered this issue while trying to make drizzle db0 integration work with better-auth drizzle adapter

db0 connectors return object rows ({ foo_bar: 1 }) instead of the positional array rows drizzle's internals expect. Previously the fields parameter was discarded and isResponseInArrayMode() returned false, so drizzle's remapping logic was never triggered and queries returned raw snake_case column names instead of the camelCase TS
property names defined in the schema.

Changes:

  • Add _utils.ts with shared rowToArray and mapRow helpers that translate db0 object rows into drizzle's expected formats
  • Fix all three dialects (SQLite, PostgreSQL, MySQL): store fields, set isResponseInArrayMode() to true, and apply the row mappers in all(), get(), execute(), and values()
  • Add column name remapping test suites for all three dialects covering select, where eq(), get(), insert().returning(), and full key assertions

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved result mapping so selected/query fields are returned with the correct property names (camelCase) across MySQL, PostgreSQL, and SQLite, including where filtering and insert().returning().
  • Bug Fixes

    • Enhanced field-aware mapping for prepared queries, ensuring consistent output when custom result mapping is used and for array/structured response modes (notably SQLite).
  • Tests

    • Added/expanded integration tests covering column name remapping across all supported drivers and additional relational/raw-SQL scenarios for PostgreSQL/PGLite.

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds field-aware Drizzle row mapping for MySQL, PostgreSQL, and SQLite. Results now convert database column names such as foo_bar to mapped Drizzle properties such as fooBar, with coverage for standard, relational, raw SQL, and returning queries.

Changes

Drizzle Column Name Remapping

Layer / File(s) Summary
Row-to-field mapping utilities
src/integrations/drizzle/_utils.ts
rowToArray extracts ordered field values, while mapRow builds nested mapped objects, normalizes nulls, and applies column value conversion.
MySQL session result mapping
src/integrations/drizzle/mysql/_session.ts
MySQL execution applies rowToArray for custom mappers and mapRow for default results.
PostgreSQL session result mapping
src/integrations/drizzle/postgres/_session.ts
execute() and all() apply field-aware mapping, and array response mode is enabled.
SQLite session result mapping
src/integrations/drizzle/sqlite/_session.ts
SQLite passes and stores response array mode, then maps all(), get(), and values() results using the new utilities.
Integration coverage
test/integrations/drizzle/mysql.test.ts, test/integrations/drizzle/pg.test.ts, test/integrations/drizzle/sqlite.test.ts
Tests verify remapped keys across selects, filters, get(), iteration, returning(), relational queries, raw SQL, and $count.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DrizzleQuery
  participant PreparedQuery
  participant Database
  participant RowMapper
  DrizzleQuery->>PreparedQuery: execute query with selected fields
  PreparedQuery->>Database: run SQL with filled parameters
  Database-->>PreparedQuery: return object rows
  PreparedQuery->>RowMapper: map rows using selected fields
  RowMapper-->>DrizzleQuery: return mapped result objects or arrays
Loading

Possibly related PRs

  • unjs/db0#219: Updates related Drizzle session and result-mapping internals in the same database integration paths.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: remapping Drizzle object rows to TypeScript field names across dialects.
Linked Issues check ✅ Passed The PR addresses #196 by remapping returned rows to schema field names and adds regression tests for the affected dialects.
Out of Scope Changes check ✅ Passed The added relational-query, bare SQL, and values()/count() fixes are consistent with the documented follow-up scope, not unrelated churn.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.


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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/integrations/drizzle/_utils.ts`:
- Around line 8-10: The current fields.map callback in
src/integrations/drizzle/_utils.ts uses row[field.name] for Column lookups,
which loses values for joined/duplicate-name projections; update the mapping to
derive the result key from dialect-aware select aliases instead of falling back
to field.name by (1) using the alias provided by SQL.Aliased when present, (2)
consulting the SQLDialect-aware aliasing strategy used by the connector (e.g., a
helper that builds column select aliases per SQLDialect) to compute the lookup
key for Column instances instead of field.name, and (3) ensuring mapRow (or the
code path that builds select projections) generates and exposes those aliases on
the row objects so fields.map can use that key reliably across dialects and
avoid nulling duplicate-named columns.

In `@src/integrations/drizzle/sqlite/_session.ts`:
- Around line 200-208: The values() method unconditionally dereferences
this.fields and throws for raw prepared queries lacking selected-field metadata;
replicate the no-field short-circuit used by all() and get() by returning rows
mapped to Object.values (or equivalent row array) when this.fields is
null/undefined. Update values() (and its use of rowToArray) to check this.fields
first—if absent, convert each row to an array of values in insertion order;
otherwise continue using rowToArray(this.fields, row) so existing behavior for
known fields remains unchanged.

In `@test/integrations/drizzle/mysql.test.ts`:
- Around line 115-125: The test is flaky because drizzleDb.select().from(events)
returns rows in unspecified order; make the assertion deterministic by adding an
ORDER BY to the query so the first row will always be the one with fooBar === 1
(e.g., call .orderBy(events.fooBar) or another stable column like events.id or
events.createdAt), then keep the assertions against res[0]; update the test case
("select returns camelCase keys, not snake_case") to use the ordered query so
res[0].fooBar consistently equals 1.

In `@test/integrations/drizzle/pg.test.ts`:
- Around line 144-154: The test relies on implicit row ordering; change the
query built by drizzleDb.select().from(events) to include an explicit ORDER BY
on a stable column (e.g., events.fooBar or events.createdAt) so the row with
fooBar = 1 is deterministically first; update the call to use the library's
orderBy API (and specify ascending or descending as needed) and adjust the
assertion if you choose the opposite sort direction.

In `@test/integrations/drizzle/sqlite.test.ts`:
- Around line 201-211: The test relies on an unordered result so change the
query in the test that calls drizzleDb.select().from(events).all() to include an
explicit order to make the assertion deterministic; update the select call (in
the test case `"select returns camelCase keys, not snake_case"`) to add an
.orderBy(...) on a stable column (e.g., events.id or events.createdAt) so res[0]
will always be the row with fooBar = 1 and the subsequent property assertions
remain valid.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bde52cd6-bc86-41f5-a0d9-f7669b17aa8a

📥 Commits

Reviewing files that changed from the base of the PR and between 60202ee and 199a683.

📒 Files selected for processing (7)
  • src/integrations/drizzle/_utils.ts
  • src/integrations/drizzle/mysql/_session.ts
  • src/integrations/drizzle/postgres/_session.ts
  • src/integrations/drizzle/sqlite/_session.ts
  • test/integrations/drizzle/mysql.test.ts
  • test/integrations/drizzle/pg.test.ts
  • test/integrations/drizzle/sqlite.test.ts

Comment thread src/integrations/drizzle/_utils.ts Outdated
Comment on lines +8 to +10
return fields.map(({ field }) => {
if (is(field, Column)) return row[field.name];
if (is(field, SQL.Aliased)) return row[field.fieldAlias];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

field.name is not a safe lookup key for shared object-row mapping.

These helpers assume every Column comes back under row[field.name]. That still breaks joined or duplicate-name projections (users.id + posts.id, etc.): the object row can only retain one key, so one value is lost before remapping, and mapRow() then silently turns the missing lookup into null. Because this utility is shared by all three connectors, the fix needs dialect-aware select aliases / result keys rather than a backend-agnostic field.name fallback.

As per coding guidelines, src/**/*.ts: Database dialect behavior must adjust SQL generation per SQLDialect (e.g., RETURNING support varies by backend).

Also applies to: 22-29

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/integrations/drizzle/_utils.ts` around lines 8 - 10, The current
fields.map callback in src/integrations/drizzle/_utils.ts uses row[field.name]
for Column lookups, which loses values for joined/duplicate-name projections;
update the mapping to derive the result key from dialect-aware select aliases
instead of falling back to field.name by (1) using the alias provided by
SQL.Aliased when present, (2) consulting the SQLDialect-aware aliasing strategy
used by the connector (e.g., a helper that builds column select aliases per
SQLDialect) to compute the lookup key for Column instances instead of
field.name, and (3) ensuring mapRow (or the code path that builds select
projections) generates and exposes those aliases on the row objects so
fields.map can use that key reliably across dialects and avoid nulling
duplicate-named columns.

Comment on lines +200 to +208
async values<T extends any[] = unknown[]>(
placeholderValues?: Record<string, unknown>,
): Promise<T[]> {
const params = fillPlaceholders(this.query.params, placeholderValues ?? {});
const placeholders = placeholderValues ?? {};
const params: any[] = fillPlaceholders(this.query.params, placeholders);
this.logger.logQuery(this.query.sql, params);
const rows = await this.stmt.all(...(params as any[]));
// db0 Statement doesn't have a values() method, so convert object rows to arrays
return (rows as Record<string, unknown>[]).map(
(row) => Object.values(row) as T,
);

const rows = (await this.stmt.all(...params)) as Record<string, unknown>[];
return rows.map((row) => rowToArray(this.fields!, row) as T);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

values() lost the no-field fallback.

all() and get() still short-circuit when fields is absent, but values() unconditionally dereferences this.fields!. Any raw prepared query that reaches .values() without selected-field metadata will now throw here instead of returning row values.

Suggested fix
   async values<T extends any[] = unknown[]>(
     placeholderValues?: Record<string, unknown>,
   ): Promise<T[]> {
     const placeholders = placeholderValues ?? {};
     const params: any[] = fillPlaceholders(this.query.params, placeholders);
     this.logger.logQuery(this.query.sql, params);

     const rows = (await this.stmt.all(...params)) as Record<string, unknown>[];
-    return rows.map((row) => rowToArray(this.fields!, row) as T);
+    if (!this.fields) {
+      return rows.map((row) => Object.values(row) as T);
+    }
+
+    return rows.map((row) => rowToArray(this.fields, row) as T);
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async values<T extends any[] = unknown[]>(
placeholderValues?: Record<string, unknown>,
): Promise<T[]> {
const params = fillPlaceholders(this.query.params, placeholderValues ?? {});
const placeholders = placeholderValues ?? {};
const params: any[] = fillPlaceholders(this.query.params, placeholders);
this.logger.logQuery(this.query.sql, params);
const rows = await this.stmt.all(...(params as any[]));
// db0 Statement doesn't have a values() method, so convert object rows to arrays
return (rows as Record<string, unknown>[]).map(
(row) => Object.values(row) as T,
);
const rows = (await this.stmt.all(...params)) as Record<string, unknown>[];
return rows.map((row) => rowToArray(this.fields!, row) as T);
async values<T extends any[] = unknown[]>(
placeholderValues?: Record<string, unknown>,
): Promise<T[]> {
const placeholders = placeholderValues ?? {};
const params: any[] = fillPlaceholders(this.query.params, placeholders);
this.logger.logQuery(this.query.sql, params);
const rows = (await this.stmt.all(...params)) as Record<string, unknown>[];
if (!this.fields) {
return rows.map((row) => Object.values(row) as T);
}
return rows.map((row) => rowToArray(this.fields, row) as T);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/integrations/drizzle/sqlite/_session.ts` around lines 200 - 208, The
values() method unconditionally dereferences this.fields and throws for raw
prepared queries lacking selected-field metadata; replicate the no-field
short-circuit used by all() and get() by returning rows mapped to Object.values
(or equivalent row array) when this.fields is null/undefined. Update values()
(and its use of rowToArray) to check this.fields first—if absent, convert each
row to an array of values in insertion order; otherwise continue using
rowToArray(this.fields, row) so existing behavior for known fields remains
unchanged.

Comment on lines +115 to +125
it("select returns camelCase keys, not snake_case", async () => {
const res = await drizzleDb.select().from(events);
expect(res.length).toBe(2);
expect(res[0]).toHaveProperty("fooBar");
expect(res[0]).toHaveProperty("createdAt");
expect(res[0]).toHaveProperty("userFullName");
expect(res[0]).not.toHaveProperty("foo_bar");
expect(res[0]).not.toHaveProperty("created_at");
expect(res[0]).not.toHaveProperty("user_full_name");
expect(res[0].fooBar).toBe(1);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make the first-row assertion deterministic.

Line 116 selects rows without ordering, but Line 124 asserts res[0].fooBar === 1. Row order is not guaranteed without ORDER BY, so this can become flaky.

Suggested fix
-      const res = await drizzleDb.select().from(events);
+      const res = await drizzleDb.select().from(events).orderBy(events.id);
       expect(res.length).toBe(2);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("select returns camelCase keys, not snake_case", async () => {
const res = await drizzleDb.select().from(events);
expect(res.length).toBe(2);
expect(res[0]).toHaveProperty("fooBar");
expect(res[0]).toHaveProperty("createdAt");
expect(res[0]).toHaveProperty("userFullName");
expect(res[0]).not.toHaveProperty("foo_bar");
expect(res[0]).not.toHaveProperty("created_at");
expect(res[0]).not.toHaveProperty("user_full_name");
expect(res[0].fooBar).toBe(1);
});
it("select returns camelCase keys, not snake_case", async () => {
const res = await drizzleDb.select().from(events).orderBy(events.id);
expect(res.length).toBe(2);
expect(res[0]).toHaveProperty("fooBar");
expect(res[0]).toHaveProperty("createdAt");
expect(res[0]).toHaveProperty("userFullName");
expect(res[0]).not.toHaveProperty("foo_bar");
expect(res[0]).not.toHaveProperty("created_at");
expect(res[0]).not.toHaveProperty("user_full_name");
expect(res[0].fooBar).toBe(1);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/integrations/drizzle/mysql.test.ts` around lines 115 - 125, The test is
flaky because drizzleDb.select().from(events) returns rows in unspecified order;
make the assertion deterministic by adding an ORDER BY to the query so the first
row will always be the one with fooBar === 1 (e.g., call .orderBy(events.fooBar)
or another stable column like events.id or events.createdAt), then keep the
assertions against res[0]; update the test case ("select returns camelCase keys,
not snake_case") to use the ordered query so res[0].fooBar consistently equals
1.

Comment on lines +144 to +154
it("select returns camelCase keys, not snake_case", async () => {
const res = await drizzleDb.select().from(events);
expect(res.length).toBe(2);
expect(res[0]).toHaveProperty("fooBar");
expect(res[0]).toHaveProperty("createdAt");
expect(res[0]).toHaveProperty("userFullName");
expect(res[0]).not.toHaveProperty("foo_bar");
expect(res[0]).not.toHaveProperty("created_at");
expect(res[0]).not.toHaveProperty("user_full_name");
expect(res[0].fooBar).toBe(1);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid depending on implicit row order.

Line 145 fetches rows without ORDER BY, but Line 153 expects the first row to be the fooBar = 1 row. That assumption is unstable across PostgreSQL implementations.

Suggested fix
-    const res = await drizzleDb.select().from(events);
+    const res = await drizzleDb.select().from(events).orderBy(events.id);
     expect(res.length).toBe(2);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("select returns camelCase keys, not snake_case", async () => {
const res = await drizzleDb.select().from(events);
expect(res.length).toBe(2);
expect(res[0]).toHaveProperty("fooBar");
expect(res[0]).toHaveProperty("createdAt");
expect(res[0]).toHaveProperty("userFullName");
expect(res[0]).not.toHaveProperty("foo_bar");
expect(res[0]).not.toHaveProperty("created_at");
expect(res[0]).not.toHaveProperty("user_full_name");
expect(res[0].fooBar).toBe(1);
});
it("select returns camelCase keys, not snake_case", async () => {
const res = await drizzleDb.select().from(events).orderBy(events.id);
expect(res.length).toBe(2);
expect(res[0]).toHaveProperty("fooBar");
expect(res[0]).toHaveProperty("createdAt");
expect(res[0]).toHaveProperty("userFullName");
expect(res[0]).not.toHaveProperty("foo_bar");
expect(res[0]).not.toHaveProperty("created_at");
expect(res[0]).not.toHaveProperty("user_full_name");
expect(res[0].fooBar).toBe(1);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/integrations/drizzle/pg.test.ts` around lines 144 - 154, The test relies
on implicit row ordering; change the query built by
drizzleDb.select().from(events) to include an explicit ORDER BY on a stable
column (e.g., events.fooBar or events.createdAt) so the row with fooBar = 1 is
deterministically first; update the call to use the library's orderBy API (and
specify ascending or descending as needed) and adjust the assertion if you
choose the opposite sort direction.

Comment on lines +201 to +211
it("select returns camelCase keys, not snake_case", async () => {
const res = await drizzleDb.select().from(events).all();
expect(res.length).toBe(2);
expect(res[0]).toHaveProperty("fooBar");
expect(res[0]).toHaveProperty("createdAt");
expect(res[0]).toHaveProperty("userFullName");
expect(res[0]).not.toHaveProperty("foo_bar");
expect(res[0]).not.toHaveProperty("created_at");
expect(res[0]).not.toHaveProperty("user_full_name");
expect(res[0].fooBar).toBe(1);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Stabilize this assertion with explicit ordering.

Line 202 returns an unordered result set, while Line 210 assumes res[0] is the row with fooBar = 1. Add an order clause to keep this test deterministic.

Suggested fix
-    const res = await drizzleDb.select().from(events).all();
+    const res = await drizzleDb.select().from(events).orderBy(events.id).all();
     expect(res.length).toBe(2);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it("select returns camelCase keys, not snake_case", async () => {
const res = await drizzleDb.select().from(events).all();
expect(res.length).toBe(2);
expect(res[0]).toHaveProperty("fooBar");
expect(res[0]).toHaveProperty("createdAt");
expect(res[0]).toHaveProperty("userFullName");
expect(res[0]).not.toHaveProperty("foo_bar");
expect(res[0]).not.toHaveProperty("created_at");
expect(res[0]).not.toHaveProperty("user_full_name");
expect(res[0].fooBar).toBe(1);
});
it("select returns camelCase keys, not snake_case", async () => {
const res = await drizzleDb.select().from(events).orderBy(events.id).all();
expect(res.length).toBe(2);
expect(res[0]).toHaveProperty("fooBar");
expect(res[0]).toHaveProperty("createdAt");
expect(res[0]).toHaveProperty("userFullName");
expect(res[0]).not.toHaveProperty("foo_bar");
expect(res[0]).not.toHaveProperty("created_at");
expect(res[0]).not.toHaveProperty("user_full_name");
expect(res[0].fooBar).toBe(1);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/integrations/drizzle/sqlite.test.ts` around lines 201 - 211, The test
relies on an unordered result so change the query in the test that calls
drizzleDb.select().from(events).all() to include an explicit order to make the
assertion deterministic; update the select call (in the test case `"select
returns camelCase keys, not snake_case"`) to add an .orderBy(...) on a stable
column (e.g., events.id or events.createdAt) so res[0] will always be the row
with fooBar = 1 and the subsequent property assertions remain valid.

@khangln-amv

Copy link
Copy Markdown

any update on this?

pi0 and others added 2 commits July 12, 2026 14:36
Relational queries (db.query.*), bare SQL selections, and values()/count()
crashed with "Cannot read properties of undefined (reading 'map')" because the
customResultMapper and values() paths dereferenced `fields`, which Drizzle's
relational query builder leaves undefined while still providing a mapper.

- rowToArray/mapRow now fall back to Object.values(row) (positional, SELECT
  order) when fields is undefined.
- Bare, non-aliased SQL expressions (e.g. sql`count(*)`) are mapped
  positionally, since db0 keys them by the raw expression text rather than the
  selection alias — previously they returned null.

Documents the object-row limitation for duplicate column names in joins and
adds relational-query, bare-SQL aggregate, and $count regression tests for
sqlite, postgres, and mysql.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@pi0x

pi0x commented Jul 12, 2026

Copy link
Copy Markdown

Reviewed and pushed a follow-up fix (788e086) addressing a few runtime crashes in the row-remapping logic:

Fixed

  1. Relational queries crash on all three dialectsdb.query.*.findMany()/findFirst() threw TypeError: Cannot read properties of undefined (reading 'map'). Drizzle's relational query builder passes fields=undefined together with a customResultMapper, but the mapper path called rowToArray(this.fields!, row). rowToArray/mapRow now fall back to Object.values(row) (positional, SELECT order) when fields is absent.
  2. values() / $count() crash (SQLite) — same root cause via session.values(), which prepares with fields=undefined.
  3. **Bare sql\...`selections returnednull** — e.g. .select({ total: sql`count()` }). db0 keys these by the raw expression text (count()`), not the selection alias, so they're now mapped positionally.

Documented (not fixable at this layer)

  • Duplicate column names in joins (e.g. selecting users.id and posts.id) collapse into a single object key, because db0 connectors only expose object rows — the driver merges same-named columns before the adapter sees them. Fully fixing this needs an array/raw row mode in db0 core. Added an inline note; Drizzle's relational query builder is unaffected since it emits unique aliases.

Added relational-query, bare-SQL aggregate, and $count regression tests for sqlite, postgres (pglite), and mysql. Each new test was confirmed to fail before the fix and pass after. tsc --noEmit and Prettier pass; note that pnpm lint (eslint) currently crashes on load due to a typescript-estree vs TS-preview version mismatch, unrelated to this change.


🤖 This review and fix were generated by Claude (Opus 4.8) via Claude Code. Please review before merging.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/integrations/drizzle/pg.test.ts`:
- Around line 233-248: Update the relational findMany and findFirst test queries
to specify deterministic ordering: add an orderBy for the nested books relation
in the authors.findMany call, and add an orderBy for books.findFirst that
selects “First” explicitly. Keep the existing assertions unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c7772c47-028a-4981-9855-810c615cde57

📥 Commits

Reviewing files that changed from the base of the PR and between 199a683 and 788e086.

📒 Files selected for processing (7)
  • src/integrations/drizzle/_utils.ts
  • src/integrations/drizzle/mysql/_session.ts
  • src/integrations/drizzle/postgres/_session.ts
  • src/integrations/drizzle/sqlite/_session.ts
  • test/integrations/drizzle/mysql.test.ts
  • test/integrations/drizzle/pg.test.ts
  • test/integrations/drizzle/sqlite.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • test/integrations/drizzle/sqlite.test.ts
  • test/integrations/drizzle/mysql.test.ts
  • src/integrations/drizzle/postgres/_session.ts

Comment on lines +233 to +248
it("relational findMany with nested relation", async () => {
const res = await drizzleDb.query.authors.findMany({
with: { books: true },
});
expect(res).toHaveLength(1);
expect(res[0].name).toBe("Ada");
expect(res[0].books.map((b) => b.title)).toEqual(["First", "Second"]);
});

it("relational findFirst with nested relation", async () => {
const res = await drizzleDb.query.books.findFirst({
with: { author: true },
});
expect(res?.title).toBe("First");
expect(res?.author?.name).toBe("Ada");
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

findFirst and findMany rely on implicit row ordering.

findFirst (line 243) returns an arbitrary book without orderBy, but the test expects "First". Similarly, findMany (line 239) asserts books are ["First", "Second"] without ordering in the with clause. Both depend on insertion order, which is not guaranteed by PostgreSQL.

🔧 Suggested fix
   it("relational findMany with nested relation", async () => {
     const res = await drizzleDb.query.authors.findMany({
-      with: { books: true },
+      with: { books: { orderBy: books.id } },
     });
     expect(res).toHaveLength(1);
     expect(res[0].name).toBe("Ada");
     expect(res[0].books.map((b) => b.title)).toEqual(["First", "Second"]);
   });

   it("relational findFirst with nested relation", async () => {
     const res = await drizzleDb.query.books.findFirst({
+      orderBy: books.id,
       with: { author: true },
     });
     expect(res?.title).toBe("First");
     expect(res?.author?.name).toBe("Ada");
   });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/integrations/drizzle/pg.test.ts` around lines 233 - 248, Update the
relational findMany and findFirst test queries to specify deterministic
ordering: add an orderBy for the nested books relation in the authors.findMany
call, and add an orderBy for books.findFirst that selects “First” explicitly.
Keep the existing assertions unchanged.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drizzle column name conversion gets lots resulting in wrong keys

4 participants