Fix second batch of code-review findings: build, loader, deploy, pull, ddl#54
Merged
Conversation
apply_role_statement had no arm for Statement::Comment, so COMMENT ON ROLE emitted by pg_dumpall fell through to the unexpected-statement branch, logged a spurious warning, and was lost. Add a comment field to RoleState, populate it from the matched COMMENT ON ROLE statement, and wire it through to both the Role and User models on write (they already had a comment field, always written None). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The removed-object drop pass in deploy keyed archive TOC entries by their literal tag (entry_key), which pg_dump renders as parameter types only. ObjectKey's function identity (function_key_name) includes parameter names when present, so a removed function with a named parameter never matched an archive entry and fell into the unordered fallback loop instead of the dependency-ordered drop pass. Add diff::function_tag_name, a type-only signature in the same shape as the archive tag, and use it (via drop_match_key) only when matching removed objects against snapshot entries for ordering. function_key_name itself is untouched, so its other callers (identity/ACL matching) are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
missing-name aborts, and unsupported dependency types - add_definition now rejects a second object with the same (desc, schema, name) as an error naming both file paths, instead of silently keeping the duplicate and letting build emit it twice - cache_and_remove_dependencies computes a cast's identity tag the same way Definition::name() does, so a cast's dependencies: block resolves instead of being dropped with a "missing" warning - a definition missing name now increments the error count and skips just that entry instead of aborting the rest of the load via `?` - from_plural_key gains the plural keys for aggregates, collations, event_triggers, materialized_views, publications, servers, subscriptions, users, and user_mappings, so dependencies: blocks naming those types are recognized rather than failing the load Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
These three Table fields were declared but never rendered, so a hand-authored or pulled value for a typed table (CREATE TABLE OF type), a partitioned table's child partitions, or a PK/UNIQUE index tablespace silently vanished from the build archive, producing an unreconcilable deploy diff (deploy::alter::table already treats all three as rebuild-triggering, but the rebuilt DDL dropped them). - from_type renders CREATE TABLE ... OF type, with column entries reduced to their WITH OPTIONS constraints per the typed-table grammar (no explicit data type). - partitions renders each child as its own archive entry: CREATE TABLE child PARTITION OF parent FOR VALUES ... / DEFAULT, depending on the parent's dump id. - index_tablespace renders USING INDEX TABLESPACE on the PRIMARY KEY constraint (or every UNIQUE constraint when there is no primary key, since the model holds one table-wide value). Added inline unit tests for each of the three renders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- COMMENT ON forms with two names (TRIGGER/RULE/POLICY ... ON ..., CONSTRAINT ... ON [DOMAIN] ...) previously had the first name overwritten by the second; both are now preserved, using the same schema/name convention as COLUMN comments (schema holds the relation, name holds the trigger/rule/policy/constraint name). - Unmatched COMMENT targets (e.g. LARGE OBJECT) now log a warning instead of silently producing an empty target. - unquote() now folds unquoted identifiers to lowercase, matching PostgreSQL's identifier folding rules; quoted identifiers are unaffected. PUBLIC is special-cased to stay uppercase since it's a keyword denoting the pseudo-role rather than a folded identifier (mirrors utils::user_mapping_subject's existing handling). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # src/build/mod.rs
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR updates TABLE SQL rendering for typed tables and partitions, adjusts identifier unquoting and COMMENT ON parsing, changes deploy drop matching for functions, expands loader path/duplicate tracking, and persists role comments through pull and write paths. ChangesTable, DDL, deploy, load, and pull updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/pull/mod.rs (1)
282-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing doc comment on new
commentfield.Sibling fields like
createdandoptionscarry doc comments explaining their semantics;commenthas none. Minor consistency nit only.🤖 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/pull/mod.rs` at line 282, The new comment field in the pull request model is missing the same style of documentation as sibling fields like created and options. Add a doc comment directly on the comment field in the Pull struct so its meaning is clear and the field matches the surrounding API documentation style.src/ddl/object.rs (1)
318-329: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining two-name COMMENT forms.
The new state handles
RULEandCONSTRAINTtoo, includingCONSTRAINT ... ON DOMAIN ..., but tests only exerciseTRIGGERandPOLICY. Add targeted cases so thepending_first_name/in_targetbehavior stays locked down.Suggested test coverage
fn parses_policy_comment_preserves_both_names() { let Statement::Comment { on, target, .. } = parse_one("COMMENT ON POLICY pol ON tbl IS 'x';") else { panic!("expected Comment") @@ assert_eq!(target.schema.as_deref(), Some("tbl")); assert_eq!(target.name, "pol"); } + + #[test] + fn parses_rule_comment_preserves_both_names() { + let Statement::Comment { on, target, .. } = + parse_one("COMMENT ON RULE rul ON tbl IS 'x';") + else { + panic!("expected Comment") + }; + assert_eq!(on, "RULE"); + assert_eq!(target.schema.as_deref(), Some("tbl")); + assert_eq!(target.name, "rul"); + } + + #[test] + fn parses_constraint_comment_preserves_both_names() { + let Statement::Comment { on, target, .. } = + parse_one("COMMENT ON CONSTRAINT chk ON tbl IS 'x';") + else { + panic!("expected Comment") + }; + assert_eq!(on, "CONSTRAINT"); + assert_eq!(target.schema.as_deref(), Some("tbl")); + assert_eq!(target.name, "chk"); + } + + #[test] + fn parses_domain_constraint_comment() { + let Statement::Comment { on, target, .. } = + parse_one("COMMENT ON CONSTRAINT chk ON DOMAIN dom IS 'x';") + else { + panic!("expected Comment") + }; + assert_eq!(on, "CONSTRAINT"); + assert_eq!(target.schema.as_deref(), Some("dom")); + assert_eq!(target.name, "chk"); + }Also applies to: 545-571
🤖 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/ddl/object.rs` around lines 318 - 329, Add targeted test coverage for the remaining two-name COMMENT ON forms in the object parsing logic so the new pending_first_name/in_target behavior stays fixed for RULE and CONSTRAINT, including the CONSTRAINT ... ON DOMAIN ... case. Extend the existing tests around the object-name parsing path in src/ddl/object.rs to assert that the leading name is preserved and the following node correctly qualifies it for these forms, not just TRIGGER and POLICY.src/project/load.rs (2)
27-29: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffParallel
pathsvector can silently desync fromproject.inventory.
pathsis only kept aligned withinventoryinsideadd_definition(Lines 392-398). Anything that growsproject.inventoryoutside that path breaks the 1:1 index correspondence with no runtime check to catch it. The new testcast_dependency_edge_resolves(Lines 564-575) does exactly this - it pushes anItemstraight intoloader.project.inventory, thenadd_definition(Line 587) appends to both vectors, leavingpathsone index behindinventoryfor the rest of the test. It happens to be harmless here because the lookup at Lines 375-380 falls back to"<unknown>"for bothNoneand out-of-range indices, but the underlying invariant is fragile and could misattribute a duplicate-diagnostic to the wrong file if this pattern is reused elsewhere.Consider storing the path directly on
Item(or onDefinition) instead of a side-channel vector, or at minimum add adebug_assert_eq!(self.paths.len(), self.project.inventory.len())after every mutation site to fail fast on desync.🤖 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/project/load.rs` around lines 27 - 29, The `Loader`’s parallel `paths` vector can drift out of sync with `project.inventory`, especially when items are pushed directly in tests or other code paths outside `add_definition`. Fix this by removing the side-channel state and storing the source path on `Item` or `Definition`, or at minimum enforce the invariant in `Loader::add_definition` and any other inventory mutation sites with a `debug_assert_eq!` against `paths.len()` and `project.inventory.len()`. Use the existing `Loader`, `project.inventory`, and `paths` symbols to locate all mutation points and keep duplicate-diagnostic lookups reliable.
346-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
pathin the other two error branches ofadd_definitionfor consistent diagnostics.
pathis now available and used for the duplicate-object diagnostic, but theto_definitionfailure and round-trip-mismatch failure logs still omit it, making it harder to locate the offending file for those errors.♻️ Suggested fix
let definition = match to_definition(ot, value.clone()) { Ok(definition) => definition, Err(error) => { log::error!( - "Failed to load {} definition: {error}", - ot.as_str() + "Failed to load {} definition from {}: {error}", + ot.as_str(), + path.map(|p| p.display().to_string()) + .unwrap_or_else(|| String::from("<unknown>")), ); self.errors += 1; return; } }; let round_trip = serde_json::to_value(&definition) .expect("model serialization cannot fail"); let normalized = strip_nulls(value); if round_trip != normalized { log::error!( - "{} {} did not round-trip: {normalized} != {round_trip}", + "{} {} ({}) did not round-trip: {normalized} != {round_trip}", ot.as_str(), definition.name(), + path.map(|p| p.display().to_string()) + .unwrap_or_else(|| String::from("<unknown>")), ); self.errors += 1; return; }🤖 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/project/load.rs` around lines 346 - 368, The error diagnostics in add_definition are inconsistent because the to_definition failure and round-trip mismatch logs do not include path, even though it is available in this scope. Update both log::error! branches in add_definition to include path alongside the existing object type and definition details, matching the duplicate-object diagnostic style so the offending file can be identified easily.
🤖 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 `@src/build/mod.rs`:
- Around line 2260-2285: render_partition_for_values currently allows empty or
ambiguous bounds, which can emit invalid DDL. Update this function to validate
the TablePartition bound fields explicitly: return an error/panic or otherwise
fail fast when none of for_values_in, for_values_from/to, or for_values_with is
set, and also when more than one bound style is provided. Keep the existing
formatting logic for the valid cases, but make the ambiguity checks happen
before constructing the clause so callers cannot get a silently wrong result.
- Around line 2239-2242: The typed-table column CHECK rendering in the column
constraint assembly path should wrap the expression in parentheses so PostgreSQL
gets a valid CHECK clause. Update the logic in the constraint-building code that
handles column.check_constraint so the rendered output becomes CHECK (expr)
instead of CHECK expr, matching the table-level constraint formatting used
elsewhere in the build module.
- Around line 1251-1252: The partition emission loop in dump_table is allowing
child partitions to be generated even when the parent table is not partitioned.
Update the logic around the dump_partition call so it only runs for generated
parents that have an actual partitioning definition, checking the same parent
partition state used when emitting PARTITION BY, and skip or guard d.partitions
unless the parent is partitioned.
In `@src/constants.rs`:
- Around line 139-163: The dependency key used for foreign data wrappers is
inconsistent with the rest of the codebase, so `src/project/load.rs` will not
recognize it when reading `project.yaml`. Update the `Category` mapping in
`src/constants.rs` to use `foreign_data_wrappers` for
`Self::ForeignDataWrapper`, matching the underscore-separated naming used
elsewhere so FDW dependencies are parsed instead of skipped.
In `@src/ddl/mod.rs`:
- Around line 384-388: `unquote` in `src/ddl/mod.rs` is incorrectly
canonicalizing the identifier `public` to the pseudo-role literal `PUBLIC`,
which breaks callers that use it for object names like schema/table parsing.
Remove the `PUBLIC` special-case from `unquote` so it remains context-free and
preserves PostgreSQL-style folded identifiers, and move the pseudo-role
normalization into the role/ACL-specific caller(s) that need it, such as the
code paths around `utils::user_mapping_subject` and the other `PUBLIC` handling
referenced in the same module.
---
Nitpick comments:
In `@src/ddl/object.rs`:
- Around line 318-329: Add targeted test coverage for the remaining two-name
COMMENT ON forms in the object parsing logic so the new
pending_first_name/in_target behavior stays fixed for RULE and CONSTRAINT,
including the CONSTRAINT ... ON DOMAIN ... case. Extend the existing tests
around the object-name parsing path in src/ddl/object.rs to assert that the
leading name is preserved and the following node correctly qualifies it for
these forms, not just TRIGGER and POLICY.
In `@src/project/load.rs`:
- Around line 27-29: The `Loader`’s parallel `paths` vector can drift out of
sync with `project.inventory`, especially when items are pushed directly in
tests or other code paths outside `add_definition`. Fix this by removing the
side-channel state and storing the source path on `Item` or `Definition`, or at
minimum enforce the invariant in `Loader::add_definition` and any other
inventory mutation sites with a `debug_assert_eq!` against `paths.len()` and
`project.inventory.len()`. Use the existing `Loader`, `project.inventory`, and
`paths` symbols to locate all mutation points and keep duplicate-diagnostic
lookups reliable.
- Around line 346-368: The error diagnostics in add_definition are inconsistent
because the to_definition failure and round-trip mismatch logs do not include
path, even though it is available in this scope. Update both log::error!
branches in add_definition to include path alongside the existing object type
and definition details, matching the duplicate-object diagnostic style so the
offending file can be identified easily.
In `@src/pull/mod.rs`:
- Line 282: The new comment field in the pull request model is missing the same
style of documentation as sibling fields like created and options. Add a doc
comment directly on the comment field in the Pull struct so its meaning is clear
and the field matches the surrounding API documentation style.
🪄 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: 654d5294-8d43-49c2-93c6-aecfa6de3599
📒 Files selected for processing (9)
src/build/mod.rssrc/constants.rssrc/ddl/mod.rssrc/ddl/object.rssrc/deploy/diff.rssrc/deploy/mod.rssrc/project/load.rssrc/pull/mod.rssrc/pull/writer.rs
…HECK Three verified fixes from CodeRabbit's review of PR #54: - unquote() no longer special-cases PUBLIC, which was corrupting every unquoted "public" identifier (e.g. the public schema in "public.foo") to the pseudo-role literal "PUBLIC". Pseudo-role canonicalization now lives in a role-scoped unquote_role() applied only at grantee (ACL) and USER MAPPING subject parse sites, where PUBLIC is a keyword rather than a folded identifier. - constants::from_plural_key now maps "foreign_data_wrappers" (the underscore form used everywhere else, including schemata and the writer) instead of "foreign data wrappers", so FDW dependencies in project.yaml are recognized rather than logged as unknown and skipped. - Typed-table column CHECK constraints render as CHECK (expr) to match table-level constraint rendering and produce valid SQL, instead of the bare CHECK expr that PostgreSQL rejects. Skipped two CodeRabbit findings as invalid/redundant: - Partition-bound validation in render_partition_for_values is already enforced by schemata/table.yml's oneOf on the partition bound styles, which the loader validates before build. - A guard requiring a parent partition key before emitting child partitions would add renderer-side validation of malformed input (the renderer trusts schema-validated input) and would break the existing renders_list_partition / renders_range_partition / renders_default_partition unit tests, which intentionally render child partitions without a parent partition key. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
🤖 This comment was posted by Claude on behalf of @gmr Addressed CodeRabbit's review (pushed as c0a51e2). Fixed (3):
Skipped (2), with reasoning on the threads:
|
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
Second batch of fixes from the 2026-07-06 code review, following #53.
Covers the correctness follow-ups deferred from the first PR: build-side
rendering of newly-captured table structure, loader safety, deploy drop
ordering, role comments on pull, and DDL COMMENT/identifier handling.
Work was split across isolated worktrees by disjoint file ownership,
integrated, and validated (
just check+just gates).Problem
builddeclared but never renderedfrom_type(typedtables),
partitions(child partitions), orindex_tablespace, sohand-authored or freshly-pulled values silently vanished from the
archive and then produced unreconcilable deploy diffs.
(emitting them twice), dropped cast
dependencies:edges, aborted thewhole load on a single missing
name, and rejected several objecttypes as dependency targets.
topologically-ordered drop pass (archive TOC tag omits parameter
names) and fell to an unordered append.
COMMENT ON ROLEfrompg_dumpallwas dropped on pull witha spurious "unexpected statement" warning.
COMMENT ON TRIGGER trg ON tbllost the trigger name;unhandled COMMENT targets were silently empty; unquoted identifiers
were not case-folded to lowercase as PostgreSQL does.
Solution
src/build/mod.rs): render typed tables(
CREATE TABLE ... OF type, columns viaWITH OPTIONS), childpartitions (
PARTITION OF ... FOR VALUES .../DEFAULT), andUSING INDEX TABLESPACEon PK/unique constraints.src/project/load.rs,src/constants.rs): error onduplicate (desc, schema, name) naming both files; compute cast
dependency tags as
(src AS tgt); skip a nameless entry instead ofaborting; map the previously-missing plural dependency keys.
src/deploy/mod.rs,diff.rs): match removed functionsin the ordered drop pass via a type-only tag key;
function_key_nameand its callers are untouched.
src/pull/mod.rs,writer.rs): captureCOMMENT ON ROLEinto role/user YAML.
src/ddl/object.rs,mod.rs): preserve both names intwo-name COMMENT forms, warn on unhandled targets, and case-fold
unquoted identifiers (quoted kept verbatim;
PUBLICpreserved).Each fix ships with unit tests.
Verification
just check— fmt, clippy-D warnings, full test suite: green.just gates— round-trip diff empty; deploy equivalence + convergenceempty; safety gate excludes drops without
--allow-drop.Notes / remaining follow-ups
schemata/dependencies.ymlhasadditionalProperties: falseand whitelists only 9 dependency keys,so the newly-mapped types can't be exercised through real YAML until
that schema is widened (a separate change).
(
--log-filedisables progress bars), L9 (overload filenamecounter), efficiency E1–E4, and duplication D1–D3.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Summary by CodeRabbit
New Features
MINVALUE/MAXVALUE).COMMENT ON ROLEcontent from dumps.Bug Fixes
COMMENT ON ...for triggers, rules, policies, and constraints, including edge cases.PUBLICrole normalization, and improved deploy matching/order for removed functions.Chores