feat: manage schema ownership - #90
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds managed-schema support across manifest expansion, model, inspection, diffing, SQL generation, CLI/operator summaries, export, CI/e2e, and tests so declared schemas can be created and their ownership converged. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant Manifest as ExpandedManifest
participant Diff as Diff Engine
participant SQL as SQL Generator
participant DB as PostgreSQL
User->>CLI: pgroles apply (manifest with schemas)
CLI->>Manifest: expand_manifest()
Manifest->>Manifest: build ExpandedSchema entries (name, owner)
Manifest-->>CLI: ExpandedManifest with schemas
CLI->>Diff: diff(desired, current)
Diff->>Diff: diff_schemas() -> CreateSchema / AlterSchemaOwner
Diff-->>CLI: change plan including schema changes
CLI->>SQL: render_statements_with_context(changes)
SQL->>SQL: emit CREATE SCHEMA / ALTER SCHEMA OWNER statements
SQL-->>CLI: SQL statements
CLI->>DB: execute statements
DB-->>CLI: success
CLI-->>User: report N change(s) (includes schema counters)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c5e367419
ℹ️ 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".
Comment on lines
+144
to
+146
| for ((object_type, schema_name), object_names) in | ||
| fetch_relation_inventory(pool, managed_schemas).await? | ||
| { |
Contributor
There was a problem hiding this comment.
Seeding inventory only from fetch_relation_inventory means wildcard normalization can miss existing objects for wildcard grants on functions/types (and sequences when ACLs are NULL), because this commit also switched privilege queries to aclexplode(...) without acldefault(...), which returns no rows for NULL ACLs. In that case normalize_wildcard_grants treats the schema as having no objects and inserts a vacuous * grant, so diff can incorrectly report no change and skip required GRANT statements on real objects.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/pgroles-cli/tests/cli.rs (1)
828-892: Harden teardown to run even when assertions fail.Both tests can leave roles/schemas behind on early failure, which may poison later live-db runs. Consider wrapping setup/teardown in a small RAII cleanup guard.
Also applies to: 894-981
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pgroles-cli/tests/cli.rs` around lines 828 - 892, Wrap the setup/teardown in an RAII cleanup guard so teardown always runs even if assertions panic: create a small guard type (e.g., TestDbCleanup) with a Drop impl that runs the teardown SQL (the DROP SCHEMA IF EXISTS "{schema}" CASCADE; DROP ROLE IF EXISTS "{owner_role}";) and instantiate it early in the test (after creating schema and owner names but before running pgroles_cmd) in apply_creates_declared_schema_with_owner and the other test referenced (lines 894-981); use existing helpers unique_name, execute_sql and write_temp_manifest to build the tearDown SQL inside the guard and ensure the guard is dropped at test end to perform cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pgroles-core/src/diff.rs`:
- Around line 40-41: The AlterSchemaOwner variant currently isn't considered
destructive, so in ReconciliationMode::Additive owner transfers will still be
emitted; update the is_destructive() implementation to return true for the
AlterSchemaOwner { name, owner } variant (or include it in whatever
match/collection determines destructive changes) so that filter_changes(...,
Additive) will suppress schema-owner transfers. Locate the enum variant
AlterSchemaOwner in diff.rs and the is_destructive() (or equivalent) function
and add a match arm/case treating AlterSchemaOwner as destructive.
In `@crates/pgroles-core/src/manifest.rs`:
- Around line 477-487: The expansion currently builds a Vec<ExpandedSchema> from
manifest.schemas without checking for duplicate schema names, which allows later
silent overrides in RoleGraph.schemas; modify the expansion logic that
constructs ExpandedSchema (the block mapping manifest.schemas to ExpandedSchema)
to detect duplicate schema_binding.name values (e.g., use a HashSet or attempt
to insert into a HashMap keyed by name) and return an early error (or propagate
a Result::Err) when a duplicate name is encountered, including the duplicate
schema name in the error message so the caller can fail fast instead of allowing
silent overrides.
In `@crates/pgroles-inspect/src/lib.rs`:
- Around line 227-236: The early return in inspect_all prevents the schema
population block from running for databases that only have schemas; update
inspect_all so schema discovery always runs before returning (or change the
early-return condition to require both no roles and no schemas). Specifically,
ensure fetch_schemas(...) and the loop that inserts into graph.schemas (using
schema_rows, schema_name, owner_name) execute prior to any return in
inspect_all, or modify the guard that causes the early return to also check
graph.schemas (or schema_refs) so databases with schemas are not dropped.
In `@crates/pgroles-inspect/src/privileges.rs`:
- Around line 144-151: The inventory preload only covers relations so
normalize_wildcard_grants() will mis-detect vacuous “*” for sequences,
functions, and types when ACLs are NULL; update the preload loop to include
inventory from those object kinds as well (e.g., call or extend
fetch_relation_inventory to also fetch sequences, functions, and types such as
via fetch_sequence_inventory, fetch_function_inventory, fetch_type_inventory or
a unified fetch_object_inventory) and ensure those helper(s) include objects
even when ACLs are NULL so inventory.insert(...) receives entries for
(object_type, schema_name) for sequences/functions/types; keep using the same
inventory map and keys so normalize_wildcard_grants() can correctly determine
whether “*” is vacuous.
---
Nitpick comments:
In `@crates/pgroles-cli/tests/cli.rs`:
- Around line 828-892: Wrap the setup/teardown in an RAII cleanup guard so
teardown always runs even if assertions panic: create a small guard type (e.g.,
TestDbCleanup) with a Drop impl that runs the teardown SQL (the DROP SCHEMA IF
EXISTS "{schema}" CASCADE; DROP ROLE IF EXISTS "{owner_role}";) and instantiate
it early in the test (after creating schema and owner names but before running
pgroles_cmd) in apply_creates_declared_schema_with_owner and the other test
referenced (lines 894-981); use existing helpers unique_name, execute_sql and
write_temp_manifest to build the tearDown SQL inside the guard and ensure the
guard is dropped at test end to perform cleanup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f107c88-fd9b-4730-99a5-4add154b0af9
📒 Files selected for processing (11)
crates/pgroles-cli/src/lib.rscrates/pgroles-cli/tests/cli.rscrates/pgroles-core/src/diff.rscrates/pgroles-core/src/export.rscrates/pgroles-core/src/manifest.rscrates/pgroles-core/src/model.rscrates/pgroles-core/src/sql.rscrates/pgroles-inspect/src/cloud.rscrates/pgroles-inspect/src/lib.rscrates/pgroles-inspect/src/privileges.rscrates/pgroles-operator/src/reconciler.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/pgroles-operator/src/crd.rs (1)
746-751:⚠️ Potential issue | 🟠 MajorAdd
#[serde(default)]to ensureChangeSummarydeserializes safely when reading pre-existing status objects.The new required
i32fields (schemas_createdandschema_owners_altered) will cause deserialization to fail on older status objects that lack these fields. Add#[serde(default)]to the struct so missing fields default to zero during upgrades.Suggested fix
/// Summary of changes applied during reconciliation. #[derive(Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] +#[serde(default)] pub struct ChangeSummary {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/pgroles-operator/src/crd.rs` around lines 746 - 751, The ChangeSummary struct will fail to deserialize older status objects because new i32 fields (schemas_created, schema_owners_altered) are required; add #[serde(default)] to the ChangeSummary struct definition (the struct annotated with Debug, Clone, Default, Serialize, Deserialize, JsonSchema) so missing fields default to zero on deserialize, ensuring compatibility with pre-existing status objects and leveraging the existing Default implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/pgroles-core/src/diff.rs`:
- Line 238: diff_schemas currently collects schema creations into the local
schema_changes vector (e.g., CreateSchema) and emits them after role alters,
breaking the repo ordering; refactor diff_schemas to produce two distinct
outputs (creates and alters) instead of a single schema_changes: ensure
CreateSchema variants are appended to the creates bucket (the same place other
create objects are emitted) and owner-alter/alter variants are appended to the
alters bucket so owner transfers remain with alters; update callers that
previously consumed schema_changes to merge the two outputs into the overall
diff assembly according to the required ordering (creates → alters/comments →
grants → ...) and adjust any helper logic that referenced schema_changes
(including the code around the existing schema_changes declaration and usages
within diff_schemas, and related spots noted near the other occurrences) so
schema creation and owner alters are emitted in their correct buckets.
In `@crates/pgroles-operator/src/crd.rs`:
- Around line 750-751: The CRD OpenAPI schema was changed by adding the fields
schemas_created and schema_owners_altered to the ChangeSummary type in crd.rs;
regenerate the CRD manifests by running the generator (cargo run --bin crdgen >
k8s/crd.yaml), then copy the generated output into the chart CRD manifest
(replace charts/pgroles-operator/crds/postgrespolicies.pgroles.io.yaml with the
new k8s/crd.yaml content) so both shipped manifests reflect the updated
ChangeSummary schema and include the new fields.
---
Outside diff comments:
In `@crates/pgroles-operator/src/crd.rs`:
- Around line 746-751: The ChangeSummary struct will fail to deserialize older
status objects because new i32 fields (schemas_created, schema_owners_altered)
are required; add #[serde(default)] to the ChangeSummary struct definition (the
struct annotated with Debug, Clone, Default, Serialize, Deserialize, JsonSchema)
so missing fields default to zero on deserialize, ensuring compatibility with
pre-existing status objects and leveraging the existing Default implementation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec734b7f-ea67-4b04-9109-ce0ed86cf391
📒 Files selected for processing (12)
README.mdcrates/pgroles-cli/tests/cli.rscrates/pgroles-core/src/diff.rscrates/pgroles-core/src/manifest.rscrates/pgroles-inspect/src/lib.rscrates/pgroles-inspect/src/privileges.rscrates/pgroles-operator/src/crd.rscrates/pgroles-operator/src/reconciler.rsdocs/src/pages/docs/adoption.mddocs/src/pages/docs/manifest-format.mddocs/src/pages/docs/operator.mddocs/src/pages/docs/profiles.md
✅ Files skipped from review due to trivial changes (3)
- docs/src/pages/docs/profiles.md
- docs/src/pages/docs/operator.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/pgroles-cli/tests/cli.rs
- crates/pgroles-inspect/src/lib.rs
- crates/pgroles-core/src/manifest.rs
This was referenced Jul 14, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
schemas[].ownerTesting
Summary by CodeRabbit
New Features
Tests
Documentation