-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed #373 -- Added CompositePrimaryKey. #18056
base: main
Are you sure you want to change the base?
Conversation
I was trying out this exciting branch and ran into this error when running a test:
The issue stems from the use of
Curious if anyone ran into this as well. Edited for traceback:
So, this is part of |
Thanks for testing and reporting the issue @grjones! Indeed, I forgot to handle this use case. I'll look into it this week. |
6a26b19
to
c75dcdd
Compare
c75dcdd
to
4be1c68
Compare
@grjones, FYI I pushed the fix |
Nice! I hope this gets merged in soon. Your branch has been working great for me. |
I may have found one other small issue. When adding a regular |
@grjones , thanks, I appreciate the feedback, I'll look into it. If a model defines |
I'll see if I can give you a solid failing test. My "unique constraint" phrasing might not be exactly right. But ultimately, I believe Django queries the DB first to see if the new object's PK already exists and throws a validation error. The composite key logic doesn't seem to be doing that and so an unhandled IntegrityError is raised instead. |
@grjones , sorry for the late reply, I've been busy last week. Could you give me more specifics? What's the error message you expect? |
Actually, I think it's mostly ok. I was using Django Spanner and it's just not quite working with composite keys and will need to be fixed there. I wrote this and it passed. It probably shouldn't say
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start!
I've left a bunch of ideas for improvement. Feel free to push back if you think I'm wrong about anything.
Thank you so much for taking the time to review my changes @LilyFoote !
|
I don't feel strongly that this is better or worse than another option here, so happy to go with what you think is best.
I like your tests quite a bit - they're pretty readable and comprehensive. The main issue I have with them is that they're written for specific databases instead of for generic database features. Where possible Django strongly prefers to test based on features because then the tests apply to as many databases as possible (including third party database libraries). I think the asserts of the actual SQL might be a bit tricky to adapt though, so we might need a different way to check what they're checking. Also, after I reviewed yesterday, I thought of some more things:
|
tests/composite_pk/models/tenant.py
Outdated
user = models.ForeignObject( | ||
User, | ||
on_delete=models.CASCADE, | ||
from_fields=("tenant_id", "user_id"), | ||
to_fields=("tenant_id", "id"), | ||
related_name="+", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think this should be a stretch goal to get it working. See the comment above about MultiColSource
.
django/db/models/fields/composite.py
Outdated
return compiler.compile(WhereNode(exprs, connector=OR)) | ||
|
||
|
||
class Cols(Expression): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is an opportunity to merge this TuplesIn
, Cols
, and friends logic with MultiColSource
so it's less of an 👽. They both do very similar thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @charettes , I'll need to look into this, I wasn't aware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged Cols
with MultiColSource
(ad51da4) however, I'm not sure this is correct.
As far as I understand, MultiColSource
was meant to represent columns in a JOIN, and as such, it has a sources
field. Cols
, on the other hand, was meant to represent a list of columns and it doesn't need a sources
field. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted back to Cols
. Please resolve if you agree.
django/db/backends/base/schema.py
Outdated
# to the table definition. | ||
# It's expected 'primary_key=True' isn't set on any fields (see E043). | ||
pk = model._meta.pk | ||
if hasattr(pk, "columns"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might have been answered somewhere else already, so please ignore the question if it was. I am wondering if it would make sense to handle single column primary keys like this as well. What would be the upside of having different objects (?) here for single vs multi-columns fields? Or do we gain some backwards compat wins here by having it like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we've considered this yet. My feeling without digging in is that it's not necessary for this feature, but it might be a nice follow-up refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if we want to do it in a follow-up refactor we should still ensure that it ends up in the same release. While Meta is probably not documented, changes there will still hurt more advanced 3rd party packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference is, primary_key=True
creates the primary key inline, while Meta.primary_key
creates it as a table level constraint.
CREATE TABLE foo (
id INTEGER PRIMARY KEY
)
vs.
CREATE TABLE foo (
id INTEGER,
PRIMARY KEY (id)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik all db backends support both inline and table-level primary keys, so there's no real difference in practice. It would take some work to move away from inline primary keys and it's out of scope for this ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I am second guessing if Meta.primary_key
is really necessary now.
You do raise the entirely valid point, why is it None
if primary_key=True
is set on a field? As a user, I would expect it to always store some info related to the primary key. And I would expect this info to be the source of truth.
So, how about we consider the following API instead?
class Foo(models.Model):
primary_key = models.CompositePrimaryKey("tenant_id", "id")
tenant_id = models.IntegerField()
id = models.IntegerField()
This is how @LilyFoote did it too in the previous PR, I just thought adding a Meta option is more user friendly.
But if it leads to confusion, I'm not sure it's worth it anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that explicit approach certainly feels less magical (ie we don't have to add an extra field to the class and the user can name it whatever they want). All in all it feels more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this feels like the right API to me given this discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know what you think, and thanks for the review @apollo13 , I appreciate it! ❤️
Hey @csirmazbendeguz, thank you for the amazing work out there! I was trying to test this branch on my local with SQLite and realised a few things:
Again thanks for this amazing initiative! 🚀 |
django/db/models/fields/composite.py
Outdated
def get_lookup(self, lookup_name): | ||
if lookup_name == "exact": | ||
return TupleExact | ||
elif lookup_name == "gt": | ||
return TupleGreaterThan | ||
elif lookup_name == "gte": | ||
return TupleGreaterThanOrEqual | ||
elif lookup_name == "lt": | ||
return TupleLessThan | ||
elif lookup_name == "lte": | ||
return TupleLessThanOrEqual | ||
elif lookup_name == "in": | ||
return TupleIn | ||
|
||
raise NotImplementedError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason to not to use CompositePrimaryKey.register_lookup
as we do for other fields (e.g. JSONField
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@csirmazbendeguz have you tried this? I think it would be nicer if it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's a good idea, I'll try
Thanks a lot for the review @omerfarukabaci ! I'll take a look |
@omerfarukabaci , I pushed the changes to support this, but note that filtering on reverse relations is one of those "gotchas" in Django, it may not produce the results you expect. EDIT: I mean it might return duplicates, you probably already know this, I'm just mentioning it just in case. |
Yes, I recently changed the API to |
@csirmazbendeguz Thanks for your answers, now the above issues seem like fixed, created migration is correct and reverse relation lookup is working as expected. Thank you! 🚀 While I was testing it further with the exact same models, I realized another issue: class TestCompositeFks(TestCase):
def test_composite_fks(self):
author = Author.objects.create(name="Author")
Book.objects.create(author=author, title="Title")
author = Author.objects.annotate(book_count=Count("books")).get()
assert author.book_count == 1 This test fails with the following error:
Executed SQL is as following: SELECT
"books_author"."id",
"books_author"."name",
COUNT("books_book"."author_id", "books_book"."title") AS "book_count"
FROM
"books_author"
LEFT OUTER JOIN "books_book" ON ("books_author"."id" = "books_book"."author_id")
GROUP BY
"books_author"."id",
"books_author"."name" If we could change the parameter we pass to the COUNT("books_book"."author_id" || '-' || "books_book"."title") it should work fine (if I am not missing something), with the exception that for some databases we need to use Note: I am not sure if concatenation works between every data type that is allowed to be a primary key, although this could be considered as an edge case. |
Thanks @omerfarukabaci , these bug reports are very helpful. Yes, I haven't considered annotations with multi-column pks. I'll look into this. |
Trac ticket number
ticket-373
Branch description
This branch adds the
CompositePrimaryKey
field. If present, Django will create a composite primary key.Proposal
Previous PR
Composite FK
Serial Fields
Checklist
main
branch.