fix(core): lower $and / $or to real AST group nodes in convertFiltersToAST - #8456
Conversation
…ToAST
`@objectstack/spec`'s `FilterCondition` declares `$and` / `$or` / `$not`, and
this repo's one lowering from the MongoDB-style filter object to the ObjectQL
AST had no branch for any of them. `$and` / `$or` fell through to the
simple-equality branch (their value is an array, so the operator loop was
skipped) and became a leaf naming a field literally called `$and` / `$or`;
`$not` entered the operator loop with its own nested object's keys read as
operator names and threw naming a nonsense operator.
Re-measured against `@objectstack/spec` 17.3.0, the leaf is NOT refused on the
wire: `parseFilterAST(['$or', '=', [...]])` is `{ $or: [...] }`, the condition
the author wrote. The defect is one door in. That leaf is a well-formed
COMPARISON node, so every AST evaluator in this repo reads `$or` as a field
name; `ValueDataSource`'s matcher looks up `record['$or']`, finds nothing, and
excludes every row — no error, no console line.
So `$and` / `$or` now lower to `['and'|'or', ...children]`, the spelling the
spec's own `FILTER_ARRAY_LOGIC_KEYWORDS` declares, with children lowered
recursively. The wire condition is unchanged (pinned); what changes is that the
node is executable as well as parseable, and that operators inside a combinator
branch now meet the same guard every other operator meets.
`$not` is refused with an accurate message rather than translated: the AST has
no negation keyword and `startswith` / `endswith` / `between` / `icontains` have
no negated counterpart in `VALID_AST_OPERATORS`, so a De Morgan rewrite would be
silently partial and would drop the NULL-safe rule of objectstack#5146. It threw
before this change too; only the diagnostic moved. Whether the AST should gain a
negation is a spec question, raised on the card.
Boolean identities (objectstack#5322) are handled at the boundary of the new
branch: an empty `$and` is TRUE and drops out, because a childless `['and']`
measures to `isFilterAST` false / `parseFilterAST` undefined — no filter at all,
i.e. every row, the one direction that must not happen. An empty `$or` is FALSE,
which the AST cannot spell, so it keeps the emission it already had — measured
to answer zero rows at both the wire and the in-memory matcher.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
|
PM contract review — accepted, flipped to ready, auto-merge armed. Two rulings below; neither is a product call, both are recordings of what your measurement already settled. The card's account of the failure is falsified, and that is the headlineThe card said the ⇒ The real failure is neither of the two modes my brief named. It is a third: the node reaches the wire and is interpreted correctly there, then is mis-evaluated to zero rows by the in-repo AST evaluator, because a well-formed comparison node makes My brief told you to establish which of two failure modes you were fixing. The correct answer was "neither" — that is the most useful thing this report contains. Ruling on
|
2 participants
Fixes #6948
The card's premise is half wrong, and that changes what this PR is
Re-derived on
ca3942729(the branch base) against@objectstack/spec17.3.0 — the card measuredpackages/core/distat40c479af2.Still holds, byte for byte. Composing each condition with a parent scope,
mergeFilterNodes({task_version:'tv-1'}, f)on the base commit reproduces every emission the card printed:The file, the three functions and the two mechanisms (array value skips the operator loop;
$not's own nested keys read as operators) all hold.filter-converter.tshas not moved onmainsince2a9513d81.Falsified. The card says the
$and/$orleaf is "a well-formed AST node carrying a nonsense field, so the server refuses it (400 INVALID_FILTER)". Measured against the spec's own doors:A
[field, '=', value]node lowers to{ [field]: value }, and$oris a legalFilterConditionkey — so the leaf round-trips into a real combinator and the server was never refusing anything. This repo already knew:data-objectstack/src/filter-entry-translation.test.tspinned that exact round trip with the note "verified, and the reason this is NOT rewritten here."So the failure mode being fixed is neither "never reaches the wire" nor "reaches it and is ignored." It is: reaches the wire and is interpreted correctly there, and is mis-evaluated — silently, to zero rows — by every AST evaluator inside this repo.
['$or','=',[...]]is a well-formed comparison node, soValueDataSource'smatchesComparisonNodereads$oras a field name, looks uprecord['$or'], finds nothing, and excludes every row. No throw, no console line, an empty list where the author asked for a union.Reachability — which consumers the claim covers
The card called this sink-level rather than related-list-level. That holds and is stronger than stated:
$andhas live in-repo producers today, not only the newly-typedField.relatedListFilter.core/src/utils/merge-filters.tsmergeFilters{ $and: [a, b] }plugin-dashboard/src/DashboardRenderer.tsx:851,858mergeFilters(scope-filter broadcast)plugin-report/src/DatasetReportRenderer.tsx:1429,1447mergeFilters(report / block runtime filter)plugin-dashboard/src/ObjectMetricWidget.tsx:413,DrillDownDrawer.tsx:106{ $and: [existing, resolved] }fields/src/widgets/FilterConditionField.tsx:209,213{ $or: [...] }/{ $and: [...] }The only in-repo AST evaluator is
ValueDataSource'smatchesASTFilter(grep fornode[0]); every other consumer forwards the node to aDataSource.The change
$and/$orlower to['and'|'or', ...children]— the spelling the spec's ownFILTER_ARRAY_LOGIC_KEYWORDSdeclares — with children lowered recursively. The wire condition is unchanged (pinned both ways); what changes is that the node is executable as well as parseable, and that operators inside a combinator branch now meet the same guard every other operator meets (a$bogusor a$regexinside a$orbranch used to travel to the wire unchecked inside the leaf's value slot).Boolean identities (objectstack#5322) are handled at the boundary of the new branch, because a new branch has to decide them:
{ $and: [] }is TRUE, so it drops out. Not['and']: measured,isFilterAST(['and'])isfalseandparseFilterAST(['and'])isundefined— no filter at all, i.e. every row, the one direction that must never happen.{ $or: [] }is FALSE, which the AST cannot spell, so it keeps the leaf it already had — measured to answer zero rows at the wire ({$or: []}) and in the in-memory matcher. Documented in place, with the alternatives and why each is worse.{}disjunct is TRUE and absorbs its$or; a{}conjunct drops out of its$and.$notis refused, and it is an open contract questionThe ObjectQL AST has no negation:
FILTER_ARRAY_LOGIC_KEYWORDSis['and','or']andVALID_AST_OPERATORS.has('not')isfalse(measured, 53 members). Rewriting the negation inward is not available either —startswith,endswith,betweenandicontainshave no negated counterpart in that set, so a De Morgan lowering would be silently partial and would quietly drop the NULL-safe rule of objectstack#5146.So
$notthrows with an accurate message instead of one naming the author's own nested field as a bogus operator. It threw before this change too, so the verdict is unchanged and only the diagnostic moved — this PR does not decide the contract. Whether the AST should gain a negation, or whether this sink may hand aFilterConditionobject to the wire when a$notis present, is raised on the card for the maintainer.The pin — row sets, both directions, and what a worse implementation would do
packages/core/src/utils/__tests__/filter-combinators-6948.test.ts(24 cases). Nothing greps the source; every assertion is either the node that reaches the wire put throughisFilterAST/parseFilterAST, or the row set a realValueDataSourcereturns.Both halves are asserted because each alone passes on something worse than the bug:
selectedIds(...)is['open-active','blocked-idle'], exactly. A converter that emits nothing usable leaves$filterunread and returns every row (selectedIds(undefined)is asserted equal toALL_IDSas the lit control).done-active,null-status,no-status-keyare absent. A converter emitting a node no evaluator reads returns no row — which is the bug itself, pinned asPRE_FIX_OR_NODEselecting[].ALL_IDSnor[], and that the fixture is not trivially either.Refusals assert the
INVALID_FILTER/400envelope plus the message clause, never a baretoThrow().Ablation — the read site, from the committed implementation
Removed the combinator dispatch block from
convertFiltersToAST(the read site, not the helper), proved it reached disk, ran, restored by state.19 named red rows across both packages, including
lowers $or to an AST group node,INCLUDES both branches of the union — not empty,$and intersects, both directions,a parent scope still narrows a union it wraps,nested combinators evaluate as written,drops $and: [] — the TRUE identity constrains nothing,runs the unknown-operator guard on combinator children, and both routes oflowers a top-level Mongo logical node to an AST group.Two rows deliberately stayed green under ablation and that is the point:
EXCLUDES the rows outside the union(the ablated converter excludes everything, so it trivially excludes those) andthe pre-fix node selected NOTHING(a control on a literal node, independent of the implementation). That is exactly why the inclusion half exists.The
data-objectstackpin going red under apackages/coresource edit is also the proof that no rebuild leg is owed: the rootvitest.config.mtsaliases@object-ui/coretopackages/core/src, so there is nodisthop to stale.Restore verified by state, not by exit code:
git diff HEADempty andhash-objectequal torev-parse HEAD:path.Changeset
.changeset/filter-ast-combinators-6948.md—@object-ui/core: minor,@object-ui/data-objectstack: patch. Verdict line:minorbecause shipped results move: a list filtered by a combinator through any in-process data source goes from zero rows to the rows the author asked for, and an unknown operator inside a combinator branch is now refused rather than shipped. Same shape as the landed precedent PR #8437.majoris forbidden here (fixed group);check-changeset-no-major.mjsexits 0.skip-changesetdeliberately not applied — it is a phantom label in this repo.Verification run
At
df25f4b1a:pnpm exec vitest run packages/core/ packages/data-objectstack/ packages/fields/ packages/plugin-dashboard/ packages/plugin-report/VERDICT command-exit 0— 429 files, 6775 testspnpm exec vitest run <the two pins> packages/plugin-list/ packages/plugin-grid/ packages/plugin-detail/ packages/plugin-form/ packages/plugin-view/VERDICT command-exit 0— 442 files, 4360 passed / 1 skippedpnpm exec vitest run packages/components/ packages/react/VERDICT command-exit 0— 313 files, 3100 testspnpm --filter '@object-ui/core' --filter '@object-ui/data-objectstack' type-check--listFiles, with a lit and a dark control)pnpm --filter '@object-ui/core' --filter '@object-ui/data-objectstack' lintnode scripts/check-control-bytes.mjs✅ OK (6668 tracked text files)node scripts/check-governed-queue-guard.mjs --test <the 4 paths>✅ NOT GOVERNEDDeclared narrowing: 1184 test files run locally, covering every direct consumer of the sink and every in-repo producer of
$and/$orenumerated above.app-shell,console,site, the examples and the remaining plugins are declared to CI — their$andreferences (ObjectFieldInspector,datasetFilterCondition) are metadata-authoring conversions that never reachconvertFiltersToAST.Merge note.
ValueDataSource.tsmoved onmainafter this branch was cut (objectui#7379: text-operator case sensitivity). The pin here uses only=and>=on non-string and string-equality fields, so it touches none of the drifted arms;filter-converter.tsitself is unmoved since2a9513d81.🤖 Generated with Claude Code
https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Generated by Claude Code