Opened 10 months ago
Last modified 3 months ago
#35957 assigned New feature
Allow AutoFields in composite primary keys
Reported by: | Csirmaz Bendegúz | Owned by: | David Sanders |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Simon Charette, Lily Foote | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
This is a follow up to #373 (CompositePrimaryKey), #8576 (Multiple AutoFields in a model), #27452 (SerialField).
At the moment, AutoField
s must set primary_key=True
.
This makes it impossible to use them with CompositePrimaryKey
.
Multiple AutoFields in a model (#8576) may not be useful, but having an AutoField
be part of a composite primary key has its uses. It's good practice to use surrogate keys, even if the table has a composite primary key (should this surrogate key be an auto-incremented field is another discussion).
So I think we should allow AutoField
s to not set primary_key=True
if they are part of a composite primary key.
Note: SQLite doesn't support AutoFields in composite primary keys.
Change History (11)
follow-up: 2 comment:1 by Natalia Bidart, 10 months ago
Triage Stage: | Unreviewed → Accepted |
---|
Hello Ben, thank for your creating this follow up ticket. I'm accepting this because of what was discussed and agreed in this Forum comment, but I do have a clarification question: in the Forum conversation, I understand it boiled down to two options:
Item (1) is tracked in ticket-8576, which is open but in a "Design Decision Needed" triage state. Allowing primary_key=False in an AutoField would enable all DB backends except SQLite to use such a field as part of a composite primary key. However, no backend except PostgreSQL supports having more than one AutoField. To progress this ticket, we need someone to take ownership and propose an outline for the implementation, so it can be moved to "Accepted" and enter the review process. Given the nature of the change, I fear this may be a longer and slower process.
On the other hand, I see value in having a way to define DB-level "counters" (independent of composite PKs), which is what item (2) addresses. Only PostgreSQL supports this, so I would prefer a PostgreSQL-specific field for this feature. While serial is no longer recommended, a GeneratedIdentityField in django.contrib.postgres, as suggested by Simon, seems like a promising alternative and could be more actionable in the short term. I recommend reopening ticket-27452 and repurposing it for this feature.
If I understand correctly, this ticket would be solving item (2) above, so we would be providing a Postgresql-specific field, correct?
in reply to: 1 comment:2 by Csirmaz Bendegúz, 10 months ago
No, it's the opposite. The goal is to make AutoField
s work with composite primary keys. It's not Postgres-specific, this would work with all database backends (except SQLite).
The difference between #8576 and this is that this ticket won't allow multiple AutoField
s. A model can only have one AutoField
, and that AutoField
must be a primary key, or must be part of a composite primary key.
e.g.:
class Foo(models.Model): pk = models.CompositePrimaryKey("bar", "id", primary_key=True) bar = models.CharField(max_length=255) id = models.AutoField()
comment:3 by Natalia Bidart, 10 months ago
Cc: | Simon Charette Lily Foote added |
---|
Thank you Ben for clarifying. This seems to be outside what we discussed in the forum. Do you have more resources to point me to understand the bigger picture? I was assuming that our agreement and path forward with AutoField
s was described in the referenced forum post but before proceeding I would like to have the full context.
comment:4 by Csirmaz Bendegúz, 10 months ago
I think it aligns with what we discussed on the forum?
We keep the one AutoField per model restriction we currently have, but we'll allow AutoField
to be part of a composite primary key.
I don't think we should allow multiple AutoField
s, since only Postgres would support that and there's no strong use case for having multiple IDENTITY columns in a table. If consensus on this changes, then #8576 can be re-opened any time as it doesn't conflict with this ticket.
While the multiple AutoFields topic is contentious, everyone seems to agree that AutoField in composite primary keys should be added.
comment:5 by Csirmaz Bendegúz, 10 months ago
related forum topic:
https://forum.djangoproject.com/t/autofield-in-compositeprimarykey/37334
comment:6 by Sarah Boyce <42296566+sarahboyce@...>, 9 months ago
In bfcb340:
Refs #373 -- Removed unused composite pk code in SQLInsertCompiler.
This logic could only be exercised if the composite primary key included an
AutoField but it's not allowed yet (refs #35957).
It was also slightly broken as it expected the AutoField to always be the first
member of returning_fields.
comment:7 by Sarah Boyce <42296566+sarahboyce@...>, 9 months ago
comment:8 by Csirmaz Bendegúz, 6 months 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:9 by Mahmoud Nasser, 4 months ago
Could you assign this to me I would like to work on this one. Thanks
comment:10 by David Sanders, 3 months ago
Has patch: | set |
---|---|
Owner: | set to David Sanders |
Status: | new → assigned |
I have a PR up but I need clarification on a couple of things (see comments in PR) - but mainly how do we get the test runner to skip checks for a model if said model has requires_db_features
set. 🤔
comment:11 by David Sanders, 3 months ago
Patch needs improvement: | set |
---|