From ea5d162421adad3cec0db298d8363601df51237d Mon Sep 17 00:00:00 2001 From: Maximiliano Osorio Date: Sun, 9 Aug 2026 21:14:55 -0400 Subject: [PATCH] fix(ui-react): delete a problem statement, task or thread bottom-up The three delete mutations removed the provenance and permission rows first, copied from the Lit app. Those rows are what authorise the delete: the user role may remove one of these only while the row still carries a CREATE provenance event from that user, and every child on the tree is gated through the thread's provenance or permission. So the whole cascade matched 0 rows, and Hasura reports a filtered-out row as `null` with no errors key, which the app read as success. Deleting the row first does not work either -- those FKs were ON DELETE RESTRICT. graphql_engine now cascades them, so this deletes bottom-up and never touches provenance or permission. Two more faults fixed with it: - The port dropped Lit's dataslice_resource, dataslice and thread_data deletes. Nothing noticed while the mutation was a no-op, but since #104 started writing those rows a fixed delete would have hit an FK error. Order is load-bearing: dataslice is filtered through thread_data, so it goes first, which the deferred FK allows. - assertDeleted raises when _by_pk comes back null, so a delete the server declines can no longer render as a success toast. Requires graphql_engine PR #14. Fixes #99 --- .../__tests__/modeling-delete-order.test.ts | 101 ++++++++++++++++++ ui-react/src/graphql/generated/modeling.ts | 80 +++++++------- .../modeling/__tests__/assertDeleted.test.ts | 31 ++++++ ui-react/src/lib/modeling/assertDeleted.ts | 18 ++++ .../pages/modeling/MintProblemStatement.tsx | 9 +- .../pages/modeling/ProblemStatementsList.tsx | 4 +- 6 files changed, 204 insertions(+), 39 deletions(-) create mode 100644 ui-react/src/graphql/__tests__/modeling-delete-order.test.ts create mode 100644 ui-react/src/lib/modeling/__tests__/assertDeleted.test.ts create mode 100644 ui-react/src/lib/modeling/assertDeleted.ts diff --git a/ui-react/src/graphql/__tests__/modeling-delete-order.test.ts b/ui-react/src/graphql/__tests__/modeling-delete-order.test.ts new file mode 100644 index 00000000..ce2652a5 --- /dev/null +++ b/ui-react/src/graphql/__tests__/modeling-delete-order.test.ts @@ -0,0 +1,101 @@ +import { describe, it, expect } from 'vitest'; +import { print, type DocumentNode } from 'graphql'; +import { + DeleteProblemStatementDocument, + DeleteTaskDocument, + DeleteThreadDocument, +} from '@/graphql/generated/modeling'; + +/** + * Regression guard for #99. + * + * The `user` role may delete a problem statement, a task or a thread only while + * the row still carries a CREATE provenance event from that user. Every child + * delete on the tree is gated the same way, through the thread's provenance or + * permission rows. + * + * These documents used to delete the provenance and permission rows first — + * copied verbatim from the Lit app, which has the same bug. That revoked the + * permission for everything after it, so all eleven root fields matched 0 rows. + * Hasura returns `null` from `delete_*_by_pk` for a filtered-out row, with no + * `errors` key, so the app reported a successful delete over an intact tree. + * + * The other order does not work either: the provenance and permission FKs were + * `ON DELETE RESTRICT`, so deleting the row first was refused by Postgres. Both + * orders were proven to fail against the dev cluster's database. The fix is the + * migration `1771200017000_modeling_provenance_cascade_on_delete`, which makes + * those six FKs `ON DELETE CASCADE` so the client never deletes them at all. + */ + +const DOCUMENTS: Array<[string, DocumentNode]> = [ + ['DeleteProblemStatement', DeleteProblemStatementDocument], + ['DeleteTask', DeleteTaskDocument], + ['DeleteThread', DeleteThreadDocument], +]; + +/** Root field names, in document order. */ +function rootFields(doc: DocumentNode): string[] { + const names: string[] = []; + for (const def of doc.definitions) { + if (def.kind !== 'OperationDefinition') continue; + for (const sel of def.selectionSet.selections) { + if (sel.kind === 'Field') names.push(sel.name.value); + } + } + return names; +} + +describe.each(DOCUMENTS)('%s', (name, doc) => { + const fields = rootFields(doc); + + it('deletes no provenance row — those cascade off the row they authorise', () => { + expect(fields.filter((f) => f.endsWith('_provenance'))).toEqual([]); + }); + + it('deletes no permission row — those cascade too', () => { + expect(fields.filter((f) => f.endsWith('_permission'))).toEqual([]); + }); + + it('ends on the _by_pk delete, so the caller can tell 0 rows from 1', () => { + expect(fields.at(-1)).toMatch(/^delete_[a-z_]+_by_pk$/); + }); + + it('deletes dataslice before thread_data, which its permission filter reads', () => { + // `dataslice`'s user-role delete filter walks dataslice -> thread_data -> + // thread. Removing thread_data first would leave every dataslice + // unmatchable, and thread_data.dataslice_id would then block the delete. + const dataslice = fields.indexOf('delete_dataslice'); + const threadData = fields.indexOf('delete_thread_data'); + expect(dataslice).toBeGreaterThanOrEqual(0); + expect(threadData).toBeGreaterThan(dataslice); + }); + + it('deletes dataslice_resource before dataslice, which it references', () => { + const resource = fields.indexOf('delete_dataslice_resource'); + expect(resource).toBeGreaterThanOrEqual(0); + expect(fields.indexOf('delete_dataslice')).toBeGreaterThan(resource); + }); + + it('deletes thread_model last of the thread_model_* rows that reference it', () => { + const threadModel = fields.indexOf('delete_thread_model'); + expect(threadModel).toBeGreaterThanOrEqual(0); + for (const dependant of fields.filter((f) => f.startsWith('delete_thread_model_'))) { + expect(fields.indexOf(dependant)).toBeLessThan(threadModel); + } + }); + + it(`names ${name} so the operation name stays stable for the caller`, () => { + expect(print(doc)).toContain(`mutation ${name}(`); + }); +}); + +describe('DeleteProblemStatement', () => { + const fields = rootFields(DeleteProblemStatementDocument); + + it('deletes threads, then tasks, then the problem statement', () => { + expect(fields.indexOf('delete_thread')).toBeLessThan(fields.indexOf('delete_task')); + expect(fields.indexOf('delete_task')).toBeLessThan( + fields.indexOf('delete_problem_statement_by_pk'), + ); + }); +}); diff --git a/ui-react/src/graphql/generated/modeling.ts b/ui-react/src/graphql/generated/modeling.ts index a0fc7d4f..0c5a254c 100644 --- a/ui-react/src/graphql/generated/modeling.ts +++ b/ui-react/src/graphql/generated/modeling.ts @@ -567,26 +567,23 @@ export type DeleteProblemStatementMutation = { delete_problem_statement_by_pk?: { id: string } | null; }; +/** + * Deletes bottom-up, and never deletes a provenance or permission row. + * + * Every `user`-role delete on this tree is authorised by the subject's own + * CREATE provenance event (or a permission row). Removing those first — which + * this document used to do, copied from the Lit app — revoked the permission + * for everything that came after, so the whole cascade silently matched 0 rows + * (#99). Provenance and permission rows now go by ON DELETE CASCADE, once the + * row they authorise is gone. + * + * Order is load-bearing: `dataslice` is filtered through `thread_data`, so it + * must go before `thread_data` does. `thread_data.dataslice_id` is DEFERRABLE + * INITIALLY DEFERRED, so that order is legal inside the one transaction Hasura + * runs these root fields in. + */ export const DeleteProblemStatementDocument = gql` mutation DeleteProblemStatement($id: String!) { - delete_thread_permission( - where: { thread: { task: { problem_statement_id: { _eq: $id } } } } - ) { affected_rows } - delete_thread_provenance( - where: { thread: { task: { problem_statement_id: { _eq: $id } } } } - ) { affected_rows } - delete_task_permission( - where: { task: { problem_statement_id: { _eq: $id } } } - ) { affected_rows } - delete_task_provenance( - where: { task: { problem_statement_id: { _eq: $id } } } - ) { affected_rows } - delete_problem_statement_permission( - where: { problem_statement_id: { _eq: $id } } - ) { affected_rows } - delete_problem_statement_provenance( - where: { problem_statement_id: { _eq: $id } } - ) { affected_rows } delete_thread_model_execution_summary( where: { thread_model: { thread: { task: { problem_statement_id: { _eq: $id } } } } } ) { affected_rows } @@ -599,6 +596,15 @@ export const DeleteProblemStatementDocument = gql` delete_thread_model_parameter( where: { thread_model: { thread: { task: { problem_statement_id: { _eq: $id } } } } } ) { affected_rows } + delete_dataslice_resource( + where: { dataslice: { thread_data: { thread: { task: { problem_statement_id: { _eq: $id } } } } } } + ) { affected_rows } + delete_dataslice( + where: { thread_data: { thread: { task: { problem_statement_id: { _eq: $id } } } } } + ) { affected_rows } + delete_thread_data( + where: { thread: { task: { problem_statement_id: { _eq: $id } } } } + ) { affected_rows } delete_thread_model( where: { thread: { task: { problem_statement_id: { _eq: $id } } } } ) { affected_rows } @@ -750,20 +756,9 @@ export type DeleteTaskMutation = { delete_task_by_pk?: { id: string } | null; }; +/** Bottom-up, provenance and permission left to cascade. See DeleteProblemStatementDocument. */ export const DeleteTaskDocument = gql` mutation DeleteTask($id: String!) { - delete_thread_permission( - where: { thread: { task_id: { _eq: $id } } } - ) { affected_rows } - delete_thread_provenance( - where: { thread: { task_id: { _eq: $id } } } - ) { affected_rows } - delete_task_permission(where: { task_id: { _eq: $id } }) { - affected_rows - } - delete_task_provenance(where: { task_id: { _eq: $id } }) { - affected_rows - } delete_thread_model_execution_summary( where: { thread_model: { thread: { task_id: { _eq: $id } } } } ) { affected_rows } @@ -776,6 +771,15 @@ export const DeleteTaskDocument = gql` delete_thread_model_parameter( where: { thread_model: { thread: { task_id: { _eq: $id } } } } ) { affected_rows } + delete_dataslice_resource( + where: { dataslice: { thread_data: { thread: { task_id: { _eq: $id } } } } } + ) { affected_rows } + delete_dataslice( + where: { thread_data: { thread: { task_id: { _eq: $id } } } } + ) { affected_rows } + delete_thread_data( + where: { thread: { task_id: { _eq: $id } } } + ) { affected_rows } delete_thread_model( where: { thread: { task_id: { _eq: $id } } } ) { affected_rows } @@ -908,14 +912,9 @@ export type DeleteThreadMutation = { delete_thread_by_pk?: { id: string } | null; }; +/** Bottom-up, provenance and permission left to cascade. See DeleteProblemStatementDocument. */ export const DeleteThreadDocument = gql` mutation DeleteThread($id: String!) { - delete_thread_permission(where: { thread_id: { _eq: $id } }) { - affected_rows - } - delete_thread_provenance(where: { thread_id: { _eq: $id } }) { - affected_rows - } delete_thread_model_execution_summary( where: { thread_model: { thread_id: { _eq: $id } } } ) { affected_rows } @@ -928,6 +927,15 @@ export const DeleteThreadDocument = gql` delete_thread_model_parameter( where: { thread_model: { thread_id: { _eq: $id } } } ) { affected_rows } + delete_dataslice_resource( + where: { dataslice: { thread_data: { thread_id: { _eq: $id } } } } + ) { affected_rows } + delete_dataslice(where: { thread_data: { thread_id: { _eq: $id } } }) { + affected_rows + } + delete_thread_data(where: { thread_id: { _eq: $id } }) { + affected_rows + } delete_thread_model(where: { thread_id: { _eq: $id } }) { affected_rows } diff --git a/ui-react/src/lib/modeling/__tests__/assertDeleted.test.ts b/ui-react/src/lib/modeling/__tests__/assertDeleted.test.ts new file mode 100644 index 00000000..cdb6f975 --- /dev/null +++ b/ui-react/src/lib/modeling/__tests__/assertDeleted.test.ts @@ -0,0 +1,31 @@ +import { describe, it, expect } from 'vitest'; +import { assertDeleted } from '@/lib/modeling/assertDeleted'; + +/** + * #99: Hasura answers `delete_*_by_pk` with `null` when the row exists but the + * role's delete permission filters it out, and sends no `errors` key with it. + * That is what let a failed delete render as "Problem statement deleted". + */ +describe('assertDeleted', () => { + it('throws on null — a filtered-out row, which Hasura reports as success', () => { + expect(() => assertDeleted(null, 'Problem statement')).toThrow(/was not deleted/); + }); + + it('throws on undefined — the field missing from a partial response', () => { + expect(() => assertDeleted(undefined, 'Task')).toThrow(/was not deleted/); + }); + + it('names the subject, so the toast says what survived', () => { + expect(() => assertDeleted(null, 'Sub-task')).toThrow(/^Sub-task /); + }); + + it('returns the row when one was deleted', () => { + const row = { id: 'abc' }; + expect(assertDeleted(row, 'Task')).toBe(row); + }); + + it('accepts a falsy row that is not null — 0 rows is the only failure', () => { + expect(assertDeleted(0, 'Task')).toBe(0); + expect(assertDeleted('', 'Task')).toBe(''); + }); +}); diff --git a/ui-react/src/lib/modeling/assertDeleted.ts b/ui-react/src/lib/modeling/assertDeleted.ts new file mode 100644 index 00000000..28492f8a --- /dev/null +++ b/ui-react/src/lib/modeling/assertDeleted.ts @@ -0,0 +1,18 @@ +/** + * Hasura answers `delete__by_pk` with `null` when the row exists but the + * role's delete permission filters it out. That is a 200 with no `errors` key, + * so Apollo resolves and the caller reports success while the row is still + * there. Issue #99 stayed invisible for exactly this reason. + * + * Call this on the `_by_pk` field of every delete so a filtered-out row raises + * instead of passing. + */ +export function assertDeleted(row: T | null | undefined, subject: string): T { + if (row === null || row === undefined) { + throw new Error( + `${subject} was not deleted. The server accepted the request and removed no row, ` + + `which usually means your account may not delete it.`, + ); + } + return row; +} diff --git a/ui-react/src/pages/modeling/MintProblemStatement.tsx b/ui-react/src/pages/modeling/MintProblemStatement.tsx index a0764977..c2957a03 100644 --- a/ui-react/src/pages/modeling/MintProblemStatement.tsx +++ b/ui-react/src/pages/modeling/MintProblemStatement.tsx @@ -39,6 +39,7 @@ import { useToast } from '@/components/ui/use-toast'; import { cn } from '@/lib/utils'; import { useAuth } from '@/lib/auth/useAuth'; import { provisionTask } from '@/lib/modeling/provisionTask'; +import { assertDeleted } from '@/lib/modeling/assertDeleted'; import { EmptyState } from '@/components/common/EmptyState'; import { @@ -279,7 +280,8 @@ export function MintProblemStatement() { async function handleDeleteTask() { if (!deleteTaskTarget) return; try { - await deleteTask({ variables: { id: deleteTaskTarget.id } }); + const { data: deleted } = await deleteTask({ variables: { id: deleteTaskTarget.id } }); + assertDeleted(deleted?.delete_task_by_pk, 'Task'); toast({ title: 'Task deleted' }); if (selectedTaskId === deleteTaskTarget.id) { setSelectedTaskId(null); @@ -369,7 +371,10 @@ export function MintProblemStatement() { async function handleDeleteThread() { if (!deleteThreadTarget) return; try { - await deleteThread({ variables: { id: deleteThreadTarget.id } }); + const { data: deleted } = await deleteThread({ + variables: { id: deleteThreadTarget.id }, + }); + assertDeleted(deleted?.delete_thread_by_pk, 'Sub-task'); toast({ title: 'Sub-task deleted' }); if (selectedThreadId === deleteThreadTarget.id) { setSelectedThreadId(null); diff --git a/ui-react/src/pages/modeling/ProblemStatementsList.tsx b/ui-react/src/pages/modeling/ProblemStatementsList.tsx index 67d173d8..416ced7a 100644 --- a/ui-react/src/pages/modeling/ProblemStatementsList.tsx +++ b/ui-react/src/pages/modeling/ProblemStatementsList.tsx @@ -55,6 +55,7 @@ import { type ProblemStatement, } from '@/graphql/generated/modeling'; import { provisionTask } from '@/lib/modeling/provisionTask'; +import { assertDeleted } from '@/lib/modeling/assertDeleted'; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -295,7 +296,8 @@ export function ProblemStatementsList({ regionId = 'DEFAULT' }: ProblemStatement async function handleDelete() { if (!deleteTarget) return; try { - await deletePS({ variables: { id: deleteTarget.id } }); + const { data: deleted } = await deletePS({ variables: { id: deleteTarget.id } }); + assertDeleted(deleted?.delete_problem_statement_by_pk, 'Problem statement'); toast({ title: 'Problem statement deleted' }); await refetch(); } catch (err) {