Fixed #373 -- Added CompositePrimaryKey-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.

django/db/models/base.py

django/db/models/fields/composite.py

tests/composite_pk/test_get.py

tests/composite_pk/test_update.py

tests/composite_pk/tests.py

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?

I reverted back to Cols. Please resolve if you agree.

…ion errors

LilyFoote

I have two minor points about error message wording. They're not urgent to address.

LilyFoote · 2024-05-17T15:11:01Z

tests/composite_pk/test_get.py

+            ValueError, "'primary_key' must be a tuple or a list"
        ):
            Comment.objects.get(pk=1)


It would be nice if this could be "'pk' must be a tuple or a list".

I don't think this is possible unfortunately, pk is translated to the field

How about this?

'exact' lookup of 'primary_key' field must be a tuple or a list

LilyFoote · 2024-05-17T15:18:11Z

django/db/models/fields/composite.py

            )
        if not all(len(self.lhs) == len(vals) for vals in self.rhs):
            raise ValueError(
-                "The left-hand side and right-hand side of the 'in' lookup must "
-                "have the same number of elements"
+                f"'{self.lhs.field.name}' must have {len(self.lhs)} elements"


I don't think this is quite clear. If the pk has two component fields, but the lookup is pk__in=([1, 2], [3]) we will raise an error 'pk' must have 2 elements but the tuple does have two elements. I'm not sure what to suggest instead though - it's tricky...

I added some changes, let me know what you think

How about this?

'exact' lookup of 'primary_key' field must have 2 elements each

csirmazbendeguz · 2024-05-20T16:01:58Z

I'm not sure what you mean by that, I don't think we can, because Options.pk refers to the field, while Options.primary_key is the list of field names.

You're completely right. In this case is pk set to CompositePrimaryKey when Meta.primary_key is defined and is primary_key set when a non-composite primary is used as well?

It would not be set, if it's a regular primary key, Meta.primary_key is None.

…primary_key

docs/howto/composite-primary-key.txt

…rror

django/db/models/lookups.py

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

LilyFoote reviewed May 17, 2024

View reviewed changes

csirmazbendeguz added 7 commits May 19, 2024 12:21

Fixed django#373 - Revert using MultiColSource

e15f55d

Fixed django#373 - Implement gt, gte lookups for tuples

79bf5e8

Fixed django#373 - Implement lt, lte lookups for tuples

92f2807

Fixed django#373 - Refactor in lookup

c5c5239

Fixed django#373 - Refactor tests

8e408b6

Fixed django#373 - Improve error messages

69143d9

Fixed django#373 - Fix subTest message

586434c

csirmazbendeguz added 11 commits May 21, 2024 14:20

Fixed django#373 - Add lookup error tests

0478d46

Fixed django#373 - Add null check for primary key fields

23e0e0b

Fixed django#373 - Raise value error instead of check error for Meta.…

a9ef232

…primary_key

Fixed django#373 - Add test for checking foreign object's unique target

694fcc8

Fixed django#373 - Simplify checks

b49ea71

Fixed django#373 - Add pk_fields to Options

9fbb766

Fixed django#373 - Adjust indentation

96156dd

Fixed django#373 - Remove try catch checking pk_set

13c0053

Fixed django#373 - Remove try catch from function

9ce6781

Fixed django#373 - Rename to _unnest_composite_fields

2f24f77

Fixed django#373 - Refactor is_set

2c8d38c

carltongibson reviewed May 23, 2024

View reviewed changes

docs/howto/composite-primary-key.txt Show resolved Hide resolved

csirmazbendeguz added 3 commits May 23, 2024 19:42

Fixed django#373 - Add docs

cff46d5

Fixed django#373 - Add duplicate pk check

39a2a09

Fixed django#373 - Throw ValueError for duplicates instead of check e…

2478e14

…rror

charettes reviewed May 23, 2024

View reviewed changes

django/db/models/lookups.py Outdated Show resolved Hide resolved

csirmazbendeguz added 5 commits May 23, 2024 22:39

Fixed django#373 - Add docs about migration

76191b5

Fixed django#373 - Refactor tuple lookups to classes

208a457

Fixed django#373 - Revert new line

b74f3ef

Fixed django#373 - Make variable names more consistent

70fd4be

Fixed django#373 - Add some comments to lookups

4caee01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

csirmazbendeguz commented Apr 7, 2024 •

edited

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

csirmazbendeguz commented May 4, 2024

LilyFoote commented May 4, 2024

charettes May 5, 2024

charettes May 5, 2024

csirmazbendeguz May 8, 2024

csirmazbendeguz May 11, 2024 •

edited

csirmazbendeguz May 23, 2024

LilyFoote left a comment

LilyFoote May 17, 2024

csirmazbendeguz May 20, 2024

csirmazbendeguz May 23, 2024

LilyFoote May 17, 2024

csirmazbendeguz May 20, 2024 •

edited

csirmazbendeguz May 23, 2024

csirmazbendeguz commented May 20, 2024 •

edited

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


		class Cols(Expression):

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

Are you sure you want to change the base?

Fixed #373 -- Added CompositePrimaryKey-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

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

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

csirmazbendeguz May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csirmazbendeguz commented May 20, 2024 • edited

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

csirmazbendeguz May 20, 2024 •

edited

csirmazbendeguz commented May 20, 2024 •

edited