Keep the declared source_field when deconstructing a FK - #2284
waketzheng merged 5 commits into
Conversation
Apps._init_relations() moves a ForeignKeyField's declared source_field onto the generated `<field>_id` backing field, then reuses the attribute on the relation field to hold that backing field's name. Model._meta and the migration writer both depend on that. deconstruct() read the attribute as if it were still the column name, so makemigrations wrote `<field>_id` instead of the declared column. A schema built from migrations then disagreed with one built by generate_schemas(), and neither path reported an error. Read the column name back off the backing field instead. Fields that declared no source_field are unaffected: the backing field holds the `<field>_id` default, which is what was already emitted. Fixes tortoise#2283 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the PR! The fix is well-targeted and the root cause is clearly explained. A few suggestions before merge:
|
Reviewer feedback on tortoise#2284: - The tests keyed off SourceFields, whose shape they do not own. Add FKSourceFields, used only by these tests, so a later refactor of another test model cannot break them. - Add a case for a declared source_field equal to the field name itself, confirming the backing field name does not override it. - Add a case calling deconstruct() on a field that was never attached to a model, covering the `model` guard. Also assert every deconstructed value is a real column in _meta.db_fields, so the reported name is checked against the schema rather than restated. No change to the fix itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the review. All three addressed in e330acd. The fix itself is unchanged — this is test work only. 1. Test robustness Added
I went with a dedicated model rather than deriving the expected value at runtime: deriving it from 2. Covered by 3. Pre-initialisation deconstruct Covered by Also added
Verification
|
|
Thanks for the update. The fix looks good overall, and the root cause analysis is clear. A few remaining points:
We use PR number. Please use
I noticed that
The newly added Everything else looks good: the fix logic, the CHANGELOG entry placement, and the test coverage. Thanks for including tests for both the FK and O2O cases. |
- CHANGELOG now references the PR (tortoise#2284) rather than the issue. - Rename `backing` to `backing_field` so it reads as the `<field>_id` field the column name was moved onto. - Group the deconstruct tests into `TestDeconstructSourceField`, following the class style used in tests/test_source_field.py. No change to the fix itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — all three are done in 3d5e809. The fix itself is unchanged. 1. CHANGELOG number Now 2. Renamed in all three places. You are right that 3. Test grouping The file is purely function-based, so there was no existing class to follow. I used the class style from I also shortened the method names, as the class name already carries "deconstruct":
One more change The class docstring referenced Verification
Happy to squash the three commits into one if you would prefer that before merge. |
|
Nice. A thing of beauty is a joy for ever. |
Description
ForeignKeyFieldandOneToOneFieldlose a declaredsource_fieldwhen they are deconstructed, somakemigrationswrites the wrong column name.Apps._init_relations()moves the declaredsource_fieldonto the generated<field>_idbacking field, and then reuses the attribute on the relation field to hold that backing field's name.Model._metaandmigrations/writer.pyboth depend on that. Butdeconstruct()read the same attribute as if it still held the column name.This reads the column name back off the backing field instead.
Fields that declare no
source_fieldare unaffected. Their backing field holds the<field>_iddefault, which is exactly what was emitted before.Motivation and Context
Fixes #2283.
Given this model:
generate_schemas()creates the columnaccount_id, which is correct.makemigrationswritessource_field='consumer_id', so a database built from that migration getsconsumer_id.The two paths disagree and neither reports an error. A project that uses
generate_schemas()in development and migrations in deployment ends up with a different column name in each, and any hand-written SQL or backfill then works in one environment and fails in the other.How Has This Been Tested?
Four new tests in
tests/fields/test_fk.py. They use the existingSourceFieldsandEventmodels, so no new test models were needed:ForeignKeyFieldwithsource_fielddeconstructs to the declared columnOneToOneFieldwithsource_fielddoes the sameForeignKeyFieldwithoutsource_fieldstill gets the<field>_iddefault_meta.db_fieldsThree of the four fail on
developand pass with this change. The fourth covers the default path and passes either way, to catch a regression there.Full suite on SQLite: 1932 passed, 159 skipped, 2 xfailed, 0 failed.
Two modules were skipped locally because they need
asyncmy/aiomysql, which I do not have installed:tests/backends/test_connection_params.pyandtests/test_default.py. Neither touches this code path.ruff format --check,ruff checkandmypyare all clean on the changed files. The threeruff checkwarnings intortoise/fields/relational.pyare pre-existing ondevelop.Environment: Python 3.14, SQLite.
Checklist:
🤖 Generated with Claude Code