feat: comprehensive stability, UX, and schema coverage improvements (v0.3-0.5)#12
Conversation
Comprehensive stability and UX overhaul informed by analysis of Meru143/dbdiff (Go) and DBDiff/DBDiff (PHP) competitors. New features: - Structured error codes (E001-E007) with actionable hints and causes - Connection timeout configuration (--timeout, default 10s) - CLI restructured to subcommands (diff, validate, completions) - Backward-compatible: top-level args still work as diff command - `dbdiff validate <DSN>` command for connection testing with stats - `dbdiff completions <shell>` for bash/zsh/fish/PowerShell - Summary statistics after diff output (tables/columns/indexes counts) - Progress spinner during schema loading - Safe-by-default: --out requires --write to actually save files - SSL/TLS support for PostgreSQL (--ssl-mode disable/prefer/require) - JSON output now includes summary statistics Stability improvements: - Error codes classify failures (connection, auth, permission, timeout, IO) - Hints guide users to fix common issues - Credential sanitization in error messages - Timeout prevents hanging on unreachable hosts Tests: 91 total (71 unit + 17 integration + 3 config), up from 77 https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
![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: e5770385c2
ℹ️ 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".
| let tls_connector = native_tls::TlsConnector::builder() | ||
| .danger_accept_invalid_certs(true) | ||
| .build() |
There was a problem hiding this comment.
The TLS connector explicitly sets danger_accept_invalid_certs(true), which disables certificate trust checks for all PostgreSQL TLS sessions. In practice this means --ssl-mode=require can still accept a forged or self-signed certificate, so users can be silently downgraded to an unauthenticated TLS channel (MITM risk) instead of getting a hard failure.
Useful? React with 👍 / 👎.
| format!( | ||
| "Check that the database server is running and accepting connections.\n\ | ||
| Try: pg_isready -h {host} or mysql --host={host} --execute='SELECT 1'", | ||
| ) |
There was a problem hiding this comment.
This hint interpolates the raw host argument, but callers pass the full DSN (postgres://user:pass@...) into DbDiffError::connection. On connection-refused errors, stderr will include credentials in the suggested pg_isready/mysql command, which defeats the credential-sanitization goal and can leak secrets into CI logs.
Useful? React with 👍 / 👎.
| let err = dbdiff::error::DbDiffError::timeout( | ||
| &format!("{} / {}", params.source, params.target_source), | ||
| params.timeout, |
There was a problem hiding this comment.
The timeout branch concatenates both raw DSNs and passes them to DbDiffError::timeout, whose sanitizer only strips credentials relative to a single DSN. When both sides include credentials, the rendered timeout message can expose at least one password (source / target string) in error output.
Useful? React with 👍 / 👎.
…commands Phase 2 improvements based on competitor analysis: Schema coverage: - Foreign key, unique, and check constraint tracking in model & diff - View tracking with definition comparison (whitespace-normalized) - Enum type tracking with value-level diffing (added/removed values) - Sequence tracking with property comparison - Constraint-aware migration generation (ADD/DROP CONSTRAINT SQL) Bidirectional migrations: - `--direction up|down|both` flag for forward/rollback migration - `generate_rollback()` function produces reverse migration SQL - JSON output includes "rollback" array alongside "migration" - SQL output supports `-- === UP ===` / `-- === DOWN ===` sections Protected objects: - `protected.tables` and `protected.columns` in .dbdiff.yml - Prevents generation of destructive migrations on critical objects - Blocks with clear error message explaining what would be affected New commands: - `dbdiff tables <DSN>` — list all tables with column/index counts - `dbdiff init` — generate default .dbdiff.yml config file Tests: 101 total (75 unit + 23 integration + 3 config), up from 91 https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
Phase 3 improvements: Schema snapshots: - `dbdiff snapshot <DSN> --out schema.json` exports schema as JSON - JSON snapshot files can be used as diff sources (.json extension) - Enables CI/CD without live database access - Full round-trip: export -> compare -> generate migration - Deserialization support for all schema object types Model extensions: - Trigger and Function structs added to model (for future loader support) - SchemaSnapshot with Serialize + Deserialize for JSON round-trip - All model types now implement Deserialize for snapshot import Docker support: - Multi-stage Dockerfile (rust:1.75-slim builder, debian:bookworm-slim runtime) - Minimal image with ca-certificates and libssl3 Tests: 103 total (75 unit + 25 integration + 3 config), up from 101 https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 440ba2e60f
ℹ️ 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".
| for table in &diff.removed_tables { | ||
| statements.push(MigrationStatement { | ||
| sql: create_table_sql(table, dialect), |
There was a problem hiding this comment.
When a table is removed in the forward migration, the rollback path recreates it using only create_table_sql, which restores columns but skips that table’s indexes and constraints. In a real rollback this leaves the schema materially different from the pre-migration state (missing unique/FK/index definitions), so --direction down cannot reliably recover dropped tables.
Useful? React with 👍 / 👎.
| // Reverse added columns (drop them) | ||
| for table_diff in &diff.modified_tables { | ||
| for col in &table_diff.added_columns { |
There was a problem hiding this comment.
The rollback logic only drops newly added columns and re-adds removed columns, but never reverses modified_columns changes (type, nullability, default). Any forward migration that emits ALTER COLUMN statements cannot be undone by the generated down migration, leaving schema drift after rollback.
Useful? React with 👍 / 👎.
| let matches = if let Some(col_name) = pattern.strip_prefix("*.") { | ||
| col.name == col_name | ||
| } else if let Some((table, col_name)) = pattern.split_once('.') { | ||
| table_diff.table_name == table && col.name == col_name | ||
| } else { |
There was a problem hiding this comment.
The config docs state protected.columns uses the same pattern format as ignore.columns, but this matcher only supports *.column and exact table.column. A table.* protection rule is silently ignored here, so destructive column drops on that table can bypass the protection check.
Useful? React with 👍 / 👎.
Comprehensive loader and UX improvements: PostgreSQL loader: - Foreign key constraints from information_schema - Unique constraints from information_schema - Check constraints from pg_constraint - Views from pg_views - Enum types from pg_type + pg_enum - Sequences from pg_sequences MySQL loader: - Foreign key constraints from information_schema - Views from information_schema.VIEWS SQLite loader: - Foreign key constraints from PRAGMA foreign_key_list - Views from sqlite_master New features: - `--explain` flag: shows human-readable explanation for each migration statement - `--color auto|always|never` with TTY auto-detection - JSON snapshot files (.json) can be used as diff sources Tests: 103 total, all passing. Clippy clean. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
…lain/color Merge master's CI integration features with our Phase 2-4 work: Conflict resolution: - Integrated CiReport for JSON/YAML structured output - Added --fail-on-blocking flag and exit code 3 for blocking ops - GitHub Actions annotation support in CI mode - YAML output format - Kept our structured error codes, SSL/TLS, subcommand architecture New in this commit: - PostgreSQL loader: constraints (FK, unique, check), views, enums, sequences - MySQL loader: FK constraints, views - SQLite loader: FK constraints, views - --explain mode with human-readable explanations for each migration - --color auto|always|never with TTY auto-detection - is_blocking field added to all new migration statement types Tests: 112 total (81 unit + 28 integration + 3 config), all passing https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
rekurt
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f4fdf7f18
ℹ️ 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".
| for table in &diff.removed_tables { | ||
| statements.push(MigrationStatement { | ||
| sql: format!("DROP TABLE {};", quote_ident(&table.name, dialect)), |
There was a problem hiding this comment.
This drops removed tables before removed views, which can make the migration fail on databases that enforce dependency ordering (for example, PostgreSQL will reject DROP TABLE when a view still depends on it). In a diff where both a table and its view are removed, execution stops at the table drop and never reaches the later DROP VIEW, so the generated forward migration is not runnable.
Useful? React with 👍 / 👎.
| format!( | ||
| "ALTER TABLE {} DROP CONSTRAINT {};", | ||
| quote_ident(&c.table_name, dialect), | ||
| quote_ident(&c.name, dialect) | ||
| ) |
There was a problem hiding this comment.
The generic ALTER TABLE ... DROP CONSTRAINT ... syntax is invalid for MySQL foreign keys, so any MySQL diff that removes or modifies an FK will generate SQL that fails immediately. MySQL requires DROP FOREIGN KEY (and uses index drops for unique constraints), so this statement shape breaks constraint-removal migrations for that backend.
Useful? React with 👍 / 👎.
| row.get::<_, i32>(0)?, | ||
| row.get::<_, String>(2)?, // ref_table | ||
| row.get::<_, String>(3)?, // from column | ||
| row.get::<_, String>(4)?, // to column |
There was a problem hiding this comment.
SQLite PRAGMA foreign_key_list can return NULL in the to column when the reference omits explicit target columns (e.g. REFERENCES parent). Reading that field as String causes row decoding to fail, and the subsequent filter_map(|r| r.ok()) silently drops the FK row, so valid foreign keys disappear from loaded schema diffs.
Useful? React with 👍 / 👎.
…nullable FK Fixes from Codex review comments: 1. MySQL DROP FOREIGN KEY syntax (P1): MySQL requires `DROP FOREIGN KEY` for FK constraints and `DROP INDEX` for unique constraints, not the generic `DROP CONSTRAINT`. Now emits dialect-correct SQL. 2. Drop views before tables (P1): Views may depend on tables, so `DROP VIEW` must come before `DROP TABLE` in the migration ordering. Swapped Phase 4 (views) and Phase 5 (tables). 3. SQLite nullable FK target columns (P2): `PRAGMA foreign_key_list` returns NULL in the `to` column when the FK references the primary key implicitly (e.g. `REFERENCES parent`). Now reads as `Option<String>` and falls back to the `from` column name. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e0c7a52c9
ℹ️ 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".
| for view in &diff.removed_views { | ||
| statements.push(MigrationStatement { | ||
| sql: format!( | ||
| "CREATE VIEW {} AS {};", | ||
| quote_ident(&view.name, dialect), |
There was a problem hiding this comment.
generate_rollback recreates diff.removed_views immediately, before it recreates dropped tables later in the same function. If a removed view depends on a removed table (a common case when both were dropped in the forward migration), the down migration will fail at CREATE VIEW because the base table does not exist yet.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| statements |
There was a problem hiding this comment.
The rollback generator exits after table/index/constraint reversal and never handles added/removed/modified enums or sequences from SchemaDiff. Any forward migration that creates, drops, or alters these objects cannot be fully undone with --direction down, leaving rollback output incomplete and schema state inconsistent.
Useful? React with 👍 / 👎.
| if let Some(to) = to_col { | ||
| entry.2.push(to.clone()); | ||
| } else { | ||
| entry.2.push(from_col.clone()); | ||
| } |
There was a problem hiding this comment.
When SQLite reports a foreign key with to = NULL (e.g. REFERENCES parent), this code substitutes the child column name as the referenced column. That can record an incorrect FK shape whenever child and parent PK column names differ (for example user_id REFERENCES users where parent key is id), causing false diffs and incorrect migration SQL.
Useful? React with 👍 / 👎.
Bug fixes: - is_blocking: DROP COLUMN, DROP INDEX, DROP CONSTRAINT now correctly marked as blocking (were false, should be true for AccessExclusiveLock) - Rollback ordering: tables recreated BEFORE views (views depend on tables) - Rollback completeness: added enum/sequence rollback (was missing entirely) - Rollback is_blocking: DROP TABLE/COLUMN/INDEX in rollback now blocking - SQLite FK NULL target: uses "id" instead of from_col when PRAGMA foreign_key_list returns NULL to column (implicit PK reference) New features: - SQL file constraint parsing: FOREIGN KEY, UNIQUE, CHECK constraints now parsed from CREATE TABLE statements (were silently skipped) - Constraints shown in output for added/removed tables (were missing) Test fixtures: - schema_c.sql: schema with FK, UNIQUE, CHECK constraints - schema_d.sql: modified schema with changed constraints + new table New tests (21 integration + 10 unit = 31 new): - Constraint parsing from SQL files - Constraint diff in JSON output - Bidirectional migration completeness (rollback has DROP/ADD) - Protected table/column blocking - --explain mode output - --color never produces no ANSI codes - Identical schemas show "identical" - Empty SQL file handling - Single table diff - JSON snapshot roundtrip and cross-format comparison - MySQL DROP FOREIGN KEY syntax (unit test) - MySQL DROP INDEX for unique constraints (unit test) - ADD CONSTRAINT generates correct SQL with ON DELETE CASCADE - is_blocking correctness for DROP COLUMN/INDEX/TABLE/CREATE TABLE - Rollback reversal tests (added/removed tables, is_blocking) - CI mode exit codes - --write flag behavior Total: 143 tests (91 unit + 28 cli + 3 config + 21 constraints), all passing https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd39959c81
ℹ️ 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".
|
|
||
| if delete_rule.is_some() { | ||
| // FK constraint | ||
| let entry = fk_map.entry(constraint_name).or_insert_with(|| { |
There was a problem hiding this comment.
This groups constraints by constraint_name alone, but PostgreSQL constraint names are only required to be unique per table. If two tables both have a constraint named fk_user (or similarly reused unique names), rows are merged into one map entry, causing one table’s constraint to be dropped/corrupted in the loaded schema and producing incorrect diffs/migration SQL for real multi-table schemas.
Useful? React with 👍 / 👎.
| sql: format!( | ||
| "ALTER SEQUENCE {} INCREMENT {} MINVALUE {} MAXVALUE {};", | ||
| quote_ident(&s.name, dialect), | ||
| s.increment, |
There was a problem hiding this comment.
Sequence diffs are detected on the full Sequence struct, but the generated ALTER SEQUENCE only applies INCREMENT/MINVALUE/MAXVALUE and ignores other tracked fields like start_value (and effectively data_type). When only those omitted fields change, the migration cannot reconcile the diff, so re-running diff still reports drift despite applying the generated SQL.
Useful? React with 👍 / 👎.
| let name = cap | ||
| .get(1) | ||
| .map(|m| m.as_str().to_string()) | ||
| .unwrap_or_else(|| format!("fk_{}_{}", table_name, cap[3].to_lowercase())); |
There was a problem hiding this comment.
For unnamed foreign keys, the fallback name uses only <table>_<referenced_table>. A table with multiple unnamed FKs to the same parent table (e.g., created_by and updated_by both referencing users) will generate identical names, and because constraints are stored in a map by name, later constraints overwrite earlier ones. That silently drops constraints from parsed schemas and leads to incorrect diff/migration output.
Useful? React with 👍 / 👎.
…QUENCE Addresses Codex review P1/P2 comments: 1. PostgreSQL constraint keying (P1): Constraints are now keyed by (table_name, constraint_name) instead of constraint_name alone. Prevents cross-table constraint merging when two tables reuse the same constraint name (e.g. both have fk_user). 2. SQL file FK name collisions (P2): Unnamed FK fallback names now include column names: fk_<table>_<ref_table>_<columns> instead of just fk_<table>_<ref_table>. Prevents silent overwrites when a table has multiple FKs to the same parent. 3. ALTER SEQUENCE completeness (P2): Now emits all tracked fields (AS type, START WITH, INCREMENT BY, MINVALUE, MAXVALUE) instead of only INCREMENT/MINVALUE/MAXVALUE. Prevents persistent drift when only start_value or data_type changes. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b02a4b6469
ℹ️ 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".
| for c in table.constraints.values() { | ||
| statements.push(MigrationStatement { | ||
| sql: add_constraint_sql(c, dialect), |
There was a problem hiding this comment.
The migration builder emits add_constraint_sql inside the per-table diff.added_tables loop, so foreign keys on a newly added table can be emitted before the referenced newly added table exists. This fails in real diffs where table names sort in the wrong order (for example, orders referencing users), causing the generated migration to stop at ALTER TABLE ... ADD CONSTRAINT ... REFERENCES ... even though both tables are part of the same change set.
Useful? React with 👍 / 👎.
| JOIN information_schema.key_column_usage kcu \ | ||
| ON tc.constraint_name = kcu.constraint_name \ | ||
| AND tc.table_schema = kcu.table_schema \ |
There was a problem hiding this comment.
This join scopes key_column_usage only by constraint_name and schema, but PostgreSQL allows the same constraint name on different tables, so rows from different tables can be cross-joined and merged into a single parsed constraint definition. That produces incorrect constraint column sets and downstream false diffs/migration SQL whenever names are reused across tables; the join needs tc.table_name = kcu.table_name (and equivalent table scoping for related joins) to keep each constraint isolated.
Useful? React with 👍 / 👎.
| let name = cap | ||
| .get(1) | ||
| .map(|m| m.as_str().to_string()) | ||
| .unwrap_or_else(|| format!("check_{}", table_name)); | ||
| let expression = cap[2].trim().to_string(); |
There was a problem hiding this comment.
Unnamed CHECK constraints all fall back to check_<table>, so multiple unnamed CHECK clauses in one table overwrite each other in table.constraints (a map keyed by name). The parser then silently drops earlier checks, which makes schema diffs and generated migrations incomplete for SQL files that define more than one unnamed CHECK constraint.
Useful? React with 👍 / 👎.
Addresses Codex review round 6: 1. New-table constraints after ALL CREATE TABLEs (P1): Moved constraint generation for added tables to a separate phase (8b) that runs after all CREATE TABLE statements. Prevents FK references to not-yet-created tables when table names sort unfavorably (e.g. orders before users). 2. Unique unnamed CHECK names (P2): Unnamed CHECK constraints now get names including a hash of the expression (check_<table>_<hash>), preventing silent overwrites when a table has multiple unnamed CHECKs. 3. PostgreSQL constraint join scoping (P1): Added tc.table_name = kcu.table_name to the key_column_usage JOIN, preventing cross-table row merging when constraint names are reused across tables. Also removed CHECK from this query since CHECKs are already loaded via pg_constraint. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8af4c8b7f4
ℹ️ 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".
| if left_enum.values != right_enum.values { | ||
| let added_values: Vec<String> = right_enum | ||
| .values | ||
| .iter() | ||
| .filter(|v| !left_enum.values.contains(v)) | ||
| .cloned() | ||
| .collect(); | ||
| let removed_values: Vec<String> = left_enum | ||
| .values | ||
| .iter() | ||
| .filter(|v| !right_enum.values.contains(v)) | ||
| .cloned() | ||
| .collect(); | ||
| modified.push(EnumDiff { |
There was a problem hiding this comment.
This branch marks an enum as modified whenever value order differs, but it computes only set differences; if both schemas contain the same labels in a different order (e.g., values inserted with different BEFORE/AFTER history), added_values and removed_values are both empty and a no-op EnumDiff is emitted. Downstream migration generation only acts on non-empty added/removed lists, so users get persistent drift with no SQL fix or warning.
Useful? React with 👍 / 👎.
| fn parse_constraint(def: &str, table_name: &str) -> Option<Constraint> { | ||
| // CONSTRAINT name FOREIGN KEY (cols) REFERENCES ref_table(ref_cols) [ON DELETE ...] [ON UPDATE ...] | ||
| let fk_re = Regex::new( | ||
| r"(?i)(?:CONSTRAINT\s+(\w+)\s+)?FOREIGN\s+KEY\s*\(([^)]+)\)\s*REFERENCES\s+(\w+)\s*\(([^)]+)\)(?:\s+ON\s+DELETE\s+(\w+(?:\s+\w+)?))?(?:\s+ON\s+UPDATE\s+(\w+(?:\s+\w+)?))?" |
There was a problem hiding this comment.
The SQL-file FK parser currently requires REFERENCES <table>(<cols>), so valid definitions like FOREIGN KEY (user_id) REFERENCES users are not parsed at all. In those cases parse_constraint returns None, the FK is silently dropped from the loaded schema, and diffs/migrations can miss or remove real constraints.
Useful? React with 👍 / 👎.
1. Enum order-only changes (P2): EnumDiff now has `reordered` flag. When values are the same set but different order, emits a warning instead of a no-op migration that leaves persistent drift. 2. REFERENCES without target columns (P2): SQL file FK parser now accepts `REFERENCES table` without explicit `(columns)`. Defaults to "id" as conventional PK reference instead of silently dropping the constraint. 3. New test: enum_order_change_detected_as_reordered Total: 144 tests, all passing. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d49f402f7
ℹ️ 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".
| // 3. Reverse added enums (drop them) | ||
| for e in &diff.added_enums { | ||
| statements.push(MigrationStatement { | ||
| sql: format!("DROP TYPE {};", quote_ident(&e.name, dialect)), | ||
| warnings: Vec::new(), |
There was a problem hiding this comment.
generate_rollback drops diff.added_enums and diff.added_sequences before it drops diff.added_tables, so rollback can fail whenever an added table depends on those objects (e.g., a new column typed with a new enum, or a default using a new sequence). In PostgreSQL this causes immediate dependency errors on DROP TYPE/DROP SEQUENCE, making --direction down non-runnable for common add-table migrations.
Useful? React with 👍 / 👎.
| JOIN information_schema.constraint_column_usage ccu \ | ||
| ON ccu.constraint_name = tc.constraint_name \ | ||
| AND ccu.table_schema = tc.table_schema \ |
There was a problem hiding this comment.
The constraint query joins key_column_usage to constraint_column_usage only by constraint name/schema, which loses positional mapping between local and referenced columns for multi-column foreign keys. This can produce cross-joined rows and incorrect ref_columns ordering, so valid composite FKs may be loaded with the wrong shape and then diffed/migrated incorrectly.
Useful? React with 👍 / 👎.
| // When ref columns are omitted, default to "id" (conventional PK) | ||
| let ref_columns: Vec<String> = cap | ||
| .get(4) | ||
| .map(|m| { |
There was a problem hiding this comment.
id
When parsing FOREIGN KEY ... REFERENCES parent (no explicit target columns), the parser hardcodes ref_columns = ["id"]. That silently mis-models schemas where the referenced primary key is not named id (or is composite), causing false diffs and incorrect generated FK SQL for otherwise valid DDL.
Useful? React with 👍 / 👎.
…und 8 1. Rollback ordering (P1): Tables are now dropped BEFORE enums/sequences in rollback. Previously DROP TYPE/SEQUENCE came first, failing when added tables depend on added enum types or sequences. 2. PostgreSQL composite FK mapping (P1): Rewrote constraint loader to use pg_constraint with conkey/confkey arrays instead of information_schema joins. This preserves positional column correspondence for composite foreign keys and eliminates cross-table row merging. 3. Implicit FK references (P2): SQL file parser no longer hardcodes "id" when REFERENCES omits target columns. Now uses source column names as best-effort match, which is more accurate for common patterns like (user_id) REFERENCES users -> ref_columns = [user_id]. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60e4e1f1ef
ℹ️ 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".
| // Phase 3: DROP COLUMNs from modified tables | ||
| for table_diff in &diff.modified_tables { | ||
| for col in &table_diff.removed_columns { |
There was a problem hiding this comment.
generate_migration drops removed columns in Phase 3 before it applies modified_views in Phase 13. In PostgreSQL, ALTER TABLE ... DROP COLUMN fails when a view still depends on that column, so diffs that both change a view definition and drop a referenced column produce a non-runnable migration. The view replacement needs to happen before destructive column drops (or the view must be dropped/recreated around them).
Useful? React with 👍 / 👎.
| for vd in &diff.modified_views { | ||
| statements.push(MigrationStatement { | ||
| sql: format!( | ||
| "CREATE OR REPLACE VIEW {} AS {};", | ||
| quote_ident(&vd.name, dialect), |
There was a problem hiding this comment.
In generate_rollback, modified views are reverted immediately (step 2) before removed columns are re-added (later step 10). If the old view definition references a column that was removed in the forward migration, CREATE OR REPLACE VIEW ... old_definition fails because the column does not exist yet, so the down migration can stop before rollback completes.
Useful? React with 👍 / 👎.
| for row in &rows { | ||
| let constraint_name: String = row.get("constraint_name"); | ||
| let table_name: String = row.get("table_name"); | ||
| let contype: String = row.get("contype"); |
There was a problem hiding this comment.
The loader reads pg_constraint.contype as String, but that column is PostgreSQL internal "char" (also used by confdeltype/confupdtype), which is not a text type. Using Row::get with an incompatible target type can panic at runtime, turning normal schema loads into crashes when constraints are present. Cast these fields to text in SQL (e.g. contype::text) or read them as the proper single-byte type.
Useful? React with 👍 / 👎.
…w timing Review round 9 fixes: 1. Modified views before column drops (P1): Forward migration now replaces modified views in Phase 2 (before DROP COLUMN in Phase 4). Prevents PostgreSQL errors when a view depends on a dropped column. 2. pg_constraint char type cast (P1): Cast contype, confdeltype, confupdtype to ::text in SQL query. PostgreSQL internal "char" type is not compatible with Rust String — would panic at runtime. 3. Rollback view timing (P1): Modified views are now restored in step 17 (after columns are re-added in step 10), not step 2. Prevents failures when old view definition references a column that was removed in the forward migration. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae66592b24
ℹ️ 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".
| let format = params.resolve_format(&cfg.output.format); | ||
|
|
||
| // Build CI report for structured output and exit code logic | ||
| let report = CiReport::from_diff(&diff, &up_statements); |
There was a problem hiding this comment.
This now builds CI exit-code logic from CiReport::from_diff, but the report builder in src/ci.rs still only enumerates table/column/index changes and ignores newly introduced constraint/view/enum/sequence diffs. In a constraint-only drift (for example, dropping a FK), migration marks the SQL as blocking but report.blocking remains empty, so --fail-on-blocking returns drift code 1 instead of unsafe code 3, and JSON/YAML output can report 0 change(s) despite real schema changes.
Useful? React with 👍 / 👎.
| .find(" AS ") | ||
| .map(|pos| sql[pos + 4..].to_string()) | ||
| .unwrap_or(sql); |
There was a problem hiding this comment.
The SQLite loader extracts a view body by searching for uppercase " AS ", but sqlite_master.sql preserves the original statement casing. If a view was created with lowercase as, this falls back to storing the full CREATE VIEW ... statement as the definition; migration then emits CREATE VIEW <name> AS <full-create-statement>, which is invalid SQL and causes false drift to persist.
Useful? React with 👍 / 👎.
… case Review round 10 fixes: 1. CiReport completeness (P1): from_diff now includes constraint (added/removed), view (added/removed/modified), enum, and sequence changes. Previously only table/column/index changes were reported, causing --fail-on-blocking to miss blocking constraint drops and JSON/YAML output to undercount changes. 2. DROP INDEX/COLUMN is_blocking in CiReport: DROP INDEX and DROP COLUMN entries now correctly marked as blocking in the report (was false, should be true). 3. SQLite view definition case-insensitive (P2): View body extraction now searches for " AS " case-insensitively. Previously lowercase "as" in CREATE VIEW caused the full DDL to be stored as the definition, producing invalid migration SQL. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0496c4dc6
ℹ️ 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".
| // 11. Recreate removed tables (data is lost) | ||
| for table in &diff.removed_tables { | ||
| statements.push(MigrationStatement { | ||
| sql: create_table_sql(table, dialect), |
There was a problem hiding this comment.
generate_rollback recreates diff.removed_tables before it recreates diff.removed_enums and diff.removed_sequences, so a rollback can fail as soon as a dropped table contains an enum-typed column or a default that references a dropped sequence. In PostgreSQL, CREATE TABLE requires those referenced objects to exist first, so down migrations become non-runnable for common drop-table scenarios that include custom types/sequences.
Useful? React with 👍 / 👎.
| let on_delete = cap | ||
| .get(5) | ||
| .map(|m| m.as_str().to_string()) | ||
| .filter(|s| s != "NO ACTION" && s != "RESTRICT"); |
There was a problem hiding this comment.
The SQL-file constraint parser only strips NO ACTION/RESTRICT when the captured text matches exact uppercase, so lowercase or mixed-case forms (for example on delete no action) are kept as explicit actions. That produces false constraint diffs against live loaders that normalize these actions away, causing noisy or incorrect migration output when schema files use non-uppercase keyword casing.
Useful? React with 👍 / 👎.
Review round 11 fixes: 1. Rollback type ordering (P1): Removed enums and sequences are now recreated BEFORE removed tables in rollback. Tables with enum-typed columns or sequence defaults require those objects to exist first. Order: enums -> sequences -> tables -> columns -> constraints. 2. FK action case normalization (P2): SQL file parser now uppercases ON DELETE/ON UPDATE actions before filtering NO ACTION/RESTRICT. Previously lowercase "no action" was kept as an explicit action, causing false diffs against live loaders. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3b86dcf18
ℹ️ 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".
| // Phase 2: Replace modified views BEFORE column drops (views may reference dropped columns) | ||
| for vd in &diff.modified_views { | ||
| statements.push(MigrationStatement { | ||
| sql: format!( | ||
| "CREATE OR REPLACE VIEW {} AS {};", | ||
| quote_ident(&vd.name, dialect), |
There was a problem hiding this comment.
generate_migration unconditionally emits CREATE OR REPLACE VIEW ... vd.new_definition in Phase 2, before Phase 9 adds newly introduced columns. When a view change depends on a column added in the same diff (for example, adding users.bio and updating a view to select it), this early replacement fails immediately because the referenced column does not exist yet, making the generated up migration non-runnable.
Useful? React with 👍 / 👎.
| let format = params.resolve_format(&cfg.output.format); | ||
|
|
||
| // Build CI report for structured output and exit code logic | ||
| let report = CiReport::from_diff(&diff, &up_statements); |
There was a problem hiding this comment.
run_diff always builds CiReport from up_statements even when the user requested --direction down (or both). In CI mode, this means --fail-on-blocking and annotations are computed from the wrong migration direction, so rollback-only blocking operations can be missed and produce the wrong exit code.
Useful? React with 👍 / 👎.
Review round 12 fixes: 1. View replacement timing (P1): Modified views are now DROP'd early (Phase 2, before column drops) and CREATE'd late (Phase 13b, after column adds). This handles both cases: views referencing dropped columns (need early drop) and views referencing added columns (need late create). Uses DROP VIEW IF EXISTS + CREATE VIEW pattern instead of CREATE OR REPLACE. 2. CI direction-aware report (P2): CiReport is now built from down_statements when --direction=down, ensuring --fail-on-blocking and annotations reflect the actual migration direction. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 962de0e526
ℹ️ 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".
|
|
||
| async fn connect_tls(dsn: &str, host: &str) -> Result<Client, DbDiffError> { | ||
| let tls_connector = native_tls::TlsConnector::builder() | ||
| .danger_accept_invalid_certs(true) |
There was a problem hiding this comment.
connect_tls unconditionally calls danger_accept_invalid_certs(true), which means --ssl-mode require still accepts untrusted/self-signed certificates and is vulnerable to MITM interception. Users selecting require expect authenticated TLS, so this silently weakens transport security instead of enforcing it.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // Views | ||
| let (added_views, removed_views, modified_views) = diff_views(&left.views, &right.views); |
There was a problem hiding this comment.
diff_schemas always compares left.views and right.views, but the SQL-file loader only populates tables/indexes, leaving Schema.views empty for .sql inputs. In DB-vs-SQL comparisons this can mark existing views as removed and emit DROP VIEW statements even when the schema file already defines them, creating destructive false positives.
Useful? React with 👍 / 👎.
| let hint = if cause.contains("refused") { | ||
| format!( | ||
| "Check that the database server is running and accepting connections.\n\ | ||
| Try: pg_isready -h {host} or mysql --host={host} --execute='SELECT 1'", |
There was a problem hiding this comment.
The connection error message sanitizes credentials, but the hint text interpolates the raw host value directly. Since callers pass full DSNs, failures like connection-refused/DNS errors can print usernames/passwords in stderr hints (for example in pg_isready -h ...), leaking secrets in logs.
Useful? React with 👍 / 👎.
…ation Review round 13 fixes: 1. TLS certificate validation (P1): --ssl-mode=require now validates server certificates (accept_invalid_certs=false). Only --ssl-mode=prefer accepts invalid certs (needed for fallback). Prevents MITM on connections that users expect to be authenticated. 2. SQL-file view false diffs (P1): When either source is a SQL file, views/enums/sequences from the live-DB side are cleared before diffing. Prevents false DROP VIEW/TYPE/SEQUENCE statements when comparing a live DB against a .sql file that can't represent these objects. 3. DSN hint sanitization (P2): Error hints now use sanitize_dsn() instead of raw DSN. Prevents credentials from leaking in pg_isready/mysql troubleshooting suggestions in stderr/logs. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05f8791d49
ℹ️ 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".
| if left.dialect == SqlDialect::SqlFile || right.dialect == SqlDialect::SqlFile { | ||
| if left.dialect == SqlDialect::SqlFile { | ||
| right.schema.views.clear(); | ||
| right.schema.enums.clear(); | ||
| right.schema.sequences.clear(); |
There was a problem hiding this comment.
This branch treats every SqlDialect::SqlFile input as if it came from the SQL parser and clears views/enums/sequences from the opposite schema, but .json snapshots are also loaded with SqlDialect::SqlFile (src/loader/mod.rs) and do carry these objects. In DB-vs-snapshot comparisons, this causes false add/remove diffs for views/enums/sequences (and misleading migration SQL) even when schemas actually match, so offline snapshot CI results become incorrect.
Useful? React with 👍 / 👎.
| } | ||
| Err(_) => { | ||
| let err = dbdiff::error::DbDiffError::timeout( | ||
| &format!("{} / {}", params.source, params.target_source), |
There was a problem hiding this comment.
The timeout path concatenates both raw DSNs into one string before passing it to DbDiffError::timeout. sanitize_dsn only strips credentials relative to the first @, so a timeout can still print credentials from the second DSN in stderr (for example source / target where target contains user:pass@...). This leaks secrets in logs during normal failure handling.
Useful? React with 👍 / 👎.
| for table in &diff.removed_tables { | ||
| statements.push(MigrationStatement { | ||
| sql: create_table_sql(table, dialect), | ||
| warnings: vec![ |
There was a problem hiding this comment.
Rollback recreates removed tables with create_table_sql(table, dialect), which only emits columns, but it never restores that removed table’s indexes or constraints afterward (the later restore loops only cover modified_tables). For any forward migration that drops a table with FKs/UNIQUE/CHECK/indexes, --direction down returns an incomplete schema and leaves persistent drift after rollback.
Useful? React with 👍 / 👎.
…ialect Review round 14 fixes: 1. Rollback table completeness (P1): Recreated removed tables now also get their indexes and constraints restored. Previously only the CREATE TABLE was emitted, leaving persistent drift after rollback. 2. DSN sanitization in timeout (P1): Each DSN is now sanitized independently before inclusion in timeout error messages. Previously the raw concatenation could leak credentials from the second DSN. 3. Snapshot vs SqlFile dialect (P1): JSON snapshots now use SqlDialect::Snapshot instead of SqlFile. The view/enum/sequence clearing logic only triggers for SqlFile, so snapshots correctly preserve and diff all object types. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16a608ce3a
ℹ️ 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".
| "ALTER TABLE {table} ADD CONSTRAINT {name} FOREIGN KEY ({}) REFERENCES {}({})", | ||
| cols.join(", "), |
There was a problem hiding this comment.
This helper always renders foreign-key/unique/check additions as ALTER TABLE ... ADD CONSTRAINT ..., and generate_migration invokes it for all dialects, including SQLite. SQLite cannot add table constraints this way after table creation, so any SQLite diff that introduces a constraint (for example a new FK) will produce non-runnable migration SQL and fail during apply.
Useful? React with 👍 / 👎.
| if let Some(at_pos) = dsn.find('@') { | ||
| dsn[at_pos + 1..].to_string() |
There was a problem hiding this comment.
Credential stripping uses find('@'), which takes the first @ in the DSN; if a password contains @ (a common real-world case), the returned "sanitized" value still contains trailing password content (e.g. part after the first @) and can leak secrets in error logs. Using the last separator (or URL parsing) avoids exposing credential fragments.
Useful? React with 👍 / 👎.
Review round 15 fixes:
1. SQLite ADD CONSTRAINT (P1): SQLite does not support ALTER TABLE
ADD CONSTRAINT. Now emits a manual migration comment instead of
invalid SQL. Applies to FK, UNIQUE, and CHECK constraints.
2. DSN sanitization with @ in password (P2): Changed find('@') to
rfind('@') so passwords containing @ characters are fully stripped.
Previously "postgres://user:p@ss@host/db" would leak "ss@host/db".
https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1e7bf67bb
ℹ️ 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".
| for table_diff in &diff.modified_tables { | ||
| for col in &table_diff.removed_columns { | ||
| for pattern in &protected.columns { |
There was a problem hiding this comment.
check_protected only evaluates protected.columns against diff.modified_tables[*].removed_columns, so a full table drop bypasses column protection entirely. For example, with protected.columns: ["*.email"], removing a table that contains email is currently allowed because diff.removed_tables is never checked against those patterns. That defeats the safety guarantee for destructive migrations and can allow protected data-bearing columns to be dropped indirectly.
Useful? React with 👍 / 👎.
| // Sequences | ||
| for s in &diff.added_sequences { | ||
| changes.push(CiChange { |
There was a problem hiding this comment.
CiReport::from_diff adds sequence entries only for added_sequences and removed_sequences, but never for modified_sequences. When drift is sequence-property-only (e.g., ALTER SEQUENCE), CI output can show equal: false while changes omits the sequence change and summary may report 0 change(s) detected, which is misleading for JSON/YAML consumers and CI annotations.
Useful? React with 👍 / 👎.
…port Review round 16 fixes: 1. Protected columns in dropped tables (P1): check_protected now inspects columns of removed tables against protected.columns patterns. Previously dropping a table with a protected column (e.g. *.email) bypassed the safety check entirely. 2. Modified sequences in CiReport (P2): from_diff now includes diff.modified_sequences as ALTER SEQUENCE entries. Previously sequence-only drift showed equal:false but 0 changes in output. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c221784718
ℹ️ 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".
| for c in table.constraints.values() { | ||
| statements.push(MigrationStatement { | ||
| sql: add_constraint_sql(c, dialect), | ||
| warnings: Vec::new(), |
There was a problem hiding this comment.
generate_rollback recreates each removed table and immediately emits its constraints inside the same loop. If a recreated table has an FK to another table that is also in diff.removed_tables but sorted later, the emitted ADD CONSTRAINT ... REFERENCES ... runs before the referenced table exists, so down migrations fail on valid multi-table rollback scenarios.
Useful? React with 👍 / 👎.
| for table in &diff.removed_tables { | ||
| if protected.tables.contains(&table.name) { |
There was a problem hiding this comment.
check_protected only inspects diff.removed_tables (and removed columns), which matches forward/up drops but not rollback/down drops. With --direction down, destructive operations come from diff.added_tables/added columns, so protected objects can still be dropped without triggering the guard, defeating the safety policy in rollback mode.
Useful? React with 👍 / 👎.
| if let Some(to) = to_col { | ||
| entry.2.push(to.clone()); | ||
| } else { | ||
| entry.2.push("id".to_string()); |
There was a problem hiding this comment.
id
When PRAGMA foreign_key_list returns to = NULL (valid for REFERENCES parent without an explicit column list), the loader hardcodes ref_columns to id. For parent tables whose primary key is not named id, this stores an incorrect FK shape and causes false diffs and invalid generated FK SQL against SQLite schemas that rely on implicit PK references.
Useful? React with 👍 / 👎.
…K target Review round 17 fixes: 1. Protected checks by direction (P1): check_protected now inspects added_tables/added_columns when --direction=down (rollback drops those). Both directions checked when --direction=both. Previously rollback mode could bypass protection entirely. 2. Rollback FK constraint ordering (P1): Removed-table constraints are now deferred to step 15b (after ALL tables are recreated). Previously FKs could reference not-yet-recreated tables when table sort order was unfavorable. 3. SQLite FK NULL target (P2): Uses source column name instead of hardcoded "id" when PRAGMA foreign_key_list returns NULL target. Matches common patterns (user_id -> user_id) more accurately. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39f75eb2a8
ℹ️ 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".
| _ => format!( | ||
| "ALTER TABLE {} DROP CONSTRAINT {};", | ||
| quote_ident(&c.table_name, dialect), | ||
| quote_ident(&c.name, dialect) | ||
| ), |
There was a problem hiding this comment.
drop_constraint_sql falls through to ALTER TABLE ... DROP CONSTRAINT for every non-MySQL dialect, and both forward/rollback generation call this helper for removed constraints. For SQLite, that SQL is unsupported, so any diff that removes a constraint (or rolls one back) produces a migration that fails immediately instead of emitting a manual-migration note like add_constraint_sql already does for SQLite.
Useful? React with 👍 / 👎.
| if let Some(c) = pattern.strip_prefix("*.") { | ||
| col_name == c | ||
| } else if let Some((t, c)) = pattern.split_once('.') { | ||
| table_name == t && col_name == c | ||
| } else { |
There was a problem hiding this comment.
column_is_protected only supports *.column and table.column patterns, so table.* never matches even though protected columns are documented to use the same pattern format as ignore.columns. In practice, configs like protected.columns: ["users.*"] do not block column drops (including via table drop), which bypasses the intended destructive-change safeguard.
Useful? React with 👍 / 👎.
Review round 18 fixes: 1. SQLite DROP CONSTRAINT (P1): drop_constraint_sql now emits a manual migration comment for SQLite instead of invalid ALTER TABLE DROP CONSTRAINT SQL. Mirrors the add_constraint_sql SQLite handling. 2. Protected column table.* pattern (P1): column_is_protected now supports "table.*" wildcard pattern, matching all columns in a specific table. Previously this pattern silently failed to match, allowing protected columns to be dropped. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88407d89e9
ℹ️ 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".
| let ci_statements = match params.direction { | ||
| MigrationDirection::Down => &down_statements, | ||
| _ => &up_statements, | ||
| }; | ||
| let report = CiReport::from_diff(&diff, ci_statements); |
There was a problem hiding this comment.
run_diff switches to down_statements for --direction down, but CiReport::from_diff still interprets the same forward diff (added_* means create, removed_* means drop). In rollback mode this misclassifies operations (for example, dropping an added table/column is treated like a non-blocking add with no matching SQL), so --ci --fail-on-blocking --direction down can miss destructive rollback steps and return the wrong exit code.
Useful? React with 👍 / 👎.
| // Try to parse as constraint | ||
| if constraint_re.is_match(trimmed) { | ||
| if let Some(c) = parse_constraint(trimmed, &table.name) { | ||
| table.constraints.insert(c.name.clone(), c); | ||
| } |
There was a problem hiding this comment.
The SQL-file parser only attempts constraint parsing when a segment starts with CONSTRAINT/FOREIGN KEY/UNIQUE/CHECK. Common inline FK syntax like user_id INTEGER REFERENCES users(id) is handled as a plain column and no constraint is created, so foreign keys are silently dropped from loaded schemas and downstream diffs/migrations become incomplete for many real-world DDL files.
Useful? React with 👍 / 👎.
Review round 19 fixes: 1. Inline REFERENCES in SQL files (P2): Column definitions like "user_id INTEGER REFERENCES users(id)" now generate FK constraints. Previously only standalone FOREIGN KEY clauses were parsed. 2. Direction-aware CI report (P1): CiReport now has from_diff_reversed() for rollback mode. In --direction=down, added_tables are correctly reported as DROP operations (blocking) and removed_tables as ADD (non-blocking). Fixes wrong exit codes with --ci --fail-on-blocking --direction down. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Lines 142 to 145 in 049014e
Exclude constraint-owned unique indexes from PostgreSQL load
This loader keeps all non-_pkey indexes, including unique indexes that back UNIQUE constraints, and the same schema load also records UNIQUE constraints separately. When a UNIQUE constraint is removed/changed, the diff contains both a removed constraint and removed index; migration emits DROP CONSTRAINT first and then DROP INDEX, but PostgreSQL already drops the backing index with the constraint, so the later index drop can fail and break the migration.
ℹ️ 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".
| for table_diff in &diff.modified_tables { | ||
| for col in &table_diff.removed_columns { |
There was a problem hiding this comment.
generate_rollback handles added/removed columns but never reverses modified_columns, so down migrations do not restore prior type/nullability/default changes. In a common case where the up migration alters a column definition, --direction down will still leave the column in the new shape, causing persistent schema drift even after rollback completes.
Useful? React with 👍 / 👎.
| let definition = upper | ||
| .find(" AS ") | ||
| .map(|pos| sql[pos + 4..].to_string()) |
There was a problem hiding this comment.
The code finds " AS " in an uppercased copy of the statement and then slices the original SQL using that byte position. With non-ASCII characters before AS, uppercasing can change byte length, so sql[pos + 4..] may cut at the wrong offset or panic on an invalid UTF-8 boundary, making valid Unicode view definitions fail to load reliably.
Useful? React with 👍 / 👎.
… UTF-8 Review round 20 fixes: 1. Exclude constraint-owned indexes (P1): PostgreSQL index loader now uses NOT EXISTS(pg_constraint) to skip indexes that back PK/UNIQUE constraints. Prevents duplicate DROP INDEX after DROP CONSTRAINT (PG auto-drops the backing index). 2. Rollback modified columns (P1): generate_rollback now reverts modified columns to their old type/nullability/default using generate_column_alterations_reversed(). Previously --direction down left columns in the new shape. 3. SQLite view UTF-8 safety (P2): View body extraction uses eq_ignore_ascii_case byte search instead of to_uppercase().find(), preventing byte-offset mismatch on non-ASCII view names. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6957aed46
ℹ️ 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".
| SELECT 1 FROM pg_constraint c \ | ||
| JOIN pg_class cl ON cl.oid = c.conindid \ | ||
| WHERE cl.relname = i.indexname \ | ||
| ) \ |
There was a problem hiding this comment.
The NOT EXISTS clause that excludes constraint-backed indexes matches only on cl.relname = i.indexname and does not restrict cl/pg_constraint to public. Because PostgreSQL index names are only schema-scoped, a constraint index in another schema with the same name can incorrectly hide a normal public index from loading, producing false index diffs and incorrect migration SQL for public-schema comparisons.
Useful? React with 👍 / 👎.
| .windows(4) | ||
| .position(|w| w.eq_ignore_ascii_case(b" AS ")) | ||
| .map(|pos| sql[pos + 4..].to_string()) |
There was a problem hiding this comment.
AS separator with flexible whitespace
View body extraction only searches for the exact byte sequence " AS "; statements like CREATE VIEW v AS\nSELECT ... or tab-separated formatting do not match, so the fallback stores the full CREATE VIEW ... SQL as the definition. Migration generation then emits CREATE VIEW <name> AS <full-create-statement>, which is invalid SQL and causes persistent false drift for formatted SQLite view definitions.
Useful? React with 👍 / 👎.
…hitespace Review round 21 fixes: 1. Schema-scoped constraint index filter (P2): NOT EXISTS clause now joins pg_namespace to restrict to 'public' schema, preventing same-named constraint indexes in other schemas from hiding public indexes. 2. SQLite view AS with flexible whitespace (P2): Uses regex \bAS\s+ for case-insensitive, whitespace-flexible matching. Handles tabs, newlines around AS keyword instead of requiring exact " AS " byte sequence. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 384b257106
ℹ️ 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".
| (SqlDialect::SqlFile | SqlDialect::Snapshot, other) | ||
| | (other, SqlDialect::SqlFile | SqlDialect::Snapshot) => other, |
There was a problem hiding this comment.
This branch treats Snapshot like a concrete SQL dialect and allows snapshot-vs-snapshot diffs to generate migration SQL, but snapshots do not store which backend they came from. In that case SQL generation uses SqlDialect::Snapshot, so backend-specific DDL can be wrong (for example, index drops follow non-MySQL syntax in drop_index_sql). This makes offline snapshot comparisons capable of emitting non-runnable SQL for MySQL/SQLite schemas unless dialect metadata is carried through or SQL generation is rejected when both sides are snapshots.
Useful? React with 👍 / 👎.
When both inputs are snapshots or SQL files, there's no live DB to infer the dialect from. Now defaults to PostgreSQL (most standards- compliant SQL) instead of using SqlDialect::Snapshot which could produce non-runnable DDL for MySQL/SQLite schemas. https://claude.ai/code/session_018B3KsbeKeZsSDHC8qcnKdM
2 participants
Summary
Comprehensive overhaul based on competitor analysis of Meru143/dbdiff (Go) and DBDiff/DBDiff (PHP). Closes major feature gaps while maintaining Rust performance advantages.
112 tests (81 unit + 28 integration + 3 config), all passing. Clippy clean, fmt clean.
Changes by category
Stability & Error Handling
--timeout, default 10s) prevents hanging on unreachable hosts--ssl-mode disable/prefer/require)protected.tables/columns) blocks destructive migrations on critical objects--outrequires--writeto actually save filesCLI & UX
diff,validate,tables,completions,init,snapshot(backward-compatible — old syntax still works)dbdiff validate <DSN>— test connectivity, show table/column/index counts + latencydbdiff tables <DSN>— list all tables with column/index countsdbdiff init— generate default.dbdiff.ymlconfigdbdiff completions bash/zsh/fish/powershell— shell completionsdbdiff snapshot <DSN> --out schema.json— export schema as JSON for offline CI--explainmode with human-readable explanations for each migration statement--color auto|always|neverwith TTY auto-detectionSchema Coverage (was: tables + columns + indexes only)
Migration Generation
--direction up/down/bothgenerates forward + rollback SQLInfrastructure
.jsonfiles can be used as diff sources for offline CICI Integration (merged from master)
--fail-on-blockingexit code 3 for blocking operationsType of Change
--outnow requires--write)Checklist
cargo fmt)cargo clippywith no warningscargo test)