feat: unify role management with configurable role names and concurrency safety #438
+960
−337
Conversation
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
… advisory locks - Add IF NOT EXISTS pre-checks before CREATE ROLE/USER operations - Add IF NOT EXISTS pre-checks before GRANT role membership operations - Add useLocks option (default: false) for optional advisory locking - Harden exception handling for CREATE ROLE/USER: - Handle and ignore: 42710 (duplicate_object), 23505 (unique_violation) - Surface: 42501 (insufficient_privilege) - Harden exception handling for GRANT role membership: - Handle and ignore: 23505 (unique_violation) - Handle gracefully: 42704 (undefined_object) - log notice and continue - Surface: 42501 (insufficient_privilege), 0LP01 (invalid_grant_operation) - Harden exception handling for REVOKE/DROP ROLE: - Ignore: 42704 (undefined_object) - Surface: 2BP01 (dependent_objects_still_exist), 55006 (object_in_use), 42501 (insufficient_privilege) Files modified: - pgpm/core/src/init/client.ts - pgpm/core/src/init/sql/bootstrap-roles.sql - pgpm/core/src/init/sql/bootstrap-test-roles.sql - postgres/pgsql-test/src/admin.ts Co-Authored-By: Dan Lynch <[email protected]>
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Explains when static SQL files vs dynamic TypeScript helpers are used: - bootstrap-roles.sql: CLI bootstrap of base roles (anonymous, authenticated, administrator) - bootstrap-test-roles.sql: CLI creation of test users (app_user, app_admin) - PgpmInit.bootstrapDbRoles(): CLI creation of custom users with provided credentials - PgpmInit.removeDbRoles(): CLI removal of users - DbAdmin.createUserRole(): Test harness user creation with configurable role names - DbAdmin.grantRole(): Flexible role granting for tests Also documents: - Role model (base roles vs users) - Call paths for each entry point - Role name configuration in pgsql-test - Concurrency safety patterns and error handling Co-Authored-By: Dan Lynch <[email protected]>
…onfigurable role names - Add shared role management module in @pgpmjs/core/src/roles/index.ts - Export SQL generators: generateCreateBaseRolesSQL, generateCreateUserSQL, generateCreateTestUsersSQL, generateCreateUserWithGrantsSQL, generateGrantRoleSQL, generateRemoveUserSQL - Update PgpmInit to use shared module with optional RoleMapping parameter - Update DbAdmin to import from @pgpmjs/core instead of duplicating SQL generation - All operations support configurable role names (defaults to canonical: anonymous, authenticated, administrator) - Update ROLES.md to document unified architecture Co-Authored-By: Dan Lynch <[email protected]>
devin-ai-integration
bot
changed the title
- Add sqlLiteral() helper for safe SQL string literal escaping - Declare role names as PL/pgSQL variables at the start of DO blocks - Use variables consistently in EXISTS checks and EXECUTE format() calls - Never concatenate role names directly into SQL identifiers - All role names now properly parameterized via variables Co-Authored-By: Dan Lynch <[email protected]>
- Remove DEFAULT_ROLES constant, use pgpmDefaults.db.roles instead - Add TestUserOptions interface for configurable test user credentials - generateCreateTestUsersSQL now accepts optional testUsers parameter - Test user defaults sourced from pgpmDefaults.db.connection - Remove static SQL files (bootstrap-roles.sql, bootstrap-test-roles.sql) - Update ROLES.md documentation to reflect dynamic implementation Co-Authored-By: Dan Lynch <[email protected]>
The bootstrap-roles.sql and bootstrap-test-roles.sql files were deleted as role management is now fully dynamic. Remove the copy:sql commands that were copying these files to dist. Co-Authored-By: Dan Lynch <[email protected]>
…resolved values - Remove getRoleMapping, getTestUserDefaults, TestUserOptions from core - Add ResolvedRoleMapping and ResolvedTestUserCredentials interfaces - Update SQL generators to accept explicit resolved values (no defaults) - Add TestUserCredentials type with app and admin fields - Add connections field to pgpmDefaults with app/admin credentials - Add env var parsing for DB_CONNECTIONS_APP_* and DB_CONNECTIONS_ADMIN_* - Update CLI commands to use getConnEnvOptions() from @pgpmjs/env - Callers now responsible for merging via deepmerge (env + defaults + overrides) Co-Authored-By: Dan Lynch <[email protected]>
… ! assertions - Delete ResolvedRoleMapping and ResolvedTestUserCredentials interfaces - SQL generators now accept RoleMapping and TestUserCredentials with ! assertions - PgpmInit methods accept RoleMapping and TestUserCredentials (optional fields) - CLI commands pass db.roles! and db.connections! directly (no fallbacks) - All ?? 'anonymous' / ?? 'app_user' etc. fallbacks eliminated - Callers use getConnEnvOptions() which merges defaults via deepmerge Co-Authored-By: Dan Lynch <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
feat: unify role management with configurable role names and concurrency safety
Summary
This PR consolidates role/user management into a single shared module (
@pgpmjs/core/src/roles/index.ts) that bothPgpmInit(CLI) andDbAdmin(test harness) now use. All operations are idempotent under concurrency with IF NOT EXISTS pre-checks and hardened exception handling for TOCTOU safety. Role names are now configurable via theRoleMappinginterface, defaulting to canonical names (anonymous,authenticated,administrator).Key changes:
pgpm/core/src/roles/index.ts - Shared SQL generators (single source of truth):
generateCreateBaseRolesSQL(roles)- Create base rolesgenerateCreateUserSQL(username, password, roles, options?)- Create user with grantsgenerateCreateTestUsersSQL(roles, connections)- Create test usersgenerateCreateUserWithGrantsSQL(username, password, rolesToGrant, options?)- Create user with custom role grantsgenerateGrantRoleSQL(role, user)- Grant single rolegenerateRemoveUserSQL(username, roles, options?)- Remove user and revoke grantspgpm/types/src/pgpm.ts - Added
TestUserCredentialstype andconnectionsfield topgpmDefaultswith app/admin credentials.pgpm/env/src/env.ts - Added env var parsing for
DB_CONNECTIONS_APP_USER,DB_CONNECTIONS_APP_PASSWORD,DB_CONNECTIONS_ADMIN_USER,DB_CONNECTIONS_ADMIN_PASSWORD.CLI commands - All admin-users commands now use
getConnEnvOptions()from@pgpmjs/envto get merged values before passing toPgpmInit.Updates Since Last Revision
ResolvedRoleMappingandResolvedTestUserCredentialsinterfaces: These were redundant wrapper types. SQL generators now accept the originalRoleMappingandTestUserCredentialstypes directly.!assertions internally: SincegetConnEnvOptions()mergespgpmDefaultsviadeepmerge.all(), values are guaranteed to be present. Generators assert with!rather than requiring callers to build "resolved" objects.?? 'anonymous'fallback patterns: No more redundant default literals scattered across CLI commands. Callers passdb.roles!anddb.connections!directly.pgpmDefaultsin@pgpmjs/types. Config resolution follows deepmerge pattern: defaults → config file → env vars → overrides.Review & Testing Checklist for Human
!assertions are safe: SQL generators assume merged config has all fields. ConfirmgetConnEnvOptions()always returns complete objects with defaults.PgpmInitmethods: Methods now requireRoleMappingandTestUserCredentialsparameters - any external code calling these directly will need updates.DB_CONNECTIONS_APP_USER=custom_userand verify it flows through to generated SQL.pgpm admin-users bootstrap,pgpm admin-users add --test, andpgpm admin-users add --username X --password Y.Recommended test plan:
pgpm admin-users bootstrapto create base rolespgpm admin-users add --testto create test usersDB_CONNECTIONS_APP_USER=custom_appand runpgpm admin-users add --testagain - verify it uses the custom usernamepgpm admin-users remove --testto remove test usersgetConnections()still works correctlyNotes
pnpm buildcompleted successfully)bootstrap-roles.sql,bootstrap-test-roles.sql) were deleted - all role management is now dynamicRoleManagementOptionsis still exported from@pgpmjs/corefor optional advisory locking supportLink to Devin run: https://app.devin.ai/sessions/25b7d2fd43de4af5a6af0961ab72abaf
Requested by: Dan Lynch (@pyramation)