fix(data-objectstack): a view's own filter no longer vanishes when the user adds one - #3072
Merged
Merged
Conversation
…e user adds one
Object-form filter entries (`[{ field, operator, value }, ...]`) were
translated only at the TOP level of a `$filter`. The moment a list has both a
stored view filter and a user filter it builds
['and', [{ field: 'stage', operator: 'eq', value: 'won' }], [['amount', '>', 1]]]
whose head is the string `and`, so the old check called the whole thing
"already AST" and shipped the rules untranslated. Both server answers to that
are wrong:
isFilterAST(above) // false — a bare rule object is not an AST child
parseFilterAST(above) // { amount: { $gt: 1 } } ← `stage = won` is GONE
Since objectstack#4121 the `isFilterAST` gate makes it a 400 and the list fails
to load; before it — or anywhere `parseFilterAST` is reached without that gate
— the view's own condition is dropped without a word and the list returns
records the view exists to exclude. Translation is now recursive through
`and`/`or` nodes and legacy flat child arrays.
Three related fixes in the same code:
- An untranslatable entry is an error, not an omission. Entries that failed to
translate were dropped, and dropping one conjunct of an `and` returns a
superset of the rows asked for — dropping the last one sent no `filter=` at
all, so the whole table came back. `find()` now throws MalformedFilterError
carrying `code: 'INVALID_FILTER'` / `httpStatus: 400`, so a failed list
renders "the filter is malformed" rather than "check your connection".
A rule with a blank `field` passes ViewFilterRuleSchema (`z.string()` admits
''), so this is reachable from real stored metadata. A mixed rule/tuple array
now keeps both halves — that case was a lost condition, not a malformed one.
- The two `find()` routes can no longer disagree. The "is this object form?"
test existed twice — in `translateFilterToAST` and inline in
`convertQueryParams` — and the copies had already drifted: the inline one
omitted a `!== null` guard, so `$filter: [null]` threw a TypeError on the
plain route while the same value was handled on the `$expand` route.
- Dropped an unreachable `entry.name` fallback. `objectFilterEntryToAST` read
`entry.field ?? entry.name` while the shape check keyed on `field` alone, so
the `name` half was dead from the commit that introduced it (4b93db4). The
spec agrees it is not a real shape — `ViewFilterRuleSchema.field` is
required, so such a rule cannot be saved as view metadata.
44 tests, driving both `find()` routes; the emitted filter is asserted against
the server's own `isFilterAST` rather than a restated shape. Reverting
index.ts fails 19 of them.
Refs objectstack#3948, objectstack#4121, #2945
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
… downloads the unsearched superset (#3078) The server-streamed export mirrored the view's `filter` and `sort`, and the code comment claimed that made the file match the screen: Mirrors the active view's filter + sort so the exported file matches what the user sees. It mirrored one half. There was no way to carry the term a user had typed into the search box — `ExportDownloadRequest` had no field for one — so exporting during a search produced MORE ROWS than the list showed, in a file that looks authoritative, with nothing indicating the difference. The client-side fallback was always correct (it serializes the already-searched `data`); only the server path was wrong, and it is the one that handles xlsx. Same family as a dropped filter (objectstack#3948, objectstack#4181): a plausible answer that is quietly broader than the one asked for. - `ExportDownloadRequest` gains `search` / `searchFields`. - `ObjectStackAdapter.exportDownload` sends them as `search=` / `searchFields=`, trimming the term and omitting both when it is blank (`searchFields` alone means nothing). - `ListView` passes the active `searchTerm` and the view's `searchableFields`, and both are now in the export callback's dependency array — a stale closure would export the wrong row set. Requires a server with objectstack#4230. Older servers ignore unknown query params on this route, so they keep today's behaviour rather than erroring. Also: the filter merge is no longer written twice. The three filter sources (view filter, filter-panel group, per-field user filters) were merged by verbatim copies in the data fetch and in the export — two copies that must agree, deciding respectively what the user SEES and what they DOWNLOAD. Both now call `buildEffectiveFilter`. That half is a PURE EXTRACTION, and the tests say so: the four parity tests added for it pass against the old duplicated code too. They exist to keep it that way — the adapter's duplicated filter-shape check had already drifted apart unnoticed (#3072). The parity tests compare the arguments of the two real calls rather than asserting an expected AST twice, which would go on passing if both copies drifted the same wrong way. Verification: 9 new tests (4 ListView export/search, 4 fetch-vs-export parity, 5 adapter query params). Reverting ListView.tsx fails the 2 search tests and passes the 4 parity ones; reverting the adapter fails 4. Full suite 758 files / 8819 tests green; tsc clean across types, data-objectstack, plugin-list; eslint 0 errors. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
os-zhuang
added a commit
that referenced
this pull request
Jul 31, 2026
…ther the query expands a lookup (#3084) #3072 single-sourced the ARRAY branch of the adapter's two `find()` routes. The object branch was left as it was: `convertQueryParams` converted a MongoDB-style filter to AST while `translateFilterToAST` returned it verbatim — so the same `$filter` went out in two formats, decided by whether the query happened to expand a lookup. Measured across 21 operator shapes, four diverged. Most of the gap turned out to be harmless, which is worth recording because it was not obvious: `{$and: […]}` survives the plain route as a `['$and','=',[…]]` comparison that `parseFilterAST` reads back as a real `$and`, and `$exists` vs `$null` is a difference the server treats identically. Two were not harmless: - THE UNKNOWN-OPERATOR GUARD ONLY RAN ON ONE ROUTE. `convertFiltersToAST` throws on an unrecognised operator, with a comment saying it does so "to avoid silent failure" — but the expanded route never called it, so a typo'd operator threw on a plain read and shipped silently whenever a lookup was expanded. - `$regex` WAS SILENTLY REWRITTEN TO `contains`. The existing test's own example makes the case: `$regex: '^John'` means "starts with John", while `contains '^John'` looks for a literal caret — so "John Smith" does not match. A different question, not a weaker version of the same one, and neither result looks wrong on screen. The rewrite sat behind a `console.warn`, which is not an error channel in a deployed app, and the function's own unknown-operator message never listed `$regex` among the supported set. The spec has no `$regex` (`FILTER_OPERATORS`, data/filter.zod.ts), so there is nothing to translate it into: it is refused now, the same treatment the neighbouring unknown operator already got. Nothing in the repo depended on the conversion. Both refusals throw `FilterOperatorError` carrying `code: 'INVALID_FILTER'` / `httpStatus: 400`. The pre-existing unknown-operator throw was a bare `Error`, which `classifyLoadError` reads as a network fault — so a malformed filter told the user to check their connection (#3066), the one thing it was not. `filter-converter.test.ts`'s `$regex` case asserted the old behaviour and is rewritten to assert the refusal, keeping the `'^John'` example because it demonstrates the harm better than any prose. Verification: 9 new/changed tests; reverting the two source files fails 9 of them. Full suite 762 files / 8875 tests green; tsc clean; eslint 0 errors. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Started as the one-line cleanup left over from the framework-followup audit — deleting a dead
?? entry.nametolerance. Removing it properly meant consolidating the duplicated shape check that made it dead, and that turned up a live defect underneath.The main bug
ObjectStackAdaptertranslated object-form filter entries ([{ field, operator, value }, ...]) only at the top level of a$filter. The moment a list has both a stored view filter and a user filter, ListView.tsx:1064 builds:The head is the string
and, so the old top-level-only check called the whole thing "already AST" and sent the rules on untranslated. Both server answers to that are wrong — measured, not inferred:isFilterAST(above)false— a bare rule object is not an AST childparseFilterAST(above){"amount":{"$gt":1}}—stage = wonis simply gone{"$and":[{"stage":"won"},{"amount":{"$gt":1}}]}So on a server with objectstack#4121 the
isFilterASTgate turns it into a 400 and the list fails to load. Before that — or anywhereparseFilterASTis reached without the gate — the view's own condition is dropped silently and the list returns records the view exists to exclude.Translation is now recursive through
and/ornodes and legacy flat child arrays.Three related fixes in the same code
An untranslatable entry is now an error, not an omission. Entries that failed to translate were dropped, and dropping one conjunct of an
andreturns a superset of the rows asked for — dropping the last one sent nofilter=at all, so the whole table came back. That is the silent over-fetch the drivers stopped doing in objectstack#3948, one layer up.find()now throwsMalformedFilterErrorcarryingcode: 'INVALID_FILTER'/httpStatus: 400, which is exactly whatclassifyLoadErrorneeds to render "the filter is malformed" instead of "check your connection" (#3066). A rule with a blankfieldpassesViewFilterRuleSchema(z.string()admits''), so this is reachable from real stored metadata.A mixed array (
[{ field, operator, value }, ['amount', '>', 1]]) keeps both halves rather than dropping the tuple — that case was a lost condition, not a malformed one.The two
find()routes can no longer disagree. The "is this object form?" test existed twice — once intranslateFilterToAST, once inline inconvertQueryParams— and the copies had already drifted: the inline one omitted a!== nullguard, so$filter: [null]threw aTypeErroron the plain route while the same value was handled on the$expandroute. Same stored filter, different outcome, decided by whether the view happened to expand a lookup. One definition now serves both.The dead tolerance itself.
objectFilterEntryToASTreadentry.field ?? entry.namewhile the shape check keyed onfieldalone, so thenamehalf was unreachable from the commit that introduced it (4b93db4e6added both in one diff). The spec agrees it is not a real shape:ViewFilterRuleSchema.fieldis required, so aname-keyed rule cannot be saved as view metadata at all. Removing it is enforce-or-remove, not a narrowing — nothing in either repo produces that shape.Verification
find()routes.isFilterASTrather than a restated shape — the contract is "the server accepts this", so the test derives from the spec instead of duplicating it.index.tsfails 19 of the 44, including the real pre-existing crash (Cannot read properties of null (reading 'field')) and the silent drop (expected null to be an instance of Error).tscclean; eslint 0 errors (111 pre-existingno-explicit-anywarnings in this file, untouched).Note on the dead-code deletion
It has no failing-on-revert test, and cannot: it was dead, so removing it changes no behavior. The test that pins a
name-keyed entry as passed through pins the decision instead — re-widening the sniff to acceptnamewould fail it.Refs objectstack#3948, objectstack#4121, #2945
🤖 Generated with Claude Code