perf: eager load EmailTeam relationship in Tool model to eliminate N+1 queries#1957
Merged
crivetimihai merged 1 commit intomainfrom Jan 7, 2026
Merged
Conversation
09c252d to
2acb317
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Tool model to use SQLAlchemy relationship-based eager loading for the EmailTeam association, replacing manual team name fetching code throughout the service layer. The changes aim to eliminate N+1 query problems by leveraging SQLAlchemy's built-in eager loading capabilities.
Key changes:
- Added
email_teamrelationship on Tool model withlazy="joined"for automatic eager loading of active teams - Introduced read-only
teamproperty that returns team name from the eagerly-loaded relationship - Removed manual team name queries and the
_get_team_namemethod from tool_service.py
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| mcpgateway/db.py | Adds email_team relationship with eager loading and team property to Tool model |
| mcpgateway/services/tool_service.py | Removes manual team fetching logic and simplifies query code to rely on relationship loading |
| tests/unit/mcpgateway/services/test_tool_service.py | Updates test mocks to align with new structure and removes now-unused imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -199,6 +200,7 @@ def mock_tool(mock_gateway): | |||
| tool.custom_name_slug = "test-tool" | |||
| tool.display_name = None | |||
| tool.tags = [] | |||
| tool.team = None | |||
There was a problem hiding this comment.
The team attribute is being set here, but in the new model definition (mcpgateway/db.py:2823-2830), team is now a read-only property without a setter. This will raise an AttributeError at runtime when trying to set this value. The mock should set tool.team_id and tool.email_team instead, or the Tool model needs to be updated with a setter for the team property.
Suggested change
| tool.team = None | |
| tool.team_id = None | |
| tool.email_team = None |
mcpgateway/db.py
Outdated
| "EmailTeam", | ||
| primaryjoin="and_(Tool.team_id == EmailTeam.id, EmailTeam.is_active == True)", | ||
| foreign_keys=[team_id], | ||
| lazy="joined", |
There was a problem hiding this comment.
The relationship should include viewonly=True parameter since it uses a filtered primaryjoin condition (EmailTeam.is_active == True). Without this, SQLAlchemy may attempt to use this relationship for write operations, which could lead to unexpected behavior. The relationship should be defined as read-only since it includes filtering logic.
Suggested change
| lazy="joined", | |
| lazy="joined", | |
| viewonly=True, |
mcpgateway/db.py
Outdated
Comment on lines
+2815
to
+2820
| # Relationship for eager loading team names (only active teams) | ||
| email_team: Mapped[Optional["EmailTeam"]] = relationship( | ||
| "EmailTeam", | ||
| primaryjoin="and_(Tool.team_id == EmailTeam.id, EmailTeam.is_active == True)", | ||
| foreign_keys=[team_id], | ||
| lazy="joined", |
There was a problem hiding this comment.
Using lazy="joined" causes SQLAlchemy to automatically add a LEFT OUTER JOIN to email_teams on every Tool query, even when the team name is not needed. This can negatively impact performance, especially for high-traffic operations like tool invocations where only the team_id is required for logging. Consider using lazy="selectin" or lazy="select" instead, which would only load the team when explicitly accessed. Alternatively, use explicit joinedload() options in queries that actually need the team name (like in list_tools).
Suggested change
| # Relationship for eager loading team names (only active teams) | |
| email_team: Mapped[Optional["EmailTeam"]] = relationship( | |
| "EmailTeam", | |
| primaryjoin="and_(Tool.team_id == EmailTeam.id, EmailTeam.is_active == True)", | |
| foreign_keys=[team_id], | |
| lazy="joined", | |
| # Relationship for loading team names (only active teams) | |
| email_team: Mapped[Optional["EmailTeam"]] = relationship( | |
| "EmailTeam", | |
| primaryjoin="and_(Tool.team_id == EmailTeam.id, EmailTeam.is_active == True)", | |
| foreign_keys=[team_id], | |
| lazy="selectin", |
2acb317 to
e6f1033
Compare
…1 queries Add email_team relationship with lazy="joined" to Tool model, replacing manual team name lookups with a single JOIN query. This eliminates N+1 query patterns when fetching tools with their team names. Changes: - Add email_team relationship to Tool class with lazy="joined" - Add primaryjoin condition to filter only active teams (is_active == True) - Add team property to Tool class for convenient team name access - Document the team property in Tool class docstring - Remove _get_team_name helper method from tool_service.py - Remove manual team_map batch loading in list_tools - Remove manual outerjoin/add_columns in list_server_tools and list_tools_for_user - Update tests to use new query pattern (.scalars().all()) - Fix flake8 issues in test file 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Jonathan Springer <jonpspri@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
e6f1033 to
1e7d003
Compare
Member
Review and Fixes AppliedIssues Found and Fixed
Final Design
Test Results
|
crivetimihai
reviewed
Jan 7, 2026
crivetimihai
pushed a commit
that referenced
this pull request
Jan 9, 2026
…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>
crivetimihai
added a commit
that referenced
this pull request
Jan 9, 2026
* perf: eager load EmailTeam relationship in Server model to eliminate 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(#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 #1962 Signed-off-by: Jonathan Springer <jps@s390x.com> * perf(#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 #1962 which listed toggle_server_status as an affected function with medium impact. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix(#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>
kcostell06
pushed a commit
to kcostell06/mcp-context-forge
that referenced
this pull request
Feb 24, 2026
…1 queries (IBM#1957) Add email_team relationship with lazy="joined" to Tool model, replacing manual team name lookups with a single JOIN query. This eliminates N+1 query patterns when fetching tools with their team names. Changes: - Add email_team relationship to Tool class with lazy="joined" - Add primaryjoin condition to filter only active teams (is_active == True) - Add team property to Tool class for convenient team name access - Document the team property in Tool class docstring - Remove _get_team_name helper method from tool_service.py - Remove manual team_map batch loading in list_tools - Remove manual outerjoin/add_columns in list_server_tools and list_tools_for_user - Update tests to use new query pattern (.scalars().all()) - Fix flake8 issues in test file Signed-off-by: Jonathan Springer <jonpspri@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
kcostell06
pushed a commit
to kcostell06/mcp-context-forge
that referenced
this pull request
Feb 24, 2026
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
email_teamrelationship to theToolmodel withlazy="joined"for automatic eager loadingteamproperty onToolthat returns the team name from the eagerly-loaded relationship_get_team_namemethod) and explicit JOIN queries intool_service.pyThis simplifies the codebase while improving query efficiency by leveraging SQLAlchemy's relationship eager loading instead of manual queries.
Closes #1964