diff --git a/client/package-lock.json b/client/package-lock.json index b19df44a..e5fdfe3f 100644 --- a/client/package-lock.json +++ b/client/package-lock.json @@ -1,12 +1,12 @@ { "name": "client", - "version": "0.27.3", + "version": "0.28.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "client", - "version": "0.27.3", + "version": "0.28.0", "dependencies": { "bootstrap": "4.6.2", "bootstrap-vue": "2.23.1", diff --git a/client/package.json b/client/package.json index f7960e9e..516e05c9 100644 --- a/client/package.json +++ b/client/package.json @@ -1,6 +1,6 @@ { "name": "client", - "version": "0.27.3", + "version": "0.28.0", "description": "DigiScript front end", "author": "DreamTeamProd", "private": true, diff --git a/client/src/store/modules/scriptConfig.js b/client/src/store/modules/scriptConfig.js index 0ca3adc5..4bcdf303 100644 --- a/client/src/store/modules/scriptConfig.js +++ b/client/src/store/modules/scriptConfig.js @@ -4,6 +4,40 @@ import log from 'loglevel'; import { makeURL } from '@/js/utils'; +/** + * Computes the page status object (added/updated/deleted/inserted) to send to the PATCH endpoint. + * + * deepDiff.added fires on a line index whenever *any* nested property is new — including a new + * element in line_parts. Only lines with id == null are truly new; lines with an existing id that + * have nested additions must be treated as updates instead. + * + * :param actualScriptPage: The current saved page from the store (will be cloned internally). + * :param tmpScriptPage: The edited in-progress page. + * :param deletedLines: Array of line indices marked for deletion. + * :param insertedLines: Array of line indices that were inserted mid-page. + * :returns: { added, updated, deleted, inserted } + */ +export function computePageStatus(actualScriptPage, tmpScriptPage, deletedLines, insertedLines) { + const augmented = JSON.parse(JSON.stringify(actualScriptPage)); + JSON.parse(JSON.stringify(insertedLines)) + .sort((a, b) => a - b) + .forEach((lineIndex) => { + augmented.splice(lineIndex, 0, JSON.parse(JSON.stringify(tmpScriptPage[lineIndex]))); + }); + + const deepDiff = detailedDiff(augmented, tmpScriptPage); + const addedIndices = Object.keys(deepDiff.added).map((x) => parseInt(x, 10)); + return { + added: addedIndices.filter((idx) => tmpScriptPage[idx]?.id == null), + updated: [ + ...Object.keys(deepDiff.updated).map((x) => parseInt(x, 10)), + ...addedIndices.filter((idx) => tmpScriptPage[idx]?.id != null), + ], + deleted: [...deletedLines], + inserted: [...insertedLines], + }; +} + export default { state: { tmpScript: {}, @@ -137,30 +171,13 @@ export default { return true; }, async SAVE_CHANGED_PAGE(context, pageNo) { - let actualScriptPage = context.getters.GET_SCRIPT_PAGE(pageNo); const tmpScriptPage = context.getters.TMP_SCRIPT[pageNo.toString()]; - - // Need to augment the actual script page to include the inserted pages, this is a hack, - // but it will allow all the other pages to show as not edited if the really haven't been - // changed - actualScriptPage = JSON.parse(JSON.stringify(actualScriptPage)); - JSON.parse(JSON.stringify(context.getters.INSERTED_LINES(pageNo))) - .sort((a, b) => a - b) - .forEach((lineIndex) => { - actualScriptPage.splice( - lineIndex, - 0, - JSON.parse(JSON.stringify(tmpScriptPage[lineIndex])) - ); - }); - - const deepDiff = detailedDiff(actualScriptPage, tmpScriptPage); - const pageStatus = { - added: Object.keys(deepDiff.added).map((x) => parseInt(x, 10)), - updated: Object.keys(deepDiff.updated).map((x) => parseInt(x, 10)), - deleted: [...context.getters.DELETED_LINES(pageNo)], - inserted: [...context.getters.INSERTED_LINES(pageNo)], - }; + const pageStatus = computePageStatus( + context.getters.GET_SCRIPT_PAGE(pageNo), + tmpScriptPage, + context.getters.DELETED_LINES(pageNo), + context.getters.INSERTED_LINES(pageNo) + ); const searchParams = new URLSearchParams({ page: pageNo, }); diff --git a/client/src/store/modules/scriptConfig.test.js b/client/src/store/modules/scriptConfig.test.js new file mode 100644 index 00000000..0b0eb6f1 --- /dev/null +++ b/client/src/store/modules/scriptConfig.test.js @@ -0,0 +1,103 @@ +import { describe, it, expect } from 'vitest'; +import { computePageStatus } from './scriptConfig'; + +function makeLine(id, numParts) { + return { + id, + act_id: 1, + scene_id: 1, + page: 1, + line_type: 1, + stage_direction_style_id: null, + line_parts: Array.from({ length: numParts }, (_, i) => ({ + id: id != null ? 100 + i : null, + line_id: id, + part_index: i, + character_id: 1, + character_group_id: null, + line_text: `Part ${i}`, + })), + }; +} + +describe('computePageStatus', () => { + it('classifies an existing line whose line_parts grew as updated, not added', () => { + // deep-object-diff marks index 0 as "added" when a new line_part appears on an existing line. + // The fix: if the line has an id it must go to updated instead. + const saved = [makeLine(42, 1)]; + const edited = [makeLine(42, 2)]; + + const status = computePageStatus(saved, edited, [], []); + + expect(status.added).not.toContain(0); + expect(status.updated).toContain(0); + expect(status.deleted).toHaveLength(0); + expect(status.inserted).toHaveLength(0); + }); + + it('classifies a genuinely new line (id == null) as added', () => { + const saved = []; + const edited = [makeLine(null, 1)]; + + const status = computePageStatus(saved, edited, [], []); + + expect(status.added).toContain(0); + expect(status.updated).not.toContain(0); + }); + + it('classifies an existing line with a changed top-level field as updated', () => { + const saved = [makeLine(42, 1)]; + const edited = [{ ...makeLine(42, 1), line_type: 2 }]; + + const status = computePageStatus(saved, edited, [], []); + + expect(status.updated).toContain(0); + expect(status.added).not.toContain(0); + }); + + it('passes deleted line indices through to the deleted array', () => { + const saved = [makeLine(10, 1), makeLine(11, 1)]; + const edited = [makeLine(10, 1), makeLine(11, 1)]; + + const status = computePageStatus(saved, edited, [1], []); + + expect(status.deleted).toContain(1); + expect(status.added).toHaveLength(0); + expect(status.updated).toHaveLength(0); + }); + + it('passes inserted line indices through to the inserted array', () => { + const saved = [makeLine(10, 1)]; + const newLine = makeLine(null, 1); + // Inserted at index 1: augmented actual includes newLine at index 1 so diff sees no change + const edited = [makeLine(10, 1), newLine]; + + const status = computePageStatus(saved, edited, [], [1]); + + expect(status.inserted).toContain(1); + }); + + it('handles a mixed page: one truly-new line and one existing line whose parts grew', () => { + const saved = [makeLine(42, 1)]; + // Index 0: existing line with extra part; index 1: brand-new line + const edited = [makeLine(42, 2), makeLine(null, 1)]; + + const status = computePageStatus(saved, edited, [], []); + + expect(status.updated).toContain(0); + expect(status.added).not.toContain(0); + expect(status.added).toContain(1); + expect(status.updated).not.toContain(1); + }); + + it('returns all empty arrays when actual and tmp pages are identical', () => { + const page = [makeLine(42, 2), makeLine(43, 1)]; + + const status = computePageStatus(page, JSON.parse(JSON.stringify(page)), [], []); + + expect(status.added).toHaveLength(0); + expect(status.updated).toHaveLength(0); + expect(status.deleted).toHaveLength(0); + expect(status.inserted).toHaveLength(0); + }); +}); diff --git a/electron/package-lock.json b/electron/package-lock.json index 4e92a7ee..d9ba4371 100644 --- a/electron/package-lock.json +++ b/electron/package-lock.json @@ -1,12 +1,12 @@ { "name": "digiscript-electron", - "version": "0.27.3", + "version": "0.28.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "digiscript-electron", - "version": "0.27.3", + "version": "0.28.0", "license": "GPL-3.0", "dependencies": { "bonjour-service": "^1.3.0", diff --git a/electron/package.json b/electron/package.json index b7ad5fad..e46a65ea 100644 --- a/electron/package.json +++ b/electron/package.json @@ -1,6 +1,6 @@ { "name": "digiscript-electron", - "version": "0.27.3", + "version": "0.28.0", "description": "DigiScript Electron Desktop Application", "author": "DreamTeamProd", "license": "GPL-3.0", diff --git a/server/alembic_config/repair_linked_list_utils.py b/server/alembic_config/repair_linked_list_utils.py new file mode 100644 index 00000000..ecf7ec61 --- /dev/null +++ b/server/alembic_config/repair_linked_list_utils.py @@ -0,0 +1,165 @@ +"""Shared SQL helpers for ScriptLineRevisionAssociation repair migrations. + +Both c2f8d4a6e0b3 and 897ae2963f6d use these primitives; they differ only in +how they select the bridge (first) line when multiple candidates exist for a page. +""" + +from collections import defaultdict +from typing import Callable + +import sqlalchemy as sa + + +def fetch_page_associations(conn, revision_id: int) -> dict: + """Return ``{page: {line_id: {next, prev, prev_page}}}`` for one revision.""" + rows = conn.execute( + sa.text( + """ + SELECT a.line_id, a.next_line_id, a.previous_line_id, + l.page AS line_page, + lp.page AS prev_page + FROM script_line_revision_association a + JOIN script_lines l ON l.id = a.line_id + LEFT JOIN script_lines lp ON lp.id = a.previous_line_id + WHERE a.revision_id = :rev_id + """ + ), + {"rev_id": revision_id}, + ).fetchall() + + by_page: dict = defaultdict(dict) + for row in rows: + line_id, next_id, prev_id, page, prev_page = row + by_page[page if page is not None else 0][line_id] = { + "next": next_id, + "prev": prev_id, + "prev_page": prev_page, + } + return by_page + + +def build_page_ordered(by_page: dict, pick_bridge: Callable) -> dict: + """Build ``{page: [line_id, ...]}`` using bridge detection. + + :param by_page: Output of :func:`fetch_page_associations`. + :param pick_bridge: ``(candidates: list[int]) -> int`` — selects the bridge + (first) line from a non-empty list of candidates. Pass ``max`` to use + the highest line_id (correct for the post-corruption case) or + ``lambda c: c[0]`` to replicate the original first-found behaviour. + """ + result = {} + for page in sorted(by_page.keys()): + page_lines = by_page[page] + candidates = [ + lid + for lid, d in page_lines.items() + if d["prev"] is None + or (d["prev_page"] is not None and d["prev_page"] == page - 1) + ] + if not candidates: + result[page] = [] + continue + + first_line_id = pick_bridge(candidates) + ordered = [] + current = first_line_id + seen: set = set() + while current is not None and current in page_lines: + if current in seen: + break + seen.add(current) + ordered.append(current) + current = page_lines[current]["next"] + result[page] = ordered + return result + + +def build_global_order(page_ordered: dict) -> list: + """Flatten ``{page: [line_id, ...]}`` into a single ordered list.""" + global_order = [] + for page in sorted(page_ordered.keys()): + global_order.extend(page_ordered[page]) + return global_order + + +def compute_expected_pointers(global_order: list) -> dict: + """Return ``{line_id: (expected_next, expected_prev)}`` for the full chain.""" + return { + line_id: ( + global_order[i + 1] if i + 1 < len(global_order) else None, + global_order[i - 1] if i > 0 else None, + ) + for i, line_id in enumerate(global_order) + } + + +def fetch_current_pointers(conn, revision_id: int) -> dict: + """Return ``{line_id: (next_line_id, previous_line_id)}`` from the DB.""" + rows = conn.execute( + sa.text( + "SELECT line_id, next_line_id, previous_line_id " + "FROM script_line_revision_association WHERE revision_id = :rev_id" + ), + {"rev_id": revision_id}, + ).fetchall() + return {row[0]: (row[1], row[2]) for row in rows} + + +def collect_pointer_updates( + expected: dict, current_by_id: dict, revision_id: int +) -> list: + """Return list of ``(new_next, new_prev, revision_id, line_id)`` tuples that need updating.""" + updates = [] + for line_id, (exp_next, exp_prev) in expected.items(): + curr_next, curr_prev = current_by_id.get(line_id, (None, None)) + if curr_next != exp_next or curr_prev != exp_prev: + updates.append((exp_next, exp_prev, revision_id, line_id)) + return updates + + +def delete_orphan_associations(conn, revision_id: int, orphan_ids) -> None: + """Delete cue and revision associations for orphaned lines (FK order).""" + for orphan_id in sorted(orphan_ids): + conn.execute( + sa.text( + "DELETE FROM script_cue_association " + "WHERE revision_id = :rev_id AND line_id = :line_id" + ), + {"rev_id": revision_id, "line_id": orphan_id}, + ) + for orphan_id in sorted(orphan_ids): + conn.execute( + sa.text( + "DELETE FROM script_line_revision_association " + "WHERE revision_id = :rev_id AND line_id = :line_id" + ), + {"rev_id": revision_id, "line_id": orphan_id}, + ) + + +def apply_pointer_updates(conn, updates: list) -> None: + """Execute UPDATE statements for all pointer corrections.""" + for new_next, new_prev, rev_id, line_id in updates: + conn.execute( + sa.text( + "UPDATE script_line_revision_association " + "SET next_line_id = :next_id, previous_line_id = :prev_id " + "WHERE revision_id = :rev_id AND line_id = :line_id" + ), + { + "next_id": new_next, + "prev_id": new_prev, + "rev_id": rev_id, + "line_id": line_id, + }, + ) + + +def fetch_all_revision_ids(conn) -> list: + """Return all script revision IDs in ascending order.""" + return [ + row[0] + for row in conn.execute( + sa.text("SELECT id FROM script_revisions ORDER BY id") + ).fetchall() + ] diff --git a/server/alembic_config/versions/897ae2963f6d_repair_linked_list_orphans_max_bridge.py b/server/alembic_config/versions/897ae2963f6d_repair_linked_list_orphans_max_bridge.py new file mode 100644 index 00000000..02a5bb74 --- /dev/null +++ b/server/alembic_config/versions/897ae2963f6d_repair_linked_list_orphans_max_bridge.py @@ -0,0 +1,142 @@ +"""repair_linked_list_orphans_max_bridge + +Revision ID: 897ae2963f6d +Revises: d2e0b8414d17 +Create Date: 2026-05-08 20:37:12.115558 + +Repairs broken ScriptLineRevisionAssociation linked lists and removes orphaned +script_lines / script_line_parts records. + +This supersedes the earlier c2f8d4a6e0b3 repair migration by fixing a bug in +bridge-line selection: when multiple associations for the same page both satisfy +the "first-line" condition (i.e. their previous_line is on page N-1), the old +migration picked whichever came first in dict-iteration order (non-deterministic). + +The fix: always pick max(line_id) among the candidates. The most recently created +line (highest auto-increment ID) is the correct saved content because every PATCH +creates new ScriptLine objects with higher IDs than the old ones it replaces. + +The earlier migration also left orphaned script_lines and script_line_parts rows +in the database. This migration removes them. + +Root cause of the corruption it repairs: when a line's line_parts array grew, +deep-object-diff classified the line index as "added". The frontend sent it in +status["added"]. The backend "added" branch created a new +ScriptLineRevisionAssociation without removing the old one, leaving a split +doubly-linked list and a NULL next_line_id at the page boundary. +""" + +from typing import Sequence, Union + +import sqlalchemy as sa +from alembic import op + +from alembic_config.repair_linked_list_utils import ( + apply_pointer_updates, + build_global_order, + build_page_ordered, + collect_pointer_updates, + compute_expected_pointers, + delete_orphan_associations, + fetch_all_revision_ids, + fetch_current_pointers, + fetch_page_associations, +) + + +# revision identifiers, used by Alembic. +revision: str = "897ae2963f6d" +down_revision: Union[str, None] = "d2e0b8414d17" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def _page_walk(conn, revision_id): + """Walk pages using bridge detection. Returns {page: [line_id, ...]}. + + When multiple lines on the same page both satisfy the bridge condition + (the corruption pattern caused by the status["added"] bug), pick + max(line_id). The highest line_id is the most recently created row and + is therefore the correct saved content. + """ + by_page = fetch_page_associations(conn, revision_id) + return build_page_ordered(by_page, max) + + +def _repair_revision(conn, revision_id): + """Repair linked-list pointers and remove orphans for one revision. + + :returns: Summary dict with counts of changes applied. + """ + page_ordered = _page_walk(conn, revision_id) + global_order = build_global_order(page_ordered) + expected = compute_expected_pointers(global_order) + current_by_id = fetch_current_pointers(conn, revision_id) + updates = collect_pointer_updates(expected, current_by_id, revision_id) + reachable_set = set(global_order) + orphan_ids = set(current_by_id.keys()) - reachable_set + + if orphan_ids: + delete_orphan_associations(conn, revision_id, orphan_ids) + if updates: + apply_pointer_updates(conn, updates) + + # Remove orphaned script_line_parts and script_lines rows. + # A script_line is orphaned when it has no revision associations at all. + # Delete parts first (FK child), then the line. + if orphan_ids: + for orphan_id in sorted(orphan_ids): + conn.execute( + sa.text( + "DELETE FROM script_line_parts WHERE line_id = :line_id " + "AND NOT EXISTS (" + " SELECT 1 FROM script_line_revision_association " + " WHERE line_id = :line_id" + ")" + ), + {"line_id": orphan_id}, + ) + for orphan_id in sorted(orphan_ids): + conn.execute( + sa.text( + "DELETE FROM script_lines WHERE id = :line_id " + "AND NOT EXISTS (" + " SELECT 1 FROM script_line_revision_association " + " WHERE line_id = :line_id" + ")" + ), + {"line_id": orphan_id}, + ) + + return { + "revision_id": revision_id, + "total": len(current_by_id), + "pointer_updates": len(updates), + "orphans_deleted": len(orphan_ids), + } + + +def upgrade() -> None: + conn = op.get_bind() + print("Repairing ScriptLineRevisionAssociation linked lists (max-bridge pass)…") + revision_ids = fetch_all_revision_ids(conn) + total_updates = 0 + total_orphans = 0 + for rev_id in revision_ids: + result = _repair_revision(conn, rev_id) + if result["pointer_updates"] or result["orphans_deleted"]: + print( + f" Revision {rev_id}: {result['pointer_updates']} pointer fix(es), " + f"{result['orphans_deleted']} orphan(s) deleted " + f"(of {result['total']} associations)" + ) + total_updates += result["pointer_updates"] + total_orphans += result["orphans_deleted"] + print( + f"Repair complete: {total_updates} pointer fix(es), " + f"{total_orphans} orphan(s) deleted across {len(revision_ids)} revision(s)." + ) + + +def downgrade() -> None: + pass # data repair cannot be reversed diff --git a/server/alembic_config/versions/c2f8d4a6e0b3_repair_script_revision_linked_lists.py b/server/alembic_config/versions/c2f8d4a6e0b3_repair_script_revision_linked_lists.py index d54598d4..d662fa9c 100644 --- a/server/alembic_config/versions/c2f8d4a6e0b3_repair_script_revision_linked_lists.py +++ b/server/alembic_config/versions/c2f8d4a6e0b3_repair_script_revision_linked_lists.py @@ -22,12 +22,22 @@ against clean data — that is why the repair is committed first. """ -from collections import defaultdict from typing import Sequence, Union -import sqlalchemy as sa from alembic import op +from alembic_config.repair_linked_list_utils import ( + apply_pointer_updates, + build_global_order, + build_page_ordered, + collect_pointer_updates, + compute_expected_pointers, + delete_orphan_associations, + fetch_all_revision_ids, + fetch_current_pointers, + fetch_page_associations, +) + # revision identifiers, used by Alembic. revision: str = "c2f8d4a6e0b3" @@ -36,123 +46,28 @@ depends_on: Union[str, Sequence[str], None] = None -# --------------------------------------------------------------------------- -# Helpers: page walk + repair logic (mirrors diagnose_linked_list.py) -# --------------------------------------------------------------------------- - - def _page_walk(conn, revision_id): - """Walk pages using bridge detection. Returns {page: [line_id, ...]}. + """Walk pages using bridge detection (first-found bridge selection). - Bridge detection: the first line of page N is the line whose - previous_line_id points to a line on page N-1 (read directly from - script_lines, not from the association — the previous line may have been - removed from this revision while still existing in script_lines). + :note: When multiple lines satisfy the bridge condition, this picks the + first one in dict-iteration order (non-deterministic). This is the + original behaviour; migration 897ae2963f6d supersedes it with a + deterministic ``max(line_id)`` selection. """ - rows = conn.execute( - sa.text( - """ - SELECT a.line_id, a.next_line_id, a.previous_line_id, - l.page AS line_page, - lp.page AS prev_page - FROM script_line_revision_association a - JOIN script_lines l ON l.id = a.line_id - LEFT JOIN script_lines lp ON lp.id = a.previous_line_id - WHERE a.revision_id = :rev_id - """ - ), - {"rev_id": revision_id}, - ).fetchall() - - # Group associations by page - by_page = defaultdict(dict) - for row in rows: - line_id = row[0] - next_line_id = row[1] - prev_line_id = row[2] - page = row[3] if row[3] is not None else 0 - prev_page = row[4] - - by_page[page][line_id] = { - "next": next_line_id, - "prev": prev_line_id, - "prev_page": prev_page, - } - - result = {} - - for page in sorted(by_page.keys()): - page_lines = by_page[page] - - # Find the bridge line (first line on this page): - # - previous_line_id IS NULL (global head), or - # - previous_line.page == page - 1 (cross-page pointer) - first_line_id = None - for line_id, d in page_lines.items(): - prev_id = d["prev"] - prev_page = d["prev_page"] - if prev_id is None or (prev_page is not None and prev_page == page - 1): - first_line_id = line_id - break - - if first_line_id is None: - result[page] = [] - continue - - # Walk within this page following next_line_id - ordered = [] - current = first_line_id - seen = set() - while current is not None and current in page_lines: - if current in seen: - break # loop guard - seen.add(current) - ordered.append(current) - current = page_lines[current]["next"] - - result[page] = ordered - - return result + by_page = fetch_page_associations(conn, revision_id) + return build_page_ordered(by_page, lambda candidates: candidates[0]) def _repair_revision(conn, revision_id): """Repair linked list pointers for one revision. - Returns a summary dict with counts of changes applied. + :returns: Summary dict with counts of changes applied. """ page_ordered = _page_walk(conn, revision_id) - - # Build global order from sorted pages - global_order = [] - for page in sorted(page_ordered.keys()): - global_order.extend(page_ordered[page]) - - # Compute expected pointers for every line in the repaired chain - expected = {} - for i, line_id in enumerate(global_order): - expected[line_id] = ( - global_order[i + 1] if i + 1 < len(global_order) else None, # next - global_order[i - 1] if i > 0 else None, # prev - ) - - # Fetch current state from DB - current_rows = conn.execute( - sa.text( - "SELECT line_id, next_line_id, previous_line_id " - "FROM script_line_revision_association WHERE revision_id = :rev_id" - ), - {"rev_id": revision_id}, - ).fetchall() - current_by_id = {row[0]: (row[1], row[2]) for row in current_rows} - - # Lines that need pointer correction - updates = [] # (new_next, new_prev, revision_id, line_id) - for line_id, (exp_next, exp_prev) in expected.items(): - curr_next, curr_prev = current_by_id.get(line_id, (None, None)) - if curr_next != exp_next or curr_prev != exp_prev: - updates.append((exp_next, exp_prev, revision_id, line_id)) - - # True orphans: in the revision but not reached by any page-local walk + global_order = build_global_order(page_ordered) + expected = compute_expected_pointers(global_order) + current_by_id = fetch_current_pointers(conn, revision_id) + updates = collect_pointer_updates(expected, current_by_id, revision_id) reachable_set = set(global_order) orphan_ids = set(current_by_id.keys()) - reachable_set @@ -161,44 +76,14 @@ def _repair_revision(conn, revision_id): f" Revision {revision_id}: deleting {len(orphan_ids)} orphan(s): " f"{sorted(orphan_ids)}" ) - # Delete cue associations for orphaned lines first (FK ordering) - for orphan_id in orphan_ids: - conn.execute( - sa.text( - "DELETE FROM script_cue_association " - "WHERE revision_id = :rev_id AND line_id = :line_id" - ), - {"rev_id": revision_id, "line_id": orphan_id}, - ) - # Delete the orphaned associations themselves - for orphan_id in orphan_ids: - conn.execute( - sa.text( - "DELETE FROM script_line_revision_association " - "WHERE revision_id = :rev_id AND line_id = :line_id" - ), - {"rev_id": revision_id, "line_id": orphan_id}, - ) + delete_orphan_associations(conn, revision_id, orphan_ids) if updates: print( f" Revision {revision_id}: fixing {len(updates)} pointer(s) " f"(out of {len(current_by_id)} associations)" ) - for new_next, new_prev, rev_id, line_id in updates: - conn.execute( - sa.text( - "UPDATE script_line_revision_association " - "SET next_line_id = :next_id, previous_line_id = :prev_id " - "WHERE revision_id = :rev_id AND line_id = :line_id" - ), - { - "next_id": new_next, - "prev_id": new_prev, - "rev_id": rev_id, - "line_id": line_id, - }, - ) + apply_pointer_updates(conn, updates) return { "revision_id": revision_id, @@ -208,29 +93,16 @@ def _repair_revision(conn, revision_id): } -# --------------------------------------------------------------------------- -# Migration -# --------------------------------------------------------------------------- - - def upgrade() -> None: conn = op.get_bind() print("Repairing ScriptLineRevisionAssociation linked lists…") - - revision_ids = [ - row[0] - for row in conn.execute( - sa.text("SELECT id FROM script_revisions ORDER BY id") - ).fetchall() - ] - + revision_ids = fetch_all_revision_ids(conn) total_updates = 0 total_orphans = 0 for rev_id in revision_ids: result = _repair_revision(conn, rev_id) total_updates += result["pointer_updates"] total_orphans += result["orphans_deleted"] - print( f"Repair complete: {total_updates} pointer fix(es), " f"{total_orphans} orphan(s) deleted across {len(revision_ids)} revision(s)." diff --git a/server/controllers/api/show/script/script.py b/server/controllers/api/show/script/script.py index 5dc074ac..cd61959a 100644 --- a/server/controllers/api/show/script/script.py +++ b/server/controllers/api/show/script/script.py @@ -430,7 +430,11 @@ async def patch(self): previous_line: Optional[ScriptLineRevisionAssociation] = None for index, line in enumerate(lines): - if index in status["added"]: + is_truly_new = index in status["added"] and line.get("id") is None + is_updated = index in status["updated"] or ( + index in status["added"] and line.get("id") is not None + ) + if is_truly_new: # Validate the line valid_status, valid_reason = self._validate_line(show, line) if not valid_status: @@ -481,83 +485,11 @@ async def patch(self): session.flush() previous_line = line_association - elif index in status["deleted"]: - curr_association: ScriptLineRevisionAssociation = session.get( - ScriptLineRevisionAssociation, - (revision.id, line["id"]), - ) - - # Logic for handling next/previous line associations - if ( - curr_association.next_line - and curr_association.previous_line - ): - # Next line and previous line, so need to update both - next_association: ScriptLineRevisionAssociation = ( - session.get( - ScriptLineRevisionAssociation, - (revision.id, curr_association.next_line.id), - ) - ) - next_association.previous_line = ( - curr_association.previous_line - ) - session.flush() - - prev_association: ScriptLineRevisionAssociation = ( - session.get( - ScriptLineRevisionAssociation, - (revision.id, curr_association.previous_line.id), - ) - ) - prev_association.next_line = next_association.line - session.flush() - elif curr_association.next_line: - # No previous line, so need to update next line only - next_association: ScriptLineRevisionAssociation = ( - session.get( - ScriptLineRevisionAssociation, - (revision.id, curr_association.next_line.id), - ) - ) - next_association.previous_line = None - session.flush() - elif curr_association.previous_line: - # No next line, so need to update previous line only - prev_association: ScriptLineRevisionAssociation = ( - session.get( - ScriptLineRevisionAssociation, - (revision.id, curr_association.previous_line.id), - ) - ) - prev_association.next_line = None - session.flush() - - # Store line_id before deleting association for orphan cleanup - line_id_to_cleanup = curr_association.line_id - - # Delete any cue associations for this (revision, line) before deleting the line association - # CueAssociation is revision-scoped, so we need to explicitly clean it up - cue_assocs = session.scalars( - select(CueAssociation).where( - CueAssociation.revision_id == revision.id, - CueAssociation.line_id == line_id_to_cleanup, - ) - ).all() - cue_ids_to_cleanup = [ca.cue_id for ca in cue_assocs] - for cue_assoc in cue_assocs: - session.delete(cue_assoc) - - session.delete(curr_association) - - # Explicitly cleanup orphaned objects after deletion - # This ensures immediate cleanup in PATCH operations - ScriptLineRevisionAssociation.cleanup_orphaned_line( - session, line_id_to_cleanup - ) - for cue_id in cue_ids_to_cleanup: - CueAssociation.cleanup_orphaned_cue(session, cue_id) - elif index in status["updated"]: + elif ( + is_updated + and index not in status["deleted"] + and index not in status["inserted"] + ): # Validate the line valid_status, valid_reason = self._validate_line(show, line) if not valid_status: @@ -600,11 +532,6 @@ async def patch(self): next_association.previous_line = line_object session.flush() - # Migrate revision-scoped associations from old line to new line - # CueAssociation and ScriptCuts must be migrated when updating a line - # (which creates new ScriptLine + ScriptLinePart objects) - - # 1. Migrate CueAssociation: (revision, old_line_id) → (revision, new_line_id) cue_assocs = session.scalars( select(CueAssociation).where( CueAssociation.revision_id == revision.id, @@ -613,17 +540,14 @@ async def patch(self): ).all() for old_cue_assoc in cue_assocs: - # Create new association with new line_id new_cue_assoc = CueAssociation( revision_id=revision.id, - line_id=line_object.id, # Point to NEW line + line_id=line_object.id, cue_id=old_cue_assoc.cue_id, ) session.add(new_cue_assoc) session.delete(old_cue_assoc) - # 2. Migrate ScriptCuts: (revision, old_line_part_id) → (revision, new_line_part_id) - # Build mapping of part_index → line_part_id for both old and new old_parts_map = { part.part_index: part.id for part in curr_line.line_parts } @@ -632,7 +556,6 @@ async def patch(self): } for part_index, old_part_id in old_parts_map.items(): - # Find any cuts for this old line_part in this revision cuts = session.scalars( select(ScriptCuts).where( ScriptCuts.revision_id == revision.id, @@ -641,12 +564,11 @@ async def patch(self): ).all() for old_cut in cuts: - # Create new cut with new line_part_id new_part_id = new_parts_map.get(part_index) if new_part_id: new_cut = ScriptCuts( revision_id=revision.id, - line_part_id=new_part_id, # Point to NEW line_part + line_part_id=new_part_id, ) session.add(new_cut) session.delete(old_cut) @@ -659,6 +581,82 @@ async def patch(self): session.flush() previous_line = curr_association + elif index in status["deleted"]: + curr_association: ScriptLineRevisionAssociation = session.get( + ScriptLineRevisionAssociation, + (revision.id, line["id"]), + ) + + # Logic for handling next/previous line associations + if ( + curr_association.next_line + and curr_association.previous_line + ): + # Next line and previous line, so need to update both + next_association: ScriptLineRevisionAssociation = ( + session.get( + ScriptLineRevisionAssociation, + (revision.id, curr_association.next_line.id), + ) + ) + next_association.previous_line = ( + curr_association.previous_line + ) + session.flush() + + prev_association: ScriptLineRevisionAssociation = ( + session.get( + ScriptLineRevisionAssociation, + (revision.id, curr_association.previous_line.id), + ) + ) + prev_association.next_line = next_association.line + session.flush() + elif curr_association.next_line: + # No previous line, so need to update next line only + next_association: ScriptLineRevisionAssociation = ( + session.get( + ScriptLineRevisionAssociation, + (revision.id, curr_association.next_line.id), + ) + ) + next_association.previous_line = None + session.flush() + elif curr_association.previous_line: + # No next line, so need to update previous line only + prev_association: ScriptLineRevisionAssociation = ( + session.get( + ScriptLineRevisionAssociation, + (revision.id, curr_association.previous_line.id), + ) + ) + prev_association.next_line = None + session.flush() + + # Store line_id before deleting association for orphan cleanup + line_id_to_cleanup = curr_association.line_id + + # Delete any cue associations for this (revision, line) before deleting the line association + # CueAssociation is revision-scoped, so we need to explicitly clean it up + cue_assocs = session.scalars( + select(CueAssociation).where( + CueAssociation.revision_id == revision.id, + CueAssociation.line_id == line_id_to_cleanup, + ) + ).all() + cue_ids_to_cleanup = [ca.cue_id for ca in cue_assocs] + for cue_assoc in cue_assocs: + session.delete(cue_assoc) + + session.delete(curr_association) + + # Explicitly cleanup orphaned objects after deletion + # This ensures immediate cleanup in PATCH operations + ScriptLineRevisionAssociation.cleanup_orphaned_line( + session, line_id_to_cleanup + ) + for cue_id in cue_ids_to_cleanup: + CueAssociation.cleanup_orphaned_cue(session, cue_id) else: previous_line = session.get( ScriptLineRevisionAssociation, diff --git a/server/pyproject.toml b/server/pyproject.toml index afcfffba..7e35f203 100644 --- a/server/pyproject.toml +++ b/server/pyproject.toml @@ -11,7 +11,7 @@ build-backend = "setuptools.build_meta" [project] name = "digiscript-server" -version = "0.27.3" +version = "0.28.0" description = "DigiScript server - Digital script management for theatrical shows" readme = "../README.md" requires-python = ">=3.13" diff --git a/server/test/controllers/api/show/script/test_script.py b/server/test/controllers/api/show/script/test_script.py index 60ded342..e75f0816 100644 --- a/server/test/controllers/api/show/script/test_script.py +++ b/server/test/controllers/api/show/script/test_script.py @@ -1674,3 +1674,258 @@ def test_post_stage_direction_rejects_empty_text(self): self.assertEqual(400, response.code) response_body = tornado.escape.json_decode(response.body) self.assertIn("must contain text", response_body["message"]) + + +class TestPatchLinePartCountChange(DigiScriptTestCase): + """Regression tests for page-boundary corruption when line_parts count changes. + + When a line's line_parts array grows, deep-object-diff classifies the line index + as both "added" and "updated". Previously, the backend treated this as "added" + (creating a new association without removing the old), which left a split + doubly-linked list and broke the page boundary. + """ + + def setUp(self): + super().setUp() + with self._app.get_db().sessionmaker() as session: + show = Show(name="Test Show", script_mode=ShowScriptType.FULL) + session.add(show) + session.flush() + self.show_id = show.id + + script = Script(show_id=show.id) + session.add(script) + session.flush() + + revision = ScriptRevision( + script_id=script.id, revision=1, description="Test" + ) + session.add(revision) + session.flush() + self.revision_id = revision.id + script.current_revision = revision.id + + act = Act(show_id=show.id, name="Act 1") + session.add(act) + session.flush() + self.act_id = act.id + + scene = Scene(show_id=show.id, act_id=act.id, name="Scene 1") + session.add(scene) + session.flush() + self.scene_id = scene.id + + character = Character(show_id=show.id, name="Character A") + session.add(character) + session.flush() + self.character_id = character.id + + admin = User(username="admin", is_admin=True, password="test") + session.add(admin) + session.flush() + self.user_id = admin.id + + session.commit() + + self._app.digi_settings.settings["current_show"].set_value(self.show_id) + self.token = self._app.jwt_service.create_access_token( + data={"user_id": self.user_id} + ) + + def _make_line(self, page, line_id, parts, act_id=None, scene_id=None): + return { + "id": line_id, + "act_id": act_id if act_id is not None else self.act_id, + "scene_id": scene_id if scene_id is not None else self.scene_id, + "page": page, + "line_type": 1, + "line_parts": [ + { + "id": None, + "line_id": line_id, + "part_index": i, + "character_id": self.character_id, + "character_group_id": None, + "line_text": f"Part {i}", + } + for i in range(parts) + ], + "stage_direction_style_id": None, + } + + def test_patch_existing_line_adding_line_part_does_not_corrupt_page(self): + """PATCH a line with status added (misclassified by deep-object-diff when line_parts + grows) should not create a duplicate association or leave the old one dangling. + After the PATCH, GET should return 200 and yield exactly one association per line. + """ + # Create one line on page 1 with 1 part + post_body = [self._make_line(1, None, 1)] + response = self.fetch( + "/api/v1/show/script?page=1", + method="POST", + body=tornado.escape.json_encode(post_body), + headers={"Authorization": f"Bearer {self.token}"}, + ) + self.assertEqual(200, response.code) + + get_resp = self.fetch( + "/api/v1/show/script?page=1", + headers={"Authorization": f"Bearer {self.token}"}, + ) + existing_line = tornado.escape.json_decode(get_resp.body)["lines"][0] + line_id = existing_line["id"] + + # PATCH the line with status=added (as deep-object-diff would send it when + # a line_part is added) but with the existing line id present + updated_line = self._make_line(1, line_id, 2) # now 2 parts + patch_data = { + "page": [updated_line], + "status": {"added": [0], "updated": [], "deleted": [], "inserted": []}, + } + response = self.fetch( + "/api/v1/show/script?page=1", + method="PATCH", + body=tornado.escape.json_encode(patch_data), + headers={"Authorization": f"Bearer {self.token}"}, + ) + self.assertEqual(200, response.code) + + # GET must still work (no "Failed to establish page line order") + get_resp = self.fetch( + "/api/v1/show/script?page=1", + headers={"Authorization": f"Bearer {self.token}"}, + ) + self.assertEqual(200, get_resp.code) + lines = tornado.escape.json_decode(get_resp.body)["lines"] + self.assertEqual(1, len(lines)) + self.assertEqual(2, len(lines[0]["line_parts"])) + + # Exactly one association must exist for this revision (no duplicates) + with self._app.get_db().sessionmaker() as session: + assocs = session.scalars( + select(ScriptLineRevisionAssociation).where( + ScriptLineRevisionAssociation.revision_id == self.revision_id + ) + ).all() + self.assertEqual( + 1, len(assocs), "Must have exactly 1 association — no duplicates" + ) + self.assertNotEqual( + line_id, assocs[0].line_id, "Association must point to new line" + ) + + def test_patch_page_n_then_page_n_plus_1_boundary_intact(self): + """Patching both page 1 and page 2 with status=added on existing IDs must not + corrupt the page boundary. + + Without the fix the failure is: + 1. PATCH page 1 (status=added) creates new associations but leaves old ones. + 2. PATCH page 2 (status=added) looks up the first page-2 line's previous_line + (which still points to the OLD last page-1 line), creates a new association + also pointing to that old line. + 3. GET page 2 now finds TWO associations with previous_line.page == 1 → 400. + + With the fix both PATCHes route through "updated", old associations are replaced, + and GET page 2 returns 200 with exactly one association for the page. + """ + # Create 2 lines on page 1 and 1 line on page 2 via POST + post_p1 = [ + self._make_line(1, None, 1), + self._make_line(1, None, 1), + ] + response = self.fetch( + "/api/v1/show/script?page=1", + method="POST", + body=tornado.escape.json_encode(post_p1), + headers={"Authorization": f"Bearer {self.token}"}, + ) + self.assertEqual(200, response.code) + + post_p2 = [self._make_line(2, None, 1)] + response = self.fetch( + "/api/v1/show/script?page=2", + method="POST", + body=tornado.escape.json_encode(post_p2), + headers={"Authorization": f"Bearer {self.token}"}, + ) + self.assertEqual(200, response.code) + + # Get existing line IDs for both pages + get_p1 = tornado.escape.json_decode( + self.fetch( + "/api/v1/show/script?page=1", + headers={"Authorization": f"Bearer {self.token}"}, + ).body + ) + line1_id = get_p1["lines"][0]["id"] + line2_id = get_p1["lines"][1]["id"] + + get_p2 = tornado.escape.json_decode( + self.fetch( + "/api/v1/show/script?page=2", + headers={"Authorization": f"Bearer {self.token}"}, + ).body + ) + line3_id = get_p2["lines"][0]["id"] + + # PATCH page 1: both lines with status=added (simulating deep-object-diff + # when line_parts count increases on existing lines) + patch_p1 = { + "page": [ + self._make_line(1, line1_id, 2), + self._make_line(1, line2_id, 2), + ], + "status": {"added": [0, 1], "updated": [], "deleted": [], "inserted": []}, + } + response = self.fetch( + "/api/v1/show/script?page=1", + method="PATCH", + body=tornado.escape.json_encode(patch_p1), + headers={"Authorization": f"Bearer {self.token}"}, + ) + self.assertEqual(200, response.code) + + # PATCH page 2 with status=added too — this is the step that creates the second + # duplicate association pointing to the old last-page-1 line. + patch_p2 = { + "page": [self._make_line(2, line3_id, 2)], + "status": {"added": [0], "updated": [], "deleted": [], "inserted": []}, + } + response = self.fetch( + "/api/v1/show/script?page=2", + method="PATCH", + body=tornado.escape.json_encode(patch_p2), + headers={"Authorization": f"Bearer {self.token}"}, + ) + self.assertEqual(200, response.code) + + # GET page 2 must return 200 — without the fix this returns 400 + # "Failed to establish page line order" + get_p2_after = self.fetch( + "/api/v1/show/script?page=2", + headers={"Authorization": f"Bearer {self.token}"}, + ) + self.assertEqual( + 200, + get_p2_after.code, + "GET page 2 must succeed after patching both pages with status=added", + ) + p2_lines = tornado.escape.json_decode(get_p2_after.body)["lines"] + self.assertEqual(1, len(p2_lines)) + + # Exactly one association must exist for page 2 (no duplicates) + with self._app.get_db().sessionmaker() as session: + assocs_p2 = session.scalars( + select(ScriptLineRevisionAssociation).where( + ScriptLineRevisionAssociation.revision_id == self.revision_id, + ScriptLineRevisionAssociation.line.has(page=2), + ) + ).all() + self.assertEqual( + 1, len(assocs_p2), "Must have exactly 1 association for page 2" + ) + prev_line = assocs_p2[0].previous_line + self.assertIsNotNone( + prev_line, "Page 2 first line must have a previous line" + ) + self.assertEqual(1, prev_line.page, "Previous line must be on page 1")