refactor(Servergroup): Refactor server group access and shared-group handling#10077
refactor(Servergroup): Refactor server group access and shared-group handling#10077lkmatsumura wants to merge 12 commits into
Conversation
…s a shared group. - Added is_shared_group property to the result to simplify the identificantion of shared group- Added is_shared_group property to the result to simplify the identification of shared group - rules centralized in get_server_groups_for_user_query() function
…and shared group logic - change to call get_server_groups_for_user() to retrive the servergroups when possible - same for get_server_group() - Simplifying the method of detecting whether the group is shared or not using the information now returned by get_server_groups_for_user() and get_server_group().
- A shared group is not allowed to be deleted. - Only the owner can do it.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactored server-group access and icon selection by introducing new query helpers to compute ChangesServer Group Access and Icon Refactoring
Sequence DiagramsequenceDiagram
participant ServerGroupView
participant get_server_group
participant get_server_groups_for_user
participant get_server_groups_for_user_query
participant Database
ServerGroupView->>get_server_group: get_server_group(gid, hide_shared=True)
get_server_group->>get_server_groups_for_user: servergroup_id=gid, hide_shared
get_server_groups_for_user->>get_server_groups_for_user_query: build visibility query
get_server_groups_for_user_query->>Database: SELECT with is_shared_group case/exists logic
Database-->>get_server_groups_for_user_query: groups with is_shared_group annotation
get_server_groups_for_user-->>get_server_group: annotated group list
get_server_group-->>ServerGroupView: single group or None
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/pgadmin/utils/server_access.py (1)
133-133: ⚡ Quick winUse
Server.sharedinstead ofServer.shared == Truein SQLAlchemy filter.In SQLAlchemy, boolean columns can be used directly in filter expressions. Using
== Trueis redundant and flagged by linters.♻️ Proposed fix
.filter( Server.servergroup_id == ServerGroup.id, - Server.shared == True + Server.shared )🤖 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/utils/server_access.py` at line 133, The SQLAlchemy filter currently uses the redundant comparison Server.shared == True; replace that expression with the boolean column itself (Server.shared) wherever used in the query/filter (e.g., in the function or method that constructs the query referencing Server.shared) so the filter reads simply Server.shared to satisfy linters and follow SQLAlchemy conventions.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@web/pgadmin/utils/server_access.py`:
- Line 133: The SQLAlchemy filter currently uses the redundant comparison
Server.shared == True; replace that expression with the boolean column itself
(Server.shared) wherever used in the query/filter (e.g., in the function or
method that constructs the query referencing Server.shared) so the filter reads
simply Server.shared to satisfy linters and follow SQLAlchemy conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66b9468b-6998-4f8c-b411-8278e08db886
📒 Files selected for processing (2)
web/pgadmin/browser/server_groups/__init__.pyweb/pgadmin/utils/server_access.py
dpage
left a comment
dpage
left a comment
There was a problem hiding this comment.
Thanks for taking this on — centralizing the visibility logic in server_access.py and replacing the per-server has_shared_server() loop with a single SQL EXISTS is a nice direction. A few things need addressing before this is mergeable:
1. Desktop mode is broken — (0).label('is_shared_group') raises AttributeError.
In get_server_groups_for_user_query(), the non-SERVER_MODE branch does:
ServerGroup.query.add_columns((0).label('is_shared_group'))int has no .label() method, so this throws at query-build time (verified locally). Since get_nodes() was also changed to always route through get_all_server_groups(), the server-group tree fails to render in desktop mode — the default deployment. Use from sqlalchemy import literal and literal(0).label('is_shared_group').
2. Access regression in update().
update() previously fetched ServerGroup.query.filter_by(user_id=current_user.id, id=gid). It now uses get_server_group(gid), which in SERVER_MODE returns groups the user does not own as long as they contain a shared server. The method then does servergroup.name = data['name']; db.session.commit() with no ownership guard, so a user can rename another user's group. Deletion is correctly gated against shared groups (can_delete), but rename needs the same owner check.
3. Style checks will fail CI.
Server.shared == True→ E712 (useServer.shared.is_(True)or justServer.shared).- Only one blank line between the two new top-level functions → E302.
- A couple of lines exceed 79 chars (the
.all()call and a docstring line) → E501. for group, is_shared in sg:has a double space.
4. No tests, no changelog entry. A refactor touching access control should have a test for the owned / shared / desktop paths, and docs/en_US/release_notes_*.rst needs a line.
Since this isn't tied to a reported issue, it'd also help to note what user-facing problem it's solving — the diff reads as pure cleanup, so the bar is "demonstrably no behaviour change," and right now #1/#2 are behaviour changes.
- In Desktop Mode get_server_groups_for_user_query() raise the error
- User cannot update a shared Group, only owned Server Groups
- Class ServerGroupView of server_groups
in desktop mode pref.preferences('hide_shared_server')
returns none. Included a treatment to check if it is None
then hide per default (anyway there are no shared servers
in desktop mode)
- 79 char per line limit - 2 blank line between top-level functions - E712 in Server.shared == True - double space in `for group, is_shared in sg:`
|
@dpage, Thank's by the review. |
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)
386-398:⚠️ Potential issue | 🟡 MinorAdd
can_deleteparameter to matchget_nodes()behavior.In
get_nodes()(line 79),can_deleteis explicitly set based onidx > 0 and not group.is_shared_group. Thenodes()method (lines 386–398) omits this parameter entirely. Both methods return similar node structures but with inconsistent delete permissions, creating a UX issue where the delete action may be displayed for the first group or shared groups via thenodes()endpoint while hidden viaget_nodes().Apply the same
can_deletelogic to thenodes()method:can_delete=True if [group condition] and not group.is_shared_group else FalseThe server-side
delete()method still enforces the restriction, so this is not a security issue—but it prevents confusing UX where the delete action is shown but fails.🤖 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 386 - 398, The nodes() method in the loop iterating through groups (lines 386-398) is missing the can_delete parameter that exists in get_nodes() at line 79. To fix this, modify the loop to use enumerate() to track the index, then add the can_delete parameter to the generate_browser_node() call with the logic: can_delete should be True only when the group index is greater than 0 AND the group is not a shared group (not group.is_shared_group), matching the behavior of get_nodes().
🤖 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 386-398: The nodes() method in the loop iterating through groups
(lines 386-398) is missing the can_delete parameter that exists in get_nodes()
at line 79. To fix this, modify the loop to use enumerate() to track the index,
then add the can_delete parameter to the generate_browser_node() call with the
logic: can_delete should be True only when the group index is greater than 0 AND
the group is not a shared group (not group.is_shared_group), matching the
behavior of get_nodes().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07569480-97dc-44fd-90f8-8e877f4272ff
📒 Files selected for processing (2)
web/pgadmin/browser/server_groups/__init__.pyweb/pgadmin/utils/server_access.py
🚧 Files skipped from review as they are similar to previous changes (1)
- web/pgadmin/utils/server_access.py
- To facilitate and to have a more reliable way to determine if the servergroup is the first servergroup owned by the user changed the query to return this info
…rking properly servergroups\__init__.py: - nodes now return can_delete and cad_edit properties to the script handle it more easily and not expose the rules - uniformize information generate passed to self.blueprint.generate_browser_node() function server_groups.js - canDelete was not used so droped it - Adjusted canDrop to use the can_delete information from itemData - Added canEdit to control if the properties can be edited or not
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/static/js/server_group.js`:
- Around line 11-14: Remove the unused `pgadmin.user_management.current_user`
dependency from the dependency array in the define() call for
`pgadmin.node.server_group`. The dependency is listed in the array but is not
passed as a parameter to the factory function (it's not in the function
signature with gettext, url_for, and pgAdmin), indicating it is dead code that
should be removed from the array.
🪄 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: f61f60fd-f6db-426d-9be4-6e1ff99b808f
📒 Files selected for processing (3)
web/pgadmin/browser/server_groups/__init__.pyweb/pgadmin/browser/server_groups/static/js/server_group.jsweb/pgadmin/utils/server_access.py
🚧 Files skipped from review as they are similar to previous changes (2)
- web/pgadmin/utils/server_access.py
- web/pgadmin/browser/server_groups/init.py
- removed unused dependency from the dependency array
|
Thanks for the updates @lkmatsumura, this has come a long way. Items 1 and 2 from my earlier review are sorted: desktop mode no longer throws (the 1.
2. On item 4 (the part you were unsure about):
One minor aside, not blocking: in Thanks again, the centralised approach reads well, it's mostly the lint and the |
- fix of make check-pep8 style errors detected - Adding can_edit to create() and nodes() - hide_shared_server default to false if the pref object is absent
- Added a flush after adding a new server group to retrieve its ID immediately. - Updated the server group retrieval to include access metadata. - Enhanced the query for server groups to filter by ID when provided, ensuring more precise results.
|
@dpage I think the style code is ok now, The tests since I don't know how to write it, is AI generated, but it's seens correct to me. |
2 participants
Summary
Refactor server group access and shared-group handling in pgAdmin4 to centralize visibility logic and simplify browser rendering.
What changed
Updated
web/pgadmin/utils/server_access.pyget_server_groups_for_user()andget_server_groups_for_user_query()get_server_group()to reuse shared access logicis_shared_groupmetadata on returned groupsUpdated
web/pgadmin/browser/server_groups/__init__.pyServerGroupModule.has_shared_server()logicNotes
Summary by CodeRabbit
Bug Fixes
Improvements