-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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-based Meta.primary_key. #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.
Trac ticket number
ticket-373
Branch description
This branch adds the
Meta.primary_key
option. If set, Django will create a composite primary key.Proposal
Previous PR
Checklist
main
branch.