Reconcile foreign objects in deploy#50
Merged
Merged
Conversation
deploy could create and drop foreign data wrappers, servers, user
mappings, and foreign tables, but never reconciled changes to them:
the three schemaless types were existence-only (left untouched when
they differed) and foreign-table option changes forced a drop+recreate.
- Make foreign data wrappers, servers, and user mappings diffable
(diff.rs `modeled` + `database_index`), so a change is detected and
resolved instead of being reported as undiffable.
- Add in-place ALTER renderers (alter.rs):
- FDW: HANDLER/VALIDATOR (incl. NO HANDLER/VALIDATOR), OPTIONS, comment
- SERVER: VERSION, OPTIONS, comment; a wrapper/TYPE change rebuilds
- USER MAPPING: per-server OPTIONS, with CREATE/DROP for added/removed
servers
- FOREIGN TABLE: OPTIONS and comment in place; server/column changes
rebuild
- Share an `OPTIONS (ADD/SET/DROP ...)` delta renderer across the four.
- Never drop a user-mapping `password` the project does not carry, so a
redacted pull (the default) cannot strip a live credential.
- Fix DROP of database-only foreign objects: order them via entry_key
(so user-mapping/foreign-table drop before server before FDW) and
render the per-server `DROP USER MAPPING ... FOR x SERVER y` form
(the generic drop_sql emitted invalid SQL for user mappings).
Covered by alter.rs unit tests, two deploy integration tests over
synthesized foreign archives, and an extension of bin/deploy-gates that
drives the wrapper/server/foreign-table option drift end-to-end against
compose PG17. Documented in docs/commands.md, including the redacted-
password safety.
Co-authored-by: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds full deploy reconciliation support for PostgreSQL foreign-data-wrapper objects (FDW, foreign server, user mapping, foreign table). Changes span the diff struct ( ChangesFDW Deploy Reconciliation
Sequence Diagram(s)sequenceDiagram
participant CLI as pglifecycle deploy
participant Diff as diff::diff()
participant Alter as alter::resolve()
participant Mod as mod::drop_sql()
CLI->>Diff: compute diff(repo, database_snapshot)
Diff->>Diff: database_index() — index FDW/server/user_mapping
Diff->>Diff: modeled() — classify ForeignDataWrapper/Server/UserMapping
Diff-->>CLI: Diff { removed: BTreeMap ObjectKey Definition, changed: [...] }
loop each changed FDW object
CLI->>Alter: resolve(repo_def, db_def)
alt ForeignDataWrapper
Alter->>Alter: fdw(repo, db) — ALTER handler/validator/options/comment
else Server
Alter->>Alter: server(repo, db) — ALTER version/options or replace
else UserMapping
Alter->>Alter: user_mapping(repo, db) — per-server CREATE/ALTER/DROP
else Table with server
Alter->>Alter: foreign_table(repo, db) — ALTER OPTIONS/comment or replace
end
Alter-->>CLI: Resolution::Alter(sql) or Resolution::Replace
end
loop each removed object
CLI->>Mod: drop_sql(key, Some(definition))
alt UserMapping definition
Mod-->>CLI: DROP USER MAPPING IF EXISTS ... FOR server per server
else other
Mod-->>CLI: DROP object_type IF EXISTS qualified_name
end
end
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 docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
src/deploy/mod.rs (1)
375-400: 💤 Low valueConsider documenting the invariant that UserMapping keys always have definitions.
If a
UserMappingkey ever reaches this function withdefinition: None, the fallback path generates invalid SQL (DROP USER MAPPING IF EXISTS name;without the requiredFOR user SERVER serverclause). The current code flow guarantees definitions are present, but the invariant is implicit.A defensive assertion or comment would prevent future refactoring from accidentally violating this:
♻️ Suggested defensive check
fn drop_sql(key: &ObjectKey, definition: Option<&Definition>) -> String { if let Some(Definition::UserMapping(mapping)) = definition { return mapping .servers .iter() .map(|server| { format!( "DROP USER MAPPING IF EXISTS FOR {} SERVER {};\n", quote_ident(&mapping.name), quote_ident(&server.name), ) }) .collect(); } + // user mappings require the definition to render per-server drops; + // the diff guarantees removed keys have definitions, so this is + // unreachable for UserMapping—assert to catch future refactoring + debug_assert!( + key.desc != constants::ObjectType::UserMapping, + "UserMapping drop requires a definition" + ); // function keys are identity signatures and must not be quoted🤖 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 `@src/deploy/mod.rs` around lines 375 - 400, The drop_sql function has an implicit invariant that UserMapping keys must always have a Definition::UserMapping provided, but if a UserMapping key reaches the fallback path with definition: None, it would generate invalid SQL missing the required FOR user SERVER server clause. Add a defensive assertion or comment at the beginning of the function to explicitly document and enforce this invariant: when key.desc indicates a UserMapping object type, the definition parameter must always be Some(Definition::UserMapping), ensuring future refactoring doesn't accidentally violate this contract.
🤖 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/commands.md`:
- Around line 89-90: The Foreign servers bullet point in the documentation
currently describes wrapper and TYPE changes, but does not document the fallback
behavior when an existing server VERSION is cleared or removed. Add
documentation to the Foreign servers section explaining that removing an
existing VERSION also triggers a rebuild, so the documented behavior matches the
actual implementation behavior and sets proper operator expectations.
In `@src/deploy/alter.rs`:
- Around line 770-806: PostgreSQL requires PUBLIC to be used as a keyword (not
quoted) in CREATE/ALTER/DROP USER MAPPING statements, but the current
implementation unconditionally quotes all identifiers using quote_ident().
Create a new helper function that returns PUBLIC unquoted and all other names
quoted, then replace the quote_ident(&repo.name) call in the user_mapping
function in src/deploy/alter.rs:771 with this helper function, and also apply
the same helper function at src/deploy/mod.rs:383 where the user mapping name is
being quoted.
---
Nitpick comments:
In `@src/deploy/mod.rs`:
- Around line 375-400: The drop_sql function has an implicit invariant that
UserMapping keys must always have a Definition::UserMapping provided, but if a
UserMapping key reaches the fallback path with definition: None, it would
generate invalid SQL missing the required FOR user SERVER server clause. Add a
defensive assertion or comment at the beginning of the function to explicitly
document and enforce this invariant: when key.desc indicates a UserMapping
object type, the definition parameter must always be
Some(Definition::UserMapping), ensuring future refactoring doesn't accidentally
violate this contract.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fb9994e-7048-41d1-85be-84028dbdf7ac
📒 Files selected for processing (7)
bin/deploy-gatesdocs/commands.mdsrc/deploy/alter.rssrc/deploy/diff.rssrc/deploy/mod.rstests/common/mod.rstests/deploy.rs
- add utils::user_mapping_subject, leaving PUBLIC as a keyword and quoting every other user name - apply it to the CREATE/DROP USER MAPPING statements in build so a public mapping no longer emits invalid FOR "PUBLIC" SQL - document the foreign-server VERSION-clearing rebuild fallback in docs/commands.md to match deploy behavior Addresses CodeRabbit review on PR #50. Co-authored-by: Claude <noreply@anthropic.com>
The deploy ALTER path (deploy::alter::user_mapping) also quoted the mapping name unconditionally, so a public mapping emitted invalid CREATE/ALTER/DROP USER MAPPING FOR "PUBLIC" SQL. Apply utils::user_mapping_subject there too and cover it with a test. Addresses CodeRabbit review on PR #50. Co-authored-by: Claude <noreply@anthropic.com>
Owner
Author
|
🤖 This comment was posted by Claude on behalf of @gmr Addressed both CodeRabbit review comments:
Quality gates pass locally: |
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
Close the deploy-side gap for foreign objects:
deploynow reconciles changes to foreign data wrappers, servers, user mappings, and foreign tables, not just creates and drops them.Problem
deploycould create foreign objects when missing and drop them when project-absent, but it never reconciled changes:modeled()set, so a change to (say) a server's options or a wrapper's handler was classifiedUndiffableand left untouched.drop_sqlemittedDROP USER MAPPING IF EXISTS <user>, which is invalid SQL — user mappings drop per server (FOR <user> SERVER <srv>) — and database-only foreign objects were dropped unordered.Solution
diff.rs): add FDW, server, and user mapping tomodeled()anddatabase_index(), and retain removed objects' definitions so drops can render correctly.alter.rs):HANDLER/VALIDATOR(incl.NO HANDLER/NO VALIDATOR),OPTIONS, comment.VERSION,OPTIONS, comment; a wrapper orTYPEchange (neither alterable) rebuilds.OPTIONS, withCREATE/DROPfor servers the project adds or removes.OPTIONSand comment in place; a server or column change rebuilds.OPTIONS (ADD/SET/DROP …)delta renderer backs all four.passwordoption the project does not carry is left untouched, so a redacted pull (the default) never strips a live credential.entry_keynow orders removed foreign objects (user-mapping/foreign-table → server → FDW), and user mappings render the per-server drop form.Impact
deployplans are now complete for foreign objects. No change to objects that already matched (verified by an empty-plan test). Drops remain gated behind--allow-dropas before.Testing
cargo fmt,cargo clippy --all-targets -- -D warnings,cargo test(140 lib + integration tests, +8 new). New coverage: alter.rs unit tests for each renderer and the redacted-password case; twotests/deploy.rsintegration tests over synthesized foreign archives (in-place ALTERs + gated/allowed server drop, and a matching-foreign empty plan).bin/deploy-gatesis extended to drive wrapper/server/foreign-table option drift end-to-end against compose PG17 (run locally withjust deploy-gates).Summary by CodeRabbit
--allow-dropwhere required.USER MAPPING ... FORsubjects, treatingPUBLICas a special-case.deploychange-reconciliation rules for the additional foreign-object categories and password redaction behavior.