fix: filter server groups by ownership and sharing status

[lkmatsumura]

fix: get_all_server_groups returning all server for admin user in server mode

fix #9933

• rewriting to return only admin user groups and groups with shared servers like the previous behaviour

Summary by CodeRabbit

• Improvements
  • Server group visibility now more accurately follows the "hide shared servers" setting: when enabled you’ll see only your own groups; when disabled you’ll see groups you own or those containing shared servers. This makes group lists more consistent with your preferences.

[coderabbitai]

Walkthrough

Refactors ServerGroupView.get_all_server_groups() to use helper predicates and list comprehensions; changes filtering so returned groups are explicitly limited to owner-owned groups and/or groups containing shared servers depending on hide_shared_server.

Changes

Server Group Visibility Filtering

Layer / File(s)	Summary
Server group visibility filtering logic web/pgadmin/browser/server_groups/__init__.py	get_all_server_groups() now uses helper predicates (is_owner, has_shared) and list comprehensions to filter server groups. When hide_shared_server is enabled the function returns only groups owned by the current user; when disabled it returns only groups owned by the user or groups that contain shared servers (instead of returning the precomputed list).

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: filter server groups by ownership and sharing status' directly and clearly describes the main change: refactoring server-group filtering logic to properly filter based on ownership and sharing status.
Linked Issues check	✅ Passed	The changes implement the required fix for issue #9933: the refactored get_all_server_groups() now correctly filters to show only groups owned by the current user or containing shared servers when hide_shared_server is disabled.
Out of Scope Changes check	✅ Passed	All changes are within scope: only the ServerGroupView.get_all_server_groups() method was modified to fix the filtering logic for the reported issue, with no unrelated alterations.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

web/pgadmin/browser/server_groups/__init__.py (1)

392-407: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

N+1 query problem when filtering server groups.

The list comprehension at lines 404-407 calls has_shared(group) for each non-owned group in server_groups. Each call to has_shared invokes ServerGroupModule.has_shared_server(group.id) (line 396), which executes a separate database query (lines 76-77). If there are N server groups, this can result in up to N additional database queries.

In environments with many users and server groups, this could significantly impact performance when administrators load the object explorer.

Consider optimizing by fetching all server group IDs that contain shared servers in a single query, then using set membership for the filter predicate.

⚡ Proposed optimization to eliminate N+1 queries

 `@staticmethod`
 def get_all_server_groups():
     """
     Returns the list of server groups to show in server mode.
     Includes groups owned by the user and groups containing
     shared servers accessible to this user.
     :return: server groups
     """
 
     # Don't display shared server if user has
     # selected 'Hide shared server'
     pref = Preferences.module('browser')
     hide_shared_server = pref.preference('hide_shared_server').get()
 
     server_groups = get_server_groups_for_user()
 
     def is_owner(group):
         return group.user_id == current_user.id
 
-    def has_shared(group):
-        return ServerGroupModule.has_shared_server(group.id)
+    # Fetch all group IDs containing shared servers in one query
+    shared_group_ids = {
+        s.servergroup_id for s in 
+        db.session.query(Server.servergroup_id).filter(
+            Server.shared == True
+        ).distinct()
+    }
+    
+    def has_shared(group):
+        return group.id in shared_group_ids
 
     if hide_shared_server:
         return [
             group for group in server_groups
             if is_owner(group)
         ]
 
     return [
         group for group in server_groups
         if is_owner(group) or has_shared(group)
     ]

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/pgadmin/browser/server_groups/__init__.py` around lines 392 - 407, The
current filter uses has_shared(group) which calls
ServerGroupModule.has_shared_server(group.id) per group causing an N+1 DB query;
instead, before filtering (when hide_shared_server is False) fetch all
server-group IDs that contain shared servers in one query (e.g., add or call a
single-method like ServerGroupModule.get_shared_server_group_ids() or a
classmethod that returns a set/list of ids), store that result in a set
(shared_group_ids), and then change the predicate to check membership (group.id
in shared_group_ids) together with is_owner(group) when constructing the final
server_groups list; keep is_owner as-is and only replace repeated has_shared
calls with set membership to eliminate the per-group DB queries.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@web/pgadmin/browser/server_groups/__init__.py`:
- Around line 392-407: The current filter uses has_shared(group) which calls
ServerGroupModule.has_shared_server(group.id) per group causing an N+1 DB query;
instead, before filtering (when hide_shared_server is False) fetch all
server-group IDs that contain shared servers in one query (e.g., add or call a
single-method like ServerGroupModule.get_shared_server_group_ids() or a
classmethod that returns a set/list of ids), store that result in a set
(shared_group_ids), and then change the predicate to check membership (group.id
in shared_group_ids) together with is_owner(group) when constructing the final
server_groups list; keep is_owner as-is and only replace repeated has_shared
calls with set membership to eliminate the per-group DB queries.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 30238474-adba-4a60-9852-77f918344829

📥 Commits

Reviewing files that changed from the base of the PR and between 8108ed6 and cdb686b.

📒 Files selected for processing (1)

• web/pgadmin/browser/server_groups/__init__.py