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 CompositeField-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.
django/db/models/base.py
Outdated
if cls._meta.primary_key and any( | ||
field for field in cls._meta.fields if field.primary_key | ||
): | ||
errors.append( | ||
checks.Error( | ||
"primary_key=True must not be set if Meta.primary_key " | ||
"is defined.", | ||
obj=cls, | ||
id="models.E042", | ||
) | ||
) |
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.
It might be nice to call out specifically which field has primary_key
incorrectly set.
django/db/models/fields/composite.py
Outdated
raise ValueError( | ||
"The right-hand side of the 'exact' lookup must be an iterable" | ||
) | ||
if len(list(self.lhs)) != len(list(self.rhs)): |
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 should need the list
calls here. Is there a particular type you were thinking of here that doesn't implement __len__
?
if len(list(self.lhs)) != len(list(self.rhs)): | |
if len(self.lhs) != len(self.rhs): |
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, I simplified it, thanks!
django/db/models/fields/composite.py
Outdated
raise ValueError( | ||
"The left-hand side of the 'exact' lookup must be an instance of Cols" | ||
) | ||
if not isinstance(self.rhs, Iterable): |
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 Iterable
is the right interface here - it doesn't check for __getitem__
based iteration or __len__
. Also, it allows rhs
to be a generator or other lazy iterator, which would be exhaustible.
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, could be restricted to tuple
s and list
s, I think that should be fine.
django/db/models/fields/composite.py
Outdated
lhs_len = len(tuple(self.lhs)) | ||
if not all(lhs_len == len(tuple(vals)) for vals in self.rhs): |
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.
lhs_len = len(tuple(self.lhs)) | |
if not all(lhs_len == len(tuple(vals)) for vals in self.rhs): | |
lhs_len = len(self.lhs) | |
if not all(lhs_len == len(vals) for vals in self.rhs): |
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 for this, you're right, it can be simplified.
django/db/models/fields/composite.py
Outdated
def __iter__(self): | ||
return iter(self.get_source_expressions()) |
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 think we should add __len__
too - we can do a cheap length check by checking len(self.targets)
.
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.
Yep, good idea - added!
tests/composite_pk/test_get.py
Outdated
def test_get_tenant_by_pk(self): | ||
test_cases = [ | ||
{"id": self.tenant.id}, | ||
{"pk": self.tenant.pk}, | ||
] | ||
|
||
for lookup in test_cases: | ||
with self.subTest(lookup=lookup): | ||
with CaptureQueriesContext(connection) as context: | ||
obj = Tenant.objects.get(**lookup) | ||
|
||
self.assertEqual(obj, self.tenant) | ||
self.assertEqual(len(context.captured_queries), 1) | ||
if connection.vendor in ("sqlite", "postgresql"): | ||
t = Tenant._meta.db_table | ||
self.assertEqual( | ||
context.captured_queries[0]["sql"], | ||
f'SELECT "{t}"."id" ' | ||
f'FROM "{t}" ' | ||
f'WHERE "{t}"."id" = {self.tenant.id} ' | ||
f"LIMIT {MAX_GET_RESULTS}", | ||
) |
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 need this test, since Tenant
itself doesn't use a CompositeField
.
tests/composite_pk/test_get.py
Outdated
with CaptureQueriesContext(connection) as context: | ||
obj = User.objects.only("pk").get(**lookup) | ||
|
||
self.assertEqual(obj, self.user) |
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.
Maybe we should have more than just one User
in the database? Perhaps one with a different id
and another with a different tenant_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.
Sure, there's no hurt in adding more test data.
tests/composite_pk/test_update.py
Outdated
with CaptureQueriesContext(connection) as context: | ||
result = User.objects.filter(pk=self.user.pk).update(id=8341) | ||
|
||
self.assertEqual(result, 1) | ||
self.assertFalse(User.objects.filter(pk=self.user.pk).exists()) |
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 think this would be a bit clearer:
with CaptureQueriesContext(connection) as context: | |
result = User.objects.filter(pk=self.user.pk).update(id=8341) | |
self.assertEqual(result, 1) | |
self.assertFalse(User.objects.filter(pk=self.user.pk).exists()) | |
old_pk = self.user.pk | |
with CaptureQueriesContext(connection) as context: | |
result = User.objects.filter(pk=self.user.pk).update(id=8341) | |
self.assertEqual(result, 1) | |
self.assertFalse(User.objects.filter(pk=old_pk).exists()) |
By storing the old_pk
we don't implicitly rely on self.user.pk
being stale.
tests/composite_pk/tests.py
Outdated
self.assertIsInstance(self.tenant.pk, int) | ||
self.assertGreater(self.tenant.id, 0) | ||
self.assertEqual(self.tenant.pk, self.tenant.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.
Since Tenant
doesn't have a CompositeField
I don't think these asserts add any value.
self.assertIsInstance(self.tenant.pk, int) | |
self.assertGreater(self.tenant.id, 0) | |
self.assertEqual(self.tenant.pk, self.tenant.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.
Fair enough, I removed these lines.
tests/composite_pk/tests.py
Outdated
) | ||
|
||
def test_error_on_pk_conflict(self): | ||
with self.assertRaises(Exception): |
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.
We should be more specific about the exception type than Exception
.
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 split this into two tests with IntegrityError
(as the second assert was actually raising a transaction management error).
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?
django/db/models/fields/composite.py
Outdated
assert all(isinstance(expr, Col) for expr in exprs) | ||
assert len(exprs) == len(self.targets) |
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.
Shouldn't this perform assigments? Or maybe it's meant to happen during clone relabeling?
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'm not sure, but as far as I've seen, this function is only called from the resolve_expression
function. I'll check more in detail as I'm not sure about the significance of this function, but it looks like it works without performing any assignments yes.
django/db/models/sql/compiler.py
Outdated
select_mask_fields = set() | ||
for field in select_mask: | ||
select_mask_fields.update( | ||
field.fields if isinstance(field, CompositePrimaryKey) else [field] |
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 guess that'll eventually be some form of check for field.composite
once/when we support something else than only CompositePrimaryKey
.
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'm not a fan of isinstance
checks either, I suppose we could check if field
has a fields
attribute to make this a little more versatile.
tests/composite_pk/test_filter.py
Outdated
self.assertEqual( | ||
context.captured_queries[0]["sql"], | ||
f'SELECT "{c}"."tenant_id", "{c}"."id", "{c}"."user_id" ' | ||
f'FROM "{c}" ' | ||
f'WHERE ("{c}"."tenant_id" = {self.tenant.id} ' | ||
f'AND "{c}"."user_id" = 2491) ' | ||
f'ORDER BY "{c}"."tenant_id", "{c}"."id" ASC', | ||
) |
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.
We'll definitely want to avoid SQL capture of this kind as it prevents other backends from being created.
We should assert against the result of the SQL and not the SQL itself.
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 see, noted
tests/composite_pk/test_filter.py
Outdated
context.captured_queries[0]["sql"], | ||
f'SELECT "{c}"."tenant_id", "{c}"."id", "{c}"."user_id" ' | ||
f'FROM "{c}" ' | ||
f'WHERE ("{c}"."tenant_id" = {self.tenant.id} ' | ||
f'AND "{c}"."user_id" = 8316) ' | ||
f'ORDER BY "{c}"."tenant_id", "{c}"."id" DESC', | ||
) |
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.
Ditto
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 removed all SQL asserts
tests/composite_pk/test_filter.py
Outdated
self.assertEqual(qs.order_by("pk", "user").first(), objs[1]) | ||
self.assertEqual(qs.order_by("pk", "user").last(), objs[2]) | ||
self.assertEqual(qs.order_by("-pk", "user").first(), objs[2]) | ||
self.assertEqual(qs.order_by("-pk", "user").last(), objs[1]) |
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 should be testing every methods out there, simply testing order_by
should be enough unless there is specialized logic for pk
handling.
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, I removed this.
Trac ticket number
ticket-373
Branch description
Proposal
Previous PR
Checklist
main
branch.