refactor(core,pgsql-test): modular role management with SQL templates #422
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
- Add new SQL template files for role operations: - ensure-base-roles.sql: Create anonymous/authenticated/administrator roles - ensure-login-role.sql: Create LOGIN role with optional advisory locks - ensure-membership.sql: Grant role membership with optional locks - grant-connect.sql: Grant CONNECT privilege on database - Add TypeScript types for role operation options: - RoleNameMapping, EnsureLoginRoleOptions, EnsureRoleMembershipsOptions - CreateDbUserOptions, BootstrapTestUsersOptions - OnMissingRoleAction for configurable error handling - Add shared role-utils.ts with modular functions: - ensureBaseRoles, ensureLoginRole, ensureRoleMembership - ensureRoleMemberships, grantConnect, createDbUser, createTestUsers - Refactor PgpmInit to use new modular functions: - bootstrapRoles now uses ensureBaseRoles - bootstrapTestRoles now uses createTestUsers - bootstrapDbRoles now uses createDbUser with configurable options - Refactor DbAdmin in pgsql-test: - grantRole now supports useLocks, lockNamespace, onMissingRole options - createUserRole now supports useLocks, grantAdmin options - Add streamSqlWithParams for parameterized queries - Export new types and functions from @pgpmjs/core This refactor consolidates duplicated role creation logic, adds optional advisory locks for concurrent CI/CD safety, and makes admin role granting explicit and configurable. Note: Test credentials (app_user/app_password, app_admin/admin_password) are intentional test fixtures matching existing bootstrap-test-roles.sql 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:
|
PostgreSQL's pg library doesn't support multiple statements in a parameterized query. Consolidate the two DO blocks and BEGIN/COMMIT into a single DO block to fix the 'cannot insert multiple commands into a prepared statement' error. Co-Authored-By: Dan Lynch <[email protected]>
DO blocks in PostgreSQL don't support parameterized queries (, , etc.)
because the block body is parsed as a string literal. This fix:
- Removes loadSqlTemplate and uses inline SQL with safe string interpolation
- Uses escapeLiteral() for SQL string values
- Uses format('%I', ...) inside PL/pgSQL for identifier escaping
- Applies fix to all role management functions
Co-Authored-By: Dan Lynch <[email protected]>
DO blocks in PostgreSQL don't support parameterized queries (, , etc.) because the block body is parsed as a string literal. This fix: - Uses escapeLiteral() for SQL string values in grantRole and createUserRole - Uses conditional string interpolation for optional advisory locks - Removes unused streamSqlWithParams method and Client import - Applies same fix pattern as role-utils.ts 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.
refactor(core,pgsql-test): modular role management with SQL templates
Summary
This PR refactors the role creation code to use modular functions with optional advisory locks for concurrent CI/CD safety. The changes consolidate duplicated role creation logic between
@pgpmjs/coreandpgsql-testpackages.Key changes:
role-utils.tsfor role operations (ensureBaseRoles, ensureLoginRole, ensureRoleMembership, grantConnect, etc.)PgpmInitclass to use the new modular functionsDbAdminclass with new options:useLocks,grantAdmin,onMissingRoleescapeLiteral()helper for safe SQL string interpolation in DO blocks@pgpmjs/coreAdvisory locks are optional (default: false) as requested.
Updates since last revision
Fixed CI failures caused by attempting to use parameterized queries (
$1,$2, etc.) inside PostgreSQL DO blocks. DO blocks are anonymous PL/pgSQL code blocks that don't support query parameters - the parameters are parsed as PL/pgSQL positional parameters, not query placeholders.The fix switches to safe string interpolation using
escapeLiteral()which properly escapes single quotes. The SQL template files insql/directory remain for reference but the actual execution uses inline SQL in TypeScript with proper escaping.Review & Testing Checklist for Human
escapeLiteral()escaping is sufficient - The function doubles single quotes and wraps in single quotes. Verify this handles edge cases like backslashes, unicode, and null bytes correctly for your PostgreSQL version.bootstrapRoles(),bootstrapTestRoles(), andbootstrapDbRoles()against a real PostgreSQL instancecreateUserRole()in pgsql-test with bothgrantAdmin: trueandgrantAdmin: falseuseLocks: trueis passedRecommended test plan:
lql admin-users bootstrapandlql admin-users add --testtest'userNotes
packages/core/src/init/sql/use$1,$2syntax but are not currently loaded - the actual SQL is inline inrole-utils.tswithescapeLiteral()escaping. Consider removing these files or documenting them as reference/future use.app_password,admin_password) increateTestUsersare intentional fixtures matching existingbootstrap-test-roles.sqlgrantAdminoption defaults totruefor backward compatibility in the test frameworkLink to Devin run: https://app.devin.ai/sessions/3a1a859673d943189e0dafcfefc870ad
Requested by: Dan Lynch ([email protected]) / @pyramation