From 3c22e3246f238d62ca70a7c4bf93864bcc74e4e9 Mon Sep 17 00:00:00 2001 From: Tim Bradgate Date: Sun, 7 Jun 2026 19:33:29 +0100 Subject: [PATCH] Fix N+1 query performance issues in stats and list endpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cast/stats and character/stats were firing 5,137 individual SQL queries per request (~8s) due to nested lazy-loaded ORM traversal. Replace with selectinload chains that load all required data in ~8 bulk queries, and a single upfront set-query for revision-scoped cuts rather than one check per line part. Also fixes character list N+1 (per-character character_group and mic_allocation lazy loads) via selectinload on CharacterController, and removes Act.lines / Scene.lines from their schemas — both relationships loaded ~950 ORM rows per act/scene on every list request but were never read by either frontend client. Adds 11 behavioural count-verification tests for cast/stats and character/stats to document and guard the exact counting semantics. Co-Authored-By: Claude Sonnet 4.6 --- server/controllers/api/v1/show/cast.py | 51 +++- server/controllers/api/v1/show/characters.py | 50 +++- server/schemas/schemas.py | 2 + .../test/controllers/api/v1/show/test_cast.py | 223 +++++++++++++++++- .../api/v1/show/test_characters.py | 184 ++++++++++++++- 5 files changed, 468 insertions(+), 42 deletions(-) diff --git a/server/controllers/api/v1/show/cast.py b/server/controllers/api/v1/show/cast.py index 85165e78..7e0d2a12 100644 --- a/server/controllers/api/v1/show/cast.py +++ b/server/controllers/api/v1/show/cast.py @@ -1,6 +1,7 @@ from collections import defaultdict from sqlalchemy import select +from sqlalchemy.orm import selectinload from tornado import escape from controllers.api.constants import ( @@ -11,8 +12,16 @@ ERROR_LAST_NAME_MISSING, ERROR_SHOW_NOT_FOUND, ) -from models.script import Script, ScriptLine, ScriptLineType, ScriptRevision -from models.show import Cast, Character, Show +from models.script import ( + Script, + ScriptCuts, + ScriptLine, + ScriptLinePart, + ScriptLineRevisionAssociation, + ScriptLineType, + ScriptRevision, +) +from models.show import Cast, CharacterGroup, Show from rbac.role import Role from schemas.schemas import CastSchema from utils.web.base_controller import BaseAPIController @@ -186,28 +195,50 @@ async def get(self): select(Script).where(Script.show_id == show.id) ).first() - if script.current_revision: - revision: ScriptRevision = session.get( - ScriptRevision, script.current_revision - ) - else: + if not script.current_revision: self.set_status(400) await self.finish( {"message": "Script does not have a current revision"} ) return + revision: ScriptRevision = session.scalars( + select(ScriptRevision) + .where(ScriptRevision.id == script.current_revision) + .options( + selectinload(ScriptRevision.line_associations) + .selectinload(ScriptLineRevisionAssociation.line) + .options( + selectinload(ScriptLine.line_parts).options( + selectinload(ScriptLinePart.character), + selectinload( + ScriptLinePart.character_group + ).selectinload(CharacterGroup.characters), + ) + ) + ) + ).first() + + # Load all cut line_part_ids for this revision in a single query. + cut_part_ids: set[int] = set( + session.scalars( + select(ScriptCuts.line_part_id).where( + ScriptCuts.revision_id == revision.id + ) + ).all() + ) + line_counts = defaultdict(lambda: defaultdict(lambda: defaultdict(int))) for line_association in revision.line_associations: line: ScriptLine = line_association.line if line.line_type != ScriptLineType.DIALOGUE: continue for line_part in line.line_parts: - if line_part.line_part_cuts is not None: + if line_part.id in cut_part_ids: continue if line_part.character_id: - character = session.get(Character, line_part.character_id) - if character.played_by: + character = line_part.character + if character and character.played_by: line_counts[character.played_by][line.act_id][ line.scene_id ] += 1 diff --git a/server/controllers/api/v1/show/characters.py b/server/controllers/api/v1/show/characters.py index d9b78188..cf02080d 100644 --- a/server/controllers/api/v1/show/characters.py +++ b/server/controllers/api/v1/show/characters.py @@ -1,6 +1,7 @@ from collections import defaultdict from sqlalchemy import select, update +from sqlalchemy.orm import selectinload from tornado import escape from controllers.api.constants import ( @@ -15,8 +16,10 @@ ) from models.script import ( Script, + ScriptCuts, ScriptLine, ScriptLinePart, + ScriptLineRevisionAssociation, ScriptLineType, ScriptRevision, ) @@ -39,9 +42,19 @@ def get(self): with self.make_session() as session: show = session.get(Show, show_id) if show: - characters = [character_schema.dump(c) for c in show.character_list] + characters = session.scalars( + select(Character) + .where(Character.show_id == show.id) + .options( + selectinload(Character.character_groups), + selectinload(Character.mic_allocations), + selectinload(Character.cast_member), + ) + ).all() self.set_status(200) - self.finish({"characters": characters}) + self.finish( + {"characters": [character_schema.dump(c) for c in characters]} + ) else: self.set_status(404) self.finish({"message": ERROR_SHOW_NOT_FOUND}) @@ -294,24 +307,45 @@ async def get(self): select(Script).where(Script.show_id == show.id) ).first() - if script.current_revision: - revision: ScriptRevision = session.get( - ScriptRevision, script.current_revision - ) - else: + if not script.current_revision: self.set_status(400) await self.finish( {"message": "Script does not have a current revision"} ) return + revision: ScriptRevision = session.scalars( + select(ScriptRevision) + .where(ScriptRevision.id == script.current_revision) + .options( + selectinload(ScriptRevision.line_associations) + .selectinload(ScriptLineRevisionAssociation.line) + .options( + selectinload(ScriptLine.line_parts).options( + selectinload( + ScriptLinePart.character_group + ).selectinload(CharacterGroup.characters), + ) + ) + ) + ).first() + + # Load all cut line_part_ids for this revision in a single query. + cut_part_ids: set[int] = set( + session.scalars( + select(ScriptCuts.line_part_id).where( + ScriptCuts.revision_id == revision.id + ) + ).all() + ) + line_counts = defaultdict(lambda: defaultdict(lambda: defaultdict(int))) for line_association in revision.line_associations: line: ScriptLine = line_association.line if line.line_type != ScriptLineType.DIALOGUE: continue for line_part in line.line_parts: - if line_part.line_part_cuts is not None: + if line_part.id in cut_part_ids: continue if line_part.character_id: line_counts[line_part.character_id][line.act_id][ diff --git a/server/schemas/schemas.py b/server/schemas/schemas.py index 9feacdb7..47af9ad3 100644 --- a/server/schemas/schemas.py +++ b/server/schemas/schemas.py @@ -170,6 +170,7 @@ class Meta: model = Act include_relationships = True load_instance = True + exclude = ("lines",) @schema @@ -178,6 +179,7 @@ class Meta: model = Scene include_relationships = True load_instance = True + exclude = ("lines",) @schema diff --git a/server/test/controllers/api/v1/show/test_cast.py b/server/test/controllers/api/v1/show/test_cast.py index ef20f32f..2b7af971 100644 --- a/server/test/controllers/api/v1/show/test_cast.py +++ b/server/test/controllers/api/v1/show/test_cast.py @@ -1,7 +1,23 @@ import tornado.escape -from models.script import Script, ScriptRevision -from models.show import Show, ShowScriptType +from models.script import ( + Script, + ScriptCuts, + ScriptLine, + ScriptLinePart, + ScriptLineRevisionAssociation, + ScriptLineType, + ScriptRevision, +) +from models.show import ( + Act, + Cast, + Character, + CharacterGroup, + Scene, + Show, + ShowScriptType, +) from test.conftest import DigiScriptTestCase @@ -10,7 +26,6 @@ class TestCastStatsController(DigiScriptTestCase): def setUp(self): super().setUp() - # Create a test show with script with self._app.get_db().sessionmaker() as session: show = Show(name="Test Show", script_mode=ShowScriptType.FULL) session.add(show) @@ -27,26 +42,69 @@ def setUp(self): ) 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 + + cast_member = Cast(show_id=show.id, first_name="Jane", last_name="Doe") + session.add(cast_member) + session.flush() + self.cast_id = cast_member.id + + character = Character( + show_id=show.id, name="Protagonist", played_by=cast_member.id + ) + session.add(character) + session.flush() + self.character_id = character.id + session.commit() self._app.digi_settings.settings["current_show"].set_value(self.show_id) - def test_get_cast_stats(self): - """Test GET /api/v1/show/cast/stats. + def _make_dialogue_line( + self, session, act_id, scene_id, character_id=None, character_group_id=None + ): + """Helper: create a DIALOGUE line + part + revision association.""" + line = ScriptLine( + act_id=act_id, scene_id=scene_id, page=1, line_type=ScriptLineType.DIALOGUE + ) + session.add(line) + session.flush() + part = ScriptLinePart( + line_id=line.id, + part_index=0, + character_id=character_id, + character_group_id=character_group_id, + line_text="Test line", + ) + session.add(part) + session.flush() + assoc = ScriptLineRevisionAssociation( + revision_id=self.revision_id, line_id=line.id + ) + session.add(assoc) + session.commit() + return line.id, part.id - This tests the query at line 171-172 in cast.py: - session.scalars(select(Script).where(Script.show_id == show.id)).first() - """ + def test_get_cast_stats(self): + """Endpoint returns 200 with line_counts key.""" response = self.fetch("/api/v1/show/cast/stats") self.assertEqual(200, response.code) - response_body = tornado.escape.json_decode(response.body) - self.assertIn("line_counts", response_body) + self.assertIn("line_counts", tornado.escape.json_decode(response.body)) def test_get_cast_stats_no_script(self): - """Test GET returns error when no script exists.""" - # Create a show without a script + """Returns error when no script exists for the show.""" with self._app.get_db().sessionmaker() as session: show2 = Show(name="Show 2", script_mode=ShowScriptType.FULL) session.add(show2) @@ -57,5 +115,144 @@ def test_get_cast_stats_no_script(self): self._app.digi_settings.settings["current_show"].set_value(show2_id) response = self.fetch("/api/v1/show/cast/stats") - # Should get an error because there's no script self.assertNotEqual(200, response.code) + + def test_stats_dialogue_line_counted(self): + """A dialogue line assigned to a character with a cast member is counted.""" + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, self.act_id, self.scene_id, character_id=self.character_id + ) + + body = tornado.escape.json_decode(self.fetch("/api/v1/show/cast/stats").body) + line_counts = body["line_counts"] + + # Keys are serialised as strings in JSON + self.assertIn(str(self.cast_id), line_counts) + self.assertIn(str(self.act_id), line_counts[str(self.cast_id)]) + self.assertEqual( + 1, + line_counts[str(self.cast_id)][str(self.act_id)][str(self.scene_id)], + ) + + def test_stats_character_without_cast_excluded(self): + """A dialogue line for a character with no cast member is not counted.""" + with self._app.get_db().sessionmaker() as session: + uncast = Character(show_id=self.show_id, name="Uncast", played_by=None) + session.add(uncast) + session.flush() + uncast_id = uncast.id + session.commit() + + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, self.act_id, self.scene_id, character_id=uncast_id + ) + + body = tornado.escape.json_decode(self.fetch("/api/v1/show/cast/stats").body) + self.assertEqual({}, body["line_counts"]) + + def test_stats_cut_line_excluded(self): + """A line part with a ScriptCuts entry for the current revision is excluded.""" + with self._app.get_db().sessionmaker() as session: + _, part_id = self._make_dialogue_line( + session, self.act_id, self.scene_id, character_id=self.character_id + ) + + with self._app.get_db().sessionmaker() as session: + session.add(ScriptCuts(line_part_id=part_id, revision_id=self.revision_id)) + session.commit() + + body = tornado.escape.json_decode(self.fetch("/api/v1/show/cast/stats").body) + self.assertEqual({}, body["line_counts"]) + + def test_stats_stage_direction_excluded(self): + """Non-dialogue lines (stage directions) are not counted.""" + with self._app.get_db().sessionmaker() as session: + line = ScriptLine( + act_id=self.act_id, + scene_id=self.scene_id, + page=1, + line_type=ScriptLineType.STAGE_DIRECTION, + ) + session.add(line) + session.flush() + assoc = ScriptLineRevisionAssociation( + revision_id=self.revision_id, line_id=line.id + ) + session.add(assoc) + session.commit() + + body = tornado.escape.json_decode(self.fetch("/api/v1/show/cast/stats").body) + self.assertEqual({}, body["line_counts"]) + + def test_stats_character_group_expands_to_members(self): + """A character group line counts each cast-assigned group member separately.""" + with self._app.get_db().sessionmaker() as session: + cast2 = Cast(show_id=self.show_id, first_name="John", last_name="Smith") + session.add(cast2) + session.flush() + cast2_id = cast2.id + + char2 = Character(show_id=self.show_id, name="Sidekick", played_by=cast2_id) + session.add(char2) + session.flush() + + char1 = session.get(Character, self.character_id) + group = CharacterGroup(show_id=self.show_id, name="Ensemble") + group.characters = [char1, char2] + session.add(group) + session.flush() + group_id = group.id + session.commit() + + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, self.act_id, self.scene_id, character_group_id=group_id + ) + + body = tornado.escape.json_decode(self.fetch("/api/v1/show/cast/stats").body) + line_counts = body["line_counts"] + + self.assertEqual( + 1, + line_counts[str(self.cast_id)][str(self.act_id)][str(self.scene_id)], + ) + self.assertEqual( + 1, + line_counts[str(cast2_id)][str(self.act_id)][str(self.scene_id)], + ) + + def test_stats_multiple_acts_and_scenes(self): + """Counts are correctly attributed per act and scene.""" + with self._app.get_db().sessionmaker() as session: + act2 = Act(show_id=self.show_id, name="Act 2") + session.add(act2) + session.flush() + act2_id = act2.id + + scene2 = Scene(show_id=self.show_id, act_id=act2.id, name="Scene 2") + session.add(scene2) + session.flush() + scene2_id = scene2.id + session.commit() + + with self._app.get_db().sessionmaker() as session: + # One line in act1/scene1, two lines in act2/scene2 + self._make_dialogue_line( + session, self.act_id, self.scene_id, character_id=self.character_id + ) + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, act2_id, scene2_id, character_id=self.character_id + ) + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, act2_id, scene2_id, character_id=self.character_id + ) + + body = tornado.escape.json_decode(self.fetch("/api/v1/show/cast/stats").body) + counts = body["line_counts"][str(self.cast_id)] + + self.assertEqual(1, counts[str(self.act_id)][str(self.scene_id)]) + self.assertEqual(2, counts[str(act2_id)][str(scene2_id)]) diff --git a/server/test/controllers/api/v1/show/test_characters.py b/server/test/controllers/api/v1/show/test_characters.py index d4674d25..2bc5dc57 100644 --- a/server/test/controllers/api/v1/show/test_characters.py +++ b/server/test/controllers/api/v1/show/test_characters.py @@ -4,8 +4,10 @@ from models.mics import Microphone, MicrophoneAllocation from models.script import ( Script, + ScriptCuts, ScriptLine, ScriptLinePart, + ScriptLineRevisionAssociation, ScriptLineType, ScriptRevision, ) @@ -18,7 +20,6 @@ class TestCharacterStatsController(DigiScriptTestCase): def setUp(self): super().setUp() - # Create a test show with script with self._app.get_db().sessionmaker() as session: show = Show(name="Test Show", script_mode=ShowScriptType.FULL) session.add(show) @@ -35,26 +36,62 @@ def setUp(self): ) 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="Protagonist") + session.add(character) + session.flush() + self.character_id = character.id + session.commit() self._app.digi_settings.settings["current_show"].set_value(self.show_id) - def test_get_character_stats(self): - """Test GET /api/v1/show/character/stats. + def _make_dialogue_line( + self, session, act_id, scene_id, character_id=None, character_group_id=None + ): + """Helper: create a DIALOGUE line + part + revision association.""" + line = ScriptLine( + act_id=act_id, scene_id=scene_id, page=1, line_type=ScriptLineType.DIALOGUE + ) + session.add(line) + session.flush() + part = ScriptLinePart( + line_id=line.id, + part_index=0, + character_id=character_id, + character_group_id=character_group_id, + line_text="Test line", + ) + session.add(part) + session.flush() + assoc = ScriptLineRevisionAssociation( + revision_id=self.revision_id, line_id=line.id + ) + session.add(assoc) + session.commit() + return line.id, part.id - This tests the query at line 183-184 in characters.py: - session.scalars(select(Script).where(Script.show_id == show.id)).first() - """ + def test_get_character_stats(self): + """Endpoint returns 200 with line_counts key.""" response = self.fetch("/api/v1/show/character/stats") self.assertEqual(200, response.code) - response_body = tornado.escape.json_decode(response.body) - self.assertIn("line_counts", response_body) + self.assertIn("line_counts", tornado.escape.json_decode(response.body)) def test_get_character_stats_no_script(self): - """Test GET returns error when no script exists.""" - # Create a show without a script + """Returns error when no script exists for the show.""" with self._app.get_db().sessionmaker() as session: show2 = Show(name="Show 2", script_mode=ShowScriptType.FULL) session.add(show2) @@ -65,9 +102,134 @@ def test_get_character_stats_no_script(self): self._app.digi_settings.settings["current_show"].set_value(show2_id) response = self.fetch("/api/v1/show/character/stats") - # Should get an error because there's no script self.assertNotEqual(200, response.code) + def test_stats_dialogue_line_counted(self): + """A dialogue line assigned to a character is counted under that character.""" + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, self.act_id, self.scene_id, character_id=self.character_id + ) + + body = tornado.escape.json_decode( + self.fetch("/api/v1/show/character/stats").body + ) + line_counts = body["line_counts"] + + self.assertIn(str(self.character_id), line_counts) + self.assertEqual( + 1, + line_counts[str(self.character_id)][str(self.act_id)][str(self.scene_id)], + ) + + def test_stats_cut_line_excluded(self): + """A line part with a ScriptCuts entry for the current revision is excluded.""" + with self._app.get_db().sessionmaker() as session: + _, part_id = self._make_dialogue_line( + session, self.act_id, self.scene_id, character_id=self.character_id + ) + + with self._app.get_db().sessionmaker() as session: + session.add(ScriptCuts(line_part_id=part_id, revision_id=self.revision_id)) + session.commit() + + body = tornado.escape.json_decode( + self.fetch("/api/v1/show/character/stats").body + ) + self.assertEqual({}, body["line_counts"]) + + def test_stats_stage_direction_excluded(self): + """Non-dialogue lines (stage directions) are not counted.""" + with self._app.get_db().sessionmaker() as session: + line = ScriptLine( + act_id=self.act_id, + scene_id=self.scene_id, + page=1, + line_type=ScriptLineType.STAGE_DIRECTION, + ) + session.add(line) + session.flush() + assoc = ScriptLineRevisionAssociation( + revision_id=self.revision_id, line_id=line.id + ) + session.add(assoc) + session.commit() + + body = tornado.escape.json_decode( + self.fetch("/api/v1/show/character/stats").body + ) + self.assertEqual({}, body["line_counts"]) + + def test_stats_character_group_expands_to_members(self): + """A character group line counts each member character separately.""" + with self._app.get_db().sessionmaker() as session: + char2 = Character(show_id=self.show_id, name="Sidekick") + session.add(char2) + session.flush() + char2_id = char2.id + + char1 = session.get(Character, self.character_id) + group = CharacterGroup(show_id=self.show_id, name="Ensemble") + group.characters = [char1, char2] + session.add(group) + session.flush() + group_id = group.id + session.commit() + + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, self.act_id, self.scene_id, character_group_id=group_id + ) + + body = tornado.escape.json_decode( + self.fetch("/api/v1/show/character/stats").body + ) + line_counts = body["line_counts"] + + self.assertEqual( + 1, + line_counts[str(self.character_id)][str(self.act_id)][str(self.scene_id)], + ) + self.assertEqual( + 1, + line_counts[str(char2_id)][str(self.act_id)][str(self.scene_id)], + ) + + def test_stats_multiple_acts_and_scenes(self): + """Counts are correctly attributed per act and scene.""" + with self._app.get_db().sessionmaker() as session: + act2 = Act(show_id=self.show_id, name="Act 2") + session.add(act2) + session.flush() + act2_id = act2.id + + scene2 = Scene(show_id=self.show_id, act_id=act2.id, name="Scene 2") + session.add(scene2) + session.flush() + scene2_id = scene2.id + session.commit() + + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, self.act_id, self.scene_id, character_id=self.character_id + ) + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, act2_id, scene2_id, character_id=self.character_id + ) + with self._app.get_db().sessionmaker() as session: + self._make_dialogue_line( + session, act2_id, scene2_id, character_id=self.character_id + ) + + body = tornado.escape.json_decode( + self.fetch("/api/v1/show/character/stats").body + ) + counts = body["line_counts"][str(self.character_id)] + + self.assertEqual(1, counts[str(self.act_id)][str(self.scene_id)]) + self.assertEqual(2, counts[str(act2_id)][str(scene2_id)]) + class TestCharacterMergeController(DigiScriptTestCase): """Test suite for POST /api/v1/show/character/merge endpoint."""