fix(adapters): cross-backend drift fixes from the architecture audit - #865
Merged
Merged
Conversation
…ence The pagination slicer applied a limit only when it was truthy and positive, so limit: 0 returned every row. Every other backend treats 0 as a value (typeorm takes 0 rows; prisma take: 0 and drizzle limit: 0 both return no rows on a real engine), and drizzle's impossible-root encoding (limit: 0) relies on that reading, making the reference backend the only outlier. The slice now applies for any non-negative numeric limit; negative values stay absence. A drizzle engine spec pins the cross-backend agreement.
Three findings the drizzle review pass fixed exist identically here and
were never backported:
- own-property provider lookups: an inherited object member must
answer unknown and reach the typed error instead of posing as a
capability preset
- distributeNegation invariant guards: a residual negation wrapper
around anything but mod/size, or a negated non-eq compare, now fails
typed instead of silently emitting the positive (inverted) form if
the core contract ever breaks
- typed ITSELF gate: a $this leaf throws featureUnsupported instead of
emitting a { '$this': ... } key and leaning on prisma's
unknown-field validation error
Deliberately not ported because prisma is immune by construction: the
metadata prototype hole (the datamodel walker is array-based) and the
numeric sort-name rejection (orderBy is array-form, key order cannot
reorder it).
…lity gates Field visibility conditions (#830) are enforced post-fetch, but the bound runner returned the delegate's rows raw while the serializer force-projects the gate operands: a findMany() user silently received unredacted gated columns. The runner now throws a typed AdapterError directing to execute() plus applyFieldConditions of @rapiq/adapter-memory. count() stays unaffected, since a gate changes column visibility, never the row set; execute() stays the pure serializer. The warning is documented in the Running the query docs section, where a runner user actually looks.
Core's PlanConditionOptions accepts string[] | boolean and the memory/prisma/drizzle adapters forward both shapes, but the sql visitor options (and with them typeorm's forwarding surface) accepted only the list form, so the blanket opt-out (true) silently did not exist on the SQL pair. The option now passes through to planCondition unchanged, which already handles the boolean. Specs pin the unfolded comparison on both backends; the option KEY split (visitor vs filters) stays untouched as an API-freeze question.
- preset resolution invariant: user-supplied names throw typed everywhere, derived facts fall back documented (resolves the typeorm-vs-prisma/drizzle unknown-name flag as two different inputs, not one policy) - adapter-sql's exported base classes including protected members are an intended extension surface consumed by adapter-typeorm, semver-relevant for external subclassers - filters contract of record: every backend consumes ICondition via planCondition; the per-operator IFilterVisitor methods are a legacy fast-path surface, removal sanctioned but unhurried
…ripwires The audit measured drizzle's parity matrix weaker than prisma's; the missing dialect-legal rows are ported (root ordering complements, the to-one null column pair, to-many relation presence, elemMatch with a null interior). Both packages gain the operator-enrollment tripwire the typeorm parity suite established: a spec walking the matrix conditions fails when a core semantics-table operator is neither exercised by the matrix nor documented typed-unsupported. The tripwire immediately caught gt and lte enrolled in NEITHER matrix; both gain rows on both backends (verified against the live engines). Prisma's inline condition list moves to test/data/matrix.ts so the tripwire runs in the default suite while the engine spec replays the same fixture.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR expands adapter parity coverage, fixes zero-limit pagination, hardens Prisma provider and filter handling, blocks unsafe field-condition queries, and adds global ChangesPagination and adapter parity
Prisma adapter hardening
Global SQL case sensitivity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PrismaAdapter
participant Delegate
Caller->>PrismaAdapter: submit findMany query
PrismaAdapter->>PrismaAdapter: detect field conditions
PrismaAdapter-->>Caller: return FEATURE_UNSUPPORTED
Caller->>PrismaAdapter: submit execute query
PrismaAdapter->>Delegate: execute serialized selection
Delegate-->>PrismaAdapter: return rows
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR applies a set of cross-backend alignment fixes identified in the architecture audit, tightening adapter semantics (pagination and filter behavior), hardening Prisma adapter behavior around unsafe runner paths, and expanding parity/enrollment tests plus documentation to lock the fleet contract in place.
Changes:
- Align pagination semantics so
limit: 0consistently means “return no rows” (including in the in-memory adapter) and add an engine-vs-memory parity spec for Drizzle. - Harden Prisma adapter behavior: safer provider preset resolution, typed failures for negation-distribution invariant breaks and unsupported
$thisusage, and a fail-closedfindMany()guard when field visibility conditions exist. - Widen
caseSensitiveoptions for SQL/TypeORM to acceptboolean | string[], add coverage, and expand/centralize engine parity matrices plus operator-enrollment “tripwire” tests.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/docs/packages/adapter-prisma.md | Documents the new findMany() refusal when field visibility conditions exist and the required safe alternative. |
| packages/docs/guide/filters.md | Documents blanket caseSensitive: true behavior across backends. |
| packages/adapter-typeorm/test/unit/filters.spec.ts | Adds coverage for caseSensitive: true (blanket opt-out of folding). |
| packages/adapter-sql/test/unit/interpreters/case-sensitivity.spec.ts | Adds coverage for blanket caseSensitive: true in SQL interpreter output. |
| packages/adapter-sql/src/visitor/types.ts | Widens caseSensitive option type to `string[] |
| packages/adapter-prisma/test/unit/run.spec.ts | Adds regression test ensuring findMany() fails closed when field conditions are present. |
| packages/adapter-prisma/test/unit/metadata.spec.ts | Adds tests for rejecting prototype-chain/inherited provider names when resolving provider presets. |
| packages/adapter-prisma/test/unit/filters.spec.ts | Adds cases for negated unsupported operators and ITSELF ($this) gating behavior. |
| packages/adapter-prisma/test/unit/enrollment.spec.ts | New “operator enrollment” tripwire enforcing parity-matrix coverage for semantics-table operators. |
| packages/adapter-prisma/test/unit/engine.db.spec.ts | Refactors engine parity suite to use shared parityConditions fixture. |
| packages/adapter-prisma/test/data/matrix.ts | New centralized Prisma engine parity condition matrix (also used by enrollment tripwire). |
| packages/adapter-prisma/src/provider/module.ts | Hardens provider preset lookup to own-properties only (prevents inherited keys from resolving). |
| packages/adapter-prisma/src/adapter/where.ts | Adds invariant guards for residual negation, blocks ITSELF ($this) rendering, and enforces typed failure on unexpected negated ordering plans. |
| packages/adapter-prisma/src/adapter/module.ts | Makes findMany() reject typed when field visibility conditions exist to prevent fail-open leakage. |
| packages/adapter-memory/test/unit/pagination.spec.ts | Adds regression coverage for limit: 0 semantics and preserves negative/undefined handling. |
| packages/adapter-memory/src/parameter/pagination/module.ts | Changes slicer logic to apply limit for any non-negative number (including 0). |
| packages/adapter-drizzle/test/unit/enrollment.spec.ts | New operator enrollment tripwire for Drizzle’s parity matrices. |
| packages/adapter-drizzle/test/unit/engine.spec.ts | Adds engine-vs-memory parity assertion for limit: 0. |
| packages/adapter-drizzle/test/data/matrix.ts | Expands Drizzle parity matrices (ordering complements, null interior elemMatch, to-many presence semantics). |
| .agents/architecture.md | Records architecture-audit rationales and clarifies fleet invariants and adapter extension/contract surfaces. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@packages/docs/guide/filters.md`:
- Around line 172-173: Update the paragraph describing caseSensitive: true to
qualify exact equality matching by backend collation, explicitly noting that
MySQL/MSSQL case-insensitive collations may still match differing cases,
including for caller-supplied authorization policies. Also update the
caseSensitive option documentation near Line 204 to describe its supported
string[] | true form.
🪄 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 Plus
Run ID: 593445f5-599c-4b41-bbb2-972f765bee9b
📒 Files selected for processing (20)
.agents/architecture.mdpackages/adapter-drizzle/test/data/matrix.tspackages/adapter-drizzle/test/unit/engine.spec.tspackages/adapter-drizzle/test/unit/enrollment.spec.tspackages/adapter-memory/src/parameter/pagination/module.tspackages/adapter-memory/test/unit/pagination.spec.tspackages/adapter-prisma/src/adapter/module.tspackages/adapter-prisma/src/adapter/where.tspackages/adapter-prisma/src/provider/module.tspackages/adapter-prisma/test/data/matrix.tspackages/adapter-prisma/test/unit/engine.db.spec.tspackages/adapter-prisma/test/unit/enrollment.spec.tspackages/adapter-prisma/test/unit/filters.spec.tspackages/adapter-prisma/test/unit/metadata.spec.tspackages/adapter-prisma/test/unit/run.spec.tspackages/adapter-sql/src/visitor/types.tspackages/adapter-sql/test/unit/interpreters/case-sensitivity.spec.tspackages/adapter-typeorm/test/unit/filters.spec.tspackages/docs/guide/filters.mdpackages/docs/packages/adapter-prisma.md
Owner
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
The import prune alongside the matrix extraction removed inArray, which the postgres-only branch of the wildcard-veto spec still uses; the sqlite leg returns before reaching it, so local runs and the mysql job stayed green while the postgres job failed with a ReferenceError.
The sorting and pagination section still documented limit: 0 as no limit, contradicting the fleet-wide explicit-0-is-a-value fix.
Review finding: resolveProvider lowercases first, so valueOf/toString can never reach the prototype chain; the only Object.prototype members that survive normalization are constructor and __proto__. The specs now assert those actually-hazardous names (keeping the camel-case spellings as contract pins for inputs the normalization blocks), and the twin provider comments in prisma and drizzle name a surviving example instead of a misleading one.
…veat Review finding: the sentence promised exact equality on all backends while the warning below correctly states that MySQL/MSSQL delegate equality to the column collation; a *_ci collated column keeps matching case-insensitively regardless of the opt-out. The schema-side option stays documented as a list: the boolean form exists only on the adapter forwarding surface.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the 2026-08-02 architecture audit: the cross-backend drift it confirmed, fixed one commit per concern, each with the regression spec that would have caught it.
Fixes
limit: 0is a value, not absence. The pagination slicer returned every row forlimit: 0while typeorm/prisma/drizzle (engine-measured) return none, and drizzle's impossible-root encoding (limit: 0) leans on that reading. The slice now applies for any non-negative numeric limit; negative values stay absence. A drizzle engine spec pins the agreement withapplyQuery.distributeNegationinvariant guards (typed failure instead of silently emitting the inverted positive form), and the typed ITSELF gate (previously an emitted$thiskey leaning on prisma's unknown-field error). Deliberately not ported because prisma is immune by construction: the metadata prototype hole (array-based walker) and numeric sort-name rejection (array-form orderBy).findMany()no longer fails open on field visibility gates (Feature: location-aware KeyValidator (root-only fields — allow at query root, strip via include) #830). The runner refuses typed when the query's fields carry conditions, directing toexecute()plusapplyFieldConditionsof @rapiq/adapter-memory;count()is unaffected. Documented in the Running the query section.caseSensitivewidened tostring[] | boolean. Core and the other three backends already accept the blanket opt-out (true); the SQL pair silently did not. Passes through toplanConditionunchanged. The option key split (visitorvsfilters) is untouched; that is an API-freeze question for the GA checklist.Tests
gt/lteenrolled in neither matrix; both backends gain rows, verified against the live engines (sqlite locally; postgres via the tests-db matrix).test/data/matrix.tsso the tripwire runs in the default suite while the engine spec replays the same fixture.Docs
IConditionviaplanConditionas the filters contract of record (per-operator visitor methods are a legacy fast-path).No public API breaks beyond the additive
caseSensitivewidening.Summary by CodeRabbit
New Features
caseSensitive: trueoption for exact equality comparisons across supported SQL and TypeORM adapters.Bug Fixes
0now correctly returns no rows.Tests