diff --git a/ui-react/src/graphql/generated/modeling.ts b/ui-react/src/graphql/generated/modeling.ts index 0c5a254..0abd068 100644 --- a/ui-react/src/graphql/generated/modeling.ts +++ b/ui-react/src/graphql/generated/modeling.ts @@ -1239,17 +1239,32 @@ export function generateModelingId(_type: 'problem_statement' | 'task' | 'thread // ─── Mutation: SetThreadModels ──────────────────────────────────────────────── /** - * Replace all model selections for a thread in a single transaction. - * Deletes existing thread_model rows then inserts the new selection. + * Apply a change of model selection for a thread in a single transaction. + * + * `$removedIds` are the `thread_model.id` values that are no longer selected. + * Rows that stay selected are not named here, so they keep their id and the + * dataset and parameter bindings that hang off it. + * + * The four child tables are deleted first, scoped to the removed rows only: + * every one of them references `thread_model.id` with `ON DELETE RESTRICT`, so + * without this the delete is refused for any thread that has been through the + * Datasets or Parameters step (monorepo#107). Hasura runs a mutation's root + * fields in order inside one transaction, so the ordering here is the ordering + * Postgres sees. */ export type SetThreadModelsMutationVariables = { threadId: string; + removedIds: string[]; models: Array<{ thread_id: string; modelcatalog_configuration_id: string }>; userid: string; notes?: string | null; }; export type SetThreadModelsMutation = { + delete_thread_model_execution_summary?: { affected_rows: number } | null; + delete_thread_model_execution?: { affected_rows: number } | null; + delete_thread_model_io?: { affected_rows: number } | null; + delete_thread_model_parameter?: { affected_rows: number } | null; delete_thread_model?: { affected_rows: number } | null; insert_thread_model?: { returning: Array<{ id: string; thread_id: string; modelcatalog_configuration_id?: string | null }>; @@ -1260,11 +1275,26 @@ export type SetThreadModelsMutation = { export const SetThreadModelsDocument = gql` mutation SetThreadModels( $threadId: String! + $removedIds: [uuid!]! $models: [thread_model_insert_input!]! $userid: String! $notes: String ) { - delete_thread_model(where: { thread_id: { _eq: $threadId } }) { + delete_thread_model_execution_summary( + where: { thread_model_id: { _in: $removedIds } } + ) { + affected_rows + } + delete_thread_model_execution(where: { thread_model_id: { _in: $removedIds } }) { + affected_rows + } + delete_thread_model_io(where: { thread_model_id: { _in: $removedIds } }) { + affected_rows + } + delete_thread_model_parameter(where: { thread_model_id: { _in: $removedIds } }) { + affected_rows + } + delete_thread_model(where: { id: { _in: $removedIds } }) { affected_rows } insert_thread_model(objects: $models) { diff --git a/ui-react/src/lib/__tests__/thread-models.test.ts b/ui-react/src/lib/__tests__/thread-models.test.ts new file mode 100644 index 0000000..a8674fc --- /dev/null +++ b/ui-react/src/lib/__tests__/thread-models.test.ts @@ -0,0 +1,59 @@ +import { describe, expect, it } from 'vitest'; +import { diffThreadModels } from '../thread-models'; + +const row = (id: string, configId: string | null) => ({ + id, + modelcatalog_configuration_id: configId, +}); + +describe('diffThreadModels', () => { + it('reports no change when the selection matches what is stored', () => { + const changes = diffThreadModels( + 't1', + [row('tm-a', 'cfgA'), row('tm-b', 'cfgB')], + ['cfgB', 'cfgA'], + ); + expect(changes).toEqual({ removedIds: [], added: [], unchanged: true }); + }); + + it('keeps a still-selected row instead of deleting and re-inserting it', () => { + const changes = diffThreadModels('t1', [row('tm-a', 'cfgA')], ['cfgA', 'cfgB']); + expect(changes.removedIds).toEqual([]); + expect(changes.added).toEqual([{ thread_id: 't1', modelcatalog_configuration_id: 'cfgB' }]); + expect(changes.unchanged).toBe(false); + }); + + it('removes only the rows whose configuration was deselected', () => { + const changes = diffThreadModels('t1', [row('tm-a', 'cfgA'), row('tm-b', 'cfgB')], ['cfgA']); + expect(changes.removedIds).toEqual(['tm-b']); + expect(changes.added).toEqual([]); + }); + + it('leaves a row with no configuration id alone', () => { + // 21 of TACC's 109 thread_model rows are in this state. The step cannot + // show them, and deleting one would hit the same ON DELETE RESTRICT wall + // this diff exists to avoid. + const changes = diffThreadModels('t1', [row('tm-legacy', null)], ['cfgA']); + expect(changes.removedIds).toEqual([]); + expect(changes.added).toEqual([{ thread_id: 't1', modelcatalog_configuration_id: 'cfgA' }]); + }); + + it('reports no change for a thread that holds only unselectable legacy rows', () => { + expect(diffThreadModels('t1', [row('tm-legacy', null)], []).unchanged).toBe(true); + }); + + it('inserts every selection for a thread that holds no rows yet', () => { + const changes = diffThreadModels('t1', [], ['cfgA', 'cfgB']); + expect(changes.added).toEqual([ + { thread_id: 't1', modelcatalog_configuration_id: 'cfgA' }, + { thread_id: 't1', modelcatalog_configuration_id: 'cfgB' }, + ]); + expect(changes.removedIds).toEqual([]); + }); + + it('does not re-insert a configuration that is stored twice', () => { + const changes = diffThreadModels('t1', [row('tm-a', 'cfgA'), row('tm-a2', 'cfgA')], ['cfgA']); + expect(changes.added).toEqual([]); + expect(changes.removedIds).toEqual([]); + }); +}); diff --git a/ui-react/src/lib/thread-models.ts b/ui-react/src/lib/thread-models.ts new file mode 100644 index 0000000..549b9bc --- /dev/null +++ b/ui-react/src/lib/thread-models.ts @@ -0,0 +1,71 @@ +/** + * Diffing the Models step's selection against the `thread_model` rows a thread + * already holds. + * + * The Models step used to save by deleting every `thread_model` row for the + * thread and re-inserting the selection. Four tables reference + * `thread_model.id` with `ON DELETE RESTRICT` — `thread_model_execution_summary`, + * `thread_model_execution`, `thread_model_io` and `thread_model_parameter` — so + * once the thread had been through the Datasets or Parameters step, the delete + * was refused and the step became a dead end (monorepo#107). + * + * The fix is to touch only what changed: keep the rows whose configuration is + * still selected, delete the rows whose configuration is not, insert the rows + * that are new. Keeping a row keeps its id, so the dataset and parameter + * bindings hanging off it survive a trip back to the Models step. + * + * A row with no `modelcatalog_configuration_id` is left alone. The step cannot + * display or select such a row, `threadModelFromGQL` skips it, and deleting it + * would hit the same RESTRICT wall on any legacy thread that holds runs. + * TACC's database holds 21 of them across 109 `thread_model` rows. + */ + +/** The `thread_model` fields this module needs; `GetThread` selects both. */ +export interface ExistingThreadModel { + id: string; + modelcatalog_configuration_id?: string | null; +} + +export interface ThreadModelInsert { + thread_id: string; + modelcatalog_configuration_id: string; +} + +export interface ThreadModelChanges { + /** `thread_model.id` values to delete, with their bindings and runs. */ + removedIds: string[]; + /** Rows to insert for newly selected configurations. */ + added: ThreadModelInsert[]; + /** True when the selection matches what is stored, so no write is needed. */ + unchanged: boolean; +} + +export function diffThreadModels( + threadId: string, + existing: readonly ExistingThreadModel[], + selectedConfigIds: Iterable, +): ThreadModelChanges { + const selected = new Set(selectedConfigIds); + const removedIds: string[] = []; + const stored = new Set(); + + for (const row of existing) { + const configId = row.modelcatalog_configuration_id; + if (!configId) continue; // unselectable legacy row — leave it in place + if (selected.has(configId)) stored.add(configId); + else removedIds.push(row.id); + } + + const added: ThreadModelInsert[] = []; + for (const configId of selected) { + if (!stored.has(configId)) { + added.push({ thread_id: threadId, modelcatalog_configuration_id: configId }); + } + } + + return { + removedIds, + added, + unchanged: removedIds.length === 0 && added.length === 0, + }; +} diff --git a/ui-react/src/pages/modeling/thread/MintModels.tsx b/ui-react/src/pages/modeling/thread/MintModels.tsx index cf3fc2e..803e2eb 100644 --- a/ui-react/src/pages/modeling/thread/MintModels.tsx +++ b/ui-react/src/pages/modeling/thread/MintModels.tsx @@ -22,6 +22,8 @@ import { useSetThreadModelsMutation, } from '@/graphql/generated/modeling'; import { useAuth } from '@/lib/auth/useAuth'; +import { diffThreadModels } from '@/lib/thread-models'; +import { useToast } from '@/components/ui/use-toast'; import { cn } from '@/lib/utils'; // ─── Types ───────────────────────────────────────────────────────────────────── @@ -209,6 +211,7 @@ interface MintModelsProps { export function MintModels({ thread, onContinue, onThreadUpdated }: MintModelsProps) { const { user } = useAuth(); + const { toast } = useToast(); const perm = getUserPermission(thread.permissions, thread.events, user?.username ?? null); const [editMode, setEditMode] = useState(() => { @@ -300,16 +303,21 @@ export function MintModels({ thread, onContinue, onThreadUpdated }: MintModelsPr async function handleSave() { if (!user?.username) return; + + const changes = diffThreadModels(thread.id, thread.thread_models ?? [], selectedIds); + if (changes.unchanged) { + setEditMode(false); + if (onContinue) onContinue(); + return; + } + setSaving(true); try { - const models = Array.from(selectedIds).map((cfgId) => ({ - thread_id: thread.id, - modelcatalog_configuration_id: cfgId, - })); await setThreadModels({ variables: { threadId: thread.id, - models, + removedIds: changes.removedIds, + models: changes.added, userid: user.username, notes: notes || null, }, @@ -317,6 +325,8 @@ export function MintModels({ thread, onContinue, onThreadUpdated }: MintModelsPr setEditMode(false); onThreadUpdated?.(); if (onContinue) onContinue(); + } catch (err) { + toast({ title: 'Save failed', description: String(err), variant: 'destructive' }); } finally { setSaving(false); } diff --git a/ui-react/src/pages/modeling/thread/wizard/ModelsStep.tsx b/ui-react/src/pages/modeling/thread/wizard/ModelsStep.tsx index 57882b6..6941503 100644 --- a/ui-react/src/pages/modeling/thread/wizard/ModelsStep.tsx +++ b/ui-react/src/pages/modeling/thread/wizard/ModelsStep.tsx @@ -13,6 +13,8 @@ import { useSetThreadModelsMutation, } from '@/graphql/generated/modeling'; import { useAuth } from '@/lib/auth/useAuth'; +import { diffThreadModels } from '@/lib/thread-models'; +import { useToast } from '@/components/ui/use-toast'; import { cn } from '@/lib/utils'; import { StepShell } from './StepShell'; import { FilteredByBanner } from './FilteredByBanner'; @@ -123,6 +125,7 @@ export function ModelsStep({ onEditIndicator, }: ModelsStepProps) { const { user } = useAuth(); + const { toast } = useToast(); const perm = getUserPermission(thread.permissions, thread.events, user?.username ?? null); const [searchText, setSearchText] = useState(''); @@ -185,17 +188,30 @@ export function ModelsStep({ async function handleContinue() { if (!user?.username || selectedIds.size === 0) return; + + const changes = diffThreadModels(thread.id, thread.thread_models ?? [], selectedIds); + // Nothing to write: walking back through the step must not touch the + // bindings the later steps have already stored against these rows. + if (changes.unchanged) { + onContinue(); + return; + } + setSaving(true); try { - const models = Array.from(selectedIds).map((cfgId) => ({ - thread_id: thread.id, - modelcatalog_configuration_id: cfgId, - })); await setThreadModels({ - variables: { threadId: thread.id, models, userid: user.username, notes: null }, + variables: { + threadId: thread.id, + removedIds: changes.removedIds, + models: changes.added, + userid: user.username, + notes: null, + }, }); onUpdated(); onContinue(); + } catch (err) { + toast({ title: 'Save failed', description: String(err), variant: 'destructive' }); } finally { setSaving(false); } diff --git a/ui-react/src/pages/modeling/thread/wizard/__tests__/ModelsStep.test.tsx b/ui-react/src/pages/modeling/thread/wizard/__tests__/ModelsStep.test.tsx index 0142f0f..75d2868 100644 --- a/ui-react/src/pages/modeling/thread/wizard/__tests__/ModelsStep.test.tsx +++ b/ui-react/src/pages/modeling/thread/wizard/__tests__/ModelsStep.test.tsx @@ -1,10 +1,19 @@ -import { describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { MockedResponse } from '@apollo/client/testing'; import userEvent from '@testing-library/user-event'; import { renderWithProviders, screen, waitFor } from '@/test/utils/render'; -import { GetModelTreeWithRegionsDocument, type Thread } from '@/graphql/generated/modeling'; +import { + GetModelTreeWithRegionsDocument, + SetThreadModelsDocument, + type Thread, +} from '@/graphql/generated/modeling'; import { ModelsStep } from '../ModelsStep'; +const toastSpy = vi.fn(); +vi.mock('@/components/ui/use-toast', () => ({ + useToast: () => ({ toast: toastSpy }), +})); + function makeThread(overrides: Partial = {}): Thread { return { __typename: 'thread', @@ -137,4 +146,148 @@ describe('ModelsStep', () => { await userEvent.click(screen.getByLabelText(/select PIHM Flood A/i)); await waitFor(() => expect(screen.getByTestId('step-continue')).toBeEnabled()); }); + + // ── Saving the selection (monorepo#107) ──────────────────────────────────── + // + // The four tables that reference thread_model.id are ON DELETE RESTRICT, so + // the step must never delete a row it means to keep. These assert the + // outgoing mutation variables, not the component's internal state. + + describe('saving', () => { + beforeEach(() => toastSpy.mockClear()); + + function threadWithModels() { + return makeThread({ + thread_models: [ + { + __typename: 'thread_model', + id: 'tm-a', + thread_id: 't1', + modelcatalog_configuration_id: 'cfgA', + }, + { + __typename: 'thread_model', + id: 'tm-b', + thread_id: 't1', + modelcatalog_configuration_id: 'cfgB', + }, + ], + }); + } + + function saveMock(sent: Record[]): MockedResponse { + return { + request: { query: SetThreadModelsDocument }, + maxUsageCount: Number.MAX_SAFE_INTEGER, + variableMatcher: (vars) => { + sent.push(vars); + return true; + }, + result: { + data: { + delete_thread_model_execution_summary: { affected_rows: 0 }, + delete_thread_model_execution: { affected_rows: 0 }, + delete_thread_model_io: { affected_rows: 0 }, + delete_thread_model_parameter: { affected_rows: 0 }, + delete_thread_model: { affected_rows: 1 }, + insert_thread_model: { returning: [] }, + insert_thread_provenance_one: { thread_id: 't1' }, + }, + }, + }; + } + + it('writes nothing when the selection is unchanged', async () => { + const sent: Record[] = []; + const onContinue = vi.fn(); + renderWithProviders( + , + { apolloMocks: [treeMock, saveMock(sent)] }, + ); + await screen.findByText('PIHM Flood A'); + await userEvent.click(screen.getByTestId('step-continue')); + + await waitFor(() => expect(onContinue).toHaveBeenCalled()); + expect(sent).toEqual([]); + }); + + it('deletes only the deselected row and re-inserts nothing', async () => { + const sent: Record[] = []; + renderWithProviders( + , + { apolloMocks: [treeMock, saveMock(sent)] }, + ); + await screen.findByText('Crop Model B'); + await userEvent.click(screen.getByLabelText(/select Crop Model B/i)); + await userEvent.click(screen.getByTestId('step-continue')); + + await waitFor(() => expect(sent).toHaveLength(1)); + expect(sent[0]).toMatchObject({ removedIds: ['tm-b'], models: [] }); + }); + + it('inserts only the newly selected row and deletes nothing', async () => { + const sent: Record[] = []; + const thread = makeThread({ + thread_models: [ + { + __typename: 'thread_model', + id: 'tm-a', + thread_id: 't1', + modelcatalog_configuration_id: 'cfgA', + }, + ], + }); + renderWithProviders( + , + { apolloMocks: [treeMock, saveMock(sent)] }, + ); + await screen.findByText('Crop Model B'); + await userEvent.click(screen.getByLabelText(/select Crop Model B/i)); + await userEvent.click(screen.getByTestId('step-continue')); + + await waitFor(() => expect(sent).toHaveLength(1)); + expect(sent[0]).toMatchObject({ + removedIds: [], + models: [{ thread_id: 't1', modelcatalog_configuration_id: 'cfgB' }], + }); + }); + + it('reports a rejected save instead of failing silently', async () => { + const onContinue = vi.fn(); + const failing: MockedResponse = { + request: { query: SetThreadModelsDocument }, + variableMatcher: () => true, + error: new Error('Foreign key violation'), + }; + renderWithProviders( + , + { apolloMocks: [treeMock, failing] }, + ); + await screen.findByText('Crop Model B'); + await userEvent.click(screen.getByLabelText(/select Crop Model B/i)); + await userEvent.click(screen.getByTestId('step-continue')); + + await waitFor(() => + expect(toastSpy).toHaveBeenCalledWith( + expect.objectContaining({ title: 'Save failed', variant: 'destructive' }), + ), + ); + expect(onContinue).not.toHaveBeenCalled(); + }); + }); });