MOD-FILTER — per-field filters + server-side sort · MOD-PERCENT — the form layer didn't know percent existed - #109
Merged
Merged
Conversation
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
PR Summary by QodoModule registers: server-side per-field filters/sort + fix
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
55 rules 1. test_module_filters doesn't pop AEC_RBAC
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+23
to
+28
| import os | ||
|
|
||
| os.environ["DATABASE_URL"] = "sqlite:///./test_mod_filters.db" | ||
| os.environ["STORAGE_DIR"] = "./test_storage_mod_filters" | ||
| os.environ["AEC_TRUST_XUSER"] = "1" | ||
|
|
There was a problem hiding this comment.
1. test_module_filters doesn't pop aec_rbac 📜 Skill insight ≡ Correctness
The new DB-backed API test sets a local test database and storage paths but does not call
os.environ.pop("AEC_RBAC", None), so RBAC can remain enabled from the environment and make the
test non-deterministic across machines/CI. This violates the requirement that DB-backed tests
explicitly remove RBAC from the environment.
Agent Prompt
## Issue description
`services/api/test_module_filters.py` is a DB-backed API test but it does not call `os.environ.pop("AEC_RBAC", None)`.
## Issue Context
Per the compliance rule, DB-backed API tests must use `test_*-prefixed` local paths *and* pop `AEC_RBAC` to ensure RBAC does not leak into the test environment.
## Fix Focus Areas
- services/api/test_module_filters.py[23-32]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+87
to
+112
| #: Columns that live on the row rather than inside `data`, and may be filtered/sorted directly. | ||
| SYSTEM_COLUMNS = {"workflow_state", "created_at", "updated_at", "ref", "assignee", "ball_in_court"} | ||
|
|
||
| #: The operators a filter may use. `contains` is a case-insensitive substring; `in` takes a | ||
| #: comma-separated list; `empty`/`nonempty` ignore the value. | ||
| FILTER_OPS = {"eq", "ne", "gte", "lte", "contains", "in", "empty", "nonempty"} | ||
|
|
||
| _NUMERIC_FIELD_TYPES = {"number", "currency", "percent"} | ||
|
|
||
|
|
||
| def _resolve_field(mod: dict, name: str) -> dict: | ||
| """The DECLARED definition of `name`, or HTTP 400. Never trust a caller-supplied field name.""" | ||
| if name in SYSTEM_COLUMNS: | ||
| return {"name": name, "type": "text", "_system": True} | ||
| for f in mod.get("fields", []): | ||
| if f.get("name") == name: | ||
| return f | ||
| raise HTTPException(400, f"unknown filter/sort field {name!r} for this module") | ||
|
|
||
|
|
||
| def _field_expr(db: Session, t, mod: dict, name: str): | ||
| """A comparable SQL expression for a declared field — cast to float when the field is numeric.""" | ||
| f = _resolve_field(mod, name) | ||
| if f.get("_system"): | ||
| return t.c[name], f | ||
| expr = _json_text(db, t.c.data, name) |
There was a problem hiding this comment.
2. Bad system columns allowlist 🐞 Bug ☼ Reliability
SYSTEM_COLUMNS includes updated_at and ball_in_court, but _field_expr() treats these as real SQL columns (t.c[name]), which will raise at runtime because the module tables don’t define them. A request like ?sort=updated_at or ?f.updated_at.gte=... can therefore produce a server 500 instead of a controlled 400.
Agent Prompt
## Issue description
`SYSTEM_COLUMNS` contains names that are not actual columns in the module tables (`updated_at`, `ball_in_court`). Because `_field_expr()` returns `t.c[name]` for any name in `SYSTEM_COLUMNS`, these inputs can crash the request path instead of returning a 400.
## Issue Context
Module table columns are defined in `modules_registry._table()`. It uses `modified_at` (not `updated_at`) and does not include `ball_in_court`.
## Fix Focus Areas
- services/api/src/aec_api/modules_query.py[87-112]
- services/api/src/aec_api/modules_registry.py[55-81]
## Suggested fix
- Remove non-existent names from `SYSTEM_COLUMNS` (e.g., drop `ball_in_court`, change `updated_at` -> `modified_at` if you intend to expose it).
- Add a guard in `_field_expr()` for system columns: if `name` is not present in `t.c`, raise `HTTPException(400, ...)` rather than dereferencing `t.c[name]`.
- Consider keeping the allowlist derived from/validated against the table schema to prevent future drift.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
185
to
+193
| stmt = stmt.where(_search_filter(db, t, q)) | ||
| if _is_postgres(db) and (tsq := _pg_tsquery(q)): | ||
| stmt = stmt.order_by(func.ts_rank(_pg_document(t), func.to_tsquery("english", tsq)).desc()) | ||
| # MOD-FILTER — per-field narrowing, applied in SQL BEFORE the limit for the same reason `q` is: | ||
| # filtering a page after fetching it answers a different question than the user asked, and looks | ||
| # identical to the right answer. | ||
| if filters: | ||
| stmt = _apply_filters(db, stmt, t, REGISTRY.get(key) or {}, filters) | ||
| stmt, _explicit_sort = _apply_sort(db, stmt, t, REGISTRY.get(key) or {}, sort, sort_dir) |
There was a problem hiding this comment.
3. Search overrides explicit sort 🐞 Bug ≡ Correctness
On Postgres, list_records() applies ORDER BY ts_rank(...) DESC when q is present, then _apply_sort() adds another order_by which becomes secondary rather than primary. This means sort=<field> will not actually be the primary ordering for search results on Postgres (it only breaks ties).
Agent Prompt
## Issue description
When `q` is present on Postgres, `list_records()` adds a relevance `ORDER BY` first, and the new `_apply_sort()` appends the requested sort afterwards. That makes explicit sort secondary during search.
## Issue Context
SQLAlchemy Core’s `order_by()` appends additional ordering criteria by default; it doesn’t replace prior orderings unless cleared.
## Fix Focus Areas
- services/api/src/aec_api/modules_query.py[182-197]
## Suggested fix
Decide on intended behavior and implement explicitly:
- If explicit `sort` should override relevance ordering, clear existing ordering before applying sort (e.g., `stmt = stmt.order_by(None)` when `sort` is provided, then apply `_apply_sort()`), then append `created_at` tiebreak.
- If you intentionally want relevance-first even with explicit sort, document that contract and consider renaming parameter/behavior accordingly to avoid surprising callers.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+32
to
+33
| const MODULES_DIR = resolve(__dirname, "../../../../services/api/modules"); | ||
| const SCHEMA_PY = resolve(__dirname, "../../../../services/api/src/aec_api/module_schema.py"); |
There was a problem hiding this comment.
4. Esm test uses __dirname 🐞 Bug ☼ Reliability
fieldTypeCoverage.test.ts uses __dirname, but apps/web is configured as ESM (`"type": "module", TS module: "ESNext"), where __dirname is undefined. This can throw ReferenceError: __dirname is not defined` before any tests execute.
Agent Prompt
## Issue description
The new test file uses `__dirname` to locate repo files. In this package, Node runs in ESM mode, so `__dirname` is not available and the test module can crash on import.
## Issue Context
`apps/web/package.json` sets `type: module` and `apps/web/tsconfig.json` uses `module: ESNext`.
## Fix Focus Areas
- apps/web/src/portal/fieldTypeCoverage.test.ts[32-34]
- apps/web/package.json[1-8]
- apps/web/tsconfig.json[1-18]
## Suggested fix
Replace `__dirname` usage with an ESM-safe equivalent:
- `import { fileURLToPath } from "node:url";`
- `import { dirname, resolve } from "node:path";`
- `const here = dirname(fileURLToPath(import.meta.url));`
- `const MODULES_DIR = resolve(here, "../../../../services/api/modules");`
(and same for `SCHEMA_PY`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
… every register A register could be narrowed by exactly two things: the full-text `q` and `workflow_state`. On a twenty-field register that means no filtering by discipline, vendor, cost code or date range — and sorting ran in the BROWSER over whichever page had been fetched, so "sort by amount" on a 500-row register ordered 200 rows and presented the top of those as the largest values in the register. Nothing looked wrong; it was the wrong 200 rows. Filtering and ordering now happen in SQL, before the LIMIT. ?f.discipline=Structural&f.amount.gte=1000&f.amount.lte=5000&sort=amount&sort_dir=desc Operators: eq · ne · gte · lte · contains · in · empty · nonempty. Filters compose with `state` and `q`, are bounded at 12 per request, and `count_records` takes them too — a total that ignores a filter the list applied is the number a user trusts without checking. TWO PROPERTIES CARRY THE DESIGN. **A field name is never taken from the caller.** `_resolve_field` looks it up in the module's declared fields and raises 400 otherwise. This is not hygiene: `_json_text` interpolates the key into a SQLite JSON path, and its docstring already said the key must be module-defined — this is what enforces it. An unknown field, an unknown operator and a crafted name are all refused. Refusing rather than ignoring also matters on its own: a silently dropped filter returns MORE rows than were asked for, which reads as data rather than as a bug. **A number is compared as a number.** JSON extraction yields text, and text comparison puts 9 above 10 and sorts 1000 before 900 — a plausible wrong answer rather than an error. Numeric fields are cast to float; dates stay text because ISO-8601 already sorts correctly. THE TEST FOR THAT SECOND PROPERTY WAS WRONG FIRST, AND THE MUTATION CAUGHT IT. The fixtures posted ints, and the cast could be DELETED with every assertion still passing. SQLite types `json_extract` by what was stored: a JSON number returns `integer` and compares numerically without help, so an int fixture exercises a path the cast does not affect. A JSON *string* returns `text`, and `'9' >= 10` is TRUE. Strings are the NORMAL case — an HTML form posts a string, a CSV import posts a string, and `validate_record` only checks that a numeric field parses, it does not coerce. Fixtures re-posted as strings; the mutation now fails with `nine` leaking into `amount >= 10`, which is the bug the test exists for. A RANGE IS TWO CLAUSES ON ONE FIELD, so the client filter type is a LIST, not a map keyed by field. The first version was a map, where `amount.gte` and `amount.lte` collide and the second overwrites the first — a range silently collapsing to a single bound, i.e. a narrower result that looks entirely correct. The UI keys its controls `<field>__<op>` for the same reason. UI: a collapsed filter bar, one control per field chosen by type — a dropdown of the declared options for a select, from/to for a date, min/max for a number, a record picker for a reference (populated lazily, so six reference columns do not fire six fetches before anyone has expressed interest in filtering), and case-insensitive `contains` for everything else. The toggle carries the active count, because a page showing a subset while looking unfiltered is indistinguishable from missing data. Column-header sort now asks the server. The existing in-browser comparator is correct and stays, but only for a column the server cannot order — `serverSortField` maps the four pseudo-columns (ref, status -> workflow_state, assignee, title -> the module's title_field) and returns null for anything else rather than sending a name the server would rightly reject. Paged sorting keeps `created_at` as the last ordering key so equal values cannot swap between pages and show a row twice or never — the classic pagination hole. No new client methods: `moduleRecordsFiltered` gained parameters, so the api surface ratchet is untouched. No version files or CHANGELOG — the release lane batches those. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found by Massing Core reviewing #108, and it is a regression that PR made live. Three places asked `f.type === "number" || f.type === "currency"` and `percent` was in none of them. That was survivable while TWO fields in the whole system were typed `percent`; #108 converted 21 more across 16 registers — every retainage, fee, overhead and contingency percentage — and each site failed differently and quietly: 1. `EDITABLE` omitted it, so those 21 fields stopped being inline-editable overnight. A capability removed with no error and no visible cause. 2. The record form rendered `<input type="text">`: no numeric keypad on a phone, no step, no browser validation. 3. The save path skipped `Number(v)` and stored the STRING "5". `validate_record` calls `float(value)` so it passed, and every consumer coerces, so nothing ever failed — the column just accumulated a mixture of `5` and `"5"` depending on which form last touched the record. The quietest of the three, and the one that corrupts data rather than UX. That third one also feeds the bug MOD-FILTER's numeric cast exists to survive: numbers stored as text are exactly what makes a SQL comparison go lexicographic, where `'9' >= 10` is true. Fixed as ONE idea rather than three ternaries: `NUMERIC_FIELD_TYPES` is named once and referenced at all three sites, so the next numeric type is one edit instead of three that can be done two-thirds of the way — which is precisely how this happened. Also: a declared `unit` now renders BESIDE the input, not only in table cells. Half of what declaring a unit is for is the moment somebody is typing the number. Read structurally rather than via `ModuleField.unit`, because that property is declared on the sweep branch which is merged to main but cannot be merged into this one right now without git overwriting two files another session is holding dirty. WHY IT SURVIVED, WHICH IS THE PART WORTH KEEPING. It was invisible from both directions: `test_module_fields.py` reads config SHAPE and says nothing about whether a UI can render it, and the web suite never built a form containing a percent field. Two green suites, one type nothing handled. So the new gate is a COVERAGE test, not a DOM test. Rendering a form and asserting one input is `type="number"` would prove `percent` works and nothing about the next type; what failed here was completeness — a hand-maintained list with no check that it covered the types in use. `fieldTypeCoverage.test.ts` asserts the partition instead: every type the 133 shipped modules declare is either editable or deliberately read-only, no type is in both lists, every magnitude-holding type is treated as numeric, and nothing in `module_schema.FIELD_TYPES` is unrenderable. `READ_ONLY_FIELD_TYPES` carries a stated reason per entry, so "not editable" is a decision on the record rather than an omission nobody noticed. Mutation-checked: removing `percent` from `NUMERIC_FIELD_TYPES` — the exact pre-fix state — fails 4 of the 6 assertions, from four independent angles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ibuilder
force-pushed
the
feat/mod-filters-pr
branch
from
July 30, 2026 13:19
71ede28 to
ac97b83
Compare
Owner
Author
|
Merging. Rebase verified and the combined-tree run is done. Evidence, with its base SHA — integrated
One correction to my own method, since it is the same trap you flagged. My first manifest union Also worth stating: I earlier cited "web 1012/1012" from the main checkout. Both trees hold 79 test The browser-side sort finding remains the best thing in this PR: ordering the 200 rows that happened to
|
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.
Two commits. The second is a live regression fix for #108 and can be prioritised independently if you'd rather split them.
1.
MOD-FILTER— per-field filters and server-side sortA register could be narrowed by exactly two things: full-text
qandworkflow_state. On a twenty-field register that means no filtering by discipline, vendor, cost code or date range. And sorting ran in the browser over whichever page had been fetched — "sort by amount" on a 500-row register ordered 200 rows and presented the top of those as the largest values in the register. Nothing looked wrong; it was the wrong 200 rows.eq · ne · gte · lte · contains · in · empty · nonempty, composing withstateandq, bounded at 12 per request, applied in SQL before the LIMIT.count_recordstakes filters too — a total that ignores a filter the list applied is the number a user trusts without checking.Two properties carry the design.
A field name is never taken from the caller.
_resolve_fieldlooks it up in the module's declared fields and 400s otherwise. This isn't hygiene:_json_textinterpolates the key into a SQLite JSON path, and its docstring already said the key must be module-defined — this is what enforces it. Refusing rather than ignoring matters separately: a silently dropped filter returns more rows than were asked for, which reads as data rather than as a bug.A number is compared as a number. JSON extraction yields text, where 9 > 10 and 1000 sorts before 900.
The test for that second property was wrong first, and the mutation caught it. My fixtures posted ints, and the cast could be deleted with every assertion still passing. SQLite types
json_extractby what was stored: a JSON number returnsintegerand compares numerically unaided, so an int fixture never exercises the cast. A JSON string returnstext, and'9' >= 10is TRUE. Strings are the normal case — an HTML form posts a string, a CSV import posts a string, andvalidate_recordonly checks that a numeric field parses. Fixtures re-posted as strings; the mutation now fails withnineleaking intoamount >= 10.A range is two clauses on one field, so the client filter type is a list, not a map keyed by field. My first version was a map, where
amount.gteandamount.ltecollide and the second overwrites the first — a range silently collapsing to a single bound, i.e. a narrower result that looks entirely correct.UI: a collapsed filter bar, one control per type — options dropdown for a select, from/to for a date, min/max for a number, a lazily-populated record picker for a reference, case-insensitive
containsotherwise. The toggle carries the active count, because a page showing a subset while looking unfiltered is indistinguishable from missing data. Header sort now asks the server;serverSortFieldmaps the four pseudo-columns (ref,status→workflow_state,assignee,title→the module'stitle_field) and returns null otherwise rather than sending a name the server would rightly reject. Paged sort keepscreated_atas the tiebreak so equal values can't swap between pages and show a row twice or never.2.
MOD-PERCENT— a live regression #108 introducedFound by Massing Core reviewing #108. Three places asked
f.type === "number" || f.type === "currency"andpercentwas in none. Survivable while 2 fields were typedpercent; #108 converted 21 more — every retainage, fee, overhead and contingency percentage:EDITABLE<input type="text">— no numeric keypad, no step, no validation"5"The third is the one that matters.
validate_recordcallsfloat(value)so it passed, every consumer coerces, so nothing failed — the column just accumulated a mix of5and"5"depending on which form last touched the record. It also feeds the bug MOD-FILTER's cast exists to survive: numbers stored as text are what make SQL comparison go lexicographic. The two halves of this PR are the same defect from opposite ends.Fixed as one idea, not three ternaries:
NUMERIC_FIELD_TYPESnamed once, referenced at all three sites, so the next numeric type is one edit rather than three that can be done two-thirds of the way — which is exactly how this happened. A declaredunitnow also renders beside the input, not only in table cells.The gate is a coverage test, not a DOM test. Rendering a form and asserting one input is numeric would prove
percentworks and nothing about the next type. What failed was completeness — a hand-maintained list with no check it covered the types in use.fieldTypeCoverage.test.tsasserts the partition: every type the 133 modules declare is either editable or deliberately read-only, none is in both, every magnitude-holding type is numeric, and nothing inmodule_schema.FIELD_TYPESis unrenderable.READ_ONLY_FIELD_TYPEScarries a reason per entry so "not editable" is a decision on the record. Mutation-checked: removingpercentfails 4 of 6 assertions from four independent angles.Verification
test_esign, which passes solo — parallel-sqlite pollution, partly residue from three suite runs I killed. Caveat worth stating: that local run used the working-treerun_tests.py, which in a shared checkout also carried another session'stest_seal_identityregistration; this branch's committed manifest has 447.pdfVendortiming out at 20s under load; passes solo in 5.69s.tsc --noEmitclean for these files.git merge-treeagainst currentmainreports no conflicts.moduleRecordsFilteredgained parameters, so the api surface ratchet is untouched andsurface.test.tspasses.No version files or CHANGELOG — the release lane batches those.
Note on the sibling branch
origin/feat/mod-filterscontains these two commits plus785462c5, an unrelated seal-identity commit from another session that landed on my branch through the shared checkout and which my push published. Nothing is lost and I have not rewritten it — this PR is raised fromfeat/mod-filters-pr, which holds only the two commits above.785462c5should be cherry-picked tomainby its author.🤖 Generated with Claude Code