Opened 7 months ago

Last modified 2 weeks ago

#35956 new New feature

Add composite foreign keys

Reported by: Csirmaz Bendegúz Owned by:
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Csirmaz Bendegúz, Mariusz Felisiak, Clifford Gama, David Sanders Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This is a follow up to #373 (CompositePrimaryKey).

Now that composite primary keys are merged, it would be great to be able to create foreign keys referencing them through the Django ORM.

My proposal is to add 2 parameters to ForeignKey: from_fields and to_fields.
They would map to the underlying ForeignObject's from_fields, to_fields parameters.

If a ForeignKey has multiple fields, it acts as a virtual field, meaning it doesn't create a database column automatically.

Change History (11)

comment:1 by Sarah Boyce, 6 months ago

Triage Stage: UnreviewedAccepted

in reply to:  description ; comment:2 by Mariusz Felisiak, 6 months ago

Replying to Csirmaz Bendegúz:

My proposal is to add 2 parameters to ForeignKey: from_fields and to_fields.

Do we need these parameters? CompositePrimaryKey is always a primary key so its fields should be detected automatically, IMO, the following example should work:

class Release(models.Model):
    pk = models.CompositePrimaryKey("version", "name")
    version = models.IntegerField()
    name = models.CharField(max_length=20)


class RefRelease(models.Model):
    release = models.ForeignKey("Release", models.CASCADE)

comment:3 by Mariusz Felisiak, 6 months ago

Cc: Mariusz Felisiak added

in reply to:  2 ; comment:4 by Csirmaz Bendegúz, 6 months ago

Replying to Mariusz Felisiak:

Do we need these parameters? CompositePrimaryKey is always a primary key so its fields should be detected automatically, IMO, the following example should work:

class Release(models.Model):
    pk = models.CompositePrimaryKey("version", "name")
    version = models.IntegerField()
    name = models.CharField(max_length=20)


class RefRelease(models.Model):
    release = models.ForeignKey("Release", models.CASCADE)

The issue with this example is RefRelease must have 2 fields matching the CompositePrimaryKey (an IntegerField() and a CharField(max_length=20)).

So release would need to create 2 fields implicitly and I think that's too restrictive.

We should allow sharing fields between composite foreign keys.

For example, in a multi-tenant app where each model has a CompositePrimaryKey("tenant_id", "id"), the tenant_id field should be shared between composite foreign keys.

Let me know what you think about this, I'm happy to consider alternatives.

Last edited 6 months ago by Csirmaz Bendegúz (previous) (diff)

comment:5 by Clifford Gama, 2 months ago

Cc: Clifford Gama added

comment:6 by Csirmaz Bendegúz, 8 weeks ago

I don't have the time to work on this right now, if someone wants to pick it up, feel free to do so

Last edited 8 weeks ago by Csirmaz Bendegúz (previous) (diff)

in reply to:  4 comment:7 by Clifford Gama, 7 weeks ago

Replying to Csirmaz Bendegúz:

Replying to Mariusz Felisiak:

Do we need these parameters? CompositePrimaryKey is always a primary key so its fields should be detected automatically, IMO, the following example should work:

class Release(models.Model):
    pk = models.CompositePrimaryKey("version", "name")
    version = models.IntegerField()
    name = models.CharField(max_length=20)


class RefRelease(models.Model):
    release = models.ForeignKey("Release", models.CASCADE)

The issue with this example is RefRelease must have 2 fields matching the CompositePrimaryKey (an IntegerField() and a CharField(max_length=20)).

So release would need to create 2 fields implicitly and I think that's too restrictive.

We should allow sharing fields between composite foreign keys.

I'm +1 on sharing fields, but I think that Felix's suggestion should work as the default.

In simple cases, having the foreign key automatically infer and create the needed fields is great developer experience — it keeps things intuitive and close to how ForeignKey works today.

That said, we should definitely allow shared fields. For example, if a model already has a tenant_id FK to Tenant, and also needs to reference a model with a composite PK like (tenant_id, id), it should be able to reuse the same tenant_id field in both relationships.

So if possible, maybe we should have implicit field creation as default, and to|from_fields as an opt-in for more advanced use cases

comment:8 by Clifford Gama, 7 weeks ago

Thinking about it further... sharing fields only work when the tenant in both relationships is always the same — which might not always be true and may possibly lead to data integrity issues. For example, a model might have a direct FK to Tenant, and also reference another model with a composite PK like (tenant_id, id). Even though both use tenant_id, they could point to different tenants unless explicitly constrained. Pointing to the same field here would actually introduce a bug

I suppose this weakens the case for shared fields. It can be useful, but in very specific cases, like, say modeling tight tenant-scoped relationships — i.e., when you know the tenant in both FKs is always the same and want to enforce that via shared fields.

in reply to:  8 comment:9 by Csirmaz Bendegúz, 7 weeks ago

Replying to Clifford Gama:

In simple cases, having the foreign key automatically infer and create the needed fields is great developer experience — it keeps things intuitive and close to how ForeignKey works today.

I think I agree, but it's going to be a lot more complex to implement

It can be useful, but in very specific cases, like, say modeling tight tenant-scoped relationships — i.e., when you know the tenant in both FKs is always the same and want to enforce that via shared fields.

Yeah that's what I meant, in a multi-tenant database you never want one tenant's records to point to another tenant's records

comment:10 by David Sanders, 2 weeks ago

Just FYI to folks subscribed to this ticket:

In addition to having composite FKs for multitenancy, there's also the new temporal PK/FK feature in Postgres 18 (currently beta) that will be useful for declaring temporal tables: https://www.depesz.com/2024/10/03/waiting-for-postgresql-18-add-temporal-foreign-key-contraints/

tl;dr it adds the new PERIOD qualifier to FKs for declaring timestamp range constraints (using contains), which will likely be used in a composite key alongside the record identifier.

Multitenancy I already use frequently and I anticipate I'll make quite good use of temporal keys :)

Last edited 2 weeks ago by David Sanders (previous) (diff)

comment:11 by David Sanders, 2 weeks ago

Cc: David Sanders added
Note: See TracTickets for help on using tickets.
Back to Top