Skip to content

Comments

fix: preserve dependency order for views in multi-file dump (#307)#310

Merged
tianzhou merged 1 commit intomainfrom
fix/issue-307-view-dependency-order
Feb 22, 2026
Merged

fix: preserve dependency order for views in multi-file dump (#307)#310
tianzhou merged 1 commit intomainfrom
fix/issue-307-view-dependency-order

Conversation

@tianzhou
Copy link
Contributor

Summary

  • The pgschema dump --multi-file mode was sorting view include directives alphabetically, breaking dependency ordering when a view depended on another view that sorted later in the alphabet (e.g., dashboard depends on item_summary, but dashboard.sql was included before item_summary.sql)
  • Now preserves the topological ordering from the diff package by tracking insertion order instead of re-sorting alphabetically
  • Added dump test case with dependent views verifying both single-file and multi-file dependency ordering

Fixes #307

Test plan

  • TestDumpCommand_Issue307ViewDependencyOrder - verifies single-file dump outputs views in dependency order
  • TestDumpCommand_Issue307MultiFileViewDependencyOrder - verifies multi-file dump include directives are in dependency order (item_summary before dashboard)
  • All existing dump tests pass (no regressions)
  • All dependency diff tests pass
  • All view and materialized view diff tests pass

🤖 Generated with Claude Code

The multi-file dump mode was sorting view includes alphabetically, which
broke dependency ordering when a view depended on another view that sorted
later in the alphabet. Now preserves the topological ordering from the diff
package instead of re-sorting alphabetically.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings February 22, 2026 04:13
@greptile-apps
Copy link

greptile-apps bot commented Feb 22, 2026

Greptile Summary

Fixed a critical bug in multi-file dump mode where view include directives were sorted alphabetically, breaking dependency ordering when a view depended on another view that sorted later alphabetically.

Key Changes:

  • Modified internal/dump/formatter.go to track insertion order instead of alphabetically sorting objects per directory
  • The diff package already provides topologically sorted views (using Kahn's algorithm), so preserving this order maintains correct dependencies
  • Added orderByDir map to track first appearance of each object name per directory
  • Removed the sort.Strings(objNames) call that was breaking the topological order

Testing:

  • TestDumpCommand_Issue307ViewDependencyOrder verifies single-file dump outputs views in dependency order
  • TestDumpCommand_Issue307MultiFileViewDependencyOrder verifies multi-file dump include directives preserve dependency order (item_summary before dashboard)
  • Test case uses views where dashboard depends on item_summary, but dashboard sorts alphabetically before item_summary

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The fix is well-isolated to the multi-file formatting logic, preserves the existing topological ordering from the diff package, includes comprehensive test coverage for both single-file and multi-file modes, and addresses a specific bug without introducing new complexity
  • No files require special attention

Important Files Changed

Filename Overview
internal/dump/formatter.go Fixed multi-file dump to preserve topological view ordering by tracking insertion order instead of alphabetically sorting
cmd/dump/dump_integration_test.go Added comprehensive tests for single-file and multi-file view dependency ordering

Last reviewed commit: bfe4d53

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug where views in multi-file dump mode were being sorted alphabetically instead of preserving the dependency order established by the diff package's topological sort. This caused SQL execution failures when a view depended on another view with an alphabetically later name (e.g., dashboard depending on item_summary).

Changes:

  • Modified multi-file formatter to preserve topological ordering by tracking insertion order instead of re-sorting alphabetically
  • Added comprehensive test coverage for both single-file and multi-file view dependency ordering
  • Removed unnecessary sort.Strings import from formatter.go

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/dump/formatter.go Replaced alphabetical sorting with insertion-order tracking via orderByDir map to preserve topological dependency ordering from diff package
cmd/dump/dump_integration_test.go Added two integration tests: one for single-file mode (using existing test harness) and one for multi-file mode (explicitly verifying include directive order)
testdata/dump/issue_307_view_dependency_order/raw.sql Test schema demonstrating the issue: dashboard view depends on item_summary view
testdata/dump/issue_307_view_dependency_order/pgschema.sql Expected output showing correct ordering: item_summary before dashboard
testdata/dump/issue_307_view_dependency_order/pgdump.sql pg_dump reference output for comparison
testdata/dump/issue_307_view_dependency_order/manifest.json Test metadata describing the test case

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tianzhou tianzhou merged commit 890d1fe into main Feb 22, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Views are not dependency-ordered

1 participant