feat: profile-level config defaults with {schema}/{profile} placeholders - #139
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds profile-level role configuration defaults with placeholder expansion, preserves configuration-bearing roles during suggestion and rendering, introduces advisory column-grant detection, updates reconciliation handling, and adds property-based and live validation with supporting schema and documentation changes. ChangesProfile configuration defaults
Column-level grant diagnostics
Diff engine validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Manifest
participant ProfileExpansion
participant GeneratedRole
participant Database
Manifest->>ProfileExpansion: define profile config
ProfileExpansion->>GeneratedRole: substitute schema/profile values
GeneratedRole->>Database: apply ALTER ROLE defaults
Database-->>GeneratedRole: persisted role configuration
sequenceDiagram
participant Inspector
participant PostgreSQL
participant CLIOrOperator
Inspector->>PostgreSQL: inspect column ACLs
PostgreSQL-->>Inspector: column-level grant rows
Inspector->>CLIOrOperator: diagnostics
CLIOrOperator-->>CLIOrOperator: fail on wildcard issues or warn on column grants
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
hardbyte
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1f993655f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Values without either placeholder are returned unchanged. | ||
| fn substitute_placeholders(value: &str, schema: &str, profile: &str) -> String { | ||
| value | ||
| .replace("{schema}", schema) |
There was a problem hiding this comment.
When a profile config value is a list-quoted GUC such as search_path, replacing {schema} as raw text breaks schemas whose names contain list separators. For example, with schema.name: "tenant, archive" and search_path: "{schema}", this expands to tenant, archive; later RoleState::from_definition/render_set_config split list GUCs on commas/whitespace, so pgroles applies two path entries (tenant and archive) instead of the single quoted schema it just created. The substitution needs to preserve placeholder values as one list element for list-GUC parameters.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/pgroles-inspect/src/lib.rs (2)
941-1035: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a direct test for
blocking_message()with column-only diagnostics.Existing tests cover
is_emptyandDisplay, but none assert thatblocking_message()returnsNonewhen onlycolumn_level_grantsis populated (no wildcard diagnostics). That's the exact behavior the CLI and operator rely on to avoid blocking reconciliation on advisory-only findings — worth a direct, explicit test given how critical that gating method is.✅ Suggested test
#[test] fn blocking_message_is_none_for_column_level_grants_only() { let mut diagnostics = InspectionDiagnostics::default(); diagnostics .column_level_grants .push(sample_column_level_grant("analytics", &["secret"])); assert!(diagnostics.blocking_message().is_none()); }🤖 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 `@crates/pgroles-inspect/src/lib.rs` around lines 941 - 1035, Add a focused test beside the existing InspectionDiagnostics tests for blocking_message(), populate only column_level_grants using sample_column_level_grant, and assert that blocking_message() returns None without any wildcard diagnostics.
626-640: 🚀 Performance & Scalability | 🔵 TrivialColumn-level grant counts aren't tracked in
InspectionStats.The new phase records timing via
stats.record_phase(...)but never sets a count field onstatsthe way the roles/grants/schemas phases do (e.g.,stats.grants = graph.grants.len()). Sincectx.observability.record_inspection(&inspection.stats)is the path that likely feeds operator metrics/dashboards, this advisory category is only visible via a conditionaldebug!log — invisible to anyone monitoringInspectionStats-derived metrics in production. Consider adding a count field so operators can alert on column-level grant trends the same way they can for other inspected categories.🤖 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 `@crates/pgroles-inspect/src/lib.rs` around lines 626 - 640, Set the column-level grant count on InspectionStats after fetch_column_level_grants returns, alongside the existing stats.record_phase call. Use diagnostics.column_level_grants.len() and the established stats field for this category, ensuring ctx.observability.record_inspection receives the count for metrics and dashboards.
🤖 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 `@docs/src/pages/docs/limitations.md`:
- Line 36: In the limitations documentation paragraph, update the phrase
“outside of pgroles” to “outside pgroles” while leaving the surrounding
explanation unchanged.
---
Nitpick comments:
In `@crates/pgroles-inspect/src/lib.rs`:
- Around line 941-1035: Add a focused test beside the existing
InspectionDiagnostics tests for blocking_message(), populate only
column_level_grants using sample_column_level_grant, and assert that
blocking_message() returns None without any wildcard diagnostics.
- Around line 626-640: Set the column-level grant count on InspectionStats after
fetch_column_level_grants returns, alongside the existing stats.record_phase
call. Use diagnostics.column_level_grants.len() and the established stats field
for this category, ensuring ctx.observability.record_inspection receives the
count for metrics and dashboards.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3d87284-c50f-437c-82b8-8ab50b758ef3
📒 Files selected for processing (26)
CHANGELOG.mdROADMAP.mdcharts/pgroles-operator/crds/postgrespolicies.pgroles.io.yamlcrates/pgroles-cli/src/lib.rscrates/pgroles-cli/src/main.rscrates/pgroles-core/src/composition.rscrates/pgroles-core/src/manifest.rscrates/pgroles-core/src/model.rscrates/pgroles-core/src/suggest.rscrates/pgroles-core/tests/diff_property.rscrates/pgroles-core/tests/suggest_property.rscrates/pgroles-inspect/src/lib.rscrates/pgroles-inspect/src/privileges.rscrates/pgroles-inspect/tests/config_roundtrip.rscrates/pgroles-operator/src/crd.rscrates/pgroles-operator/src/reconciler.rsdocs/src/components/Layout.jsxdocs/src/pages/docs/architecture.mddocs/src/pages/docs/executor-privileges.mddocs/src/pages/docs/installation.mddocs/src/pages/docs/limitations.mddocs/src/pages/docs/manifest-reference.mddocs/src/pages/docs/memberships.mddocs/src/pages/docs/profiles.mddocs/src/pages/docs/quick-start.mdk8s/crd.yaml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/pgroles-inspect/src/lib.rs (2)
941-1035: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a direct test for
blocking_message()with column-only diagnostics.Existing tests cover
is_emptyandDisplay, but none assert thatblocking_message()returnsNonewhen onlycolumn_level_grantsis populated (no wildcard diagnostics). That's the exact behavior the CLI and operator rely on to avoid blocking reconciliation on advisory-only findings — worth a direct, explicit test given how critical that gating method is.✅ Suggested test
#[test] fn blocking_message_is_none_for_column_level_grants_only() { let mut diagnostics = InspectionDiagnostics::default(); diagnostics .column_level_grants .push(sample_column_level_grant("analytics", &["secret"])); assert!(diagnostics.blocking_message().is_none()); }🤖 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 `@crates/pgroles-inspect/src/lib.rs` around lines 941 - 1035, Add a focused test beside the existing InspectionDiagnostics tests for blocking_message(), populate only column_level_grants using sample_column_level_grant, and assert that blocking_message() returns None without any wildcard diagnostics.
626-640: 🚀 Performance & Scalability | 🔵 TrivialColumn-level grant counts aren't tracked in
InspectionStats.The new phase records timing via
stats.record_phase(...)but never sets a count field onstatsthe way the roles/grants/schemas phases do (e.g.,stats.grants = graph.grants.len()). Sincectx.observability.record_inspection(&inspection.stats)is the path that likely feeds operator metrics/dashboards, this advisory category is only visible via a conditionaldebug!log — invisible to anyone monitoringInspectionStats-derived metrics in production. Consider adding a count field so operators can alert on column-level grant trends the same way they can for other inspected categories.🤖 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 `@crates/pgroles-inspect/src/lib.rs` around lines 626 - 640, Set the column-level grant count on InspectionStats after fetch_column_level_grants returns, alongside the existing stats.record_phase call. Use diagnostics.column_level_grants.len() and the established stats field for this category, ensuring ctx.observability.record_inspection receives the count for metrics and dashboards.
🤖 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 `@docs/src/pages/docs/limitations.md`:
- Line 36: In the limitations documentation paragraph, update the phrase
“outside of pgroles” to “outside pgroles” while leaving the surrounding
explanation unchanged.
---
Nitpick comments:
In `@crates/pgroles-inspect/src/lib.rs`:
- Around line 941-1035: Add a focused test beside the existing
InspectionDiagnostics tests for blocking_message(), populate only
column_level_grants using sample_column_level_grant, and assert that
blocking_message() returns None without any wildcard diagnostics.
- Around line 626-640: Set the column-level grant count on InspectionStats after
fetch_column_level_grants returns, alongside the existing stats.record_phase
call. Use diagnostics.column_level_grants.len() and the established stats field
for this category, ensuring ctx.observability.record_inspection receives the
count for metrics and dashboards.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3d87284-c50f-437c-82b8-8ab50b758ef3
📒 Files selected for processing (26)
CHANGELOG.mdROADMAP.mdcharts/pgroles-operator/crds/postgrespolicies.pgroles.io.yamlcrates/pgroles-cli/src/lib.rscrates/pgroles-cli/src/main.rscrates/pgroles-core/src/composition.rscrates/pgroles-core/src/manifest.rscrates/pgroles-core/src/model.rscrates/pgroles-core/src/suggest.rscrates/pgroles-core/tests/diff_property.rscrates/pgroles-core/tests/suggest_property.rscrates/pgroles-inspect/src/lib.rscrates/pgroles-inspect/src/privileges.rscrates/pgroles-inspect/tests/config_roundtrip.rscrates/pgroles-operator/src/crd.rscrates/pgroles-operator/src/reconciler.rsdocs/src/components/Layout.jsxdocs/src/pages/docs/architecture.mddocs/src/pages/docs/executor-privileges.mddocs/src/pages/docs/installation.mddocs/src/pages/docs/limitations.mddocs/src/pages/docs/manifest-reference.mddocs/src/pages/docs/memberships.mddocs/src/pages/docs/profiles.mddocs/src/pages/docs/quick-start.mdk8s/crd.yaml
🛑 Comments failed to post (1)
docs/src/pages/docs/limitations.md (1)
36-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Minor wording redundancy.
"outside of pgroles" → "outside pgroles" ("of" is redundant here).
Based on static analysis: `[style] This phrase is redundant. Consider using "outside".`✏️ Suggested wording fix
-PostgreSQL does not expose password hashes for comparison, so pgroles cannot detect when a password has been changed directly in the database outside of pgroles. +PostgreSQL does not expose password hashes for comparison, so pgroles cannot detect when a password has been changed directly in the database outside pgroles.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.PostgreSQL does not expose password hashes for comparison, so pgroles cannot detect when a password has been changed directly in the database outside pgroles. Passwords are re-applied on every `apply` from the configured source (CLI environment variable or operator Secret), and password-only changes are excluded from `diff --exit-code` drift detection since they always appear in the plan. See [passwords and drift detection](/docs/manifest-reference#roles) in the manifest reference.🧰 Tools
🪛 LanguageTool
[style] ~36-~36: This phrase is redundant. Consider using “outside”.
Context: ...s been changed directly in the database outside of pgroles. Passwords are re-applied on ev...(OUTSIDE_OF)
🤖 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 `@docs/src/pages/docs/limitations.md` at line 36, In the limitations documentation paragraph, update the phrase “outside of pgroles” to “outside pgroles” while leaving the surrounding explanation unchanged.Source: Linters/SAST tools
d1f9936 to
71be823
Compare
6fb3f48 to
bd50540
Compare
Profiles can now declare the same ALTER ROLE ... SET config map as
roles, applied to every generated role with {schema} and {profile}
substituted in values (keys stay literal parameter names — a
placeholder key fails the existing parameter-name validation). A
per-schema search_path default can therefore be declared once:
profiles:
editor:
config:
search_path: "{schema}"
Generated roles flow through the existing RoleDefinition.config
pipeline, so parameter-name validation, the config.role membership
cross-check, list-GUC canonicalization, and diffing all apply
unchanged. pgroles generate --suggest-profiles keeps config-carrying
roles flat rather than clustering them into profiles, preserving the
expand(suggest(M)) == expand(M) round-trip contract.
Operator CRD ProfileSpec gains the same field; CRDs regenerated.
Live round-trip test covers placeholder substitution end to end.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TwqLzRSJR8WHpLvvVqkCg
Two finder agents reviewed the profile-config diff; fixes: - render-bundle's default-stripping matched `role_pattern` by leaf key name only, so a role/profile config parameter literally named role_pattern with the default-pattern value was silently stripped from rendered bundles (pre-existing since role config landed, but widened by profile config). Stripping is now exact-path (schemas[i].role_pattern only), with a regression test, and the render round-trip test now asserts config maps survive verbatim rather than only comparing role names. - The suggest fuzz corpus now generates config-carrying roles (~1 in 6) so the suggester's config-disqualification path is exercised across seeds, not just by one hand-written unit test. - RoleAndSchemaCleanup in the live round-trip tests drops schemas before roles — DROP ROLE fails via pg_shdepend while the role still holds grants on a surviving schema, so the old order would leak roles if a future test grants schema privileges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019TwqLzRSJR8WHpLvvVqkCg
71be823 to
6e79ce4
Compare
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 `@crates/pgroles-inspect/tests/diff_property_live.rs`:
- Around line 128-168: Update SeedCleanup::drop so a failed PgPool::connect is
handled without panicking: log the cleanup connection error and return from the
cleanup future instead of calling expect. Preserve SeedCleanup::run for
successful connections and ensure cleanup remains non-panicking during
unwinding.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c685c5aa-8e8d-44b4-a03f-ece67465ec47
📒 Files selected for processing (20)
CHANGELOG.mdcharts/pgroles-operator/crds/postgrespolicies.pgroles.io.yamlcrates/pgroles-cli/src/lib.rscrates/pgroles-cli/src/main.rscrates/pgroles-core/src/composition.rscrates/pgroles-core/src/manifest.rscrates/pgroles-core/src/model.rscrates/pgroles-core/src/suggest.rscrates/pgroles-core/tests/diff_property.rscrates/pgroles-core/tests/suggest_property.rscrates/pgroles-inspect/src/lib.rscrates/pgroles-inspect/src/privileges.rscrates/pgroles-inspect/tests/config_roundtrip.rscrates/pgroles-inspect/tests/diff_property_live.rscrates/pgroles-operator/src/crd.rscrates/pgroles-operator/src/reconciler.rsdocs/src/pages/docs/limitations.mddocs/src/pages/docs/manifest-reference.mddocs/src/pages/docs/profiles.mdk8s/crd.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- crates/pgroles-core/src/composition.rs
- k8s/crd.yaml
- charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml
- docs/src/pages/docs/limitations.md
- crates/pgroles-cli/src/main.rs
- crates/pgroles-core/tests/suggest_property.rs
- docs/src/pages/docs/manifest-reference.md
- crates/pgroles-core/src/model.rs
- docs/src/pages/docs/profiles.md
- crates/pgroles-core/src/suggest.rs
- crates/pgroles-inspect/tests/config_roundtrip.rs
- crates/pgroles-operator/src/crd.rs
- crates/pgroles-operator/src/reconciler.rs
- crates/pgroles-core/src/manifest.rs
- crates/pgroles-core/tests/diff_property.rs
- crates/pgroles-inspect/src/privileges.rs
- crates/pgroles-inspect/src/lib.rs
- CHANGELOG.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@crates/pgroles-inspect/tests/diff_property_live.rs`:
- Around line 128-168: Update SeedCleanup::drop so a failed PgPool::connect is
handled without panicking: log the cleanup connection error and return from the
cleanup future instead of calling expect. Preserve SeedCleanup::run for
successful connections and ensure cleanup remains non-panicking during
unwinding.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c685c5aa-8e8d-44b4-a03f-ece67465ec47
📒 Files selected for processing (20)
CHANGELOG.mdcharts/pgroles-operator/crds/postgrespolicies.pgroles.io.yamlcrates/pgroles-cli/src/lib.rscrates/pgroles-cli/src/main.rscrates/pgroles-core/src/composition.rscrates/pgroles-core/src/manifest.rscrates/pgroles-core/src/model.rscrates/pgroles-core/src/suggest.rscrates/pgroles-core/tests/diff_property.rscrates/pgroles-core/tests/suggest_property.rscrates/pgroles-inspect/src/lib.rscrates/pgroles-inspect/src/privileges.rscrates/pgroles-inspect/tests/config_roundtrip.rscrates/pgroles-inspect/tests/diff_property_live.rscrates/pgroles-operator/src/crd.rscrates/pgroles-operator/src/reconciler.rsdocs/src/pages/docs/limitations.mddocs/src/pages/docs/manifest-reference.mddocs/src/pages/docs/profiles.mdk8s/crd.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- crates/pgroles-core/src/composition.rs
- k8s/crd.yaml
- charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml
- docs/src/pages/docs/limitations.md
- crates/pgroles-cli/src/main.rs
- crates/pgroles-core/tests/suggest_property.rs
- docs/src/pages/docs/manifest-reference.md
- crates/pgroles-core/src/model.rs
- docs/src/pages/docs/profiles.md
- crates/pgroles-core/src/suggest.rs
- crates/pgroles-inspect/tests/config_roundtrip.rs
- crates/pgroles-operator/src/crd.rs
- crates/pgroles-operator/src/reconciler.rs
- crates/pgroles-core/src/manifest.rs
- crates/pgroles-core/tests/diff_property.rs
- crates/pgroles-inspect/src/privileges.rs
- crates/pgroles-inspect/src/lib.rs
- CHANGELOG.md
🛑 Comments failed to post (1)
crates/pgroles-inspect/tests/diff_property_live.rs (1)
128-168: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reconnect failure inside
Drop::droppanics during unwinding — risks aborting the process and losing the real assertion failure.
SeedCleanup::dropcalls.expect("failed to connect for cleanup")onPgPool::connect. Whenrun_seedpanics on a genuine property violation (the entire point of this suite),cleanupis dropped during unwinding; if that reconnect also fails (transient network blip, connection-limit exhaustion from the many pools already opened this run, etc.), the second panic inside a destructor while already unwinding aborts the whole test process, discarding the original diagnostic message that pinpoints the seed and mismatch.🛡️ Proposed fix: log instead of panicking on reconnect failure in the cleanup path
impl Drop for SeedCleanup { fn drop(&mut self) { let roles = self.roles.clone(); let schemas = self.schemas.clone(); with_runtime(async move { - let pool = PgPool::connect(&database_url()) - .await - .expect("failed to connect for cleanup"); - Self::run(&pool, &roles, &schemas).await; + match PgPool::connect(&database_url()).await { + Ok(pool) => Self::run(&pool, &roles, &schemas).await, + Err(error) => { + eprintln!("SeedCleanup: failed to connect for cleanup: {error}"); + } + } }); } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./// Drop-guard that removes every generated schema and role, even on panic. /// Runs in sync context (its own runtime + connection), like `RoleCleanup` in /// `config_roundtrip.rs`. Errors are ignored — objects may not exist. struct SeedCleanup { roles: Vec<String>, schemas: Vec<String>, } impl SeedCleanup { async fn run(pool: &PgPool, roles: &[String], schemas: &[String]) { for schema in schemas { let _ = pool .execute(format!("DROP SCHEMA IF EXISTS {} CASCADE;", quote_ident(schema)).as_str()) .await; } for role in roles { // DROP OWNED clears grants and default-privilege entries that // would otherwise block DROP ROLE. It fails when the role does // not exist — ignored like everything else here. let _ = pool .execute(format!("DROP OWNED BY {};", quote_ident(role)).as_str()) .await; let _ = pool .execute(format!("DROP ROLE IF EXISTS {};", quote_ident(role)).as_str()) .await; } } } impl Drop for SeedCleanup { fn drop(&mut self) { let roles = self.roles.clone(); let schemas = self.schemas.clone(); with_runtime(async move { match PgPool::connect(&database_url()).await { Ok(pool) => Self::run(&pool, &roles, &schemas).await, Err(error) => { eprintln!("SeedCleanup: failed to connect for cleanup: {error}"); } } }); } }🧰 Tools
🪛 ast-grep (0.44.1)
[error] 138-139: SQL query is built with a
format!macro that interpolates dynamic values directly into the query string. Passing this to a query/execute sink (e.g.sqlx::query,diesel::sql_query,conn.execute) allows SQL injection. Use parameterized queries with bind placeholders (``/?and `.bind(...)`, or `diesel`'s `.bind::<Type, _>(value)`) instead of string interpolation.
Context: pool
.execute(format!("DROP SCHEMA IF EXISTS {} CASCADE;", quote_ident(schema)).as_str())
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').(sql-injection-format-rust)
[error] 146-147: SQL query is built with a
format!macro that interpolates dynamic values directly into the query string. Passing this to a query/execute sink (e.g.sqlx::query,diesel::sql_query,conn.execute) allows SQL injection. Use parameterized queries with bind placeholders (``/?and `.bind(...)`, or `diesel`'s `.bind::<Type, _>(value)`) instead of string interpolation.
Context: pool
.execute(format!("DROP OWNED BY {};", quote_ident(role)).as_str())
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').(sql-injection-format-rust)
[error] 149-150: SQL query is built with a
format!macro that interpolates dynamic values directly into the query string. Passing this to a query/execute sink (e.g.sqlx::query,diesel::sql_query,conn.execute) allows SQL injection. Use parameterized queries with bind placeholders (``/?and `.bind(...)`, or `diesel`'s `.bind::<Type, _>(value)`) instead of string interpolation.
Context: pool
.execute(format!("DROP ROLE IF EXISTS {};", quote_ident(role)).as_str())
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').(sql-injection-format-rust)
🤖 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 `@crates/pgroles-inspect/tests/diff_property_live.rs` around lines 128 - 168, Update SeedCleanup::drop so a failed PgPool::connect is handled without panicking: log the cleanup connection error and return from the cleanup future instead of calling expect. Preserve SeedCleanup::run for successful connections and ensure cleanup remains non-panicking during unwinding.
2 participants
Fourth in the stacked draft series (#136 ← #137 ← #138 ← this); base is
claude/column-grant-detectionso this PR shows only its own diff. Merge the stack in order and this retargets as parents merge.What
Profiles can now declare the same role-level
configmap (ALTER ROLE ... SETdefaults) that roles gained in #134, applied to every generated role with{schema}and{profile}substituted in values only:generates
inventory-editorwithsearch_path: inventory. A per-schemasearch_pathdefault is declared once per profile instead of repeated on every generated role.Design
config.role→ membership cross-check apply to generated roles exactly as to hand-written ones (tested both ways). Keys are never substituted; a{schema}key fails the existing parameter-name validation.pgroles generate --suggest-profilesnever clusters a config-carrying role into a profile (same disqualification mechanism aspassword/superuser), because collapsing config maps modulo placeholder substitution risks silently changing what gets applied. The suggest fuzz corpus (~1 in 6 generated roles now carries config) keeps this path exercised across seeds, not just in one hand-written test.ProfileSpecgains the field; both committed CRD copies regenerated; composition/render-bundlereuse the coreProfiletype so config flows through bundles automatically.Adversarial review findings (fixed in
d1f9936)render-bundle's default-stripping matchedrole_patternby leaf key name anywhere in the manifest tree, so a role/profile config parameter literally namedrole_patternwith the default-pattern value was silently stripped from rendered bundles. Stripping is now exact-path (schemas[i].role_patternonly), with a regression test; the render round-trip test now asserts config maps survive verbatim instead of only comparing role names.pg_shdepend(and silently leak) once a test grants schema privileges — order flipped.Testing
config.rolemembership validation on generated roles (rejects and accepts), suggest round-trip preserving config, render-bundle regression.--include-ignored): newprofile_config_placeholder_round_trips— manifest with schema-bound profile config → expand → diff (asserts substitutedALTER ROLE ... SET) → execute → re-inspect → empty plan.🤖 Generated with Claude Code
https://claude.ai/code/session_019TwqLzRSJR8WHpLvvVqkCg
Summary by CodeRabbit
{schema}and{profile}placeholder support.