fix: filter server groups by ownership and sharing status#10001
Open
lkmatsumura wants to merge 9 commits into
Open
fix: filter server groups by ownership and sharing status#10001lkmatsumura wants to merge 9 commits into
lkmatsumura wants to merge 9 commits into
Conversation
…ver mode - rewriting to return only admin user groups and groups with shared servers like the previous behaviour
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds ADMIN_CAN_SEE_ALL_SERVERS config, an explicit ServerGroup↔Server ORM relationship, refactors server/group access helpers to support only_owned and to respect the admin flag, and updates browser server-group code to bulk-prefetch shared-group IDs and use those for icon/visibility decisions. ChangesServer & ServerGroup Visibility
sequenceDiagram
participant Browser as ServerGroupView
participant Access as server_access.get_server_groups_for_user
participant DB as Database
Browser->>Access: request visible groups (only_owned/hide_shared)
Access->>DB: build/union queries for owned groups and groups with shared servers
DB-->>Access: return query/results
Access-->>Browser: return groups (possibly filtered) and shared_group_ids
🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
lkmatsumura
changed the title
- If hide_shared_server is true then show only user owned groups
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 liftN+1 query problem when filtering server groups.
The list comprehension at lines 404-407 calls
has_shared(group)for each non-owned group inserver_groups. Each call tohas_sharedinvokesServerGroupModule.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
📒 Files selected for processing (1)
web/pgadmin/browser/server_groups/__init__.py
- Create ADMIN_CAN_SEE_ALL_SERVERS config - If false admin see only owned and shared server from other administrators same as a common user - Change in ServerGroup model to allow servers relationship - Change in get_server_groups_for_user of server_access.py to facilitate to meet the criterias
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@web/pgadmin/browser/server_groups/__init__.py`:
- Around line 412-414: The returned server_groups list from
get_server_groups_for_user should preserve the current user's default group at
index 0 so ServerGroupModule.get_nodes() (which checks idx == 0 to prevent
delete) continues to work; after materializing server_groups =
get_server_groups_for_user(...).all(), find the current user's default group id
(or object) and reorder the list so that default group is first (move it to
index 0) before returning; update references to server_groups,
get_server_groups_for_user, and ensure compatibility with
ServerGroupModule.get_nodes() which relies on position rather than changing that
caller.
In `@web/pgadmin/model/__init__.py`:
- Line 255: The ServerGroup.servers and Server.servers relationships are defined
over the same FK (server.servergroup_id → servergroup.id) causing overlapping
SQLAlchemy relationships; update the pair to be a single coherent relationship
by replacing the implicit backrefs with an explicit paired relationship using
back_populates on both sides (e.g., in ServerGroup define servers =
relationship('Server', back_populates='servergroup', ...) and in Server define
servergroup = relationship('ServerGroup', back_populates='servers', ...) ), or
if you intentionally need two relationships over the same join, mark the
intentional overlap with overlaps='servers,servergroup' on the relationship
definitions to suppress mapper warnings and keep in-memory state consistent.
In `@web/pgadmin/utils/server_access.py`:
- Around line 74-86: get_servers_from_group currently returns all Server rows
for a group when only_owned is False, exposing other users' non-shared servers
to callers like ServerGroupModule.has_shared_server/has_not_shared_server;
update get_servers_from_group to apply the same access predicate used elsewhere
by calling or composing get_user_server_query(servergroup_id=gid) (or reapply
the "owned or shared" filter that get_user_server_query enforces) so that the
returned query only includes servers visible to the current user while
preserving the only_owned branch behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1e0754ca-3b4f-4fe7-8d6d-3ebce6ebef67
📒 Files selected for processing (4)
web/config.pyweb/pgadmin/browser/server_groups/__init__.pyweb/pgadmin/model/__init__.pyweb/pgadmin/utils/server_access.py
Comment on lines
412
to
414
| server_groups = get_server_groups_for_user( only_owned=hide_shared_server).all() | ||
|
|
||
| return server_groups |
There was a problem hiding this comment.
Keep the current user's default group first before returning this list.
ServerGroupModule.get_nodes() still uses list position at Line 122 (idx == 0) to decide which group cannot be deleted. After this refactor, .all() materializes an unordered result set that can include other users' shared groups, so the wrong node can lose its delete action while the actual default group appears deletable until the delete endpoint rejects it.
One safe way to preserve the existing UI contract
- server_groups = get_server_groups_for_user( only_owned=hide_shared_server).all()
-
- return server_groups
+ server_groups = get_server_groups_for_user(
+ only_owned=hide_shared_server
+ ).all()
+ server_groups.sort(key=lambda g: (g.user_id != current_user.id, g.id))
+ return server_groups🤖 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 412 - 414, The
returned server_groups list from get_server_groups_for_user should preserve the
current user's default group at index 0 so ServerGroupModule.get_nodes() (which
checks idx == 0 to prevent delete) continues to work; after materializing
server_groups = get_server_groups_for_user(...).all(), find the current user's
default group id (or object) and reorder the list so that default group is first
(move it to index 0) before returning; update references to server_groups,
get_server_groups_for_user, and ensure compatibility with
ServerGroupModule.get_nodes() which relies on position rather than changing that
caller.
added 2 commits
June 3, 2026 11:47
do not need to check if is server mode anymore or note in get_nodes
- Refactoring as suggested due potential major issue to avoid expose other users non shared servers
- Adjusting relationship - Server has one ServerGroup not servers - Using back_populates instead of backref for better control - Adjusted references to ServerGroup in server: server.servergroup.name instead server.servers.name
Author
|
May fix #9933 |
lkmatsumura
commented
Jun 3, 2026
Author
lkmatsumura
left a comment
lkmatsumura
left a comment
There was a problem hiding this comment.
Hi, i finished the changes coderrabit demanded.
I think it is ready for maintaners review.
Hope to this changes to be useful, thanks for this great tool.
- Added ServerGroupView.get_shared_server_group_ids() to fetch all shared-server group IDs in one query - Updated ServerGroupModule.get_nodes() to use pre-fetched - Changed get_icon_css_class() to use membership check instead of repeated has_shared_server() callsID set
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/pgadmin/browser/server_groups/__init__.py (1)
118-133:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't derive deleteability from the visible list index.
Once shared groups are mixed into this ordered result,
idx == 0can point at another user's shared group with a lower global id. The real default owned group then renders as deletable until the delete endpoint rejects it. Compare against the first owned group's id explicitly instead of relying on list position.🤖 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 118 - 133, The code currently uses the visible list index to decide deletability, which is wrong when shared groups are mixed in; instead, compute the first owned group's id up-front by scanning ServerGroupView.get_all_server_groups() for the first group whose user_id matches the current user's id (or otherwise identifies ownership), store it (e.g. first_owned_group_id), and then when yielding nodes in the loop use can_delete = True if group.id != first_owned_group_id (and still True only for owned groups), updating the logic around get_icon_css_class and generate_browser_node accordingly so deletability is based on group.id compared to first_owned_group_id rather than enumerate index.
🤖 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.
Inline comments:
In `@web/pgadmin/browser/server_groups/__init__.py`:
- Around line 436-438: The current filter uses Server.shared directly on a
ServerGroup query (in the block using get_server_groups_for_user and producing
group_ids), which doesn't join servers and thus treats the predicate
incorrectly; update the filtering to correlate Server to ServerGroup (e.g. join
Server or use an EXISTS subquery referencing Server.servergroup_id ==
ServerGroup.id and Server.shared) so the filter only selects groups that
actually have shared servers before building group_ids from the
get_server_groups_for_user(...) query.
In `@web/pgadmin/utils/server_access.py`:
- Around line 103-104: The admin fast-paths in get_server_group and
get_server_groups_for_user currently ignore the only_owned flag when
config.ADMIN_CAN_SEE_ALL_SERVERS is true; change those branches to honor
only_owned by returning a filtered query when only_owned is True (e.g. add an
ownership filter using the ServerGroup ownership field or relation and the
current user id) and only bypass ownership checks when only_owned is False.
Specifically update the logic around _is_admin() &&
config.ADMIN_CAN_SEE_ALL_SERVERS to conditionally apply an ownership filter
(using ServerGroup.query.filter_by(..., owned_by=current_user.id) or equivalent)
so both get_server_group(...) and get_server_groups_for_user(...) respect
only_owned.
---
Duplicate comments:
In `@web/pgadmin/browser/server_groups/__init__.py`:
- Around line 118-133: The code currently uses the visible list index to decide
deletability, which is wrong when shared groups are mixed in; instead, compute
the first owned group's id up-front by scanning
ServerGroupView.get_all_server_groups() for the first group whose user_id
matches the current user's id (or otherwise identifies ownership), store it
(e.g. first_owned_group_id), and then when yielding nodes in the loop use
can_delete = True if group.id != first_owned_group_id (and still True only for
owned groups), updating the logic around get_icon_css_class and
generate_browser_node accordingly so deletability is based on group.id compared
to first_owned_group_id rather than enumerate index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c949fc0-4aa9-43a9-a360-cc4217095747
📒 Files selected for processing (6)
web/config.pyweb/pgadmin/browser/server_groups/__init__.pyweb/pgadmin/model/__init__.pyweb/pgadmin/tools/schema_diff/__init__.pyweb/pgadmin/tools/sqleditor/__init__.pyweb/pgadmin/utils/server_access.py
added 2 commits
June 3, 2026 16:15
…roup - Enforce that the id of first owned group id is not a shared group
- even if the server or group is not owned by the admin, if config.ADMIN_CAN_SEE_ALL_SERVERS this is de desired behaviour
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
1 participant
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
Adds a new
ADMIN_CAN_SEE_ALL_SERVERSconfiguration option for server mode and fixes server-group retrieval so admins only see allowed server groups and shared servers according to current settings.Background
In server mode, administrator users were incorrectly seeing all servers and server groups. This branch restores the expected behavior by:
Hide shared serverpreference when listing groups.What changed
ADMIN_CAN_SEE_ALL_SERVERS = Falseinweb/config.pyweb/pgadmin/model/__init__.py:serversrelationship onServerGroupweb/pgadmin/utils/server_access.py:get_server()now honorsADMIN_CAN_SEE_ALL_SERVERSget_server_groups_for_user()now returns owned groups plus shared-server groups only when allowedweb/pgadmin/browser/server_groups/__init__.py:ServerGroupView.get_all_server_groups()now uses the new access helperhide_shared_serverVerification / Test steps
ADMIN_CAN_SEE_ALL_SERVERSinweb/config.py:False: admin should see only owned groups and groups with shared servers.True: admin sees all servers/groups.Hide shared serverhides shared-server groups as expected.Docs
Recommend updating admin/server-mode configuration documentation with:
ADMIN_CAN_SEE_ALL_SERVERSFalse)Risk
Low. The fix is configuration-driven and restores previous expected behavior when
ADMIN_CAN_SEE_ALL_SERVERSremains disabled.Summary by CodeRabbit
New Features
Improvements
Bug Fixes