Skip to content

Remove redundant db calls in PermissionService#2696

Merged
crivetimihai merged 1 commit intomainfrom
remove-redundant-db-call
Feb 7, 2026
Merged

Remove redundant db calls in PermissionService#2696
crivetimihai merged 1 commit intomainfrom
remove-redundant-db-call

Conversation

@madhav165
Copy link
Copy Markdown
Collaborator

@madhav165 madhav165 commented Feb 4, 2026

Bug-fix PR

Closes #2695


Summary

Fixes redundant database queries in PermissionService.check_permission() that caused unnecessary performance overhead on every permission check.

Two separate redundancies were identified and fixed:

  1. UserRole table queried twice: Once for permissions, again for audit logging
  2. EmailTeamMember table queried twice: Once for membership check, again for role lookup

Reproduction Steps

Issue: Redundant DB calls in permission checks

To observe the issue:

  1. Enable query logging in SQLAlchemy
  2. Call any endpoint that triggers check_permission() with audit_enabled=True
  3. Observe duplicate SELECT statements for user_roles and email_team_members tables

Root Cause

Redundancy #1 (UserRole):

  • check_permission() called get_user_permissions() which internally called _get_user_roles() to fetch UserRole objects
  • get_user_permissions() extracted only the permission strings and discarded the role objects
  • Later, _get_roles_for_audit() called _get_user_roles() again to get role names for audit logging
  • Same query executed twice per permission check when auditing enabled

Redundancy #2 (EmailTeamMember):

  • _check_team_fallback_permissions() called _is_team_member() to check if user was a team member
  • Then immediately called _get_user_team_role() to get the user's role
  • Both methods queried the same email_team_members table with identical filters

Fix Description

Fix #1: Modified check_permission() to:

  • Call _get_user_roles() directly once
  • Extract permissions inline from the fetched role objects
  • Reuse the same role objects to build audit data (no second query)

Fix #2: Modified _check_team_fallback_permissions() to:

  • Call only _get_user_team_role() which returns None if not a member
  • Check for None instead of separate membership query
  • Refactored _is_team_member() to delegate to _get_user_team_role() for backward compatibility

DB Call Reduction:

User Type Before After Saved
Non-admin with audit 5 3 2
Non-admin (teams.* fallback) 7 5 2
Non-admin without audit 3 2 1

Verification

Check Command Status
Lint suite make lint Pass
Unit tests make test Pass (50/50)
Coverage >= 80 % make coverage
Manual regression no longer fails Query logging shows single queries

MCP Compliance (if relevant)

  • Matches current MCP spec
  • No breaking change to MCP clients

Checklist

  • Code formatted (make black isort pre-commit)
  • No secrets/credentials committed
  • Updated tests to mock _get_user_roles instead of get_user_permissions

@madhav165 madhav165 marked this pull request as draft February 4, 2026 10:39
@madhav165 madhav165 marked this pull request as ready for review February 4, 2026 12:06
@crivetimihai crivetimihai self-assigned this Feb 6, 2026
@crivetimihai crivetimihai added this to the Release 1.0.0-RC1 milestone Feb 7, 2026
@crivetimihai crivetimihai force-pushed the remove-redundant-db-call branch 2 times, most recently from 8156853 to dc95145 Compare February 7, 2026 16:15
Closes #2695

- check_permission() now calls _get_user_roles() once and reuses results
  for both permission extraction and audit logging (was calling
  get_user_permissions then _get_roles_for_audit separately)
- _check_team_fallback_permissions() uses _get_user_team_role() directly
  instead of calling _is_team_member() then _get_user_team_role()
- _is_team_member() delegates to _get_user_team_role() for backward compat
- Remove now-unused _get_roles_for_audit() method
- Update tests to mock _get_user_roles instead of get_user_permissions

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
@crivetimihai crivetimihai force-pushed the remove-redundant-db-call branch from dc95145 to b678a59 Compare February 7, 2026 16:27
@crivetimihai crivetimihai merged commit 591f08a into main Feb 7, 2026
50 of 51 checks passed
@crivetimihai crivetimihai deleted the remove-redundant-db-call branch February 7, 2026 16:38
kcostell06 pushed a commit to kcostell06/mcp-context-forge that referenced this pull request Feb 24, 2026
Closes IBM#2695

- check_permission() now calls _get_user_roles() once and reuses results
  for both permission extraction and audit logging (was calling
  get_user_permissions then _get_roles_for_audit separately)
- _check_team_fallback_permissions() uses _get_user_team_role() directly
  instead of calling _is_team_member() then _get_user_team_role()
- _is_team_member() delegates to _get_user_team_role() for backward compat
- Remove now-unused _get_roles_for_audit() method
- Update tests to mock _get_user_roles instead of get_user_permissions

Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Redundant database queries in PermissionService.check_permission()

2 participants