Add new options like INHERIT and SET to the Role's membership tab. #6451 #9539
Conversation
WalkthroughRefactors role membership to list-based structures, adds PostgreSQL 16+ INHERIT/SET handling, makes UI and formatters version-aware, introduces 16_plus SQL templates for role create/update/properties/sql, updates default templates to use new lists, adds version/type field visibility checks, and expands tests for membership permutations and special-character role names. Changes
Sequence Diagram(s)sequenceDiagram
participant User as UI
participant Frontend as SchemaView / Membership UI
participant Backend as pgAdmin Server
participant Templates as SQL Templating
participant DB as PostgreSQL
User->>Frontend: Open Role dialog / edit membership
Frontend->>Frontend: Read node_info.server.version -> show admin/inherit/set fields
User->>Frontend: Add/modify membership entries (rolmembership_list / rol_members_list)
Frontend->>Backend: Submit role payload (list-based membership)
Backend->>Backend: transform & validate payload (version-gated)
Backend->>Templates: Render CREATE/ALTER SQL (16_plus or default template)
Templates->>DB: Present or execute SQL
DB-->>Backend: Execution result / metadata
Backend-->>Frontend: Response (success/errors)
Frontend-->>User: Display result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sql (1)
33-38: Password NULL branch is unreachable with current outer condition.Outer guard is
if data.rolpassword(false whenNone), but the body checksdata.rolpassword is none(never true). If you intend to support explicitPASSWORD NULLon create, the outer condition should be based on key presence.Proposed fix
- VALID UNTIL {{ data.rolvaliduntil|qtLiteral(conn) }} {% endif %}{% if data.rolpassword %} + VALID UNTIL {{ data.rolvaliduntil|qtLiteral(conn) }} {% endif %}{% if 'rolpassword' in data %} PASSWORD {% if data.rolpassword is none %}NULL{% else %}{% if dummy %}'xxxxxx'{% else %} {{ data.rolpassword|qtLiteral(conn) }}{% endif %}{% endif %}{% endif %};web/pgadmin/browser/server_groups/servers/roles/__init__.py (2)
204-236: Avoid aliasing/mutating request payload lists in_process_rolemembership
In create mode (Line 219) you aliasrolmembershipintorolmembership_list, and in update mode youextend()a list pulled from the request (Line 221-233). This can cause surprising side effects if later code still reads the original structure.Proposed adjustment (defensive copy)
if id == -1: - data['rolmembership_list'] = data['rolmembership'] + data['rolmembership_list'] = list(data['rolmembership']) else: - data['rolmembership_list'] = data['rolmembership'].get('added', []) + data['rolmembership_list'] = list(data['rolmembership'].get('added', [])) @@ - data['rolmembership_list'].extend( - data['rolmembership'].get('changed', [])) + data['rolmembership_list'].extend( + data['rolmembership'].get('changed', []))
270-348: Fix API contract: Documentation shows'updated'but code expects'changed'for role member updatesThe update-mode message (line 326+) documents an
'updated'key, but_validate_input_dict_for_update(line 155) and_process_rolmembers(lines 258–265) expect'changed'. Tests and all template code use'changed'. Any API client following the documented format will silently fail validation.Additionally, for PostgreSQL < 16, both the create and update messages (lines 287–289, 313–315) document
inheritandsetfields, but validation only requires['role', 'admin'](lines 292–293, 337–338), creating a misleading contract.Suggestion: Change the documented message keys from
'updated'to'changed'to match the actual implementation.
🤖 Fix all issues with AI agents
In `@web/pgadmin/browser/server_groups/servers/roles/__init__.py`:
- Around line 237-269: The code assigns lists from data['rolmembers'] directly
(e.g., in _process_rolmembers setting data['rol_members_list'] =
data['rolmembers'].get('added', []) and similar for changed/deleted) which
creates aliases that are later mutated with .extend(); make defensive copies
instead by wrapping those .get(...) results with list(...) so mutations do not
modify the original input structures, and apply the same defensive-copy change
in the analogous function _process_rolemembership for its list assignments.
In `@web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js`:
- Around line 61-66: The code building the badges inside rawData.forEach uses
member.admin, member.inherit and member.set directly (when computing badges for
serverVersion >= 160000) which can throw if those properties are undefined;
update the badge construction in the rawData.forEach callback (the block using
serverVersion, member.admin, member.inherit, member.set and appending to
members) to defensively coerce missing values to safe defaults (e.g., use
(member.inherit ?? false).toString().toUpperCase(), (member.set ??
false).toString().toUpperCase(), and similar for admin, and ensure member.role
is present before concatenation) so undefined/null won’t cause .toString() to
throw and concatenation into members is safe.
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql`:
- Around line 33-38: The template has an unreachable branch because it first
tests `{% if data.rolpassword %}` (truthy) and inside checks `{% if
data.rolpassword is none %}` (never hit); change the outer check to a
key-presence/definition test (e.g., `{% if data.rolpassword is defined %}`) so
the inner `is none` branch can run when the key exists but is null, ensuring the
`PASSWORD NULL` path is reachable; update the `PASSWORD` block surrounding
`data.rolpassword|qtLiteral(conn)` accordingly.
- Around line 38-43: The guard that checks for any of 'admin'/'inherit'/'set' in
data.rolmembership_list is fine, but the template then accesses item.admin,
item.inherit and item.set unguarded; update the WITH clause in the roles SQL
template to use safe defaults so missing keys don't break rendering—change the
clause to use Jinja's default filter for each key (e.g. ADMIN {{
item.admin|default(false) }}, INHERIT {{ item.inherit|default(false) }}, SET {{
item.set|default(false) }}), and apply the same change for the second occurrence
handling lines 57-62 so both GRANT renderings reference
item.admin|default(false), item.inherit|default(false) and
item.set|default(false).
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/properties.sql`:
- Around line 16-31: The rolmembers ARRAY SELECT is nondeterministic and uses
unqualified catalog names; update the subquery that builds rolmembers (the
SELECT returning CASE...||pg.usename from the derived table aliased pg) to
qualify system tables/functions with pg_catalog (e.g., pg_catalog.pg_roles,
pg_catalog.pg_auth_members) and add a deterministic ORDER BY (for example by
pg.usename and the option bits) in the outer SELECT so the ARRAY(...) produces
stable, repeatable output when rid is provided.
- Around line 1-15: The rolmembership array can contain NULL entries when
rm.rolname is NULL and its ORDER is nondeterministic; change the rolmembership
subselect to exclude NULL role names (e.g., add "WHERE rm.rolname IS NOT NULL"
or filter on rm.oid IS NOT NULL) and add an ORDER BY rm.rolname to the
ARRAY(...) SELECT to guarantee deterministic ordering; for seclabels, make the
array_agg deterministic by adding an ORDER BY inside array_agg, e.g.,
"array_agg(provider || '=' || label ORDER BY provider, label)" so results are
stable across runs.
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql`:
- Around line 3-10: In update.sql replace the implicit truthiness check "{% if
data.rolvaliduntil %}" with the explicit null check used in create.sql (e.g.,
"{% if data.rolvaliduntil and data.rolvaliduntil is not none %}" or at minimum
"{% if data.rolvaliduntil is not none %}") so that an empty string does not get
treated as a valid value; apply the same change to both the 16_plus and default
update.sql templates to mirror create.sql and ensure rolvaliduntil is handled
consistently.
- Around line 50-61: The template guard allows rendering if any of
'admin','inherit','set' exist but then unconditionally outputs all three, which
can produce broken SQL; update the roles SQL template so it either (a) requires
all three keys present in the condition (replace "if 'admin' in item or
'inherit' in item or 'set' in item" with an AND check) or (b) render each clause
only when its key exists (use per-key checks or Jinja default filters for
item.admin, item.inherit, item.set) so GRANT lines only include valid "WITH
ADMIN ..., INHERIT ..., SET ..." fragments for the current item (refer to the
template variables item, rolname, conn and the GRANT/REVOKE blocks).
In
`@web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sql`:
- Around line 16-17: The expected output in alter_role_options4.sql is missing
two GRANT statements corresponding to the rolmembers "added" actions: add GRANT
for test_rolemembership_3 that grants "Role2_$%{}[]()&*^!@""'`\/#" TO
test_rolemembership_3 WITH ADMIN OPTION, INHERIT OPTION, SET OPTION, and add
GRANT for test_rolemembership_4 that grants "Role2_$%{}[]()&*^!@""'`\/#" TO
test_rolemembership_4; insert these two GRANT statements alongside the existing
GRANT lines for test_rolemembership_2 and test_rolemembership_1 so the expected
output matches the alter_role_options4.msql template.
In `@web/pgadmin/browser/server_groups/servers/static/js/membership.ui.js`:
- Around line 76-80: The depChange handler currently forces state.set = false
when undefined; update the depChange for the "set" field (function depChange) to
either leave state.set undefined or initialize it to true to match the
PostgreSQL 16+ default (i.e., change the fallback from false to true or remove
the override altogether) so new rows inherit the intended default; ensure you
only touch depChange and keep the rest of the field logic intact.
🧹 Nitpick comments (7)
web/pgadmin/static/js/SchemaView/common.js (1)
19-31: Improve readability and add defensive check forhelperProps.The proposed refactor is recommended:
versionis correctly typed asPropTypes.number(safe for comparisons), but the code lacks a guard forhelperPropsitself before accessinghelperProps.serverInfo. While PropTypes enforcement currently ensureshelperPropsis provided, adding an explicit check makes the function more robust and improves readability through sequential returns instead of nested ternaries.Proposed refactor
export const isVersionSupportedByField = (field, helperProps) => { - return ( - // serverInfo not found - _.isUndefined(helperProps.serverInfo) || - // serverInfo found and it's within range - ( - _.isUndefined(field.min_version) ? true : - (helperProps.serverInfo.version >= field.min_version) - ) && ( - _.isUndefined(field.max_version) ? true : - (helperProps.serverInfo.version <= field.max_version) - )); + if (_.isUndefined(helperProps) || _.isUndefined(helperProps.serverInfo)) return true; + const v = helperProps.serverInfo.version; + if (!_.isUndefined(field.min_version) && v < field.min_version) return false; + if (!_.isUndefined(field.max_version) && v > field.max_version) return false; + return true; };docs/en_US/role_dialog.rst (1)
77-87: Documentation inconsistency: Inherit and Set options not mentioned in section descriptions.Lines 77-81 describe using "Admin Option" checkboxes for both sections, but the note on line 87 mentions that "Inherit Option" and "Set Option" are also available for PostgreSQL 16+. Consider updating lines 77-81 to mention that additional options (Inherit, Set) are available as checkboxes when using PostgreSQL 16 or later.
📝 Suggested documentation improvement
-* Use *Member of* section to specify roles of which the current role - is a member. To assign *Admin Option* for a selected role, click on - the appropriate checkbox. -* Use *Members* section to specify roles that are members of the current - role. To assign *Admin Option* for a selected role, click on the appropriate checkbox. +* Use *Member of* section to specify roles of which the current role + is a member. To assign options for a selected role, click on + the appropriate checkboxes (*Admin*, *Inherit*, *Set*). +* Use *Members* section to specify roles that are members of the current + role. To assign options for a selected role, click on the appropriate checkboxes.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msql (1)
5-5: Minor: Extra whitespace beforeWITH.There's a double space between the role name and
WITH ADMIN. While this doesn't affect functionality, it's inconsistent with the other lines.📝 Suggested fix
-GRANT "Role2_$%{}[]()&*^!@""'`\/#" TO test_rolemembership_3 WITH ADMIN False, INHERIT False, SET False; +GRANT "Role2_$%{}[]()&*^!@""'`\/#" TO test_rolemembership_3 WITH ADMIN False, INHERIT False, SET False;web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.json (1)
156-209: Regex replacement for VALID UNTIL may be brittle across timestamp formatting changes.If the SQL generator changes spacing/timezone rendering,
VALID UNTIL '[0-9\\-T:+ ]*'might stop matching. Consider constraining less to formatting specifics (or matching onVALID UNTIL '.*'if your harness is safe against overmatching).web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql (1)
108-119: The template condition-access mismatch can be made more defensive, but validation guarantees key existence for PG16+.The GRANT clause at line 117 uses OR logic to check for keys (
'admin' in item or 'inherit' in item or 'set' in item) but unconditionally accesses all three ({{ item.admin }}, {{ item.inherit }}, {{ item.set }}). While this appears unsafe, PG16+ validation (lines 294-300, 336-338) strictly requires all four keys including 'role', 'admin', 'inherit', 'set' in each item before the template is rendered—so the pattern cannot fail at runtime.However, adding defensive defaults is reasonable for template robustness. A similar issue exists at line 59 in the
rolmembership_listGRANT clause.Suggested fix for line 117
{% if data.rol_members_list and data.rol_members_list|length > 0 %} {% for item in data.rol_members_list %} -GRANT {{ conn|qtIdent(rolname) }} TO {{ conn|qtIdent(item.role) }} {% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}{% endif %}; +{% set admin_opt = item.admin|default(false) %} +{% set inherit_opt = item.inherit|default(false) %} +{% set set_opt = item.set|default(false) %} +GRANT {{ conn|qtIdent(rolname) }} TO {{ conn|qtIdent(item.role) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ admin_opt }}, INHERIT {{ inherit_opt }}, SET {{ set_opt }}{% endif %}; {% endfor %} {% endif %}web/pgadmin/browser/server_groups/servers/roles/__init__.py (2)
349-427: Sameupdatedvschangedmismatch in_validate_rolemembership(+ PG<16 message mentions inherit/set)
Same issue as above: the update-mode message documentsupdated(Line 405+) but backend expects'changed'. Also, messages includeinherit/setfor PG<16 even though they’re not required there.
842-865: Reduce duplication + add same regex guard intransform()rolemembership parsing
transform()duplicates the same parsing logic as_set_rolemembers()(Line 848-861) and has the same “regex can be None” crash risk. Recommend extracting a small helper to parse the flag-prefixed role strings (for both rolmembership and rolmembers) and reusing it from both places.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/en_US/images/role_membership.pngis excluded by!**/*.pngdocs/en_US/images/role_sql.pngis excluded by!**/*.png
📒 Files selected for processing (41)
docs/en_US/role_dialog.rstweb/pgadmin/browser/server_groups/servers/roles/__init__.pyweb/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.jsweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.jsonweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options1.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options2.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options3.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/test.jsonweb/pgadmin/browser/server_groups/servers/static/js/membership.ui.jsweb/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsxweb/pgadmin/static/js/SchemaView/common.js
🧰 Additional context used
🧬 Code graph analysis (2)
web/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsx (1)
web/pgadmin/static/js/SchemaView/common.js (2)
isVersionSupportedByField(19-31)isVersionSupportedByField(19-31)
web/pgadmin/browser/server_groups/servers/static/js/membership.ui.js (1)
web/pgadmin/static/js/SchemaView/base_schema.ui.js (1)
BaseUISchema(17-212)
🔇 Additional comments (39)
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.msql (1)
1-9: LGTM!The CREATE ROLE statement correctly escapes the complex role name with special characters and sets appropriate privileges for a login role test case. The SQL syntax is valid for PostgreSQL 16+.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.msql (1)
1-1: LGTM!The COMMENT ON ROLE statement is syntactically correct with proper identifier escaping matching the corresponding role creation test.
web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options3.msql (1)
1-3: LGTM!The REVOKE statements correctly test both membership directions - revoking a role from the complex-named role and revoking the complex-named role from another role. The identifier escaping is consistent.
web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.sql (1)
16-17: LGTM!The GRANT statements are correctly split into individual statements, allowing granular control over the ADMIN OPTION per membership. This aligns with the PR's objective to support enhanced role membership options.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.sql (1)
1-13: LGTM!The SQL script correctly represents a PostgreSQL 16+ role with LOGIN privileges and comprehensive attributes. The commented DROP ROLE follows the standard pattern for generated SQL output, and the COMMENT ON ROLE provides metadata for the test scenario.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.sql (1)
1-2: Special-character role identifier quoting looks correct (embedded"doubled as""; other characters safely contained within quoted identifier).Also applies to: 13-13
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.sql (2)
16-18: DB-scoped setting + role comment statements look good and well-terminated.
4-15: CREATE statement in an ALTER-type test fixture is semantically inconsistent.This file is referenced in a test marked as
"type": "alter"for the operation "Alter Login Role options", yet the expected SQL begins withCREATE ROLErather thanALTER ROLE. While the file also contains anALTER ROLE ... IN DATABASEstatement on line 16 and aCOMMENT ON ROLEon line 18, the leadingCREATEstatement is unusual for a fixture documenting an alter operation.If this fixture represents the actual SQL statements to be executed for the ALTER operation, it should start with
ALTER ROLE. If it instead represents the role's final state representation, the filename should reflect that (e.g.,alter_login_role_options_final_state.sql).Suggested change (if this should show actual ALTER statements)
-CREATE ROLE "Role2_$%{}[]()&*^!@""'`\/#" WITH +ALTER ROLE "Role2_$%{}[]()&*^!@""'`\/#" WITH LOGIN NOSUPERUSER INHERIT NOCREATEDB CREATEROLE NOREPLICATION NOBYPASSRLS CONNECTION LIMIT 100 ENCRYPTED PASSWORD '<PASSWORD>' VALID UNTIL '<TIMESTAMPTZ_1>';web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options1.msql (1)
12-19: Ensure expected output fixtures were updated to match the new SQL statements.The GRANT and ALTER ROLE statements added (lines 12-19) are syntactically valid, but this test file change requires corresponding updates to expected-output files (likely
.sqlor.jsonfixtures in the same test directory). Without confirmation that test expectations were updated to reflect these new statements, the test may fail or not properly validate the altered behavior.Verify that:
- Expected output fixtures for
alter_role_options1test scenario have been updated- Test framework expectations align with the added GRANT statements and ALTER ROLE configuration
web/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsx (1)
11-12: Version-gated column visibility logic is correct. ThecolumnVisibilityfield properly ANDs both mode and version support checks on lines 50–52. WhenserverInfois absent,isVersionSupportedByFieldreturnstrue, allowing version-gated fields to remain visible—which is the intended fallback behavior.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.sql (1)
1-13: LGTM!The test file correctly demonstrates PostgreSQL role creation with a complex special-character identifier and proper comment attachment. The double-quote escaping (
"") within the identifier is correctly applied in both theCREATE ROLEandCOMMENT ON ROLEstatements.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.msql (1)
1-1: LGTM!The
.msqlfile correctly isolates theCOMMENT ON ROLEstatement for testing the role description modification, with proper identifier escaping matching the companion.sqlfile.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.sql (1)
1-11: LGTM!The test file correctly defines a login role with full privileges using proper identifier escaping. The commented
DROP ROLE IF EXISTSprovides a useful cleanup reference for manual testing.web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options2.msql (1)
1-7: LGTM!The test correctly uses pre-PostgreSQL 16 syntax for role membership alterations, with
GRANT ... WITH ADMIN OPTIONandREVOKE ADMIN OPTION FORstatements. TheINHERITandSEToptions are appropriately omitted since they are only available in PostgreSQL 16+, and this is in thedefaulttest directory.web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.msql (1)
1-7: LGTM!The test correctly exercises bidirectional role membership modifications with
ADMIN OPTIONgrants and revokes, using appropriate pre-PostgreSQL 16 syntax. The proper identifier escaping for the special-character role name is maintained throughout.web/pgadmin/browser/server_groups/servers/roles/tests/default/test.json (2)
45-86: LGTM!The additional test roles (3 and 4) provide proper coverage for testing membership scenarios with multiple roles.
148-149: LGTM!The test scenarios comprehensively cover membership operations (add, change, delete, and combined) for both
rolmembershipandrolmembers. The delete scenario correctly includes all four test role IDs.Also applies to: 163-164, 179-180, 197-202, 220-220
web/pgadmin/browser/server_groups/servers/static/js/membership.ui.js (2)
24-30: LGTM on version-gated defaults.The constructor properly gates
adminandsetdefaults based on server version, and theinheritfield is correctly added withundefineddefault (to be set via depChange).
61-70: LGTM on theinheritfield.The field is properly gated with
min_version: 160000and thedepChangecorrectly initializes undefined values tofalse.web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql (2)
31-56: LGTM!The GRANT statement generation correctly handles the PostgreSQL 16+ membership options (admin, inherit, set) with proper conditional formatting. The grouping by option flags efficiently batches grants with identical option sets.
1-29: LGTM!The CREATE ROLE statement template properly generates all role attributes including the PostgreSQL 16+ BYPASSRLS option and handles password encryption detection correctly.
web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js (1)
180-183: LGTM!Wrapping in an arrow function
(raw) => obj.memberDataFormatter(raw)ensures properthisbinding when the formatter is invoked as a callback.Also applies to: 201-204
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.sql (1)
1-11: LGTM!The SQL test fixture correctly demonstrates PostgreSQL 16+ role creation with proper identifier quoting. The special characters in the role name are appropriately escaped (doubled double-quotes for embedded quotes), and the role attributes follow standard PostgreSQL syntax.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.msql (1)
1-9: LGTM!The SQL test fixture is valid and properly includes
CONNECTION LIMIT -1(unlimited connections). The attribute ordering differs slightly fromcreate_role.sql, which is acceptable as these likely test different template code paths.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.sql (1)
1-13: LGTM!The test fixture correctly demonstrates a role with proper identifier escaping and includes a
COMMENT ON ROLEstatement. This appears to represent the expected SQL output for the role after a rename operation in the test workflow.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.msql (1)
1-2: LGTM!The
ALTER ROLE ... RENAME TOstatement correctly demonstrates role renaming with properly escaped special characters in both the source and target role names.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.msql (1)
1-3: LGTM!The test fixture correctly demonstrates bidirectional role membership revocation. The first statement revokes
test_rolemembership_1from the complex-named role (removing its membership in that role), while the second revokes the complex-named role fromtest_rolemembership_3. Proper identifier quoting is maintained throughout.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.msql (1)
1-2: LGTM!The ALTER ROLE RENAME statement correctly handles the complex identifier with proper double-quote escaping. The SQL syntax is valid for PostgreSQL.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.sql (1)
1-21: LGTM!The SQL test file correctly demonstrates PostgreSQL 16+ role creation with:
- Proper role options (SUPERUSER, CREATEDB, etc.)
- Correct GRANT syntax using
WITH ADMIN OPTION, INHERIT OPTION, SET OPTION- Database-scoped configuration via
ALTER ROLE IN DATABASE- Appropriate use of placeholders for sensitive/variable data
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.msql (1)
1-12: LGTM!The ALTER ROLE statements are syntactically valid. The ISO 8601 timestamp format with 'T' separator is accepted by PostgreSQL.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sql (1)
1-21: Approve overall structure and syntax.The CREATE ROLE statement and supporting SQL are syntactically correct for PostgreSQL 16+. The file properly demonstrates role creation with membership grants using the
WITH ... OPTIONsyntax.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msql (1)
1-19: LGTM - Comprehensive test coverage for PostgreSQL 16+ role membership options.The test correctly exercises:
- ALTER ROLE with various attributes
- GRANT statements in both directions (roles granted to this role, and this role granted to others)
- All three new PostgreSQL 16+ options (ADMIN, INHERIT, SET) with both True and False values
- Proper escaping of special characters in the role name
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.sql (1)
1-20: LGTM - Expected state file correctly shows PostgreSQL 16+ GRANT syntax.The file uses the standard
WITH ADMIN OPTION, INHERIT OPTION, SET OPTIONsyntax for the remaining granted membership, which is appropriate for expected-state test files.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.sql (1)
16-17: LGTM - Correctly shows differentiated GRANT syntax.Line 16 omits options when all are false, while line 17 includes all options when true. This matches the expected behavior where options are only included in the SQL output when enabled.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msql (1)
1-7: Test file correctly covers all PostgreSQL 16+ membership option combinations.The file tests GRANT statements in both directions with both True and False values for all three options (ADMIN, INHERIT, SET).
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sql (2)
50-69: LGTM - Role membership handling correctly refactored to list-based structure.The template correctly:
- Iterates over
rolmembership_revoked_listfor REVOKE statements- Iterates over
rolmembership_listfor GRANT statements with optional ADMIN OPTION- Handles
rolmembership_revoked_adminsfor revoking admin privilegesThe inner condition check on lines 64-67 filters items during iteration, which is functionally correct.
116-135: LGTM - Symmetric handling for the reverse membership direction.The template correctly mirrors the
rolmembership_*logic forrol_members_*lists, handling:
- Revoking this role from other members
- Granting this role to other members with optional ADMIN OPTION
- Revoking admin option from specific members
This maintains consistency between both membership directions.
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sql (1)
38-43: List-driven membership grants look good for pre-16 behavior (ADMIN OPTION only).Also applies to: 57-62
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.json (1)
1-299: Test payload transformation is correctly implemented and properly version-gated.The transformation layer in
roles/__init__.pycorrectly maps the test payloads:
_process_rolemembership()transformsrolmembership.{added,changed,deleted}→rolmembership_list+rolmembership_revoked_list_process_rolmembers()transformsrolmembers.{added,changed,deleted}→rol_members_list+rol_members_revoked_listBoth methods include proper version gating (< 160000 vs >= 160000) to handle the additional
inheritandsetproperties in PostgreSQL 16+. The test payloads use the correct public API shape and will be properly transformed before template rendering.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js
Outdated
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.msql
Show resolved
Hide resolved
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web/pgadmin/browser/server_groups/servers/roles/__init__.py`:
- Around line 848-861: In transform(), the re.search() calls used to parse role
strings can return None and are not checked; update the transform() logic to
verify the regex match before accessing groups: after calling re.search(...)
assign to a variable (e.g., match), if match is None skip/handle the malformed
entry (e.g., continue or log and skip) and only then use match.group(...) to
build the dict appended to res; apply this for both the pre-160000 and else
branches that reference match.group(1..4).
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql`:
- Line 18: The CASE that emits the CONNECTION LIMIT currently uses "rolconnlimit
> 0" which omits the valid "CONNECTION LIMIT 0"; update the condition in the
CASE expression that references rolconnlimit (in the roles/sql template) to
check "rolconnlimit != -1" instead (matching the pattern used in create.sql and
update.sql) so that 0 is included and only -1 (unlimited) is excluded.
♻️ Duplicate comments (8)
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sql (3)
43-46: Consider aligningrolvaliduntilcheck with create.sql for consistency.The check
{% if data.rolvaliduntil %}treats empty strings as falsy, generatingVALID UNTIL 'infinity'. The create.sql uses a more explicit pattern. While this is minor, aligning the templates would improve maintainability.
56-61: Guard-render mismatch can produce invalid SQL if keys are missing.The condition
'admin' in item or 'inherit' in item or 'set' in itemallows entry if ANY key exists, but then unconditionally renders all three values ({{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}). If validation is bypassed or data is malformed, this produces broken SQL likeWITH ADMIN true, INHERIT , SET.Suggested fix using defaults
{% if data.rolmembership_list and data.rolmembership_list|length > 0 %} {% for item in data.rolmembership_list %} -GRANT {{ conn|qtIdent(item.role) }} TO {{ conn|qtIdent(rolname) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}{% endif %}; +GRANT {{ conn|qtIdent(item.role) }} TO {{ conn|qtIdent(rolname) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin|default(false) }}, INHERIT {{ item.inherit|default(true) }}, SET {{ item.set|default(true) }}{% endif %}; {% endfor %} {% endif %}
114-118: Same guard-render mismatch exists for rol_members_list.Apply the same defensive fix with Jinja defaults here for consistency.
Suggested fix
{% for item in data.rol_members_list %} -GRANT {{ conn|qtIdent(rolname) }} TO {{ conn|qtIdent(item.role) }} {% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin }}, INHERIT {{ item.inherit }}, SET {{ item.set }}{% endif %}; +GRANT {{ conn|qtIdent(rolname) }} TO {{ conn|qtIdent(item.role) }}{% if 'admin' in item or 'inherit' in item or 'set' in item %} WITH ADMIN {{ item.admin|default(false) }}, INHERIT {{ item.inherit|default(true) }}, SET {{ item.set|default(true) }}{% endif %}; {% endfor %}web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/properties.sql (2)
11-15: LEFT JOIN can produce NULL entries in rolmembership array; seclabels ordering remains nondeterministic.The LEFT JOIN on line 12 can yield
rm.rolname = NULLwhen the role no longer exists, causing the concatenation to produce NULL entries in the array. Additionally, thearray_aggon line 15 lacksORDER BY, making results nondeterministic across executions.Suggested fix
ARRAY( SELECT CASE WHEN am.admin_option THEN '1' ELSE '0' END || CASE WHEN am.inherit_option THEN '1' ELSE '0' END || CASE WHEN am.set_option THEN '1' ELSE '0' END || rm.rolname FROM (SELECT * FROM pg_catalog.pg_auth_members WHERE member = r.oid) am - LEFT JOIN pg_catalog.pg_roles rm ON (rm.oid = am.roleid) + JOIN pg_catalog.pg_roles rm ON (rm.oid = am.roleid) ORDER BY rm.rolname ) AS rolmembership, - (SELECT pg_catalog.array_agg(provider || '=' || label) FROM pg_catalog.pg_shseclabel sl1 WHERE sl1.objoid=r.oid) AS seclabels + (SELECT pg_catalog.array_agg(provider || '=' || label ORDER BY provider, label) FROM pg_catalog.pg_shseclabel sl1 WHERE sl1.objoid=r.oid) AS seclabels
17-31: Add catalog qualification and ORDER BY for deterministic rolmembers output.The inner query references
pg_rolesandpg_auth_memberswithoutpg_catalog.qualification, and the outer ARRAY SELECT lacks anORDER BYclause, making results nondeterministic.Suggested fix
,ARRAY( SELECT CASE WHEN pg.admin_option THEN '1' ELSE '0' END || CASE WHEN pg.inherit_option THEN '1' ELSE '0' END || CASE WHEN pg.set_option THEN '1' ELSE '0' END || pg.usename FROM - (SELECT pg_roles.rolname AS usename, + (SELECT pg_catalog.pg_roles.rolname AS usename, pg_auth_members.admin_option AS admin_option, pg_auth_members.inherit_option AS inherit_option, pg_auth_members.set_option AS set_option - FROM pg_roles - JOIN pg_auth_members ON pg_roles.oid=pg_auth_members.member AND pg_auth_members.roleid={{ rid|qtLiteral(conn) }}::oid) pg + FROM pg_catalog.pg_roles + JOIN pg_catalog.pg_auth_members ON pg_catalog.pg_roles.oid=pg_catalog.pg_auth_members.member AND pg_catalog.pg_auth_members.roleid={{ rid|qtLiteral(conn) }}::oid) pg + ORDER BY pg.usename ) rolmembersweb/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js (1)
61-67: Add defensive checks for potentially undefined member properties.When
serverVersion >= 160000, the code directly calls.toString()onmember.admin,member.inherit, andmember.set. If these properties are undefined or null (e.g., from legacy data migration or API inconsistencies), this will throw a TypeError.Suggested defensive fix
- const serverVersion = this.nodeInfo && this.nodeInfo.server && this.nodeInfo.server.version || 0; + const serverVersion = this.nodeInfo?.server?.version ?? 0; rawData.forEach(member => { - let badges = serverVersion >= 160000 ? ` [WITH ADMIN ${member.admin.toString().toUpperCase()}, INHERIT ${member.inherit.toString().toUpperCase()}, SET ${member.set.toString().toUpperCase()}]` : member.admin ? ' [WITH ADMIN OPTION]' : ''; + let badges = ''; + if (serverVersion >= 160000) { + const admin = (member.admin ?? false).toString().toUpperCase(); + const inherit = (member.inherit ?? true).toString().toUpperCase(); + const set = (member.set ?? true).toString().toUpperCase(); + badges = ` [WITH ADMIN ${admin}, INHERIT ${inherit}, SET ${set}]`; + } else { + badges = member.admin ? ' [WITH ADMIN OPTION]' : ''; + }web/pgadmin/browser/server_groups/servers/roles/__init__.py (2)
218-236: Aliasing issue: list assignments are mutated via.extend().The direct assignment at lines 219 and 221 creates references to the original input data, which are then mutated by
.extend()calls at lines 224-225 and 231-232. This can cause unexpected side effects on the caller's data.As noted in a prior review, wrap these with
list()for defensive copies:if id == -1: - data['rolmembership_list'] = data['rolmembership'] + data['rolmembership_list'] = list(data['rolmembership']) else: - data['rolmembership_list'] = data['rolmembership'].get('added', []) + data['rolmembership_list'] = list(data['rolmembership'].get('added', []))
821-840: Regex search result not guarded againstNone→ potentialAttributeError.The
re.search()calls at lines 827 and 833 returnNoneif the pattern doesn't match, causing.group()to raiseAttributeError. This was flagged in a prior review.Add guards as previously suggested:
if self.manager.version < 160000: - role = re.search(r'([01])(.+)', role) + m = re.match(r'^([01])(.+)$', role or '') + if not m: + continue rolmembers.append({ - 'role': role.group(2), - 'admin': True if role.group(1) == '1' else False + 'role': m.group(2), + 'admin': m.group(1) == '1' })
🧹 Nitpick comments (2)
web/pgadmin/browser/server_groups/servers/roles/__init__.py (2)
210-216: Consider extracting_part_dict_listas a module-level or class-level helper.The same helper function
_part_dict_listis defined identically in both_process_rolemembership(line 210) and_process_rolmembers(line 243). This violates DRY principle and increases maintenance burden.♻️ Proposed refactor
Extract to a static method or module-level function:
+ `@staticmethod` + def _part_dict_list(dict_list, condition): + """Filter dict_list by condition, returning matching dicts.""" + return [d for d in dict_list if condition(d)] + def _process_rolemembership(self, id, data): """ Process the input rolemembership list to appropriate keys :param id: id of role :param data: input role data """ - def _part_dict_list(dict_list, condition): - ret_val = [] - for d in dict_list: - if condition(d): - ret_val.append(d) - - return ret_val - if id == -1: data['rolmembership_list'] = data['rolmembership'] else: - data['rolmembership_list'] = data['rolmembership'].get('added', []) + data['rolmembership_list'] = list(data['rolmembership'].get('added', [])) # ... rest of method using self._part_dict_listAlso applies to: 243-249
280-301: Error message includesinherit/setfields but validation is version-dependent.The error message (lines 280-291) shows
inheritandsetfields as required, but validation (lines 293-301) only enforces them for PostgreSQL 16+. This could confuse users on older PostgreSQL versions.Consider making the error message version-aware to match the actual validation requirements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
docs/en_US/images/role_membership.pngis excluded by!**/*.pngdocs/en_US/images/role_sql.pngis excluded by!**/*.png
📒 Files selected for processing (42)
docs/en_US/role_dialog.rstweb/pgadmin/browser/server_groups/servers/roles/__init__.pyweb/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.jsweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/properties.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/update.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sqlweb/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.jsonweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options1.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options2.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options3.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.msqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.sqlweb/pgadmin/browser/server_groups/servers/roles/tests/default/test.jsonweb/pgadmin/browser/server_groups/servers/static/js/membership.ui.jsweb/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsxweb/pgadmin/static/js/SchemaView/common.jsweb/pgadmin/static/js/SchemaView/options/index.js
🚧 Files skipped from review as they are similar to previous changes (26)
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.sql
- web/pgadmin/browser/server_groups/servers/static/js/membership.ui.js
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options2.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.msql
- web/pgadmin/static/js/SchemaView/common.js
- web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/create.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_description.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options2.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options3.msql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_description.msql
- web/pgadmin/static/js/SchemaView/DataGridView/utils/createGridColumns.jsx
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_name.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.msql
- docs/en_US/role_dialog.rst
- web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_login_role.sql
- web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.sql
🧰 Additional context used
🧬 Code graph analysis (1)
web/pgadmin/static/js/SchemaView/options/index.js (1)
web/pgadmin/static/js/SchemaView/common.js (4)
isModeSupportedByField(15-17)isModeSupportedByField(15-17)isFieldSupportedByPgVersion(19-34)isFieldSupportedByPgVersion(19-34)
🔇 Additional comments (17)
web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options4.msql (1)
1-7: AI summary is inconsistent with actual file content.The AI summary claims this file adds "four GRANT statements" including
WITH INHERIT OPTIONandWITH SET OPTION, but the actual file contains 2 GRANT statements and 2 REVOKE statements, all using onlyWITH ADMIN OPTION. This is correct behavior since this file is in thedefault/folder (pre-PostgreSQL 16), and INHERIT/SET options are PostgreSQL 16+ features expected in the16_plus/folder.The SQL syntax is correct, and the special character escaping in the role name using
""to escape embedded double quotes is properly handled.web/pgadmin/browser/server_groups/servers/roles/tests/default/alter_role_options1.msql (1)
10-19: LGTM!The test file correctly exercises role membership scenarios including:
- Granting a role TO the complex-named role (lines 10, 12)
- Granting the complex-named role TO other roles (lines 17, 19)
- Both WITH ADMIN OPTION and without variants
The special character escaping in the role name (
"Role2_$%{}[]()&*^!@""'/#"`) is properly handled with double-quote escaping.web/pgadmin/static/js/SchemaView/options/index.js (2)
18-21: Good refactor to centralize version-gating logic.The new imports properly leverage the shared helper functions from
../commonfor consistent field visibility evaluation across the codebase.
48-57: LGTM!The
VISIBLEevaluator now correctly composes three checks:
isModeSupportedByField- validates field mode compatibilityisFieldSupportedByPgVersion- validates server type and version constraints- Original
field[VISIBLE]evaluationThis ensures fields with
min_version,max_version, orserver_typeconstraints are properly hidden when the connected server doesn't meet the requirements. The short-circuit evaluation order is correct.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_login_role_options.sql (1)
1-18: LGTM!This test file provides good coverage for PostgreSQL 16+ role features:
- LOGIN capability with various role attributes
- Database-specific configuration (
SET application_name)- Role comments
- Proper escaping of special characters in role names
The use of
<PASSWORD>and<TIMESTAMPTZ_1>placeholders follows the test framework's parameterization pattern.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.msql (1)
1-3: LGTM!Good test coverage for membership revocation scenarios:
- Revoking a role FROM the complex-named role (line 1)
- Revoking the complex-named role FROM another role (line 3)
This properly tests the bidirectional REVOKE operations that complement the GRANT tests in other files.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_name.sql (1)
1-13: LGTM!The test file correctly covers PostgreSQL 16+ role creation with:
- Extensive special character handling in the role name
- Explicit role attribute settings (NOLOGIN, NOSUPERUSER, INHERIT, etc.)
- Role comments
The commented DROP statement (line 2) serves as documentation showing how to clean up the role if needed.
web/pgadmin/browser/server_groups/servers/roles/tests/default/test.json (2)
45-86: LGTM! Test role creation scenarios are well-structured.The new test roles (Role 3 and Role 4) follow the same pattern as existing roles and provide adequate test coverage for the
rolmembersfunctionality being added alongsiderolmembership.
148-149: LGTM! Alter scenarios comprehensively test membership operations.The test data properly covers add, change, and delete operations for both
rolmembershipandrolmembers, ensuring the new list-based structure is exercised through various update paths.Also applies to: 163-165, 179-181, 197-202
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/default/update.sql (2)
50-69: LGTM! List-based membership handling is correctly implemented.The template properly iterates over
rolmembership_revoked_list,rolmembership_list, androlmembership_revoked_adminsto generate REVOKE/GRANT statements. The identifier quoting viaqtIdentis applied correctly, and the admin option revocation logic appropriately checks for'admin' in item and not item.admin.
116-135: LGTM! Role members handling mirrors the membership pattern correctly.The
rol_members_*lists follow the same structure asrolmembership_*, with the grant/revoke direction correctly reversed (grantingrolnameTOitem.roleinstead of vice versa).web/pgadmin/browser/server_groups/servers/roles/static/js/role.ui.js (1)
180-183: LGTM! Arrow function wrappers correctly preservethiscontext.The change from direct method reference to arrow function wrapper
(raw) => obj.memberDataFormatter(raw)ensures properthisbinding when the formatter is invoked.Also applies to: 201-204
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql (1)
31-56: LGTM! Well-structured GRANT statement generation for PostgreSQL 16+.The conditional handling of ADMIN OPTION, INHERIT OPTION, and SET OPTION with proper comma placement is correctly implemented. Grouping by all three flags ensures grants with identical options are consolidated into single statements.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/create_role.msql (1)
1-9: LGTM! Good test coverage for special character handling in role names.The test correctly validates SQL generation for role names containing special characters including quotes, backslashes, and various symbols. This ensures proper identifier escaping in the templating system.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.sql (1)
1-21: LGTM! Comprehensive test for role alteration with PostgreSQL 16+ membership options.The test correctly validates the expected SQL output including:
- Role creation with attributes and password/validity placeholders
- GRANT statements both with and without the new WITH options (ADMIN, INHERIT, SET)
- Database-specific settings and comments
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/test.json (2)
148-149: LGTM! Good coverage of membership options with admin, inherit, and set flags.The test data correctly exercises the new PostgreSQL 16+ membership options including added and changed membership entries with all three boolean flags (admin, inherit, set).
172-209: LGTM! Comprehensive alter role options tests.The test scenarios 2-4 effectively cover:
- Changed memberships (options 2)
- Deleted memberships (options 3)
- Combined add + change operations (options 4)
This provides good coverage for the membership management workflows.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql`:
- Around line 35-41: The current CASE that conditionally appends the WITH clause
(the block using admin_option, inherit_option, set_option) must be changed to
always emit all three options with explicit TRUE/FALSE instead of only emitting
when true; update the expression that builds the WITH clause (the CASE WHEN
admin_option OR inherit_option OR set_option THEN ... END) so it always produces
" WITH ADMIN OPTION " || (CASE WHEN admin_option THEN 'TRUE' ELSE 'FALSE' END)
|| ', INHERIT OPTION ' || (CASE WHEN inherit_option THEN 'TRUE' ELSE 'FALSE'
END) || ', SET OPTION ' || (CASE WHEN set_option THEN 'TRUE' ELSE 'FALSE' END)
(keeping the surrounding concatenation with the GRANT ... TO ... || ... || ';'
and preserving quote_ident/pg_get_userbyid usage).
♻️ Duplicate comments (1)
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/create.sql (1)
35-37:PASSWORD NULLbranch is unreachable due to truthy check.The outer
if data.rolpasswordprevents theis nonebranch from ever firing, so explicit NULL isn’t emitted when the key is present. Consider switching to a key-presence check.Proposed fix
-{% if data.rolpassword %} +{% if 'rolpassword' in data %}
🧹 Nitpick comments (6)
web/pgadmin/browser/server_groups/servers/static/js/membership.ui.js (1)
24-30: Simplify version check logic.The condition
(node_info) && node_info.version >= 160000is potentially misleading. Sincenode_infodefaults to an empty object{}, the truthiness check(node_info)will always pass. Consider using optional chaining for clarity:♻️ Suggested refinement
constructor(roleMembersOptions, node_info={}) { super({ role: undefined, - admin: (node_info) && node_info.version >= 160000 ? false : undefined, + admin: node_info?.version >= 160000 ? false : undefined, inherit: undefined, - set: (node_info) && node_info.version >= 160000 ? true : undefined, + set: node_info?.version >= 160000 ? true : undefined, });web/pgadmin/browser/server_groups/servers/roles/__init__.py (3)
210-216: Consider extracting_part_dict_listto reduce duplication.The
_part_dict_listhelper function is defined identically in both_process_rolemembership(lines 210-216) and_process_rolmembers(lines 243-249). Consider extracting it as a module-level or class-level helper.♻️ Suggested extraction
# At module level or as a static method def _part_dict_list(dict_list, condition): return [d for d in dict_list if condition(d)]Then remove the nested definitions and use the shared helper in both methods.
Also applies to: 243-249
221-235: Potential aliasing issue: list mutations may affect original input.At line 221,
data['rolmembership_list']is assigned directly fromdata['rolmembership'].get('added', []). Subsequent.extend()calls (lines 224-226, 231-232) mutate this list, which could modify the originaladdedlist if no copy was made.Consider using a defensive copy:
♻️ Suggested fix
else: - data['rolmembership_list'] = data['rolmembership'].get('added', []) + data['rolmembership_list'] = list(data['rolmembership'].get('added', []))Based on past review comments, this aliasing concern was previously raised.
254-268: Same aliasing concern applies torol_members_list.Apply the same defensive copy pattern here:
♻️ Suggested fix
else: - data['rol_members_list'] = data['rolmembers'].get('added', []) + data['rol_members_list'] = list(data['rolmembers'].get('added', []))Based on past review comments.
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options4.sql (1)
14-14: Consider using a placeholder for the timestamp.Other test fixtures in this PR use
<TIMESTAMPTZ_1>as a placeholder for theVALID UNTILvalue (e.g.,alter_login_role_options.sql,alter_role_options1.sql), but this file uses a hardcoded value'2050-01-01 00:00:00+05:30'. Using a consistent placeholder pattern would improve maintainability.web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options3.sql (1)
14-14: Consider using a placeholder for the timestamp.Similar to
alter_role_options4.sql, this file uses a hardcoded timestamp instead of the<TIMESTAMPTZ_1>placeholder pattern used elsewhere.
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msql`:
- Around line 17-19: The GRANT lines use an invalid role identifier with
unescaped quotes and special chars (e.g. "Role2_$%{}[]()&*^!@""'`\/#") causing
bad SQL; update the SQL template that emits these GRANT statements so role
identifiers are properly quoted/escaped (use quote_ident or double any internal
double-quote by replacing " with "") and produce valid PostgreSQL 16+ syntax for
role-to-member grants (e.g., GRANT "RoleNameWith""Quote" TO member WITH ADMIN
OPTION, INHERIT, SET as appropriate); locate the GRANT lines that emit the
identifier (the lines containing GRANT "Role2_...") and change the quoting logic
to safely escape special characters and internal quotes.
♻️ Duplicate comments (1)
web/pgadmin/browser/server_groups/servers/roles/templates/roles/sql/16_plus/sql.sql (1)
35-41: Use correct PostgreSQL 16 GRANT syntax with explicit TRUE/FALSE for all options.The current implementation uses deprecated "OPTION" suffix syntax (e.g.,
ADMIN OPTION) and only emits options when true. PostgreSQL 16's correct syntax isWITH ADMIN {TRUE|FALSE}, INHERIT {TRUE|FALSE}, SET {TRUE|FALSE}. Additionally, omitting FALSE options means regenerated DDL won't preserve the actual membership settings.Proposed fix
- CASE WHEN admin_option OR inherit_option OR set_option THEN - ' WITH ' || - (CASE WHEN admin_option THEN 'ADMIN OPTION' ELSE '' END || - CASE WHEN inherit_option THEN (CASE WHEN admin_option THEN ', ' ELSE '' END) || 'INHERIT OPTION' ELSE '' END || - CASE WHEN set_option THEN (CASE WHEN admin_option OR inherit_option THEN ', ' ELSE '' END) || 'SET OPTION' ELSE '' END) - ELSE '' END || ';' AS sql + ' WITH ADMIN ' || CASE WHEN admin_option THEN 'TRUE' ELSE 'FALSE' END || + ', INHERIT ' || CASE WHEN inherit_option THEN 'TRUE' ELSE 'FALSE' END || + ', SET ' || CASE WHEN set_option THEN 'TRUE' ELSE 'FALSE' END || ';' AS sql
🧹 Nitpick comments (2)
web/pgadmin/browser/server_groups/servers/roles/__init__.py (2)
218-235: Consider defensive copies to avoid mutating input data.Lines 219 and 221 assign lists directly from the input
data['rolmembership'], and subsequent.extend()calls (lines 224, 231) mutate these lists in place. This modifies the original input dictionary, which could cause unexpected side effects.♻️ Suggested fix using defensive copies
if id == -1: - data['rolmembership_list'] = data['rolmembership'] + data['rolmembership_list'] = list(data['rolmembership']) else: - data['rolmembership_list'] = data['rolmembership'].get('added', []) + data['rolmembership_list'] = list(data['rolmembership'].get('added', []))
250-268: Same defensive copy concern in_process_rolmembers.Lines 251 and 254 have the same aliasing pattern as
_process_rolemembership. The.extend()calls on lines 257 and 264 will mutate the original input data.♻️ Suggested fix
if id == -1: - data['rol_members_list'] = data['rolmembers'] + data['rol_members_list'] = list(data['rolmembers']) else: - data['rol_members_list'] = data['rolmembers'].get('added', []) + data['rol_members_list'] = list(data['rolmembers'].get('added', []))
web/pgadmin/browser/server_groups/servers/roles/tests/16_plus/alter_role_options1.msql
Show resolved
Hide resolved
3 participants
Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.