fix: Harden parser, schema input, layout, diff, and CLI output gating#133
Merged
Conversation
The wildcard arm in `parse_comment` panicked via `unreachable!` if the table-parts slice ever turned out to be empty. The early `parts.len() < 2` guard makes that flow unreachable today, but the implicit invariant is fragile against future refactors and violates the workspace policy of avoiding panics in production paths. Emit a `parse_unsupported` warning and bail out instead so unexpected `ObjectName` shapes degrade gracefully rather than aborting the CLI/WASM process.
… runtime fallback `Schema::validate()` was previously exercised only by unit tests, so structural inconsistencies (duplicate tables, FKs to unknown columns/tables, malformed enums, etc.) silently slipped through into downstream stages. Run validation on every successfully loaded schema and translate each `ValidationError` into a `SCHEMA-4` warning diagnostic so the CLI/WASM/action surfaces them like any other parser-level issue. Also remove the `block_in_place` branch from `schema_from_db_url`. The sync entry point is only used by the CLI, which never runs inside an existing Tokio runtime, and the fallback would have panicked on a current-thread runtime if any future async caller picked it up. Async callers should use `schema_from_db_url_async` instead. Two existing fixtures intentionally point a foreign key at a removed target to exercise lock-risk rules; lock down their new validation warnings with `expected_diagnostics.json`.
`tracing::error!(error = %e, ...)` previously rendered the raw `sqlx::Error` Display, which can include the original DSN for the `Configuration`, `Tls`, and `Io` variants. That meant a credential- bearing URL could leak into CI logs or downstream SIEM systems even though the structured `IntrospectError` returned to the caller was already sanitized. Pipe the rendered error message through the existing `crate::url::sanitize_error_message` before emitting the log line so all three drivers strip credentials and query-string secrets at the logging boundary, matching the protection already applied to the returned error.
`compute_repulsion_with_grid` walked the `HashMap<(i32, i32), Vec<usize>>` directly and accumulated repulsion forces in iteration order. Because floating-point addition is not associative, the resulting `forces` vector — and therefore the rendered SVG/HTML — was sensitive to the per-process `RandomState` seed once a graph crossed the 64-node spatial-grid threshold. Mirror the pattern already used by `resolve_force_overlaps`: collect forward-only candidate pairs into a `Vec`, sort with `sort_unstable`, then apply repulsion in that fixed order. The graph traversal is unchanged (each pair still appears at most once thanks to the four forward-neighbour offsets), but the per-pair traversal sequence is now independent of `HashMap` iteration order, restoring byte-for-byte determinism for snapshots, CI, and the playground.
Reordering keeps --out and --fail-on-warning/--deny independent: previously a warning aborted the command before write_output ran, leaving --out empty even though the requested artifact had been produced successfully.
Column names in column, FK, and index diffs are now lowercased before keying and comparison, so identifier case differences no longer manifest as spurious add/remove/modified entries. The unnamed-index key is also rebuilt with a length-prefixed encoding to avoid the existing '_'-join collision (e.g., ['a_b', 'c'] vs ['a', 'b_c']).
The fallback span (0..min(input.len(), 100)) pointed at the start of the input rather than the actual error site, which made diagnostics misleading. Now we either attach the resolved span when available or omit the span entirely so the message stands on its own.
The shim has had no remaining callers in the workspace. Per the project's no-back-compat-for-public-library policy, drop it instead of carrying it forward.
codes::lint_no_pk and codes::lint_orphan_table duplicated the canonical mapping in LintRuleId::diagnostic_code() and were already drifting (the shims encoded an older numbering: LINT001/LINT002 instead of the current LINT001/LINT004). Remove both shims and update the affected output tests to call the canonical accessor directly.
Code Metrics Report
Details | | main (9f42bad) | #133 (ffad881) | +/- |
|---------------------|----------------|----------------|-------|
+ | Coverage | 94.8% | 94.9% | +0.0% |
| Files | 81 | 81 | 0 |
| Lines | 39201 | 39233 | +32 |
+ | Covered | 37186 | 37238 | +52 |
- | Test Execution Time | 1m33s | 1m34s | +1s |Code coverage of files in pull request scope (94.5% → 94.6%)Reported by octocov |
Schema reviewTip ✅ No risk findings — schema changes look safe to merge. |
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
parse_unsupporteddiagnostic instead of crashing the parser viaunreachable!.block_in_place, which would panic on a current-thread Tokio runtime.connect_errorcontinues to use the existing redaction path.--outnow writes its artifact before--fail-on-warning/--denyare evaluated, so a warning no longer leaves the requested file empty.parse_schemashim is removed, and unusedcodes::lint_no_pk/codes::lint_orphan_tableare deleted in favor of the canonicalLintRuleId::diagnostic_code()mapping.Changes
_ => unreachable!(...)arm inparse_commentwith an explicit fallback that emits aparse_unsupportedwarning and returns, so malformedCOMMENT ON COLUMNstatements no longer panic the parser.Schema::validate()intoschema_from_input_with_contextand translate eachValidationErrorinto aSCHEMA-4warning diagnostic alongside an existing schema load.Handle::try_current()/block_in_placebranch fromschema_from_db_url, which would panic when called from a current-thread Tokio runtime.sanitize_error_messagein the Postgres/MySQL/SQLite connect-failure handlers so DSN credentials cannot leak into structured logs even when the user's URL embeds them.resolve_force_overlapsso layouts no longer drift across runs due to HashMap iteration order.write_outputahead ofcheck_diagnostics/check_diagnostics_at_or_abovein render, diff, doc, export, inspect, and lint so--outis honored even when--fail-on-warningor--denyultimately trips the exit code.diff_columns_withand when forming FK column pairs, so identifier case differences no longer surface as add/remove or modified entries.push_key_partlength prefixes, fixing a pre-existing collision where[\"a_b\", \"c\"]and[\"a\", \"b_c\"]shared the sameidx_a_b_cbucket.SourceSpan::new(0, input.len().min(100))fallback that pointed at the start of input rather than the actual error site, attaching only the resolved span when one is available.#[deprecated]parse_schemare-export; no in-tree callers remain and the project's policy disallows backwards-compat shims for the public library.codes::lint_no_pkandcodes::lint_orphan_table, which duplicated and had drifted from the canonical numbering provided byLintRuleId::diagnostic_code().LintRuleId::OrphanTable.diagnostic_code()directly so they reflect the live LINT004 mapping.