Skip to content

fix(relations): support models whose primary key is a OneToOneField - #2299

Open
d3vyce wants to merge 3 commits into
tortoise:developfrom
d3vyce:fix/o2o-primary-key
Open

d3vyce wants to merge 3 commits into
tortoise:developfrom
d3vyce:fix/o2o-primary-key

Conversation

@d3vyce

@d3vyce d3vyce commented Oct 6, 2026 •

Copy link
Copy Markdown

Description

Fixes three bugs with a model whose primary key is a OneToOneField. Both the relation and its <field>_id backing field are flagged pk, and these code paths took the relation, which has no column:

class User(Model):
    name = fields.CharField(50)

class Profile(Model):
    user = fields.OneToOneField("models.User", primary_key=True, related_name="profile")
    bio = fields.TextField()

class Post(Model):
    profile = fields.ForeignKeyField("models.Profile", related_name="posts")

# before: the "post" table has no "profile_id" column, the link is never written or read back
await Profile.get(pk=1)                          # FieldError: Unknown filter param 'pk'
await Profile.filter(user__name="ada").delete()  # KeyError: 'user' (same for update())

A ForeignKeyField to such a model now targets the backing field. Relations are set up in model name order, so when Post comes first, Profile.user is now initialized first, with or without to_field="user_id". That is also what makemigrations now writes: it wrote to_field='user', and the migrated table had no column either. A to_field naming a OneToOneField, as those migrations do, now resolves to its backing field. In that case Profile's table is named early, as Profile.user names its reverse relation after its table; other table names are assigned as before.

The pk filters are registered when the backing field becomes the primary key, and _pk_in_subquery() (#2269) selects _meta.db_pk_column. One-to-one fields referencing each other in a cycle now raise ConfigurationError instead of creating tables without columns.

Motivation and Context

I found no issue for these; related to #1771 / #1783 (many-to-many on such a model). A one-to-one primary key extends a table row for row, and other tables reference the extension: their foreign key silently stored nothing.

How Has This Been Tested?

New tests in tests/test_relations.py (foreign keys from models set up before and after the target, to_field naming the backing field or a one-to-one, pk filters), tests/test_queryset.py (delete and update through a related field), tests/test_table_name.py (M2M through names unchanged with a table_name_generator) and tests/model_setup/ (cycle error, foreign key from a later init_models() call). The full suite passes on SQLite; CI covers PostgreSQL, MySQL and MSSQL.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

This fix has been partially implemented by Claude Opus 5.5. All modifications made by the AI have been reviewed by me.

@d3vyce
d3vyce force-pushed the fix/o2o-primary-key branch from 85d2908 to 39ac9e9 Compare October 6, 2026 21:01
@codspeed

codspeed Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 21 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 24 untouched benchmarks


Comparing d3vyce:fix/o2o-primary-key (2680985) with develop (cec625e)

Open in CodSpeed

@waketzheng

Copy link
Copy Markdown
Contributor

The fix addresses the reported symptoms, but a few things feel under-specified for framework-level code. Flagging the ones I'd want pinned down before merge:

1. MetaInfo registers "pk" filters inside the field loop

if value.pk and value.has_db_field:
    self._filters.update(get_filters_for_field(field_name="pk", field=value, ...))

Every field with pk and has_db_field overwrites the previous "pk" entry. It happens to work because only the _id backing field satisfies has_db_field in the O2O-pk case, but it's an order-dependent implicit contract with no assertion. If a model ever has two such fields, the last one silently wins. This belongs in finalise_fields() as a single lookup, not a repeated update in the loop.

2. Table names are now assigned before relations, globally

The loop runs for every model, not just O2O-pk targets. Default related_name and FK constraint names depend on table names, so this can shift naming for unrelated models. There's no regression test asserting that plain models' db_table, reverse names, and constraint names are unchanged. That test is worth adding.

3. Recursive init_fk_o2o_field has no partial-side-effect protection

The initialized guard stops re-entry for the same (model, field), but a cyclic O2O-pk pair can still leave one side in a half-initialized state (fields_map updated, backing field not yet added). There's no test for a two-way O2O-pk cycle. Either add one or assert that partial state can't be observed.

4. _pk_in_subquery silently skips pk fields without a column

if field.pk and name in projection

This is correct for O2O-pk today, but if fields_db_projection is ever incomplete, or if a model has multiple pk fields with columns (composite key), the generated pk IN (...) semantics change silently. An assert len(pk_cols) == 1 or a comment stating the assumption would make this explicit.

5. to_field="author_id" exposes the backing-field name

The test pins to_field="author_id", which is an internal auto-generated name. Ideally to_field="author" would resolve to the backing field. As written, users are expected to know the internal naming.

6. Backend coverage

MySQL and MSSQL are left to CI, but this touches FK column generation and table-name assignment timing — both dialect-sensitive. Please confirm those jobs run the new tests, not just the existing suite.

7. Tests depend on model-name ordering

FkToO2oPk (F < O) and PointsToO2oPk (P > O) rely on _init_relations iterating in name order. That's fine as a test technique, but the dependency should be stated in the test comment or the ordering made explicit, otherwise a future change to init order will break these tests for reasons unrelated to O2O-pk.

@d3vyce
d3vyce force-pushed the fix/o2o-primary-key branch from 2680985 to be3899d Compare October 24, 2026 19:45
@d3vyce
d3vyce force-pushed the fix/o2o-primary-key branch from be3899d to aafc452 Compare October 24, 2026 19:48
@d3vyce

d3vyce commented Oct 24, 2026

Copy link
Copy Markdown
Author

Thanks for the review! Addressed in aafc452 (also rebased on develop):

  1. The pk filters are now registered once, in _init_relations, where the backing field becomes the pk. A model has a single pk (_parse_custom_pk).
  2. Good catch: with a table_name_generator, default M2M through names changed. I reverted the global pass. Only a one-to-one pk target is named early now, and a test pins the existing naming.
  3. A one-to-one cycle now raises ConfigurationError (before, it silently created tables without columns, on develop too). Also fixed: a second init_models() call re-initializing the target.
  4. _pk_in_subquery now uses _meta.db_pk_column, and the Tuple branch is removed.
  5. to_field="author" now resolves to the backing field, for any OneToOneField.
  6. The MySQL/MSSQL jobs run the whole tests/ directory, so the new tests ran there, and they're green.
  7. The test model docstring now says the ordering comes from dir(module).

This branch has not been deployed

No deployments
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.

2 participants