Feature 2385 modelview factsheet hardening - #2420
Draft
jh-RLI wants to merge 12 commits into
Draft
Conversation
`TestViewsTestCase` offered a `get()` helper and nothing else, so no app in this repository had any POST coverage -- `modelview` least of all, where `processPost` has never been tested and a 192-field form is about to be rewritten. Add `post()` and `delete()` beside it, sharing the URL-reversal and login handling that `get()` already did inline. Both take an optional `expect_status`, because the interesting write-path assertions are refusals: a 403 from a permission check, or a redirect to login. `get()` keeps its old behaviour exactly -- `expect_status=None` still means "any success" -- and it has 94 call sites across 11 apps, so that is verified against the full suite rather than assumed. `delete()` sends a real DELETE, which is what an `hx-delete` button sends and what `curl -X DELETE` sends, so a hidden button is never evidence that an operation is refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The read path of the Model/Framework Factsheet app had no assertions worth the name: 37 lines of GET-only smoke test over a single factsheet. That is why the 2025-10-23 collapse -- a template membership test that made the list page O(tag edges squared) -- ran for ten months unnoticed. `modelview/tests/corpus.py` seeds the kinds of rows the later slices need to assert against: N factsheets of either sheet type over an M-tag vocabulary, the first K of them carrying a whole-tag-table snapshot, which is the damage today's tag editor does on every save. It is deliberately NOT the benchmark harness: that one seeds production's measured shape to answer "what is the curve" and is far too slow and too prod-calibrated for CI. Two things the factory had to get right, both encoded as comments because both are easy to get wrong: `Tag`'s primary key is a CharField, and the factsheet classes are multi-table inheritance over `BasicFactsheet`, so `bulk_create` is refused and the tag through-table's foreign key is named for the parent. A third is derived rather than listed -- the NOT NULL fields with no default (`model_name`, `contact_email`, and `data_api` on frameworks only) -- so adding another such field will not break every test here. The corpus needed a larger vocabulary than the ticket assumed. A corrupted factsheet attaches the whole tag table, and production's detection rule is `tags > 200`, so a 50-tag vocabulary cannot express a detectable corruption at all. The default is 260. The assertions themselves are the ones that stay true through every later slice: both list pages render against a corpus carrying corruption, the unfiltered CSV returns one row per factsheet in pk order, and the factory really does produce the corruption it claims to. One finding, pinned as a test rather than fixed here: the CSV download has a `tags` column in which every cell is the constant `dataedit.Tag.None`. Many-to-many fields DO have an `attname`, so the header code emits the column, but the row code hands the csv writer the related manager and writes its `__str__`. It is stable rather than random, which is why nobody noticed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every performance number on the Model Factsheet work came from this harness, and it lived only in the working tree -- so the numbers were reproducible only for whoever held the copy. That is exactly how the original bulk-upload harness was lost. It asks Django's own test runner for a throwaway database, seeds production's measured *shape* into it (305 factsheets, 817 tags, the measured bimodal tag distribution) and measures in-process. All it needs is a reachable Postgres, so nothing has to touch production -- which matters here more than usual: one request to /factsheets/models/ pins one of four mod_wsgi processes for ~400 s, a quarter of the platform for nearly seven minutes. Confirmed working against the project's own dev Postgres (docker/docker-compose.dev.yaml) after the move: at 25 factsheets it reports 177 queries, which is exactly the 7 x N + 2 the wayfinder map predicts. The README now points at that compose service instead of an ad-hoc container. Two unused imports removed so it passes the repo's flake8, and `benchmarks/results/` is gitignored -- the harnesses are committed, their output is not, which is what the bulk-upload harness already does implicitly. The package docstring came from that harness and claimed the whole package is standard-library-only and Django-independent. True there, false here, so it now says what each benchmark actually needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The REUSE check runs on every push and pull request, REUSE.toml has no annotation covering `benchmarks/`, and the harness .py files carry no SPDX headers -- so the previous commit would have failed CI. Fixed the way the sibling bulk-upload harness branch already does it: one `benchmarks/**` annotation with `precedence = "override"`. The addition is byte-identical to that branch's, so the two resolve trivially when both land. The two new test files carried `SPDX-FileCopyrightText: none`, which names no holder unlike every other file here, and is not REUSE's uppercase NONE sentinel either. They now carry the maintainer's line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Standards axis: * `_request_url` returned a URL and, as a side effect, logged the test client in or out -- a name that told you half of what it did. Split into `_url` and `_authenticate`, with one `_request` doing dispatch and the status assertion, so `get`/`post`/`delete` no longer repeat six lines each. * `_empty_value` mapped ten field types where a factsheet has three. It now handles those three and raises for anything else, because silently returning `""` for a numeric column fails later, in the database, with a message pointing at the wrong thing. Spec axis: * The read-path tests seeded 6 factsheets where the ticket asked for ~25. At 6 rows a per-factsheet query is not distinguishable from noise, which defeats the point of the corpus -- the later slices need one that can tell O(1) from O(N). Now 25 models and 12 frameworks, and the assertions count the corpus rather than repeating a literal. * `test_an_empty_submit_creates_no_factsheet` passed for the wrong reason: a request that never reached the view also creates no factsheet, and `expect_status=None` accepts a login redirect. It now asserts 200 and the template, so only the view running satisfies it. * The CSV header was asserted for models only, and only by `assertIn`. Asserted for both sheet types now, plus the property a consumer actually needs: two requests give the same columns in the same order, with no duplicates. * Added the first query-count bound in this repository, on the invariant that is already true: the CSV's query count does not grow with the number of factsheets. That is the map's decisive contrast made executable, and it proves the measurement surface works. The list page's own bound stays with T6, which owns taking it from 7 x N + 2 to 3. * Renamed the CSV tags-column test to say it is a characterisation test, and said in its docstring that it is the one assertion here expected to change. * `benchmarks/__init__.py` described a harness that is not on this branch. One defect the new coverage found in the factory itself: `BasicFactsheet.model_name` is UNIQUE and sits on the shared parent, so a factsheet name is unique across BOTH sheet types. Seeding twice in one test collided; names now come from a process-wide sequence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verified on production, anonymously: the Delete button and the Edit link were in the HTML of every factsheet detail page, wrapped in no permission check at all. So any registered account could destroy any of the 339 factsheets with one click -- irreversibly, and with no record of who did it, because this app keeps no history whatsoever. Not a crafted request: a rendered button. The tests written first confirmed it precisely -- a logged-in non-admin's DELETE returned **200** and the factsheet was gone. Gated in both places, which is the whole point: * the view refuses a non-admin with 403, before it looks the factsheet up, so it is not an existence oracle either; * the button is hidden in both detail templates. Either alone would be wrong. `hx-delete` issues a real DELETE request and so does `curl -X DELETE`, so hiding the button protects nothing; and a template check alone is what the last ten months already were. Uses the `is_admin` boolean the user model already carries, so there is no migration and no new model. Note "delete via the Django admin" was never an available fallback: the admin app is installed but no admin URL is routed, and this app registers nothing. Editing stays open to any logged-in account with no ownership check. That is now stated policy rather than an oversight -- community contribution is the intent -- and two tests guard against over-tightening it. Edit and Delete are also no longer rendered for anonymous visitors, for whom both are affordances that cannot work. A successful delete emits one structured log line carrying sheet type, pk, action, user and the number of tags that went with it. That last field is deliberate: with no audit model there is no undo, and the absence of exactly this record is what made the tag corruption unrepairable. A refusal emits nothing, since a refusal is not a write. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The list page took ~400 s on production. This is 86% of it, measured.
Two defects sat in the same ten lines of the sidebar. The view assembled the
filter list with `tags |= model.tags.all()` once per factsheet;
`QuerySet.__or__` OR-combines SQL rather than concatenating results, so 305
iterations built one statement carrying ~305 joins and returned a row per tag
ATTACHMENT. Measured locally at 20 factsheets: 831 checkboxes rendered where
260 distinct tags are in use. On production that is 12,156 checkboxes for 825
tags, 6.04 MB, 30% of the page.
But the seconds were not in the query or the bytes. They were in
`{% if t.pk in tags %}`, evaluated twice per rendered checkbox against a
QuerySet -- and Django 5.1 defines no `QuerySet.__contains__`, so `in` fell
back to `__iter__` and scanned the whole result cache each time. At
production's shape that is ~296 million model-equality calls, O(attachments
squared). It was also always False, comparing a pk against model instances,
so those thirty seconds computed a constant. Removed here; T5 reintroduces
the pre-check for real, driven by the URL.
Replaced by one scoped query. Scoping matters most for frameworks: unscoped,
that page offered 290 tags where 71 are in use, so 219 of its checkboxes
returned nothing when clicked -- which is why the defect hid, since the page
it broke worst is the one nobody measured. On models only 17 of 290 were
spurious.
Measured with the committed harness, same machine, production's shape
(305 factsheets, 12,156 edges):
before 32.555 s 2137 q 19.66 MB
after 4.560 s 2137 q 14.14 MB
Queries are unchanged, and that is the honest reading: the OR-loop was always
a single query. The remaining 2137 are the template emitting each row's tags
seven times over, which T6 takes to 3. So do not add this saving to T8's
either -- 95% of all tag attachments sit on the 23 corrupted factsheets, the
two overlap, and whichever lands first collects the credit.
The durable win is structural rather than either number: the list is now
O(distinct tags in use) instead of O(tag attachments), so it cannot grow with
corruption or with heavy tagging again. A test asserts exactly that.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Opening any factsheet for editing pre-checked every tag in the system (~825) and showed them all as attached, so saving attached the lot, one `Tag.objects.get()` at a time -- the "now somehow all tags are set" of #2385 and the "took veeery long on submit all" of #2381, in one mechanism. And every save ran `model.tags.clear()` unconditionally before re-adding, so any submit without the tag widget wiped them. `tags` was always a real field on the factsheet model, and both forms already mark it not-required -- the form was designed for this. The only thing that ever stopped it working is `processPost` flattening Django's `QueryDict` to a plain `dict`, which destroys `getlist` and makes every multi-value widget arrive empty. So: * `processPost` keeps the `QueryDict`. The dead no-op `else` branch, the dead `new`/`study` branch and the `key` parameter it needed all go (no factsheet model has a `study` field and the single caller never passed `key`); the `field_N` array contract is unchanged. * The edit view's `tags = Tag.objects.all()` context variable goes, and with it the bug. Checkbox state comes off the bound form, so the invalid-form path also comes back with the user's selection intact. * `save_m2m()` replaces the clear-then-add block. * The widget posts one multi-valued `name="tags"` of raw pks; `select_<pk>` survives as a DOM id only. The dead `table` hidden input, inherited from `dataedit`'s parallel implementation, goes. * "Remove all tags" and a live selected count on the Tags tab, both of which #2385 asked for. "Current tags" is rebuilt from the checkboxes, or removing all tags would leave the pills on screen beside a "no tags are attached" message. * One logfmt line per create and update, through a shared `log_factsheet_write` that the delete path now uses too. 18 tests in `modelview/tests/test_tag_write_path.py` -- the first POST coverage this app has ever had, and the first test `processPost` has ever had. 20 of their subtests fail against the pre-fix code. The round-trip test posts every one of the ~192 fields and reads them back, which is where the rewrite's risk sits: the array serialisation lost every entry past the ninth two months ago and shipped that way. Closes #2385, closes #2381. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two user-visible defects sharing one mechanism. The active tag filter lived
only in the DOM, so a filtered view could not be reloaded, bookmarked or
shared. And the "Download CSV" link silently returned a header-only file
whenever a filter was applied: the page built it as `?tags=select_<pk>`, the
CSV view filters on raw pks, so it matched nothing and reported no error --
which a user reads as "no matches" rather than as a bug. Verified live before
this: 0 rows where the correct value returns 59.
The query string becomes the canonical home of the filter, carrying raw pks.
`tag_filter_pks` parses it for both the sidebar's checkbox state and the CSV
download, so there is no second format left to drift out of step. The
template's membership test is against a SET of pks, never a queryset -- `in`
on a queryset has no `__contains__` to use and rescans the result cache once
per checkbox, which is the quadratic T4 removed.
The prefix is accepted only where the old links actually point -- the CSV
endpoint -- and not by a bare strip. A tag's pk is its normalised name, so one
can legitimately begin with it (`Tag.get_name_normalized("Select data")` is
`select_data`), and stripping blindly would filter on `data`: a wrong answer,
silently, which is the class of failure this removes. The raw value wins when
it names a real tag; only the leftovers are unprefixed.
Both defects lived in the page's JavaScript, where no test in this repo could
see them -- and so did two more found in review: the checkbox restored from
the URL rendered with no tick (the input is `display: none`; the tick comes
from a class `oep-tags.js` only toggles on click), and `history.replaceState`
rebuilt the whole query string, dropping every other parameter and the
fragment. So the filter's logic moves into `modelview/static/modelview/
tag_filter.js` and gets 24 vitest tests, one per defect among them. The
DataTables wiring and the payload stay in the template.
happy-dom rather than jsdom: jsdom's `html-encoding-sniffer` `require()`s an
ESM-only package and fails to load. The DOM environment is asked for per file
by docblock, so the existing node suites stay fast -- which is what the note
in `vitest.config.mjs` already anticipated.
Also folds the three near-identical checkbox scrapers this app's tests had
grown into one `modelview/tests/html.py`.
15 Django tests in `test_tag_filter_url.py`, 8 of them red before this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CONTRIBUTING asks for a changelog entry per change; the four slices on this branch had none. One entry each for the tag editor, the tag filter list, the filter URL plus the CSV download, and the delete restriction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The row payload was a JS object literal assembled in `modellist.html`, and its
`model_name` and `tags` keys sat INSIDE the loop over the view-property groups
-- so every row emitted them once per group: seven times for a model
factsheet, four for a framework. At production's shape that is 85,092 tag
objects for 12,156 attachments and 2,135 duplicate `model_name` lines. Duplicate
keys in an object literal silently overwrite, which is why it survived ten
months of review. With no prefetch anywhere in the view, `model.tags.all` in
that loop also meant `7 x N` queries: 2,138 on production.
`modelview/list_payload.py` builds the rows instead, fed by one
`prefetch_related("tags")`. A dict cannot have the duplicate-key bug -- that is
why a builder rather than the two-line template hoist that would also have hit
the query target. The list page now issues 3 queries whatever N is, for both
sheet types. Locally that buys no wall time (a query over a unix socket is
~0.1 ms), which is why the query count is a bound in its own right.
The payload reaches the page through `json_script`, not a JS literal, so a
factsheet whose text contains a closing script tag can no longer break out of
the element. That retires `stringify`'s hand-rolled JS-literal quoting -- but
NOT its HTML escaping: the table writes every cell through `innerHTML`, and
any logged-in account can edit any factsheet, so `_cell` keeps escaping and
two tests pin it. The 12-word truncation moves into the builder unchanged.
Every row carries its FULL tag array, never the five the renderer displays:
the client-side filter iterates the whole list to match the active selection.
The `type="module"` import of `tag_filter.js` and the DataTables search
extension that calls `rowMatchesTags` are untouched, and a test pins them,
because they live in the block this rewrites.
25 tests in `modelview/tests/test_list_payload.py`. The duplicate-key
assertion runs on the emitted JSON text through a `json.loads` hook, because a
parsed dict has already dropped the duplicates.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pager was always real DataTables paging -- but it paged a payload that had
already been queried, rendered and shipped whole: 305 factsheets x 171 fields,
a 20.1 MB page, to display eight columns. The list now sends the default
columns for every row (8 keys for a model, 5 for a framework) and fetches the
rest once from `modelview:list-payload`, which reuses the same builder over
the same queryset. 1.32 MB at production's shape, 0.16 MB once the corrupted
factsheets are repaired -- against 0.33 MB for a single 25-row page of the full
record, so the payload costs less than a page without paginating at all.
The fetch is triggered by the first column toggle OR the first search
keystroke. The search trigger is not optional: DataTables searches hidden
columns, so with eight columns loaded a search that used to find a model by
its citation text would return nothing and explain nothing. Measured against
the repo's DataTables driving the real rendered page: 0 hits before the fetch,
1 after. A visitor who neither toggles nor searches -- a crawler included --
never pays for it.
`defaultContent: ''` on every column definition, which is mandatory rather
than tidy: the table builds a row cache across every definition, visible or
not, so handing 173 of them rows with eight keys throws on the first draw.
Measured: 3 errors without it, 0 with.
Three things this had to get right, all of them silent when wrong:
* `draw(false)`, not `draw()`. In DataTables 1.10 a bare `draw()` resets
paging, so a reader on page 3 who toggled a column was thrown back to page
1. Reproduced, then fixed, then re-measured.
* Both views `order_by("pk")`. No factsheet model declares a `Meta.ordering`,
so the two payloads agreeing on row order was a Postgres accident -- and the
loader swaps the rows wholesale.
* Fetch, then reveal. Revealing first shows a column of empty cells for the
length of the request, which reads as broken rather than as loading.
`toggleColumns` and `bindSearchTrigger` live in the module with `createLoader`
because "once" is browser state that no Django test can see: 17 vitest tests,
including one that lets a failed fetch be retried rather than caching the
rejection. 14 Django tests bound what the server sends. `view_props()` gathers
the sheet-type ternary that was about to be copied a fourth time.
Co-Authored-By: Claude Opus 5 (1M context) <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.
Summary of the discussion
Describe the findings of the discussion in the issue or meeting.
Type of change (CHANGELOG.md)
Features
(#)
Changes
(#)
Bugs
(#)
Removed
(#)
Documentation updates
(#)
Workflow checklist
Automation
Closes #
PR-Assignee
CONTRIBUTING.md
CHANGELOG.md
mkdocs
Reviewer
Reviewer Guidelines