Skip to content

fix: emit composite foreign keys as one constraint, not one per column - #23

Merged
TheCrab13 merged 2 commits into
mainfrom
fix/foreign-keys-definition-issue-for-composite-primary-key/no-ref
Aug 20, 2026
Merged

fix: emit composite foreign keys as one constraint, not one per column#23
TheCrab13 merged 2 commits into
mainfrom
fix/foreign-keys-definition-issue-for-composite-primary-key/no-ref

Conversation

@TheCrab13

Copy link
Copy Markdown
Contributor

Summary

Foreign keys were carried on Column, so a composite key had nowhere to live: the
emitters wrote one ALTER TABLE … ADD FOREIGN KEY per column, and each half pointed
at a key that isn't unique on its own. Postgres rejects the result with "there is no
unique constraint matching given keys for referenced table"
. The split also invented
constraints the source never had — a two-column FK came out as two unrelated ones.

This moves foreign keys to the table level. ForeignKey now holds columns and
ref_columns (paired positionally), a shared helper groups the per-column rows every
catalog returns into one constraint, and the emitters write one statement per
constraint. All five readers select the constraint name and order by column position.

Type of change

  • Bug fix (fix:) — non-breaking change that fixes an issue
  • Feature (feat:) — non-breaking change that adds capability
  • Performance (perf:)
  • Refactor (refactor:) — no behaviour change
  • Documentation (docs:)
  • CI / build / tooling (ci: / chore:)
  • Breaking change — describe the migration path below

Column.foreign_key is gone; use Table.foreign_keys, a list of ForeignKey. The old
shape could not represent a composite key, so keeping it as an alias would have meant
lying about half the constraints.

# before
for column in table.columns.values():
    fk = column.foreign_key
    if fk:
        ... fk.column

# after
for fk in table.foreign_keys:
    ... fk.columns, fk.ref_columns

Linked issues

How was this tested?

Simple and composite keys are now covered at each layer: the five readers (SQLite gets
its PRAGMA rows out of order on purpose, to pin the seq sort), both emitters, and a
CLI end-to-end run over a real SQLite database with a composite PK and FK. Reader tests
keep both kinds of constraint on the same table, so a regression that merged two
distinct FKs into one would fail too.

The four Docker fixtures gained an assembly / assembly_vote pair with a composite
key, with an assertion per engine.

I also checked the reported dump (510 FK statements): 481 were already valid, the other
29 span the 9 composite-PK parents this PR fixes. No FK there references a non-PK
column, so regenerating should apply cleanly.

  • pytest tests/unit tests/cli passes locally — 598 passed
  • lint-imports passes (no driver leaked into domain/application)
  • tox -e syntax passes (black, isort, flake8, mypy, pylint) — ran black, isort,
    flake8 and mypy directly on db2sql/ and installer/; pylint and tox not run
  • For changes touching readers/writers/emitters: tests/functional ran against the
    docker stack — not run, no Docker available where I worked. The new fixture
    SQL was only parsed per dialect, never executed. Needs a run-functional run, and
    run-oracle for the Oracle half.

Checklist

  • Commit messages follow the Conventional Commits / Angular preset
  • Public API changes are documented in docs/ and CHANGELOG.md — the autodoc
    pages pick up the new model, but there is no migration note yet; CHANGELOG.md is
    generated by semantic-release
  • Coverage is maintained at or above the current threshold (80%)
  • No credentials, hostnames, or other sensitive data leaked in tests / fixtures
  • If this is a breaking change, the README / docs migration notes are updated

Notes for the reviewer

The fix: commit carries a BREAKING CHANGE: footer, so semantic-release will cut a
major release.

Two things left out on purpose:

  • The emitters still never emit UNIQUE constraints. An FK referencing a unique
    non-primary key would fail the same way — not the case in the dump that triggered this,
    but it is the same class of bug and deserves its own PR.
  • SQLite's PRAGMA foreign_key_list returns a NULL target column when the child clause
    omits the parent columns. That was already mishandled before this change and still is;
    resolving it needs a second pass over the parent's primary key.

BREAKING CHANGE: foreign keys moved from Column to Table

Column.foreign_key is removed; a table now exposes Table.foreign_keys, a list
of ForeignKey carrying paired columns/ref_columns. The per-column shape could
not represent a composite key: it was emitted one column at a time, and each
half referenced a key that is not unique on its own.

Iterate table.foreign_keys and read fk.columns / fk.ref_columns instead of
column.foreign_key and fk.column.
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   92.05%   92.09%   +0.03%     
==========================================
  Files          78       79       +1     
  Lines        3159     3161       +2     
==========================================
+ Hits         2908     2911       +3     
+ Misses        251      250       -1     
Files with missing lines Coverage Δ
db2sql/domain/model/column.py 100.00% <ø> (ø)
db2sql/domain/model/foreign_key.py 100.00% <100.00%> (ø)
db2sql/domain/model/table.py 100.00% <100.00%> (ø)
db2sql/domain/policy/dependency_order.py 97.72% <100.00%> (-0.15%) ⬇️
db2sql/infrastructure/emit/mssql/emitter.py 98.53% <100.00%> (-0.01%) ⬇️
db2sql/infrastructure/emit/postgres/emitter.py 98.98% <100.00%> (-0.01%) ⬇️
db2sql/infrastructure/persistence/foreign_keys.py 100.00% <100.00%> (ø)
db2sql/infrastructure/persistence/mssql/reader.py 98.21% <100.00%> (-0.07%) ⬇️
db2sql/infrastructure/persistence/mysql/reader.py 96.73% <100.00%> (-0.20%) ⬇️
db2sql/infrastructure/persistence/oracle/reader.py 97.91% <100.00%> (-0.07%) ⬇️
... and 2 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@TheCrab13
TheCrab13 merged commit 93548d7 into main Aug 20, 2026
7 checks passed
@TheCrab13
TheCrab13 deleted the fix/foreign-keys-definition-issue-for-composite-primary-key/no-ref branch August 20, 2026 09:18
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.

1 participant