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 , 6 months ago
Triage Stage: | Unreviewed → Accepted |
---|
follow-up: 4 comment:2 by , 6 months ago
comment:3 by , 6 months ago
Cc: | added |
---|
follow-up: 7 comment:4 by , 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.
comment:5 by , 2 months ago
Cc: | added |
---|
comment:6 by , 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
comment:7 by , 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 theCompositePrimaryKey
(anIntegerField()
and aCharField(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
follow-up: 9 comment:8 by , 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.
comment:9 by , 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 , 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 :)
comment:11 by , 2 weeks ago
Cc: | added |
---|
Replying to Csirmaz Bendegúz:
Do we need these parameters?
CompositePrimaryKey
is always a primary key so its fields should be detected automatically, IMO, the following example should work: