fix(core)!: make query filter merges conjunctive - #890
Merged
Merged
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR replaces sealed conditions with branded ChangesCondition composition and preservation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/core/test/unit/parameter/merge.spec.ts (2)
382-413: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a merge case with preserved operands on both sides.
The suite covers a preserved right operand at line 389 and preserved receivers for
and()andor(). Add a case where both operands are preserved. It confirms that the two markers survive as two separate conjuncts and that the new root AND stays unpreserved.🤖 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/core/test/unit/parameter/merge.spec.ts` around lines 382 - 413, Add a unit test near the existing preserved merge coverage that merges two separately preserved operands, asserting the result contains both preserved operands as distinct conjuncts and that the resulting root AND is not preserved. Reuse the existing preserve, and, and Filter merge APIs and marker assertions.
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider sharing one
CustomConditiontest fixture insidepackages/core/test.The same
CustomConditionsubclass now exists in several core test files, includingpackages/core/test/unit/parameter/condition-contract.spec.ts,packages/core/test/unit/build/module.spec.ts, andpackages/core/test/unit/parser/parameter/key-validation.spec.ts. Move it into the sharedpackages/core/test/datahelpers so the branded-condition contract has one definition. Keep the copy inpackages/codec-urlbecause it is a separate package.🤖 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/core/test/unit/parameter/merge.spec.ts` around lines 34 - 38, Move the CustomCondition fixture from this test file into the shared packages/core/test/data helper area, then update core tests such as condition-contract.spec.ts, module.spec.ts, and key-validation.spec.ts to import and reuse that single definition. Remove their local subclasses while preserving the existing constructor and branded-condition behavior; leave the packages/codec-url copy unchanged.packages/core/src/parameter/filters/collection/module.ts (1)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the leftover commented-out statement.
Line 36 holds a dead mutation statement from the previous mutable implementation.
flatten()is now immutable, so the comment can mislead readers.♻️ Proposed cleanup
flatten(aggregatedResult?: T[]) : IFilters<T> { - // this.value.splice(0, this.value.length, ...next); - return new Filters(🤖 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/core/src/parameter/filters/collection/module.ts` around lines 35 - 43, Remove the leftover commented-out splice statement from the immutable flatten method in Filters.flatten, leaving the method’s current return logic unchanged.
🤖 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 @.agents/architecture.md:
- Line 32: Update the validator residual documentation in the “Parse to IR”
section of .agents/architecture.md to use preserve(and(<client leaf>, <policy
residual>)), matching the implementation in validate.ts and the authoritative
relation-prune.spec.ts behavior; do not alter code or tests unless the intended
contract differs.
In @.agents/structure.md:
- Line 55: Update the filters tree entry in structure.md to include NOT
alongside compound AND/OR filters, using “compound and/or/not” or an explicitly
non-exhaustive description. Keep the summary consistent with the documented
filter operations in architecture.md and packages/core/README.md.
In `@packages/docs/guide/recipes/frontend.md`:
- Line 41: The frontend recipe must preserve the age >= 18 baseline when search
filters are present. Update the filter construction around currentFilters,
defaultFilters, and mergeQueries so defaultFilters is combined with
currentFilters rather than selected only via nullish fallback; otherwise,
explicitly rename and document it as an empty-search-only fallback.
---
Nitpick comments:
In `@packages/core/src/parameter/filters/collection/module.ts`:
- Around line 35-43: Remove the leftover commented-out splice statement from the
immutable flatten method in Filters.flatten, leaving the method’s current return
logic unchanged.
In `@packages/core/test/unit/parameter/merge.spec.ts`:
- Around line 382-413: Add a unit test near the existing preserved merge
coverage that merges two separately preserved operands, asserting the result
contains both preserved operands as distinct conjuncts and that the resulting
root AND is not preserved. Reuse the existing preserve, and, and Filter merge
APIs and marker assertions.
- Around line 34-38: Move the CustomCondition fixture from this test file into
the shared packages/core/test/data helper area, then update core tests such as
condition-contract.spec.ts, module.spec.ts, and key-validation.spec.ts to import
and reuse that single definition. Remove their local subclasses while preserving
the existing constructor and branded-condition behavior; leave the
packages/codec-url copy unchanged.
🪄 Autofix
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: 58fb762b-7659-4646-ba4b-b873811f9462
📒 Files selected for processing (42)
.agents/architecture.md.agents/structure.mdREADME.mdpackages/adapter-prisma/test/unit/acceptance.spec.tspackages/adapter-typeorm/test/unit/acceptance.spec.tspackages/codec-url/test/unit/acceptance.spec.tspackages/codec-url/test/unit/simple-roundtrip.spec.tspackages/core/README.mdpackages/core/src/build/parameter/filters/module.tspackages/core/src/errors/code.tspackages/core/src/errors/schema.tspackages/core/src/parameter/fields/collection/module.tspackages/core/src/parameter/filters/collection/module.tspackages/core/src/parameter/filters/collection/types.tspackages/core/src/parameter/filters/condition.tspackages/core/src/parameter/filters/index.tspackages/core/src/parameter/filters/preserve.tspackages/core/src/parameter/filters/record/module.tspackages/core/src/parameter/filters/record/types.tspackages/core/src/parameter/filters/seal.tspackages/core/src/parameter/merge.tspackages/core/src/parser/parameter/filters/validate.tspackages/core/src/parser/relation-prune.tspackages/core/test/unit/build/module.spec.tspackages/core/test/unit/parameter/condition-contract.spec.tspackages/core/test/unit/parameter/filters-non-node.spec.tspackages/core/test/unit/parameter/merge.spec.tspackages/core/test/unit/parser/parameter/filters/validate.spec.tspackages/core/test/unit/parser/parameter/key-validation.spec.tspackages/core/test/unit/parser/relation-prune.spec.tspackages/docs/guide/building-queries.mdpackages/docs/guide/concepts.mdpackages/docs/guide/errors.mdpackages/docs/guide/filters.mdpackages/docs/guide/merging-queries.mdpackages/docs/guide/recipes/authorization.mdpackages/docs/guide/recipes/express-typeorm.mdpackages/docs/guide/recipes/frontend.mdpackages/docs/guide/relations.mdpackages/docs/packages/codec-url.mdpackages/docs/packages/core.mdpackages/parser-simple/test/unit/parser/relations-traversal.spec.ts
💤 Files with no reviewable changes (1)
- packages/core/src/parameter/filters/seal.ts
| ├── parameter/ # Query AST node classes + visitor interfaces | ||
| │ ├── fields/ # Fields/Field (include/exclude operators) | ||
| │ ├── filters/ # Filters (compound and/or) + Filter (field-op-value condition) | ||
| │ ├── filters/ # Filters (compound and/or) + Filter (field-op-value condition); CONDITION_MARKER/Condition identity and preserve() pruning wrapper |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document NOT in the filters tree.
.agents/architecture.md and packages/core/README.md describe not as a supported filter operation. This entry still says compound and/or, so the package summary is incomplete. Change it to compound and/or/not or mark the list as non-exhaustive.
As per coding guidelines, keep this file and all corresponding files in .agents updated as the project evolves.
🤖 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 @.agents/structure.md at line 55, Update the filters tree entry in
structure.md to include NOT alongside compound AND/OR filters, using “compound
and/or/not” or an explicitly non-exhaustive description. Keep the summary
consistent with the documented filter operations in architecture.md and
packages/core/README.md.
Source: Coding guidelines
…ract Review findings on #890. `preserve()`'s catch-all overload promised `IFilters` for an `ICondition` argument, but overload resolution admits every subtype, so an argument merely *typed* as `ICondition` still reaches the leaf branch and comes back an `IFilter`. Measured: `preserve(c).and(...)` compiles and throws a TypeError. That is the shape the filters `validate` hook hands out, the one place the docs point at. The catch-all now returns `ICondition`; the precise `IFilter` / `IFilters` overloads are unaffected, so only code that relied on the wrapper claim needs `isFilter` / `isFilters` to narrow. Relation pruning searched for a preserved descendant only below `elemMatch`, while the drop it guards takes the whole subtree with it whatever the operator: `new Filter('customOp', 'items', preserve(...))` pruned silently where the `elemMatch` twin threw. The refusal is now decided once, over the subtree, at the single drop site; descending stays gated on `elemMatch`, which is the only operator known to address its interior relative to the element. `isCondition` tested only that the brand key was present, so `{ [CONDITION_MARKER]: false, ... }` passed as a live condition, and `ICondition` typed the brand `boolean`, which made that object legal at compile time too. The brand is now the literal `true` and the guard asserts the value. The footers below cover this branch as a whole: the earlier commits carry the `!` marker without a body, so this is where the migration notes for the release changelog live. BREAKING CHANGE: `Filters.merge()` and `mergeQueries()` compose filters as an ordered logical AND. Same-field conditions are no longer replaced by the receiver, they are retained as conjuncts, so two `eq` conditions on one field now select nothing instead of the receiver's value. Replace transient UI state before building the query, or select the current `IFilters` node and pass only that to `defineQuery`. BREAKING CHANGE: `seal()`, `ICondition.seal()`, `IFilter.seal()`, `IFilters.seal()`, the `sealed` marker and `ConditionOptions.sealed` are removed, together with `ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED` and `SchemaError.sealedConditionPruned()`. Sealing existed to survive a replace-merge, which no longer drops anything. Relation-pruning protection moves to `preserve()` / `ICondition.preserved`, reported as `ErrorCode.SCHEMA_PRESERVED_CONDITION_PRUNED`. `Filters.and()` / `.or()` no longer mark what they inject, since composition cannot displace it. BREAKING CHANGE: `ICondition` requires the non-serializable `CONDITION_MARKER` brand, valued `true`. Extend the `Condition` base class or declare `readonly [CONDITION_MARKER] = true` on a structural implementation. A condition that made a JSON/RPC/cache round trip is no longer accepted as live. BREAKING CHANGE: `preserve()` returns `ICondition` for an argument typed as `ICondition`; narrow with `isFilter` / `isFilters` where the concrete kind is needed. `ICondition[CONDITION_MARKER]` is the literal `true` rather than `boolean`, so a structural implementation declaring `boolean` no longer satisfies the interface.
Review findings on #890. The two snippets illustrating pre-IR replacement selected an `age >= 18` default against a `name contains <search>` current value. The fields are unrelated, so `currentFilters ?? defaultFilters` drops the age baseline the moment the user types in the search box: the example taught the opposite of the rule it was introduced to teach. Selection is only meaningful between the values one control can hold, so the guide now selects a status control against its own default and warns that an unrelated baseline is not an alternative to that choice. The frontend recipe keeps its baseline in `defaults`, where it merges in on every request, and its search box goes back to contributing a condition only when it is filled. Both snippets were run and type-checked against the current core.
Closes the third ask of #889. Making `Filters.merge` conjunctive removed per-field replace outright, and the issue asked for it to move to the build-input layer rather than disappear: overriding a default on the same field is a real need, it is just hazardous on a tree that may carry a server-authored scope. Object spread almost covers it, since build input is plain data, and it was the cheaper answer. Two measured failures are why it is not enough, both silent. A spread compares keys, and `FiltersBuildInput` addresses one field under two notations, so `{'realm.name': 'a'}` spread with `{realm: {name: 'b'}}` emits two conditions on one field. A spread is also shallow, so `{realm: {name: 'a', id: 1}}` spread with `{realm: {name: 'b'}}` drops the `realm.id` default with the branch that replaced it. The second is the same failure the previous commit fixed in the guide, which is the argument against leaving it to documentation. `mergeFiltersInput` reduces each input to canonical dotted paths and keeps the first value seen for each, so both notations replace each other and a nested record is replaced key by key. First-occurrence priority is applied per path rather than per argument, so it holds within one input too. Replacement is per field, not per operator, which is what the removed `Filters.merge` did; an `undefined` value claims no field, matching the build layer and `Pagination.merge`. What makes replacement safe here is the input type: no `ICondition` overload, a `CONDITION_MARKER` exclusion so an untyped call site is rejected at compile time too, and a `BuildError` at runtime. A build input cannot carry a server-authored scope, so per-field replace cannot displace one. The leaf-versus-branch rule is now single-sourced in an internal module that both the lowering and the canonicalizer consult, since a drift between them would be silent in exactly the way the two spread failures are. Verified load-bearing: breaking it fails three new merge specs and two pre-existing `defineFilters` specs.
Owner
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Owner
Author
|
@coderabbitai full review |
|
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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/core/src/build/parameter/filters/merge.ts`:
- Around line 106-128: Update the merge path in mergeFiltersInput so the output
accumulator is created with a null prototype instead of a plain object literal,
preserving the existing key-merge logic in collectPaths and the root `$`
handling. This will let `__proto__` be stored as a normal own field without
triggering the inherited setter, while keeping `Object.keys`, `Object.hasOwn`,
and the downstream defineFilters spread behavior unchanged.
🪄 Autofix
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: 19632836-25df-42ce-ba33-a7e763eb9893
📒 Files selected for processing (46)
.agents/architecture.md.agents/structure.mdREADME.mdpackages/adapter-prisma/test/unit/acceptance.spec.tspackages/adapter-typeorm/test/unit/acceptance.spec.tspackages/codec-url/test/unit/acceptance.spec.tspackages/codec-url/test/unit/simple-roundtrip.spec.tspackages/core/README.mdpackages/core/src/build/parameter/filters/index.tspackages/core/src/build/parameter/filters/merge.tspackages/core/src/build/parameter/filters/module.tspackages/core/src/build/parameter/filters/value.tspackages/core/src/errors/code.tspackages/core/src/errors/schema.tspackages/core/src/parameter/fields/collection/module.tspackages/core/src/parameter/filters/collection/module.tspackages/core/src/parameter/filters/collection/types.tspackages/core/src/parameter/filters/condition.tspackages/core/src/parameter/filters/index.tspackages/core/src/parameter/filters/preserve.tspackages/core/src/parameter/filters/record/module.tspackages/core/src/parameter/filters/record/types.tspackages/core/src/parameter/filters/seal.tspackages/core/src/parameter/merge.tspackages/core/src/parser/parameter/filters/validate.tspackages/core/src/parser/relation-prune.tspackages/core/test/unit/build/filters-merge.spec.tspackages/core/test/unit/build/module.spec.tspackages/core/test/unit/parameter/condition-contract.spec.tspackages/core/test/unit/parameter/filters-non-node.spec.tspackages/core/test/unit/parameter/merge.spec.tspackages/core/test/unit/parser/parameter/filters/validate.spec.tspackages/core/test/unit/parser/parameter/key-validation.spec.tspackages/core/test/unit/parser/relation-prune.spec.tspackages/docs/guide/building-queries.mdpackages/docs/guide/concepts.mdpackages/docs/guide/errors.mdpackages/docs/guide/filters.mdpackages/docs/guide/merging-queries.mdpackages/docs/guide/recipes/authorization.mdpackages/docs/guide/recipes/express-typeorm.mdpackages/docs/guide/recipes/frontend.mdpackages/docs/guide/relations.mdpackages/docs/packages/codec-url.mdpackages/docs/packages/core.mdpackages/parser-simple/test/unit/parser/relations-traversal.spec.ts
💤 Files with no reviewable changes (2)
- packages/core/src/parameter/filters/seal.ts
- packages/core/src/parameter/filters/record/types.ts
Comment on lines
+106
to
+128
| const field = prefix ? `${prefix}.${key}` : key; | ||
|
|
||
| // a `$`-prefixed root key is not a field. It is passed through | ||
| // unresolved so defineFilters still reports it, rather than being | ||
| // reinterpreted as a path segment here. | ||
| if (!prefix && key.substring(0, 1) === '$') { | ||
| if (!Object.hasOwn(output, key)) { | ||
| output[key] = value; | ||
| } | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| if (isNestedRecordValue(field, value)) { | ||
| collectPaths(value, field, output); | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| if (!Object.hasOwn(output, field)) { | ||
| output[field] = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A __proto__ key is silently dropped instead of being merged as a field.
output is a plain object literal (Line 65). The write at Line 126 and Line 113 uses bracket assignment. If key is the string __proto__, Object.hasOwn(output, '__proto__') returns false, so the guard passes, but the assignment invokes the inherited __proto__ setter. No own property is created. The result: the field is lost, and the prototype of the returned object changes when the value is an object.
buildConditions in packages/core/src/build/parameter/filters/module.ts treats __proto__ as an ordinary field name and lowers it to a condition. So mergeFiltersInput and defineFilters disagree for that key. Global Object.prototype is not affected, so this is a correctness divergence rather than prototype pollution.
Give output a null prototype. Object.keys, Object.hasOwn, and the spread in defineFilters all still work.
🐛 Proposed fix
- const output : Record<string, unknown> = {};
+ // a null prototype keeps `__proto__` an ordinary field name: a plain
+ // literal would route the assignment to the inherited setter and drop it.
+ const output : Record<string, unknown> = Object.create(null);🤖 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/core/src/build/parameter/filters/merge.ts` around lines 106 - 128,
Update the merge path in mergeFiltersInput so the output accumulator is created
with a null prototype instead of a plain object literal, preserving the existing
key-merge logic in collectPaths and the root `$` handling. This will let
`__proto__` be stored as a normal own field without triggering the inherited
setter, while keeping `Object.keys`, `Object.hasOwn`, and the downstream
defineFilters spread behavior unchanged.
Merged
tada5hi
added a commit
to authup/authup
that referenced
this pull request
Aug 7, 2026
Filter composition is conjunctive now (tada5hi/rapiq#890, closing #889): `IFilters.merge` and `mergeQueries` retain every conjunct of both sides instead of replacing same-field conditions, so no composition step can drop an injected scope. That makes non-displaceability structural and retires the machinery built for it over the previous releases: the beta.15 `FILTERS_NOT_FLAT` throw, the beta.16 seal marker, and the beta.18 `ICondition.seal()` contract member are all gone. `and()` no longer wraps an injection in a distinguishable subtree, so the encoded filter goes back to the plain AND it reads as and the collection expectations return to their pre-beta.16 shape. Authup needs no code change beyond the notes. The kit already keeps interactive filters in its own state and strips them before `mergeQueries`, so the one caller-facing consequence, that two `eq` conditions on a field now intersect to nothing rather than the receiver winning, cannot reach it. Nothing here calls `preserve()` or merges a filter tree, so the surviving pruning marker and its `SCHEMA_PRESERVED_CONDITION_PRUNED` contradiction check are out of reach too.
tada5hi
added a commit
to authup/authup
that referenced
this pull request
Aug 7, 2026
…ition The collection manager stripped filters out of every `mergeQueries` call and recombined them by hand afterwards, because the old `Filters.merge` did per-field replace: a search input on a scoped field would have displaced an injected realm or owner scope rather than narrowing it. rapiq beta.19 (tada5hi/rapiq#890) made filter merging conjunctive, so that hazard is gone and the workaround with it. Both call sites collapse to a plain `mergeQueries`, and `stripFilters` and `combineScopedFilters` are removed. Behaviour is unchanged, including conjunct order: the whole collection suite passes with no expectation edits, and the two cases that pin the property that motivated the carve-out, *search input cannot displace the injected scope* and *composes context query and props query, both non-displaceable*, still assert the same filter strings.
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.
Summary
mergeQueriescompose filters as an ordered logicalANDso every predicate survives, including same-field ranges and contradictionsCONDITION_MARKERandpreserve()seal,sealed, andseal()contractselemMatch, relation pruning, and schema defaultsBreaking changes
Filters.merge()ormergeQueries(); they remain conjunctsConditionor provideCONDITION_MARKERseal(),sealed, and condition seal methods are removedSCHEMA_SEALED_CONDITION_PRUNEDis renamed toSCHEMA_PRESERVED_CONDITION_PRUNEDValidation
npm run lint -- --ignore-pattern '.claude/worktrees/**'NX_SKIP_NX_CACHE=true npm run buildNX_SKIP_NX_CACHE=true npm test(2,132 tests)git diff --check origin/master...HEADCloses #889
Summary by CodeRabbit
preserve()for protecting filter conditions during validation and relation pruning.mergeFiltersInput()for predictable, field-level filter input merging.seal()and sealed-condition terminology with preservation semantics.