From 7886840362b8b8a8dead4a0e64832b6f8a349f2c Mon Sep 17 00:00:00 2001 From: Ben Stull Date: Sun, 7 Jun 2026 21:03:29 -0700 Subject: [PATCH] fix(slice5): validate raw metadata input + prune stale bulk selections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review follow-ups: - Validate the raw submitted value before apply_values coerces it, in both the single-edit and bulk endpoints — a scalar set onto a tags field now rejects (422 / rejected) instead of silently char-splitting into a list. - Catalog prunes bulk selections to the currently-visible (filtered) entries after each list fetch, so the bulk bar can't act on rows the user has filtered out of view. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/app/api_metadata.py | 15 +++++---- backend/tests/test_metadata_bulk_endpoint.py | 33 ++++++++++++++++++++ frontend/src/components/Catalog.jsx | 11 ++++++- frontend/src/components/Catalog.test.jsx | 24 ++++++++++++++ 4 files changed, 76 insertions(+), 7 deletions(-) diff --git a/backend/app/api_metadata.py b/backend/app/api_metadata.py index 7cc5759..ef8d565 100644 --- a/backend/app/api_metadata.py +++ b/backend/app/api_metadata.py @@ -89,11 +89,13 @@ def make_router(config: Config, gitea: Gitea, bot: Bot) -> APIRouter: if st is None: raise HTTPException(404, f"{md_path} not found") - new_entry = metadata_mod.apply_values(st.entry, body.values) - problems = metadata_schema.validate( - metadata_mod.metadata_dict(new_entry), fields) + # Validate the *raw* submitted values (INV-4): catch a type mismatch + # before `apply_values` coerces it — e.g. a scalar handed to a `tags` + # field would otherwise char-split into a valid-looking list. + problems = metadata_schema.validate(body.values, fields) if problems: raise HTTPException(422, {"problems": [p.as_dict() for p in problems]}) + new_entry = metadata_mod.apply_values(st.entry, body.values) files = metadata_mod.write_entry_files(md_path, new_entry, st) try: @@ -153,14 +155,15 @@ def make_router(config: Config, gitea: Gitea, bot: Bot) -> APIRouter: rejected.append({"slug": slug, "reason": "not found"}) continue new_value = _apply_op(st.entry, body.op, body.field, body.value) - new_entry = metadata_mod.apply_values(st.entry, {body.field: new_value}) - problems = metadata_schema.validate( - metadata_mod.metadata_dict(new_entry), fields) + # Validate the *raw* new value before coercion (see edit_meta) so a + # scalar `set` onto a tags field is rejected, not char-split. + problems = metadata_schema.validate({body.field: new_value}, fields) if problems: rejected.append({"slug": slug, "reason": "; ".join(p.message for p in problems)}) continue applied.append(slug) + new_entry = metadata_mod.apply_values(st.entry, {body.field: new_value}) if metadata_mod.metadata_dict(new_entry) != metadata_mod.metadata_dict(st.entry): all_ops.extend(metadata_mod.write_entry_files(md_path, new_entry, st)) diff --git a/backend/tests/test_metadata_bulk_endpoint.py b/backend/tests/test_metadata_bulk_endpoint.py index 7847eb6..d46a92d 100644 --- a/backend/tests/test_metadata_bulk_endpoint.py +++ b/backend/tests/test_metadata_bulk_endpoint.py @@ -119,6 +119,39 @@ def test_bulk_remove_tag(app_with_fake_gitea): assert _sidecar(fake, "a")["tags"] == ["y"] +def test_bulk_set_scalar_on_tags_rejected(app_with_fake_gitea): + # A scalar `set` onto a tags field must reject, not char-split into a list. + app, fake = app_with_fake_gitea + with TestClient(app) as client: + _set_fields({"tags": {"type": "tags"}}) + _seed_legacy(fake, "a", tags=["x"]) + _refresh() + _login_owner(client) + r = client.post(f"{BASE}/meta/bulk", + json={"slugs": ["a"], "op": "set", + "field": "tags", "value": "checkout"}) + assert r.status_code == 200, r.text + assert r.json()["applied"] == [] + assert len(r.json()["rejected"]) == 1 + assert r.json()["committed"] is False + assert not _has_sidecar(fake, "a") + + +def test_bulk_set_list_on_tags_ok(app_with_fake_gitea): + app, fake = app_with_fake_gitea + with TestClient(app) as client: + _set_fields({"tags": {"type": "tags"}}) + _seed_legacy(fake, "a", tags=["x"]) + _refresh() + _login_owner(client) + r = client.post(f"{BASE}/meta/bulk", + json={"slugs": ["a"], "op": "set", + "field": "tags", "value": ["x", "y"]}) + assert r.status_code == 200, r.text + assert r.json()["applied"] == ["a"] + assert _sidecar(fake, "a")["tags"] == ["x", "y"] + + def test_bulk_add_remove_requires_tags_field(app_with_fake_gitea): app, fake = app_with_fake_gitea with TestClient(app) as client: diff --git a/frontend/src/components/Catalog.jsx b/frontend/src/components/Catalog.jsx index 9e21d4f..4dee847 100644 --- a/frontend/src/components/Catalog.jsx +++ b/frontend/src/components/Catalog.jsx @@ -93,7 +93,16 @@ export default function Catalog({ viewer, onProposeRFC, version }) { Object.entries(selections).map(([f, set]) => [f, [...set]]) ) listRFCs(pid, cid, { selections: selObj, malformed: malformedOnly }) - .then(d => { setRfcs(d.items); setFacets(d.facets || {}) }) + .then(d => { + setRfcs(d.items); setFacets(d.facets || {}) + // §22.4a SLICE-5: drop any bulk selections that the new (filtered) + // list no longer contains, so the bar can't act on hidden rows. + const visible = new Set(d.items.map(it => it.slug)) + setSelected(prev => { + const kept = [...prev].filter(s => visible.has(s)) + return kept.length === prev.size ? prev : new Set(kept) + }) + }) .catch(() => { setRfcs([]); setFacets({}) }) .finally(() => setLoading(false)) }, [version, pid, cid, selections, malformedOnly]) diff --git a/frontend/src/components/Catalog.test.jsx b/frontend/src/components/Catalog.test.jsx index b04a51a..373961b 100644 --- a/frontend/src/components/Catalog.test.jsx +++ b/frontend/src/components/Catalog.test.jsx @@ -90,6 +90,30 @@ describe('Catalog faceted pane', () => { }) }) + it('drops a selection when a filter change hides the row', async () => { + getCollection.mockResolvedValue({ + fields: { priority: { type: 'enum', values: ['P0', 'P1'] } }, + viewer: { can_contribute: true }, entry_noun: 'RFC', + }) + // Content-driven: unfiltered → row "a"; once the P0 facet is selected the + // server response omits "a" (drives the prune). Robust to extra mount-time + // fetches the collection effect triggers. + listRFCs.mockImplementation((p, c, opts) => Promise.resolve({ + items: opts?.selections?.priority?.includes('P0') + ? [{ slug: 'b', title: 'B', state: 'active', tags: [], starred_by_me: false }] + : [{ slug: 'a', title: 'A', state: 'active', tags: [], starred_by_me: false }], + facets: { priority: { P0: 1 }, state: { active: 1 } }, + })) + const { container } = renderCatalog() + fireEvent.click(await screen.findByLabelText(/select A/i)) + expect(await screen.findByText(/1 selected/i)).toBeInTheDocument() + // Toggle the P0 facet checkbox → re-fetch returns a list without "a". + fireEvent.click(container.querySelector('.facet-group input[type="checkbox"]')) + await waitFor(() => { + expect(screen.queryByText(/1 selected/i)).not.toBeInTheDocument() + }) + }) + it('does not offer row selection for a non-contributor', async () => { getCollection.mockResolvedValue({ fields: { priority: { type: 'enum', values: ['P0'] } },