Skip to content

fix(sql-orm-client): selectIncludeStrategy reads namespaced capability flags#425

Merged
saevarb merged 6 commits intomainfrom
fix/orm-client-include-strategy
May 7, 2026
Merged

fix(sql-orm-client): selectIncludeStrategy reads namespaced capability flags#425
saevarb merged 6 commits intomainfrom
fix/orm-client-include-strategy

Conversation

@saevarb
Copy link
Copy Markdown
Contributor

@saevarb saevarb commented May 5, 2026

Walkthrough — selectIncludeStrategy reads namespaced capability flags

Sources

  • Commit range: origin/main...fix/orm-client-include-strategy
  • Discovered while building a Pothos integration prototype against the orm-client.

Key snippet (Before / After)

// BEFORE — packages/3-extensions/sql-orm-client/src/include-strategy.ts
const hasLateral = hasCapability(capabilities?.['lateral']);
const hasJsonAgg = hasCapability(capabilities?.['jsonAgg']);
// AFTER — same file
const hasLateral = capabilityFlag(contract, 'lateral');
const hasJsonAgg = capabilityFlag(contract, 'jsonAgg');

function capabilityFlag(contract: Contract<SqlStorage>, flag: string): boolean {
  return (
    contract.capabilities[contract.targetFamily]?.[flag] === true ||
    contract.capabilities[contract.target]?.[flag] === true
  );
}

Intent

Make selectIncludeStrategy read capability flags from the namespaces the contract emitter actually populates (capabilities[targetFamily] and capabilities[target]) instead of from the top level. Without this, every emitted SQL contract silently falls back to the multi-query strategy for nested includes — even on adapters that support single-query lateral / correlated emission.

Change map

The story

  1. The strategy detector reads from where the contract actually carries flags. The Contract type declares capabilities: Record<string, Record<string, boolean>> — namespaced — and the contract emitter populates it under targetFamily (e.g. 'sql') and target (e.g. 'postgres'). The previous detector looked at capabilities['lateral'] directly, never finding anything. The new lookup uses the contract's own declared targetFamily and target to find the right namespaces, so cross-target false positives ("postgres.lateral found while running SQLite") are impossible by construction.
  2. The unit-test surface follows. The previous suite tested a top-level shape ({ lateral: { enabled: true } }) that no real emitted contract produces; those scenarios were never reachable from real usage. They're replaced by tests against the principled shape: family-only, target-only, split-namespace, unrelated-namespace, explicit false, non-boolean values.
  3. Three pre-existing tests had to change because they implicitly relied on the bug. They asserted on outcomes (e.g. 'multiQuery' for the default test contract) that were only true because the buggy detector failed to see the test contract's nested postgres.{lateral,jsonAgg} flags. They're rewritten to express their intent independently of the bug — clearing capabilities, replacing rather than merging, etc.
  4. Two regression guards lock the fix in. Both run a depth-1 include against an emitted-shape contract and assert exactly one SQL execution. One is unit-level (MockRuntime, fast); one is integration-level (real Postgres, end-to-end). Verified locally by reverting the fix — the unit-level guard fails with expected 2 to be 1.
  5. Test ergonomics: a withCapabilities helper. The narrow TestContract type fixes the postgres namespace's shape to specific readonly fields. Tests that override capabilities for a scenario don't fit that narrow shape and previously needed per-call as unknown as TestContract casts. The withCapabilities helper centralizes that structural cast in one place; call sites are clean.

Behavior changes & evidence

  • Capability lookup is namespace-aware. Adds a principled lookup that reads contract.capabilities[contract.targetFamily]?.[flag] and contract.capabilities[contract.target]?.[flag] only.

  • Single-query include path is now reached on emitted contracts. Before → after: a depth-1 db.User.include('comments').all() against an emitted contract fired 2 SQL statements (parent + child stitched in JS); now it fires 1 (correlated subquery + json_group_array).

  • Test ergonomics: withCapabilities helper. Adds a single function that owns the structural cast tests need when overriding capabilities away from the narrow TestContract shape. No behavior change.

    • Why: the previous pattern had per-call as unknown as TestContract casts in seven test cases; the helper is one named, documented place that makes the cast intentional rather than incidental.
    • Implementation:

Compatibility / migration / risk

  • Capability shape: this PR doesn't change what the emitter outputs. It changes what the orm-client reads. Consumers of selectIncludeStrategy (only dispatchWithIncludeStrategy today) are unaffected at the API level.
  • SQL emission shift: queries that previously fell back to multi-query will now use lateral / correlated single-query emission. That is the intended behavior, but it's an observable change in SQL count. Any user log analytics or per-query monitoring that asserts on N+1 patterns for nested includes will see a step change. Not a breaking API change; a perf characteristic change.
  • Cross-target false positives: impossible by construction — only the running contract's targetFamily and target namespaces are consulted, so a hand-crafted contract carrying flags for a different target is ignored.

Follow-ups / open questions

  • Combine + scalar still falls back. compileSelectWithIncludeStrategy still throws on scalar / combine descriptors, and hasComplexIncludeDescriptors short-circuits to multi-query. That's a separate, larger fix with codec-semantics implications (SQL SUM vs JS-side reducer for parameterized codecs); not in scope here.
  • Nested includes (depth ≥ 2) still fall back. hasNestedIncludes short-circuits the dispatcher; the lateral / correlated emission paths are depth-1 only today. Also separate.

Non-goals

  • This PR doesn't change the multi-query path, the dispatcher's strategy ladder, or any emission code.
  • This PR doesn't add support for combine, scalar, or nested-include single-query emission. See follow-ups.

Summary by CodeRabbit

  • Tests

    • Expanded and clarified include-strategy and integration tests; added a regression ensuring a depth-1 include runs a single query and returns correct nested payload.
    • New test helpers and updated fixtures model namespaced capability shapes to validate strategy selection across scenarios.
  • Refactor

    • Capability lookup made namespace-aware and strictly boolean for the running target’s namespaces to avoid cross-namespace false positives.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 5, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Refactors include-strategy capability detection to use a new capabilityFlag(contract, flag) helper that reads boolean flags from contract.capabilities[targetFamily] and contract.capabilities[target]. Tests and fixtures were updated to provide namespaced boolean capability shapes and to exercise the revised lookup semantics.

Changes

Namespace-Aware Capability Detection & Test Adaptation

Layer / File(s) Summary
Core Detection Logic
packages/3-extensions/sql-orm-client/src/include-strategy.ts
Added capabilityFlag(contract, flag) that reads boolean flags from contract.capabilities[targetFamily] or contract.capabilities[target]. Reworked selectIncludeStrategy to use capabilityFlag and removed the previous recursive hasCapability helper. Expanded JSDoc describing namespace mapping.
Test Helpers / Types
packages/3-extensions/sql-orm-client/test/helpers.ts, packages/3-extensions/sql-orm-client/test/collection-fixtures.ts
Added withCapabilities<TCaps> to produce contracts with explicit namespaced capability shapes. Introduced type-only imports and updated contextForContract and createCollectionFor to accept Contract<SqlStorage>.
Unit Tests: include strategy
packages/3-extensions/sql-orm-client/test/include-strategy.test.ts
Rewrote tests to supply capabilities under sql (family) and postgres (target) as booleans. Covered cases: absent capabilities, jsonAgg only, lateral+jsonAgg same namespace, flags split across namespaces, unrelated namespaces ignored, non-boolean values treated as missing, explicit false for lateral.
Unit Tests: dispatch behavior
packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts
Replaced local merging logic with withCapabilities usage. Added withEmittedSqlCapabilities for emitted-shape flags. Added regression that a depth-1 include against an emitted contract triggers a single SQL execution. Forced a multi-query test to use an empty capabilities object via withCapabilities(...).
Integration Tests
packages/3-extensions/sql-orm-client/test/integration/include.test.ts
createUsersCollectionWithCapabilities now replaces contract.capabilities with provided object. Updated multiple integration tests to use simplified postgres: { lateral: true, jsonAgg: true } and sql: { jsonAgg: true } capability shapes. Added integration regression asserting single-query execution for an emitted contract.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

"🐰
I hopped through flags both near and far,
From sql to postgres each little star.
Boolean truths now guide the include art,
Tests snugly follow — a tidy new start. 🥕"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.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 PR title 'fix(sql-orm-client): selectIncludeStrategy reads namespaced capability flags' directly and accurately summarizes the main change: fixing how selectIncludeStrategy detects capabilities by reading namespaced flags instead of top-level keys.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/orm-client-include-strategy

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 and usage tips.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 5, 2026

Open in StackBlitz

@prisma-next/mongo-runtime

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-runtime@425

@prisma-next/family-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/family-mongo@425

@prisma-next/sql-runtime

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-runtime@425

@prisma-next/family-sql

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/family-sql@425

@prisma-next/extension-arktype-json

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/extension-arktype-json@425

@prisma-next/middleware-telemetry

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/middleware-telemetry@425

@prisma-next/mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo@425

@prisma-next/extension-paradedb

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/extension-paradedb@425

@prisma-next/extension-pgvector

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/extension-pgvector@425

@prisma-next/postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/postgres@425

@prisma-next/sql-orm-client

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-orm-client@425

@prisma-next/sqlite

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sqlite@425

@prisma-next/target-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/target-mongo@425

@prisma-next/adapter-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/adapter-mongo@425

@prisma-next/driver-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/driver-mongo@425

@prisma-next/contract

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/contract@425

@prisma-next/utils

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/utils@425

@prisma-next/config

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/config@425

@prisma-next/errors

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/errors@425

@prisma-next/framework-components

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/framework-components@425

@prisma-next/operations

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/operations@425

@prisma-next/ts-render

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/ts-render@425

@prisma-next/contract-authoring

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/contract-authoring@425

@prisma-next/ids

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/ids@425

@prisma-next/psl-parser

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/psl-parser@425

@prisma-next/psl-printer

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/psl-printer@425

@prisma-next/cli

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/cli@425

@prisma-next/emitter

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/emitter@425

@prisma-next/migration-tools

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/migration-tools@425

prisma-next

npm i https://pkg.pr.new/prisma/prisma-next@425

@prisma-next/vite-plugin-contract-emit

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/vite-plugin-contract-emit@425

@prisma-next/mongo-codec

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-codec@425

@prisma-next/mongo-contract

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-contract@425

@prisma-next/mongo-value

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-value@425

@prisma-next/mongo-contract-psl

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-contract-psl@425

@prisma-next/mongo-contract-ts

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-contract-ts@425

@prisma-next/mongo-emitter

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-emitter@425

@prisma-next/mongo-schema-ir

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-schema-ir@425

@prisma-next/mongo-query-ast

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-query-ast@425

@prisma-next/mongo-orm

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-orm@425

@prisma-next/mongo-query-builder

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-query-builder@425

@prisma-next/mongo-lowering

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-lowering@425

@prisma-next/mongo-wire

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-wire@425

@prisma-next/sql-contract

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract@425

@prisma-next/sql-errors

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-errors@425

@prisma-next/sql-operations

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-operations@425

@prisma-next/sql-schema-ir

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-schema-ir@425

@prisma-next/sql-contract-psl

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract-psl@425

@prisma-next/sql-contract-ts

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract-ts@425

@prisma-next/sql-contract-emitter

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract-emitter@425

@prisma-next/sql-lane-query-builder

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-lane-query-builder@425

@prisma-next/sql-relational-core

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-relational-core@425

@prisma-next/sql-builder

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-builder@425

@prisma-next/target-postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/target-postgres@425

@prisma-next/target-sqlite

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/target-sqlite@425

@prisma-next/adapter-postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/adapter-postgres@425

@prisma-next/adapter-sqlite

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/adapter-sqlite@425

@prisma-next/driver-postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/driver-postgres@425

@prisma-next/driver-sqlite

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/driver-sqlite@425

commit: 38cdaef

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/3-extensions/sql-orm-client/src/include-strategy.ts (1)

50-55: 💤 Low value

Optional: tighten flag to a literal union.

flag: string is loose — only 'lateral' and 'jsonAgg' are ever passed today. Constraining it would document the supported flag set and prevent accidental drift if someone adds a third flag.

♻️ Suggested signature
-function capabilityFlag(contract: Contract<SqlStorage>, flag: string): boolean {
+function capabilityFlag(contract: Contract<SqlStorage>, flag: 'lateral' | 'jsonAgg'): boolean {
🤖 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 `@packages/3-extensions/sql-orm-client/src/include-strategy.ts` around lines 50
- 55, The capabilityFlag function accepts a loose flag: string; tighten it to a
literal union of the supported flags (e.g., 'lateral' | 'jsonAgg') so callers
cannot pass arbitrary strings. Update the function signature (capabilityFlag) to
use the literal union type for the flag parameter and adjust any callers or
tests that pass flags to use one of 'lateral' or 'jsonAgg'; ensure the property
lookups contract.capabilities[...][flag] still type-check with the new narrower
type.
🤖 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.

Nitpick comments:
In `@packages/3-extensions/sql-orm-client/src/include-strategy.ts`:
- Around line 50-55: The capabilityFlag function accepts a loose flag: string;
tighten it to a literal union of the supported flags (e.g., 'lateral' |
'jsonAgg') so callers cannot pass arbitrary strings. Update the function
signature (capabilityFlag) to use the literal union type for the flag parameter
and adjust any callers or tests that pass flags to use one of 'lateral' or
'jsonAgg'; ensure the property lookups contract.capabilities[...][flag] still
type-check with the new narrower type.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 3c29ce1b-2d33-43a4-b1dd-e051877fb1e9

📥 Commits

Reviewing files that changed from the base of the PR and between 1f05af9 and 3530713.

📒 Files selected for processing (4)
  • packages/3-extensions/sql-orm-client/src/include-strategy.ts
  • packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts
  • packages/3-extensions/sql-orm-client/test/include-strategy.test.ts
  • packages/3-extensions/sql-orm-client/test/integration/include.test.ts

@saevarb saevarb force-pushed the fix/orm-client-include-strategy branch from 3530713 to c600e7f Compare May 5, 2026 14:35
Comment thread packages/3-extensions/sql-orm-client/test/helpers.ts Outdated
@saevarb saevarb force-pushed the fix/orm-client-include-strategy branch from b7ac2c6 to ca015a3 Compare May 6, 2026 13:53
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/3-extensions/sql-orm-client/test/integration/include.test.ts (1)

45-58: ⚡ Quick win

Reuse withCapabilities() here instead of a loose local override helper.

This helper accepts Record<string, unknown> and then casts the result back to the base contract, so a future test can quietly drift back to stale capability fixtures while still looking valid. Reusing the shared withCapabilities() helper here would keep the integration tests locked to the emitted boolean-map shape this PR is guarding.

As per coding guidelines, "Try to minimize type casts. Instead, prefer explicit types that would make type casts unnecessary."

🤖 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 `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts` around
lines 45 - 58, Replace the local loose capability override in
createUsersCollectionWithCapabilities with the shared withCapabilities helper:
change the capabilities parameter from Record<string, unknown> to the exact
shape produced by withCapabilities (use ReturnType<typeof withCapabilities>),
call withCapabilities(...) to produce the capability map, and construct the
contract using { ...base, capabilities: withCapabilities(capabilities) } (or
simply assign the returned boolean-map) instead of casting to typeof base;
update usages of createUsersCollectionWithCapabilities accordingly and remove
the unnecessary type cast and comment about merging. Ensure you reference
getTestContract, createUsersCollectionWithCapabilities, and withCapabilities
while making the change.
🤖 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.

Nitpick comments:
In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts`:
- Around line 45-58: Replace the local loose capability override in
createUsersCollectionWithCapabilities with the shared withCapabilities helper:
change the capabilities parameter from Record<string, unknown> to the exact
shape produced by withCapabilities (use ReturnType<typeof withCapabilities>),
call withCapabilities(...) to produce the capability map, and construct the
contract using { ...base, capabilities: withCapabilities(capabilities) } (or
simply assign the returned boolean-map) instead of casting to typeof base;
update usages of createUsersCollectionWithCapabilities accordingly and remove
the unnecessary type cast and comment about merging. Ensure you reference
getTestContract, createUsersCollectionWithCapabilities, and withCapabilities
while making the change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: bc885ef3-04c2-414c-851d-3b5d6156860b

📥 Commits

Reviewing files that changed from the base of the PR and between 3530713 and ca015a3.

📒 Files selected for processing (6)
  • packages/3-extensions/sql-orm-client/src/include-strategy.ts
  • packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts
  • packages/3-extensions/sql-orm-client/test/collection-fixtures.ts
  • packages/3-extensions/sql-orm-client/test/helpers.ts
  • packages/3-extensions/sql-orm-client/test/include-strategy.test.ts
  • packages/3-extensions/sql-orm-client/test/integration/include.test.ts

@saevarb saevarb force-pushed the fix/orm-client-include-strategy branch from ca015a3 to 8c98e73 Compare May 7, 2026 06:38
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/3-extensions/sql-orm-client/test/integration/include.test.ts (1)

47-58: ⚡ Quick win

Tighten capability fixture typing to avoid cast-masked test mistakes.

Record<string, unknown> plus as typeof base can silently accept malformed capability shapes. Narrowing this to the contract capability type (or using withCapabilities) makes fixture mistakes fail fast.

Proposed refactor
-import { getTestContext, getTestContract, isSelectAst } from '../helpers';
+import { getTestContext, getTestContract, isSelectAst, withCapabilities } from '../helpers';

 function createUsersCollectionWithCapabilities(
   runtime: Parameters<typeof createUsersCollection>[0],
-  capabilities: Record<string, unknown>,
+  capabilities: ReturnType<typeof getTestContract>['capabilities'],
 ) {
   const base = getTestContract();
-  const contract = {
-    ...base,
-    capabilities,
-  } as typeof base;
+  const contract = withCapabilities(base, capabilities);

   const context = { ...getTestContext(), contract };
   return new Collection({ runtime, context }, 'User');
 }
🤖 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 `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts` around
lines 47 - 58, The test fixture accepts capabilities as Record<string, unknown>
and then force-casts the merged object to typeof base, which can mask malformed
capability shapes; change the parameter type from Record<string, unknown> to the
concrete capability type (e.g., typeof getTestContract().capabilities or the
exported ContractCapabilities type) and remove the unsafe `as typeof base` cast
when creating `contract`; alternatively call the helper `withCapabilities` (or
equivalent test factory) to produce a properly-typed contract so that malformed
capability shapes fail at compile time instead of being silenced by a cast
(refer to getTestContract, contract, and withCapabilities in the diff).
🤖 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.

Nitpick comments:
In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts`:
- Around line 47-58: The test fixture accepts capabilities as Record<string,
unknown> and then force-casts the merged object to typeof base, which can mask
malformed capability shapes; change the parameter type from Record<string,
unknown> to the concrete capability type (e.g., typeof
getTestContract().capabilities or the exported ContractCapabilities type) and
remove the unsafe `as typeof base` cast when creating `contract`; alternatively
call the helper `withCapabilities` (or equivalent test factory) to produce a
properly-typed contract so that malformed capability shapes fail at compile time
instead of being silenced by a cast (refer to getTestContract, contract, and
withCapabilities in the diff).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 046ed3d7-0ff8-4344-b247-6b9bb1764891

📥 Commits

Reviewing files that changed from the base of the PR and between ca015a3 and 8c98e73.

📒 Files selected for processing (6)
  • packages/3-extensions/sql-orm-client/src/include-strategy.ts
  • packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts
  • packages/3-extensions/sql-orm-client/test/collection-fixtures.ts
  • packages/3-extensions/sql-orm-client/test/helpers.ts
  • packages/3-extensions/sql-orm-client/test/include-strategy.test.ts
  • packages/3-extensions/sql-orm-client/test/integration/include.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/3-extensions/sql-orm-client/src/include-strategy.ts
  • packages/3-extensions/sql-orm-client/test/helpers.ts
  • packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts
  • packages/3-extensions/sql-orm-client/test/include-strategy.test.ts

saevarb added 3 commits May 7, 2026 11:21
the contract's target/family namespaces

`selectIncludeStrategy` previously checked `contract.capabilities['lateral']`
and `contract.capabilities['jsonAgg']` at the top level only. The
`Contract` type declares capabilities as
`Record<string, Record<string, boolean>>` — a namespaced map — and the
contract emitter populates the flags one level deeper, under the
contract's `targetFamily` (e.g. `'sql'`) and `target` (e.g. `'postgres'`)
namespaces. Top-level access never found them, so every nested include
fell back to the multi-query strategy on emitted contracts even when
the runtime supported single-query lateral / correlated emission.

Concrete impact: a 2-level include like `db.User.include('comments').all()`
fired 2 SQL statements instead of the expected 1.

Why CI didn't catch it: the strategy unit tests and the integration
tests that exercised the lateral path all populated capabilities at the
top level (`{ lateral: { enabled: true }, jsonAgg: { enabled: true } }`),
a shape the buggy detection happened to match but that no real emitted
contract produces. The lateral / correlated branches of the dispatcher
were dead code in practice, but tests appeared to cover them.

The fix uses the contract's own declared identity:

  contract.capabilities[contract.targetFamily]?.[flag] === true ||
  contract.capabilities[contract.target]?.[flag] === true

Universal SQL flags like `jsonAgg` declared by the SQL family at
`capabilities.sql.jsonAgg` are picked up; target-specific flags like
`lateral` declared at `capabilities.postgres.lateral` are picked up;
flags split across both namespaces compose correctly. Cross-namespace
false positives ("`postgres.lateral` found while running SQLite") are
impossible because we only inspect the running target's namespaces.

Tests:

- `include-strategy.test.ts` rewritten to use the principled shape:
  `{ sql: { jsonAgg: true } }`, `{ postgres: { jsonAgg: true, lateral:
  true } }`, split namespaces, unrelated namespaces, explicit false,
  non-boolean values. The previous suite tested the buggy top-level
  shape; those scenarios were never reachable from an emitted contract
  and have been replaced.

- `collection-dispatch.test.ts` `withSingleQueryCapabilities` helper
  updated to populate capabilities under the contract's actual
  target/family namespaces rather than at the top level. Same logical
  intent, now matching the type's shape.

- `integration/include.test.ts` `createUsersCollectionWithCapabilities`
  helper now replaces capabilities entirely (instead of merging with
  the base contract's `postgres` namespace), so passing
  `{ sql: { jsonAgg: true } }` truly tests the correlated path. Four
  call sites updated to use the namespaced shape.

All 479 sql-orm-client tests pass.
lookup; centralize the structural cast in a `withCapabilities` helper

Adds two tests that assert depth-1 includes against an emitted-shape
contract (capabilities under `targetFamily` and `target` namespaces)
fire exactly one SQL execution. Either fails fast if
`selectIncludeStrategy`'s namespace-aware lookup regresses.

Verified the unit-level guard fails when the strategy detection
reverts to top-level-only access.

Test ergonomics:

- Adds `withCapabilities(contract, capabilities)` to `helpers.ts`.
  The narrow `TestContract` type fixes capabilities to the exact
  shape from `contract.json` (e.g. the `postgres` namespace's specific
  readonly fields). Tests need to construct contracts with arbitrary
  capability shapes, which don't fit the narrow type. The helper
  centralizes the structural cast in one named, documented place.

- Removes per-test `as unknown as ReturnType<typeof getTestContract>`
  / `as unknown as TestContract` casts from `include-strategy.test.ts`
  (was 7) and `collection-dispatch.test.ts` (was 3) by routing through
  `withCapabilities`.

- The pre-existing `withSingleQueryCapabilities` and the new
  `withEmittedSqlCapabilities` helpers in `collection-dispatch.test.ts`
  delegate to `withCapabilities`.

Regression guards:

- `collection-dispatch.test.ts` — `withEmittedSqlCapabilities` produces
  `{ sql: { jsonAgg: true }, postgres: { jsonAgg: true, lateral: true } }`.
  Asserts on MockRuntime's `executions.length`.
- `integration/include.test.ts` — uses `getTestContract()` directly
  (whose capabilities are already in the emitted shape via the
  `postgres` namespace) against the dev Postgres. Asserts
  `runtime.executions.length === 1`.

All 481 sql-orm-client tests pass.
…lities

Replace the `as unknown as TestContract` cast with a generic over
`TCaps extends Record<string, Record<string, boolean>>`, returning
`Omit<TestContract, 'capabilities'> & { readonly capabilities: TCaps }`.
Capability-dependent type checks now see the caller's literal
capability shape instead of the narrow `TestContract` slot.

Addresses @aqrln's review note: "I'd make this helper produce an
accurate contract types from day one if it's not too complex."

Widens `contextForContract` and `createCollectionFor` in
`collection-fixtures.ts` to accept `Contract<SqlStorage>` so the more
precise return type flows through; the existing internal cast inside
`contextForContract` already absorbs the structural gap. Drops the
explicit `: TestContract` return annotations on the two thin
`with…Capabilities` wrappers in `collection-dispatch.test.ts` so the
inferred result keeps its literal capability shape.

All 481 sql-orm-client tests pass.
@saevarb saevarb force-pushed the fix/orm-client-include-strategy branch from 8c98e73 to 5e32fab Compare May 7, 2026 09:38
@saevarb saevarb enabled auto-merge (squash) May 7, 2026 09:38
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/3-extensions/sql-orm-client/test/integration/include.test.ts (1)

55-58: 💤 Low value

Use a double cast (as unknown as typeof base) instead of the single direct cast.

The coding guidelines require as unknown as X for mocks in tests to make the unsafe boundary explicit; a direct as typeof base may also be rejected by the TypeScript compiler if Record<string, unknown> doesn't overlap with the actual capabilities field type on the contract.

🔧 Proposed fix
  const contract = {
    ...base,
    capabilities,
-} as typeof base;
+} as unknown as typeof base;

As per coding guidelines: "Use double casts (as unknown as X) for mocks and dynamic proxies in tests instead of direct casts."

🤖 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 `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts` around
lines 55 - 58, The test creates `contract` by casting a mock object directly to
`typeof base` which violates the guideline; change the cast on the constructed
object (the `contract` assignment that spreads `base` and `capabilities`) to use
a double cast (`as unknown as typeof base`) so the unsafe mock boundary is
explicit; update the expression that currently uses `as typeof base` to `as
unknown as typeof base` referring to the `contract`, `base`, and `capabilities`
symbols.
🤖 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.

Nitpick comments:
In `@packages/3-extensions/sql-orm-client/test/integration/include.test.ts`:
- Around line 55-58: The test creates `contract` by casting a mock object
directly to `typeof base` which violates the guideline; change the cast on the
constructed object (the `contract` assignment that spreads `base` and
`capabilities`) to use a double cast (`as unknown as typeof base`) so the unsafe
mock boundary is explicit; update the expression that currently uses `as typeof
base` to `as unknown as typeof base` referring to the `contract`, `base`, and
`capabilities` symbols.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: fe7145ae-7462-4885-9831-7376e8b75479

📥 Commits

Reviewing files that changed from the base of the PR and between 8c98e73 and 5e32fab.

📒 Files selected for processing (6)
  • packages/3-extensions/sql-orm-client/src/include-strategy.ts
  • packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts
  • packages/3-extensions/sql-orm-client/test/collection-fixtures.ts
  • packages/3-extensions/sql-orm-client/test/helpers.ts
  • packages/3-extensions/sql-orm-client/test/include-strategy.test.ts
  • packages/3-extensions/sql-orm-client/test/integration/include.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/3-extensions/sql-orm-client/test/helpers.ts
  • packages/3-extensions/sql-orm-client/src/include-strategy.ts
  • packages/3-extensions/sql-orm-client/test/collection-dispatch.test.ts
  • packages/3-extensions/sql-orm-client/test/include-strategy.test.ts

saevarb added 3 commits May 7, 2026 13:07
`multi-query path handles empty parent result sets` was using the bare
`getTestContract()`. That contract has `postgres.lateral` and
`postgres.jsonAgg` set in the emitted shape, so post-fix the test
routes through the single-query lateral path instead of multi-query
— leaving the empty-parent early return at `collection-dispatch.ts:209`
unhit and dropping package branch coverage just under the 93% gate.

Pre-fix this never showed up because `selectIncludeStrategy` could
not read namespaced capabilities and silently fell back to multi-query
for every emitted contract — so this test was hitting the multi-query
empty-parent branch by accident.

Use the same `withCapabilities(getTestContract(), {})` trick the
sibling test on line 245 already uses to force the multi-query path.
The test now actually exercises what its name describes.
Copy link
Copy Markdown
Contributor

@wmadden wmadden left a comment

Choose a reason for hiding this comment

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

Your fix is more correct than the current implementation but the root cause of this problem is that capabilities shouldn't be namespaced at all

Comment on lines +52 to +53
contract.capabilities[contract.targetFamily]?.[flag] === true ||
contract.capabilities[contract.target]?.[flag] === true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is an old mistake in the emission process: capability flags should not be namespaced, they're either present or not, and we don't care which framework component contributes them. Your fix is more correct than the current implementation, but still working around this core mistake.

@saevarb saevarb merged commit 91a1853 into main May 7, 2026
16 checks passed
@saevarb saevarb deleted the fix/orm-client-include-strategy branch May 7, 2026 13:30
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.

3 participants