diff --git a/app/routes/tryouts.py b/app/routes/tryouts.py index 2c545a7..84b6e7b 100644 --- a/app/routes/tryouts.py +++ b/app/routes/tryouts.py @@ -37,6 +37,25 @@ def can_manage(): return isinstance(current_user, (Admin, Manager)) +def _users_by_id(user_ids): + """Load these users in one query, keyed by id. + + Replaces the `User.query.get()`-inside-a-loop that view_tryout used in + three separate places (PERF-001). Missing ids are simply absent from + the result, which is what a per-row get() returning None amounted to. + + Args: + user_ids: Iterable of primary keys, may repeat and may be empty. + + Returns: + dict[int, User] + """ + wanted = {user_id for user_id in user_ids if user_id} + if not wanted: + return {} + return {user.id: user for user in User.query.filter(User.id.in_(wanted)).all()} + + @tryouts_bp.route('') @login_required def list_tryouts(): @@ -289,19 +308,29 @@ def view_tryout(tryout_id): flash(_('You do not have permission to view this tryout.'), 'danger') return redirect(url_for('tryouts.list_tryouts')) + # Everything below used to run one query per row (PERF-001): one + # User.query.get() per registration, one Evaluation lookup per player, + # one TeamMember query per team and one more User.query.get() per + # member. Thirty registrants and four teams put this page well past a + # hundred round trips, on unindexed columns. registrations = TryoutRegistration.query.filter_by(tryout_id=tryout_id).all() - registered_players = [User.query.get(r.player_id) for r in registrations if r.player_id] + registered_player_ids = [r.player_id for r in registrations if r.player_id] + players_by_id = _users_by_id(registered_player_ids) + registered_players = [ + players_by_id[player_id] + for player_id in registered_player_ids + if player_id in players_by_id + ] evaluations = Evaluation.query.filter_by(tryout_id=tryout_id).all() player_eval_status = {} if current_user.can_evaluate(): - for p in registered_players: - existing = Evaluation.query.filter_by( - tryout_id=tryout_id, - player_id=p.id, - evaluator_id=current_user.id, - ).first() - player_eval_status[p.id] = existing is not None + evaluated_by_me = { + row.player_id + for row in evaluations + if row.evaluator_id == current_user.id and row.player_id + } + player_eval_status = {p.id: p.id in evaluated_by_me for p in registered_players} is_registered = ( TryoutRegistration.query.filter_by( @@ -312,17 +341,16 @@ def view_tryout(tryout_id): ) teams = Team.query.filter_by(tryout_id=tryout_id).all() - team_data = [] - for team in teams: - members = TeamMember.query.filter_by(team_id=team.id).all() - team_data.append( - { - 'team': team, - 'members': [ - {'player': User.query.get(m.player_id), 'position': m.position} for m in members - ], - } - ) + team_ids = [team.id for team in teams] + members_by_team = {} + if team_ids: + member_rows = TeamMember.query.filter(TeamMember.team_id.in_(team_ids)).all() + member_players = _users_by_id([m.player_id for m in member_rows if m.player_id]) + for row in member_rows: + members_by_team.setdefault(row.team_id, []).append( + {'player': member_players.get(row.player_id), 'position': row.position} + ) + team_data = [{'team': team, 'members': members_by_team.get(team.id, [])} for team in teams] can_edit = current_user.can_manage_this_tryout(tryout) @@ -382,15 +410,18 @@ def view_tryout(tryout_id): else [], } elif match.match_type == 'player_vs_player': + # Filtered from the list already in hand. Asking the dynamic + # relationship again cost two more round trips per match for + # rows that were loaded a dozen lines above. team1_players = [ {'name': p.player.username, 'position': p.position} - for p in match.participants.filter_by(team_side=1).all() - if p.player + for p in all_participants + if p.team_side == 1 and p.player ] team2_players = [ {'name': p.player.username, 'position': p.position} - for p in match.participants.filter_by(team_side=2).all() - if p.player + for p in all_participants + if p.team_side == 2 and p.player ] participants = { 'team1': 'Team 1', diff --git a/tests/test_query_shape.py b/tests/test_query_shape.py index 593516f..3557788 100644 --- a/tests/test_query_shape.py +++ b/tests/test_query_shape.py @@ -231,3 +231,84 @@ class TestPendingEvaluations: db.session.commit() assert self._pending(client) == 1 + + +class TestViewTryout: + """PERF-001 — the most-visited page in the application ran one query + per registration, one per player evaluated, one per team, and one per + team member. Thirty registrants and four teams put it past a hundred + round trips for a single render.""" + + @pytest.fixture + def populated_tryout(self, app, make_user): + """A tryout with ten registrants split across two teams.""" + from app.models import Match, MatchParticipant, Team, TeamMember + + admin_id = make_user('admin') + player_ids = [make_user('player') for _ in range(10)] + + with app.app_context(): + tryout = Tryout( + title='Spring', game='Valorant', date=date(2030, 3, 1), created_by=admin_id + ) + db.session.add(tryout) + db.session.flush() + + for player_id in player_ids: + db.session.add(TryoutRegistration(tryout_id=tryout.id, player_id=player_id)) + + for index in range(2): + team = Team(tryout_id=tryout.id, name=f'Team {index}', created_by=admin_id) + db.session.add(team) + db.session.flush() + for player_id in player_ids[index * 5 : (index + 1) * 5]: + db.session.add(TeamMember(team_id=team.id, player_id=player_id)) + + match = Match( + tryout_id=tryout.id, + title='Scrim', + date=date(2030, 3, 2), + match_type='player_vs_player', + created_by=admin_id, + ) + db.session.add(match) + db.session.flush() + for side, player_id in enumerate(player_ids[:4]): + db.session.add( + MatchParticipant( + match_id=match.id, player_id=player_id, team_side=(side % 2) + 1 + ) + ) + db.session.commit() + return tryout.id, admin_id, player_ids + + def test_the_page_still_shows_everyone(self, app, client, as_role, populated_tryout): + tryout_id, _admin_id, player_ids = populated_tryout + as_role('admin') + + body = client.get(f'/tryouts/{tryout_id}').get_data(as_text=True) + + with app.app_context(): + usernames = [db.session.get(User, pid).username for pid in player_ids] + for username in usernames: + assert username in body, f'{username} is missing from the page' + + def test_the_cost_does_not_grow_with_the_squad( + self, app, client, as_role, populated_tryout, count_queries + ): + tryout_id, _admin_id, _player_ids = populated_tryout + as_role('admin') + + with app.app_context(): + counter = count_queries() + try: + response = client.get(f'/tryouts/{tryout_id}') + finally: + counter.stop() + + assert response.status_code == 200 + # Ten registrants, two teams of five, one match of four. The old + # shape spent more than thirty queries on those rows alone; the + # budget here is deliberately loose but far below that, and it can + # only be lowered. + assert 1 <= counter.total <= 25, f'{counter.total} SELECTs to render one tryout'