fix(#1962): resolve N+1 query fixes breaking tests by jonpspri · Pull Request #1975 · IBM/mcp-context-forge

jonpspri · 2026-01-08T12:28:01Z

Summary

Fix convert_server_to_read to explicitly build server dict from attributes instead of using __dict__.copy(), which can return empty dict with SQLAlchemy's scalars().all() eager loading pattern
Add missing joinedload(DbServer.email_team) to get_server and update_server methods for consistent team name loading
Update test mocks to accept new options parameter for db.get() calls
Update mock_server fixture with explicit None values for optional tracking fields to prevent MagicMock auto-creation

Closes #1962

Copilot

Pull request overview

This pull request fixes N+1 query issues introduced by previous optimizations to the server service. The changes address the problem where __dict__.copy() returns an empty dictionary when using SQLAlchemy's scalars().all() eager loading pattern, and ensures consistent team name loading across all server operations.

Key Changes

Refactored convert_server_to_read to explicitly build server dictionaries from attributes instead of using __dict__.copy()
Added new email_team relationship and team property to the Server model for efficient team name loading
Added joinedload(DbServer.email_team) to query methods (list_servers, list_servers_for_user, get_server, update_server)
Removed the _get_team_name helper method that was causing N+1 queries

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
mcpgateway/db.py	Added `email_team` relationship and `team` property to Server model for efficient team name loading via joinedload
mcpgateway/services/server_service.py	Refactored `convert_server_to_read` to build dict explicitly; added `joinedload(DbServer.email_team)` to list/get/update operations; removed `_get_team_name` method
tests/unit/mcpgateway/services/test_server_service.py	Updated mock_server fixture with explicit None values; updated test mocks to accept `options` parameter; added regression test for team name loading

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-08T12:34:12Z

tests/unit/mcpgateway/services/test_server_service.py

        test_db.get.assert_called_once_with(DbServer, 1)
-        # commit called twice: once for status change, once in _get_team_name to release transaction
-        assert test_db.commit.call_count == 2
+        # commit called once for status change (team name loaded via email_team relationship property)
+        assert test_db.commit.call_count == 1


The comment on line 818-819 correctly notes that team name is loaded via the email_team relationship property, but the assertion on line 817 still expects test_db.get.assert_called_once_with(DbServer, 1) without an options parameter. Since toggle_server_status accesses server.team property (which uses the email_team relationship), the service method should use eager loading with joinedload(DbServer.email_team) to avoid N+1 queries. The test should either be updated to expect the options parameter (like line 560) or the service method needs to be updated to include eager loading.

Copilot · 2026-01-08T12:34:12Z

mcpgateway/db.py

+        """Return the team name from the eagerly-loaded email_team relationship.
+


The docstring states "Return the team name from the eagerly-loaded email_team relationship," but the relationship uses default lazy loading according to the comment on lines 4243-4245. The docstring should clarify that the relationship may be lazily loaded unless explicitly eager-loaded with joinedload(DbServer.email_team). Consider updating to: "Return the team name from the email_team relationship. The relationship uses lazy loading by default and should be eagerly loaded with joinedload for optimal performance."

Suggested change

"""Return the team name from the eagerly-loaded email_team relationship.

"""Return the team name from the email_team relationship.

The email_team relationship uses lazy loading by default and may trigger

an additional query when accessed. For list or admin views, it should be

eagerly loaded with joinedload(Server.email_team) for optimal performance.

…N+1 queries (#1957) Add email_team relationship and team property to Server model, replacing the _get_team_name helper method. Use joinedload for list operations to fetch team names in a single query instead of per-server lookups. Signed-off-by: Jonathan Springer <jps@s390x.com>

- Fix convert_server_to_read to explicitly build server dict from attributes instead of using __dict__.copy(), which can return empty dict with SQLAlchemy's scalars().all() eager loading pattern - Add missing joinedload(DbServer.email_team) to get_server and update_server methods for consistent team name loading - Update test mocks to accept new options parameter for db.get() calls - Update mock_server fixture with explicit None values for optional tracking fields to prevent MagicMock auto-creation Closes #1962 Signed-off-by: Jonathan Springer <jps@s390x.com>

Add selectinload/joinedload options to db.get() call in toggle_server_status to prevent N+1 queries when accessing server relationships (tools, resources, prompts, a2a_agents, email_team). This completes the N+1 fix for issue #1962 which listed toggle_server_status as an affected function with medium impact. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

- admin.py: Remove manual team assignment loop that would fail since Server.team is now a read-only @Property. Add joinedload for email_team relationship instead. - export_service.py: Add selectinload for tools relationship in _export_selected_servers to prevent N+1 queries when accessing db_server.tools in a loop. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

crivetimihai · 2026-01-09T09:59:37Z

Additional Fixes Applied During Review

This PR was rebased onto main and extended with additional fixes discovered during code review.

Commits Added

Commit	Description
`cf95ec61`	perf(#1962): add eager loading to toggle_server_status - The original PR missed this function which was listed in issue #1962 as affected. Added `selectinload`/`joinedload` options to `db.get()` call.
`33c97b8e`	fix(#1962): fix team property assignment and export N+1 query - Two fixes: (1) admin.py was assigning `p.team = ...` which would fail at runtime since `Server.team` is now a read-only `@property`; (2) export_service.py had N+1 queries accessing `db_server.tools` in a loop without eager loading.

Changes Summary

mcpgateway/services/server_service.py

toggle_server_status(): Added eager loading options to db.get() (was missing from original PR)

mcpgateway/admin.py

Removed manual team name fetching loop that would break with new @property
Added joinedload(DbServer.email_team) to query options
Team names now loaded via relationship property automatically

mcpgateway/services/export_service.py

Added selectinload(DbServer.tools) to _export_selected_servers() batch query
Fixes N+1 when accessing db_server.tools in loop

tests/unit/mcpgateway/services/test_server_service.py

Updated test_toggle_server_status to expect options=ANY parameter

Out of Scope

Gateway N+1 queries (get_gateway, toggle_gateway_status, etc.) are tracked in #1994 - the Gateway model needs the same email_team relationship pattern added.

Test Results

✅ 4249 unit tests pass
✅ 149 admin/export tests pass
✅ 79 performance tests pass

Add email_team relationship and team property to Gateway model, mirroring the optimization applied to Server model in PR #1962 and #1975. Changes: - Add email_team relationship to Gateway model (db.py) - Add team property for convenient access to team name - Update gateway_service.py to use joinedload(DbGateway.email_team) - Remove _get_team_name helper method (no longer needed) - Update tests to mock db.execute instead of db.get This reduces database queries by ~50% for gateway operations that need team information, eliminating the N+1 query pattern. Closes #1994 Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>

Add email_team relationship and team property to Gateway model, mirroring the optimization applied to Server model in PR #1962 and #1975. Changes: - Add email_team relationship to Gateway model (db.py) - Add team property for convenient access to team name - Update gateway_service.py to use joinedload(DbGateway.email_team) - Remove _get_team_name helper method (no longer needed) - Update tests to mock db.execute instead of db.get This reduces database queries by ~50% for gateway operations that need team information, eliminating the N+1 query pattern. Closes #1994 Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>

* perf: eager load EmailTeam relationship in Server model to eliminate N+1 queries (IBM#1957) Add email_team relationship and team property to Server model, replacing the _get_team_name helper method. Use joinedload for list operations to fetch team names in a single query instead of per-server lookups. Signed-off-by: Jonathan Springer <jps@s390x.com> * fix(IBM#1962): resolve N+1 query fixes breaking tests - Fix convert_server_to_read to explicitly build server dict from attributes instead of using __dict__.copy(), which can return empty dict with SQLAlchemy's scalars().all() eager loading pattern - Add missing joinedload(DbServer.email_team) to get_server and update_server methods for consistent team name loading - Update test mocks to accept new options parameter for db.get() calls - Update mock_server fixture with explicit None values for optional tracking fields to prevent MagicMock auto-creation Closes IBM#1962 Signed-off-by: Jonathan Springer <jps@s390x.com> * perf(IBM#1962): add eager loading to toggle_server_status Add selectinload/joinedload options to db.get() call in toggle_server_status to prevent N+1 queries when accessing server relationships (tools, resources, prompts, a2a_agents, email_team). This completes the N+1 fix for issue IBM#1962 which listed toggle_server_status as an affected function with medium impact. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(IBM#1962): fix team property assignment and export N+1 query - admin.py: Remove manual team assignment loop that would fail since Server.team is now a read-only @Property. Add joinedload for email_team relationship instead. - export_service.py: Add selectinload for tools relationship in _export_selected_servers to prevent N+1 queries when accessing db_server.tools in a loop. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> --------- Signed-off-by: Jonathan Springer <jps@s390x.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>

Add email_team relationship and team property to Gateway model, mirroring the optimization applied to Server model in PR IBM#1962 and IBM#1975. Changes: - Add email_team relationship to Gateway model (db.py) - Add team property for convenient access to team name - Update gateway_service.py to use joinedload(DbGateway.email_team) - Remove _get_team_name helper method (no longer needed) - Update tests to mock db.execute instead of db.get This reduces database queries by ~50% for gateway operations that need team information, eliminating the N+1 query pattern. Closes IBM#1994 Signed-off-by: Keval Mahajan <mahajankeval23@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>

Copilot AI review requested due to automatic review settings January 8, 2026 12:28

jonpspri requested review from crivetimihai, kevalmahajan and madhav165 as code owners January 8, 2026 12:28

jonpspri marked this pull request as draft January 8, 2026 12:28

Copilot started reviewing on behalf of jonpspri January 8, 2026 12:29 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

jonpspri force-pushed the 1962-nplus1-queries branch 2 times, most recently from b19b4da to 44a36c8 Compare January 8, 2026 14:50

jonpspri marked this pull request as ready for review January 8, 2026 15:06

jonpspri force-pushed the 1962-nplus1-queries branch 2 times, most recently from 8403e6a to a9e4203 Compare January 8, 2026 18:22

crivetimihai self-assigned this Jan 9, 2026

Jonathan Springer and others added 3 commits January 9, 2026 02:43

crivetimihai force-pushed the 1962-nplus1-queries branch from a9e4203 to cf95ec6 Compare January 9, 2026 09:37

crivetimihai mentioned this pull request Jan 9, 2026

[PERFORMANCE]: Fix N+1 queries in Gateway single-entity retrieval functions #1994

Closed

crivetimihai merged commit 97924e9 into main Jan 9, 2026
52 checks passed

crivetimihai deleted the 1962-nplus1-queries branch January 9, 2026 10:01

kevalmahajan mentioned this pull request Jan 20, 2026

Optimization: Fix Gateway N+1 Queries via Eager Loading #2204

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#1962): resolve N+1 query fixes breaking tests#1975

fix(#1962): resolve N+1 query fixes breaking tests#1975
crivetimihai merged 4 commits intomainfrom
1962-nplus1-queries

jonpspri commented Jan 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

jonpspri Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

jonpspri Jan 8, 2026

Uh oh!

crivetimihai commented Jan 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		"""Return the team name from the eagerly-loaded email_team relationship.

-        """Return the team name from the eagerly-loaded email_team relationship.
+        """Return the team name from the email_team relationship.
+        The email_team relationship uses lazy loading by default and may trigger
+        an additional query when accessed. For list or admin views, it should be
+        eagerly loaded with joinedload(Server.email_team) for optimal performance.

Conversation

jonpspri commented Jan 8, 2026

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

jonpspri Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

jonpspri Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

crivetimihai commented Jan 9, 2026

Additional Fixes Applied During Review

Commits Added

Changes Summary

Out of Scope

Test Results

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants