Fixed #373 -- Added CompositeField-based Meta.primary_key. #18056

csirmazbendeguz · 2024-04-07T04:28:04Z

Trac ticket number

Branch description

class Tenant(models.Model):
    pass


class User(models.Model):
    tenant = models.ForeignKey(Tenant, on_delete=models.CASCADE)
    id = models.SmallIntegerField()

    class Meta:
        primary_key = ("tenant_id", "id")


class Comment(models.Model):
    tenant = models.ForeignKey(Tenant, on_delete=models.CASCADE)
    id = models.SmallIntegerField()
    user_id = models.SmallIntegerField()
    user = models.ForeignObject(
        User,
        on_delete=models.CASCADE,
        from_fields=("tenant_id", "user_id"),
        to_fields=("tenant_id", "id"),
        related_name="+",
    )

    class Meta:
        primary_key = ("tenant_id", "id")

Checklist

This PR targets the main branch.
The commit message is written in past tense, mentions the ticket number, and ends with a period.
I have checked the "Has patch" ticket flag in the Trac system.
I have added or updated relevant tests.
I have added or updated relevant docs, including release notes if applicable.
For UI changes, I have attached screenshots in both light and dark modes.

grjones · 2024-04-17T19:04:43Z

I was trying out this exciting branch and ran into this error when running a test:

<...>/lib/python3.12/site-packages/django/db/models/lookups.py:30: in __init__
    self.rhs = self.get_prep_lookup()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = TupleIn(<django.db.models.fields.composite.Cols object at 0x107560980>, <django.db.models.sql.query.Query object at 0x1074e23f0>)

    def get_prep_lookup(self):
        if not isinstance(self.lhs, Cols):
            raise ValueError(
                "The left-hand side of the 'in' lookup must be an instance of Cols"
            )
        if not isinstance(self.rhs, Iterable):
>           raise ValueError(
                "The right-hand side of the 'in' lookup must be an iterable"
            )
E           ValueError: The right-hand side of the 'in' lookup must be an iterable

The issue stems from the use of isnull like so:

MyModel.objects.filter(
    type_override__severity__isnull=False
).update(severity="high")

Curious if anyone ran into this as well.

Edited for traceback:

<...>
lib/python3.12/site-packages/django/db/models/sql/compiler.py:2080: in pre_sql_setup
    self.query.add_filter("pk__in", query)
lib/python3.12/site-packages/django/db/models/sql/query.py:1601: in add_filter
    self.add_q(Q((filter_lhs, filter_rhs)))
lib/python3.12/site-packages/django/db/models/sql/query.py:1617: in add_q
    clause, _ = self._add_q(q_object, self.used_aliases)
lib/python3.12/site-packages/django/db/models/sql/query.py:1649: in _add_q
    child_clause, needed_inner = self.build_filter(
lib/python3.12/site-packages/django/db/models/sql/query.py:1563: in build_filter
    condition = self.build_lookup(lookups, col, value)
lib/python3.12/site-packages/django/db/models/sql/query.py:1393: in build_lookup
    lookup = lookup_class(lhs, rhs)
lib/python3.12/site-packages/django/db/models/lookups.py:30: in __init__
    self.rhs = self.get_prep_lookup()

So, this is part of SQLUpdateCompiler and is coming from the update code path.

csirmazbendeguz · 2024-04-18T02:58:00Z

Thanks for testing and reporting the issue @grjones! Indeed, I forgot to handle this use case. I'll look into it this week.

csirmazbendeguz · 2024-04-20T00:52:41Z

@grjones, FYI I pushed the fix

grjones · 2024-04-20T01:25:37Z

@grjones, FYI I pushed the fix

Nice! I hope this gets merged in soon. Your branch has been working great for me.

grjones · 2024-04-22T16:54:36Z

I may have found one other small issue. When adding a regular primary_key=True on a single field, a unique constraint is added. But when using this branch, it becomes an IntegrityError instead. Adding a UniqueConstraint on the composite fields is a work-a-round but ideally would be captured in this PR. Imo, this PR is sooooo close. I'm excited for it to be merged in.

csirmazbendeguz · 2024-04-23T04:44:18Z

@grjones , thanks, I appreciate the feedback, I'll look into it. If a model defines Meta.primary_key, defining primary_key=True on a field should not be possible - could you give me a code example so I know how to reproduce the issue? I didn't know Django added unique constraints to primary keys, I'll check, but isn't that redundant?

grjones · 2024-04-23T05:39:35Z

@grjones , thanks, I appreciate the feedback, I'll look into it. If a model defines Meta.primary_key, defining primary_key=True on a field should not be possible - could you give me a code example so I know how to reproduce the issue? I didn't know Django added unique constraints to primary keys, I'll check, but isn't that redundant?

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.

csirmazbendeguz · 2024-05-01T08:48:18Z

@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?

grjones · 2024-05-02T03:09:49Z

@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 Id though?

from django.core.exceptions import ValidationError
from django.test import TestCase

from .models import Tenant, User


class CompositePKCleanTests(TestCase):
    """
    Test the .clean() method of composite_pk models.
    """

    @classmethod
    def setUpTestData(cls):
        cls.tenant = Tenant.objects.create()

    def test_validation_error_is_raised_when_pk_already_exists(self):
        test_cases = [
            {"tenant": self.tenant, "id": 2412, "email": "user2412@example.com"},
            {"tenant_id": self.tenant.id, "id": 5316, "email": "user5316@example.com"},
            {"pk": (self.tenant.id, 7424), "email": "user7424@example.com"},
        ]
        expected = "{'id': ['User with this Id already exists.']}"
        for fields in test_cases:
            User.objects.create(**fields)
            with self.assertRaisesMessage(ValidationError, expected):
                User(**fields).clean()

LilyFoote

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.

LilyFoote · 2024-05-03T21:31:38Z

django/db/models/base.py

+        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",
+                )
+            )


It might be nice to call out specifically which field has primary_key incorrectly set.

LilyFoote · 2024-05-03T21:35:52Z

django/db/models/fields/composite.py

+            raise ValueError(
+                "The right-hand side of the 'exact' lookup must be an iterable"
+            )
+        if len(list(self.lhs)) != len(list(self.rhs)):


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__?

Suggested change

if len(list(self.lhs)) != len(list(self.rhs)):

if len(self.lhs) != len(self.rhs):

Yes, I simplified it, thanks!

LilyFoote · 2024-05-03T21:41:13Z

django/db/models/fields/composite.py

+            raise ValueError(
+                "The left-hand side of the 'exact' lookup must be an instance of Cols"
+            )
+        if not isinstance(self.rhs, Iterable):


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.

Yeah, could be restricted to tuples and lists, I think that should be fine.

LilyFoote · 2024-05-03T21:42:47Z

django/db/models/fields/composite.py

+        lhs_len = len(tuple(self.lhs))
+        if not all(lhs_len == len(tuple(vals)) for vals in self.rhs):


Suggested change

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):

Thanks for this, you're right, it can be simplified.

LilyFoote · 2024-05-03T21:46:01Z

django/db/models/fields/composite.py

+    def __iter__(self):
+        return iter(self.get_source_expressions())


I think we should add __len__ too - we can do a cheap length check by checking len(self.targets).

Yep, good idea - added!

LilyFoote · 2024-05-03T22:42:34Z

tests/composite_pk/test_get.py

+    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}",
+                    )


I don't think we need this test, since Tenant itself doesn't use a CompositeField.

LilyFoote · 2024-05-03T22:46:55Z

tests/composite_pk/test_get.py

+                with CaptureQueriesContext(connection) as context:
+                    obj = User.objects.only("pk").get(**lookup)
+
+                self.assertEqual(obj, self.user)


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?

Sure, there's no hurt in adding more test data.

LilyFoote · 2024-05-03T22:53:46Z

tests/composite_pk/test_update.py

+        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())


I think this would be a bit clearer:

Suggested change

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.

LilyFoote · 2024-05-03T22:55:58Z

tests/composite_pk/tests.py

+        self.assertIsInstance(self.tenant.pk, int)
+        self.assertGreater(self.tenant.id, 0)
+        self.assertEqual(self.tenant.pk, self.tenant.id)
+


Since Tenant doesn't have a CompositeField I don't think these asserts add any value.

Suggested change

self.assertIsInstance(self.tenant.pk, int)

self.assertGreater(self.tenant.id, 0)

self.assertEqual(self.tenant.pk, self.tenant.id)

Fair enough, I removed these lines.

LilyFoote · 2024-05-03T22:58:55Z

tests/composite_pk/tests.py

+        )
+
+    def test_error_on_pk_conflict(self):
+        with self.assertRaises(Exception):


We should be more specific about the exception type than Exception.

I split this into two tests with IntegrityError (as the second assert was actually raising a transaction management error).

csirmazbendeguz · 2024-05-04T03:31:50Z

Thank you so much for taking the time to review my changes @LilyFoote !
I have two questions:

If Meta.primary_key is defined, this PR will automatically add a composite field called primary_key to the model. What do you think about this approach? I felt like it was easier to handle the composite primary keys this way as we can run checks against the meta class instead of traversing the model's fields for a composite field.
I wrote a lot of tests testing the underlying queries made by the ORM. It makes a lot of sense to me, but I haven't seen this type of tests that much in the Django source code - do these tests look okay to you?

LilyFoote · 2024-05-04T12:13:30Z

If Meta.primary_key is defined, this PR will automatically add a composite field called primary_key to the model. What do you think about this approach?

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 wrote a lot of tests testing the underlying queries made by the ORM. It makes a lot of sense to me, but I haven't seen this type of tests that much in the Django source code - do these tests look okay to you?

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:

We should add migrations tests to make sure that adding/removing Meta.primary_key works correctly and that removing a field that's part of a primary key also does something appropriate.
We might want tests for composite keys in forms and the admin. Maybe there's other areas too that we need to check the interactions.

charettes · 2024-05-05T19:51:19Z

tests/composite_pk/models/tenant.py

+    user = models.ForeignObject(
+        User,
+        on_delete=models.CASCADE,
+        from_fields=("tenant_id", "user_id"),
+        to_fields=("tenant_id", "id"),
+        related_name="+",
+    )


yeah I think this should be a stretch goal to get it working. See the comment above about MultiColSource.

charettes · 2024-05-05T19:51:35Z

django/db/models/fields/composite.py

+        return compiler.compile(WhereNode(exprs, connector=OR))
+
+
+class Cols(Expression):


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.

Thanks @charettes , I'll need to look into this, I wasn't aware.

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?

…ion errors

charettes · 2024-05-10T03:15:33Z

django/db/models/fields/composite.py

+        assert all(isinstance(expr, Col) for expr in exprs)
+        assert len(exprs) == len(self.targets)


Shouldn't this perform assigments? Or maybe it's meant to happen during clone relabeling?

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.

charettes · 2024-05-10T03:19:19Z

django/db/models/sql/compiler.py

+        select_mask_fields = set()
+        for field in select_mask:
+            select_mask_fields.update(
+                field.fields if isinstance(field, CompositePrimaryKey) else [field]


I guess that'll eventually be some form of check for field.composite once/when we support something else than only CompositePrimaryKey.

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.

charettes · 2024-05-10T03:22:07Z

tests/composite_pk/test_filter.py

+            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',
+            )


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.

I see, noted

charettes · 2024-05-10T03:22:21Z

tests/composite_pk/test_filter.py

+                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',
+            )


I removed all SQL asserts

charettes · 2024-05-10T03:23:13Z

tests/composite_pk/test_filter.py

+        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])


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.

Ok, I removed this.

…L compiler

csirmazbendeguz mentioned this pull request Apr 7, 2024

Fixed #373 -- Added CompositeField-based Meta.primary_key. #18031

Closed

csirmazbendeguz force-pushed the ticket_373 branch 2 times, most recently from 6a26b19 to c75dcdd Compare April 19, 2024 12:22

Fixed django#373 -- Added CompositeField-based Meta.primary_key.

4be1c68

csirmazbendeguz force-pushed the ticket_373 branch from c75dcdd to 4be1c68 Compare April 20, 2024 00:12

LilyFoote reviewed May 3, 2024

View reviewed changes

charettes reviewed May 5, 2024

View reviewed changes

csirmazbendeguz added 12 commits May 8, 2024 18:53

Fixed django#373 -- Add field to error message, simplify tuple operat…

cee213d

…ion errors

Fixed django#373 -- Rename CompositeField to CompositePrimaryKey

e2d8868

Fixed django#373 -- Add __len__ to CompositePrimaryKey

259605a

Fixed django#373 -- Add some docs

9e01443

Fixed django#373 -- Fix docs formatting

fa4af00

Fixed django#373 -- Add more docs

8354bd0

Fixed django#373 -- Fix docs

d20f8e0

Fixed django#373 -- Fix integrity error tests

dcd78fc

Fixed django#373 -- Remove unnecessary asserts

3576dec

Fixed django#373 -- Make a test simpler

1ef76e4

Fixed django#373 -- Make get tests better

3d0dcf3

Fixed django#373 -- Fix test

6a2bb92

charettes reviewed May 10, 2024

View reviewed changes

csirmazbendeguz added 10 commits May 10, 2024 13:57

Fixed django#373 -- Address create tests comments

a2ec10c

Fixed django#373 -- Address delete tests comments

3071ad9

Fixed django#373 -- Address filter tests comments

b7179b6

Fixed django#373 -- Add test for ordering comments

cb4f300

Fixed django#373 -- Fix ordering tes

6922337

Fixed django#373 - Simplify tests

b7b9580

Fixed django#373 - Simplify tests

9b17831

Fixed django#373 - Don't check for CompositePrimaryKey directly in SQ…

6285cff

…L compiler

Fixed django#373 - Simplify statement

22d6c9d

Fixed django#373 - Use MultiColSource instead of Cols

ad51da4

	if len(list(self.lhs)) != len(list(self.rhs)):
	if len(self.lhs) != len(self.rhs):

		lhs_len = len(tuple(self.lhs))
		if not all(lhs_len == len(tuple(vals)) for vals in self.rhs):

		def __iter__(self):
		return iter(self.get_source_expressions())

	self.assertIsInstance(self.tenant.pk, int)
	self.assertGreater(self.tenant.id, 0)
	self.assertEqual(self.tenant.pk, self.tenant.id)

		return compiler.compile(WhereNode(exprs, connector=OR))


		class Cols(Expression):

		assert all(isinstance(expr, Col) for expr in exprs)
		assert len(exprs) == len(self.targets)

Fixed #373 -- Added CompositeField-based Meta.primary_key. #18056

Are you sure you want to change the base?

Fixed #373 -- Added CompositeField-based Meta.primary_key. #18056

Conversation

csirmazbendeguz commented Apr 7, 2024 • edited

Trac ticket number

Branch description

Checklist

grjones commented Apr 17, 2024 • edited

csirmazbendeguz commented Apr 18, 2024 • edited

csirmazbendeguz commented Apr 20, 2024

grjones commented Apr 20, 2024

grjones commented Apr 22, 2024

csirmazbendeguz commented Apr 23, 2024

grjones commented Apr 23, 2024

csirmazbendeguz commented May 1, 2024 • edited

grjones commented May 2, 2024 • edited

LilyFoote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csirmazbendeguz commented May 4, 2024

LilyFoote commented May 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csirmazbendeguz May 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csirmazbendeguz commented Apr 7, 2024 •

edited

grjones commented Apr 17, 2024 •

edited

csirmazbendeguz commented Apr 18, 2024 •

edited

csirmazbendeguz commented May 1, 2024 •

edited

grjones commented May 2, 2024 •

edited

csirmazbendeguz May 11, 2024 •

edited