Fix insert review findings and record the gap review by Makisuo · Pull Request #14 · MapleTechLabs/effect-orm

Makisuo · 2026-10-04T13:17:41Z

Fixes the INSERT issues found by the review against Drizzle 1.0-rc.5 and Kysely 0.28, and saves the full review as design/gap-review.md.

Fixes

returning() with no arguments returns every column, as Drizzle's bare .returning() does. Before, it type-checked and then failed to compile, which migrated Drizzle code would have hit.
insertInto(table) returns CHInsertStart, which offers only values and select. An insert without rows no longer type-checks or reaches Database.run.
TableOptions.computed marks generated columns on table() (Postgres GENERATED ALWAYS): not insertable, still readable.
INSERT ... SELECT accepts a plain primitive into a branded column, the same rule values and comparisons use.
Test gap: jsonb and array values in an upsert's set now have a PGlite round trip.

Gap review

design/gap-review.md lists P0/P1/P2 gaps with Maple call-site counts, where effect-orm is ahead, and the order of work. UPDATE and DELETE are next.

Testing

bun run typecheck and bun run test pass: 513 unit tests and the doc checks.

🤖 Generated with Claude Code

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

Summary by CodeRabbit

New Features
- Insert results now include all columns when returning() is called without specifying columns.
- Tables can identify generated columns that remain readable but cannot be supplied in insert rows.
- INSERT ... SELECT now accepts plain values for branded target columns.
Bug Fixes
- Inserts must specify rows or a selection before they can be compiled or run.
- PostgreSQL upserts correctly handle JSONB and text-array values, including text containing apostrophes.
Documentation
- Updated insert and table guides to explain these behaviors.

- returning() with no arguments returns every column, as Drizzle's bare .returning() does; it type-checked and then failed to compile. - insertInto returns CHInsertStart (values and select only), so an insert without rows no longer type-checks or reaches Database.run. - TableOptions.computed marks generated columns on table(): not insertable, still readable. - INSERT ... SELECT accepts a plain primitive into a branded column, the rule values and comparisons already use. - PGlite round trip for jsonb and array values in an upsert's SET. design/gap-review.md records the comparison with Drizzle 1.0-rc.5 and Kysely 0.28 and the order of work that follows from it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-04T13:17:54Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 41 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 33818784-420c-462f-a9d8-c080c6c51873

📥 Commits

Reviewing files that changed from the base of the PR and between 0410c22 and 4a5eb05.

📒 Files selected for processing (18)

CHANGELOG.md
README.md
design/gap-review.md
design/writes.md
docs/README.md
docs/database.md
docs/reference.md
docs/updates-and-deletes.md
src/ch/compile.ts
src/ch/dialect.ts
src/ch/index.ts
src/ch/insert.test-d.ts
src/ch/insert.ts
src/ch/update.test.ts
src/ch/update.ts
src/database/database.test.ts
src/database/database.ts
tests/database.clickhouse.test.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID: a71d92bb-6aff-45a5-be18-77230f2d9c6c

📥 Commits

Reviewing files that changed from the base of the PR and between 70f674a and 0410c22.

📒 Files selected for processing (11)

CHANGELOG.md
design/gap-review.md
docs/inserts.md
docs/reference.md
docs/tables-and-types.md
src/ch/index.ts
src/ch/insert.test-d.ts
src/ch/insert.test.ts
src/ch/insert.ts
src/ch/table.ts
src/database/database.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

The insert builder now supports computed-column metadata, restricts the initial insert API until rows are supplied, returns all columns from bare returning(), and accepts widened types in INSERT ... SELECT. Tests and documentation cover these changes. A separate design review records feature gaps and a proposed work order.

Changes

Insert Builder Updates

Layer / File(s)	Summary
Computed column support `src/ch/table.ts`, `src/ch/insert.test-d.ts`, `src/ch/insert.test.ts`, `docs/tables-and-types.md`	`TableOptions` accepts computed column names. Tests check that computed columns remain readable and are excluded from insert values.
Insert API and returning behavior `src/ch/insert.ts`, `src/ch/index.ts`, `src/ch/insert.test-d.ts`, `src/ch/insert.test.ts`, `src/database/database.test.ts`, `docs/inserts.md`, `docs/reference.md`, `CHANGELOG.md`	`insertInto` returns `CHInsertStart`, which exposes `values` and `select`. Bare `returning()` selects all columns, and `INSERT ... SELECT` accepts widened source types. Tests also cover JSONB and text-array values in upserts. Documentation and the changelog describe these updates.

Effect-ORM Gap Review

Layer / File(s)	Summary
Comparison, gap inventory, and work plan `design/gap-review.md`	The review compares effect-orm with Drizzle and Kysely, lists prioritized feature gaps and documented areas of difference, and proposes an order of work.

Priority: ➖ Normal

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 0410c

The insert-builder changes have no identified issue requiring resolution before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0410c

The changes strengthen insert construction and computed-column restrictions, but a bare returning() now returns every declared column. No introduced vulnerability is established; downstream authorization and exposure of sensitive fields remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The changed output surface can include every declared column of the insert target when bare returning() is used. Effective tenant and sensitive-data exposure depends on the caller, database privileges, and response handling; the supplied evidence does not establish an attacker-facing route or a broader deployment blast radius.

Trust Boundaries and Controls

observed — The existing execution path checks dialect compatibility and transaction lifetime, then invokes an optional observer before SQL execution. It contains no built-in tenantScope or returned-field authorization check. That pre-existing separation of execution from authorization does not establish a PR-introduced vulnerability, and deployed enforcement remains unverified.

Resilience and Maintainability Implications

observed — The insert contract does not move transaction or recovery ownership into the builder. Existing transaction handling rolls back failures, defects, and interruption, and the executor rejects statements carrying a closed transaction context. No new retry or recovery guarantee is introduced by staging.

Hardening Proposals

proposed — Where returned insert rows cross an application trust boundary, prefer an explicit allowed-column projection or an authorized response schema rather than forwarding bare returning() output. This is a safeguard for the expanded output contract, not an observed disclosure finding.

🚥 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%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files. (5 skipped: 5…	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 INSERT fixes and the addition of the gap review.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files. (5 skipped: 5 unsupported.)

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.}

update(table).set(...).where(...) and deleteFrom(table).where(...), with returning on Postgres and settings on ClickHouse. They share the insert's value encoding, SET record, RETURNING and settings, now factored into valueCells, setAssignments, returningOf and writeSettingsClause. A write with no where() is a defect unless allRows() says so; a where() whose conditions all came out undefined fails, since that comes from data and would widen a filtered write to every row. ClickHouse compiles UPDATE to an ALTER TABLE ... UPDATE mutation and DELETE to a lightweight DELETE, both with WHERE 1 for allRows() and settings last; checked on 26.2 and 26.8. Tenant scope is derived from the WHERE, and an update that moves rows to another tenant is cross-tenant. CompiledQuery.kind gains update and delete, and Database.run sends any write without RETURNING through command. DialectClauses.insertSettings (unreleased) becomes writeSettings; alterTableUpdate is new. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

A subquery in an UPDATE or DELETE's SET or WHERE, or in an insert's VALUES or onConflictDoUpdate, was compiled for its SQL and its scope dropped, so a pinned write that read every tenant through a subquery reported single-tenant, and a write into an untenanted table reported untenanted. Writes now record each subquery's scope (a string subquery is cross-tenant) and combine it with their own, as queries do. A where() condition that renders to nothing no longer leaves a dangling WHERE, and does not count as a filter: a write left with none fails unless allRows() says so. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Add update() and deleteFrom() for Postgres and ClickHouse

Makisuo mentioned this pull request Oct 4, 2026

Add update() and deleteFrom() for Postgres and ClickHouse #15

Merged

Makisuo and others added 2 commits October 4, 2026 15:34

Merge pull request #15 from MapleTechLabs/feat/update-delete

4a5eb05

Add update() and deleteFrom() for Postgres and ClickHouse

Makisuo merged commit 7e43f59 into main Oct 4, 2026
3 checks passed

Navigation Menu

Fix insert review findings and record the gap review - #14

Fix insert review findings and record the gap review#14
Makisuo merged 4 commits into
mainfrom
fix/insert-review

Makisuo commented Oct 4, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 4, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Makisuo commented Oct 4, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixes

Gap review

Testing

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Security Architecture Review

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Makisuo commented Oct 4, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 4, 2026 •

edited

Loading