Fixed #27452 -- Added serial fields to contrib.postgres. #18123

csirmazbendeguz · 2024-05-03T05:59:08Z

Trac ticket number

ticket-27452

Branch description

Added SmallSerialField, SerialField, BigSerialField to contrib.postgres.

Previous PR

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.
I have attached screenshots in both light and dark modes for any UI changes.

LilyFoote

This looks like a great start!

In addition to the specific comments I added, we'll need some migrations tests to cover adding/removing serial fields and converting between serial and auto fields.

django/contrib/postgres/fields/serial.py

django/db/backends/postgresql/operations.py

django/db/backends/postgresql/schema.py

LilyFoote · 2024-05-04T13:36:46Z

tests/schema/tests.py

+    @unittest.skipUnless(connection.vendor == "postgresql", "PostgreSQL specific")
+    @isolate_apps("schema")
+    @tag("serial")
+    def test_serial_to_integer(self):
+        from django.contrib.postgres.fields import SerialField
+
+        self.serial_to_integer_test(SerialField, IntegerField)
+
+    @unittest.skipUnless(connection.vendor == "postgresql", "PostgreSQL specific")
+    @isolate_apps("schema")
+    @tag("serial")
+    def test_serial_to_small_integer(self):
+        from django.contrib.postgres.fields import SerialField
+
+        self.serial_to_integer_test(SerialField, SmallIntegerField)
+
+    @unittest.skipUnless(connection.vendor == "postgresql", "PostgreSQL specific")
+    @isolate_apps("schema")
+    @tag("serial")
+    def test_serial_to_big_integer(self):
+        from django.contrib.postgres.fields import SerialField
+
+        self.serial_to_integer_test(SerialField, BigIntegerField)
+
+    @unittest.skipUnless(connection.vendor == "postgresql", "PostgreSQL specific")
+    @isolate_apps("schema")
+    @tag("serial")
+    def test_small_serial_to_integer(self):
+        from django.contrib.postgres.fields import SmallSerialField
+
+        self.serial_to_integer_test(SmallSerialField, IntegerField)
+
+    @unittest.skipUnless(connection.vendor == "postgresql", "PostgreSQL specific")
+    @isolate_apps("schema")
+    @tag("serial")
+    def test_small_serial_to_small_integer(self):
+        from django.contrib.postgres.fields import SmallSerialField
+
+        self.serial_to_integer_test(SmallSerialField, SmallIntegerField)
+
+    @unittest.skipUnless(connection.vendor == "postgresql", "PostgreSQL specific")
+    @isolate_apps("schema")
+    @tag("serial")
+    def test_small_serial_to_big_integer(self):
+        from django.contrib.postgres.fields import SmallSerialField
+
+        self.serial_to_integer_test(SmallSerialField, BigIntegerField)
+
+    @unittest.skipUnless(connection.vendor == "postgresql", "PostgreSQL specific")
+    @isolate_apps("schema")
+    @tag("serial")
+    def test_big_serial_to_big_integer(self):
+        from django.contrib.postgres.fields import BigSerialField
+
+        self.serial_to_integer_test(BigSerialField, BigIntegerField)
+
+    @unittest.skipUnless(connection.vendor == "postgresql", "PostgreSQL specific")
+    @isolate_apps("schema")
+    @tag("serial")
+    def test_big_serial_to_integer(self):
+        from django.contrib.postgres.fields import BigSerialField
+
+        self.serial_to_integer_test(BigSerialField, IntegerField)
+
+    @unittest.skipUnless(connection.vendor == "postgresql", "PostgreSQL specific")
+    @isolate_apps("schema")
+    @tag("serial")
+    def test_big_serial_to_small_integer(self):
+        from django.contrib.postgres.fields import BigSerialField
+
+        self.serial_to_integer_test(BigSerialField, SmallIntegerField)


This feels like a good use case for subTest to me.

Yes, the issue is we would either need to generate the test models with a dynamic name, or clean up the test models after the subtests - IMO it's more trouble than worth it.

tests/schema/tests.py

csirmazbendeguz · 2024-05-05T11:09:05Z

This looks like a great start!

In addition to the specific comments I added, we'll need some migrations tests to cover adding/removing serial fields and converting between serial and auto fields.

@LilyFoote , thanks for the review! Could you point me to some tests I could copy to get started? I'm not sure how to test the migrations other than with the tests/schema tests.

LilyFoote · 2024-05-05T12:13:36Z

For database defaults I added tests in tests/migrations/test_autodetector.py and tests/migrations/test_operations.py.

csirmazbendeguz · 2024-05-25T06:33:01Z

Thanks a lot @sarahboyce , I addressed your comments, let me know what you think.

sarahboyce

Thank you for the updates @csirmazbendeguz
I think there are a few tests/asserts we need to add 👍

django/contrib/postgres/fields/serial.py

sarahboyce · 2024-05-27T10:19:34Z

django/contrib/postgres/fields/serial.py

+    def __init__(self, *args, **kwargs):
+        kwargs["blank"] = True
+        super().__init__(*args, **kwargs)
+
+    def deconstruct(self):
+        name, path, args, kwargs = super().deconstruct()
+        kwargs.pop("blank")
+        return name, path, args, kwargs


I can remove the __init__ and deconstruct methods without any tests failing, can you add a test on why these are needed?

I added a test (needed for cleaning the fields, for example on full_clean)

sarahboyce · 2024-05-27T10:23:34Z

django/core/management/commands/inspectdb.py

+                def is_postgres_row(row):
+                    field_type, _, _ = self.get_field_type(connection, table_name, row)
+                    return field_type.startswith("postgres.fields.")
+
+                if any(is_postgres_row(row) for row in table_description):
+                    yield "from django.contrib import postgres"


This is untested

To be fair the other import is untested as well. But actually, this might be incorrect, let me check and fix.

I had to do a little more refactoring in this function, please check

django/db/backends/postgresql/operations.py

csirmazbendeguz · 2024-05-27T16:49:41Z

Thanks @sarahboyce , fair enough, I made some adjustments.

InvalidInterrupt

I haven't really read many of the tests yet; not much has caught my eye that isn't already being discussed.

InvalidInterrupt · 2024-05-28T03:25:04Z

django/contrib/postgres/fields/serial.py

+    def _check_default(self):
+        if self.default is NOT_PROVIDED:
+            return []
+        return [
+            checks.Error(
+                f"{self.__class__.__name__} does not accept default values.",
+                obj=self,
+                id="fields.E014",
+            ),
+        ]


While I would expect most users to operate this way; it feels overly prescriptive to me.

Suppose for some reason I only wanted a subset of objects to be part of the sequence, and the rest to default to 0, I might want to set default on the field and pass DatabaseDefault() only when I want to use the sequence.

Thanks @InvalidInterrupt !

I don't think this is an issue, it's possible to create an object without using the sequence:

class Dummy(Model): serial = SerialField() Dummy.objects.create(serial=0).serial # 0 Dummy.objects.create(serial=0).serial # 0 Dummy.objects.create(serial=0).serial # 0 Dummy.objects.create().serial # 1 Dummy.objects.create().serial # 2 Dummy.objects.create().serial # 3 Dummy.objects.create(serial=1).serial # 1 Dummy.objects.create().serial # 4

Is this what you're suggesting?

class Dummy(Model): serial = SerialField(default=0) Dummy.objects.create().serial # 0 Dummy.objects.create().serial # 0 Dummy.objects.create().serial # 0 Dummy.objects.create(serial=DatabaseDefault()).serial # 1 Dummy.objects.create(serial=DatabaseDefault()).serial # 2 Dummy.objects.create(serial=DatabaseDefault()).serial # 3 Dummy.objects.create(serial=1).serial # 1 Dummy.objects.create(serial=DatabaseDefault()).serial # 4

I think the first one is better because it's consistent with how AutoField works.

I've been testing on a little project, if we add a SerialField to an existing model I cannot make it nullable or assign a default and so I have to give a temporary default in order to create a migration and add the field.

However, whatever I give here is ignored and the field is populated everywhere with as a serial starting from 1
I feel like it shouldn't prompt us in that case (forcing my input and then ignoring it to me is a bug).
Also considering this, a default option might be nice 🤔 (cc @LilyFoote)

I see 🤔 Thanks for catching that @sarahboyce ! I tested it too but I didn't notice the the input was ignored. 😱 I'll investigate.

Yes, default makes sense in the context of migrations.

Good catch! I think the minimal change feature-wise would be to adapt the migration generation code to know that SerialField can be created without an explicit default.

For a user-chosen default my question would be "why a SerialField and not an IntegerField?".

Yes, I think that makes sense.
Thinking some more, I think default would be misleading and I think what is being discussed is an optional start parameter. If that's wanted, it could be added in later 🤔

I added the has_db_default function to solve this the way @LilyFoote suggested.

docs/ref/contrib/postgres/fields.txt

csirmazbendeguz · 2024-05-28T07:39:16Z

Thanks for the review @InvalidInterrupt , it's much appreciated! 🙏

sarahboyce · 2024-05-28T08:56:57Z

django/core/management/commands/inspectdb.py

@@ -75,6 +76,7 @@ def handle_inspection(self, options):
                "field names."
            )
            yield "from %s import models" % self.db_module
+


@csirmazbendeguz these are minor clean up changes that will happen on the PR either by you or by a merger

we want to limit the changed lines to keep the diff really tight to what's important, occasionally you have added a new line or removed a line, I will probably revert some of this (you can too if you like). This is considered formatting changes

you have added @tag("serial") for your tests, this is not how we organise tests generally and needs to be removed

the commits need to be squashed. This will likely be 1 commit (or if we can think of a nice way of splitting them we will)

I added this for informational purposes 👍

I forgot to say that I have tested this with a local project and tried out a number of things to try and see if I can find any issues (had a look in the admin, reversed migrations, used them in GeneratedFields) and this comment is the only thing I'm concerned about. It looks really good ⭐

…_db_default function

sarahboyce · 2024-05-28T13:59:44Z

django/db/migrations/autodetector.py

@@ -1134,7 +1134,7 @@ def _generate_added_field(self, app_label, model_name, field_name):
        preserve_default = (
            field.null
            or field.has_default()
-            or field.db_default is not models.NOT_PROVIDED
+            or field.has_db_default()
            or field.many_to_many
            or (field.blank and field.empty_strings_allowed)
            or (isinstance(field, time_fields) and field.auto_now)


Would it be better instead of overwriting has_db_default to True for serial fields, to do an or isinstance(field, serial_fields) check similar to the time fields check here?

Read the SQL, it does have a database default, this makes sense now (sorry)

.has_db_default() can be used in more places (if you search for .db_default is you should see quite a few places)
We can do this refactor in a new branch/PR to be merged in before this PR. This will keep this PR small. Are you happy to make a PR for this change? (adding def has_db_default(self) to Field and using has_db_default everywhere) Does that make sense?

Yeah I agree @sarahboyce , I'll open a new PR for this refactor tomorrow. I think this makes sense since there's a has_default function too, this would be the same but for db_default.

Thanks for your support!

csirmazbendeguz force-pushed the ticket_27452 branch from 65d6140 to fd0e9b4 Compare May 3, 2024 06:09

ngnpope self-requested a review May 3, 2024 06:37

csirmazbendeguz force-pushed the ticket_27452 branch 4 times, most recently from e892da4 to 97cbc23 Compare May 3, 2024 08:31

csirmazbendeguz marked this pull request as draft May 3, 2024 10:21

csirmazbendeguz force-pushed the ticket_27452 branch 12 times, most recently from fccac26 to 2b48193 Compare May 4, 2024 13:05

LilyFoote reviewed May 4, 2024

View reviewed changes

csirmazbendeguz force-pushed the ticket_27452 branch 2 times, most recently from e7fdd21 to 8052e52 Compare May 5, 2024 10:37

csirmazbendeguz force-pushed the ticket_27452 branch from 8052e52 to 42c53c4 Compare May 5, 2024 11:16

csirmazbendeguz force-pushed the ticket_27452 branch 5 times, most recently from 569c960 to 0ebc30b Compare May 6, 2024 06:40

Fixed django#373 - Fix docs

d04ee2c

csirmazbendeguz force-pushed the ticket_27452 branch from 0ec1583 to d04ee2c Compare May 25, 2024 05:20

csirmazbendeguz added 5 commits May 25, 2024 14:03

Fixed django#373 - Adjust check error messages

9d3b5f8

Fixed django#373 - Remove irrelevant docs changes

681d57c

Fixed django#373 - Remove irrelevant docs changes

3c13670

Fixed django#373 - Rename check method

7b79f2b

Fixed django#373 - Fix docs

c88bf13

sarahboyce reviewed May 27, 2024

View reviewed changes

csirmazbendeguz added 6 commits May 27, 2024 20:32

Fixed #27452 - Small adjustment

ae07471

Fixed #27452 - Add full clean test

918402f

Fixed #27452 - Fix postgres import

43fb609

Fixed #27452 - Rename TableIntrospect to TableInspect

f61a7de

Fixed #27452 - Add test tag

007bd7b

Fixed #27452 - Fix test

1c3ad58

InvalidInterrupt reviewed May 28, 2024

View reviewed changes

Fixed #27452 - Add docs

e01d0d0

sarahboyce reviewed May 28, 2024

View reviewed changes

csirmazbendeguz added 9 commits May 28, 2024 18:45

Fixed #27452 - Don't ask for input when adding serial field / add has…

0cc6e9a

…_db_default function

Fixed #27452 - Remove new lines

b0fa769

Fixed #27452 - Add more full_clean tests

24a9bcd

Fixed #27452 - Add more tests

e090c24

Fixed #27452 - Fix test

b129734

Fixed #27452 - Add removed line

a04d6d0

Fixed #27452 - Add assert for adding serial field

ea71b04

Fixed #27452 - Remove tags

b46f9c4

Fixed #27452 - Remove tag

5212ef3

sarahboyce reviewed May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #27452 -- Added serial fields to contrib.postgres. #18123

Fixed #27452 -- Added serial fields to contrib.postgres. #18123

csirmazbendeguz commented May 3, 2024 •

edited

LilyFoote left a comment

LilyFoote May 4, 2024

csirmazbendeguz May 5, 2024

csirmazbendeguz commented May 5, 2024

LilyFoote commented May 5, 2024

csirmazbendeguz commented May 25, 2024

sarahboyce left a comment

sarahboyce May 27, 2024

csirmazbendeguz May 27, 2024 •

edited

sarahboyce May 27, 2024

csirmazbendeguz May 27, 2024

csirmazbendeguz May 27, 2024

csirmazbendeguz commented May 27, 2024

InvalidInterrupt left a comment

InvalidInterrupt May 28, 2024

csirmazbendeguz May 28, 2024

sarahboyce May 28, 2024

csirmazbendeguz May 28, 2024

LilyFoote May 28, 2024 •

edited

sarahboyce May 28, 2024

csirmazbendeguz May 28, 2024

csirmazbendeguz commented May 28, 2024

sarahboyce May 28, 2024

sarahboyce May 28, 2024

sarahboyce May 28, 2024

sarahboyce May 28, 2024 •

edited

csirmazbendeguz May 28, 2024

Fixed #27452 -- Added serial fields to contrib.postgres. #18123

Are you sure you want to change the base?

Fixed #27452 -- Added serial fields to contrib.postgres. #18123

Conversation

csirmazbendeguz commented May 3, 2024 • edited

Trac ticket number

Branch description

Checklist

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

csirmazbendeguz commented May 5, 2024

LilyFoote commented May 5, 2024

csirmazbendeguz commented May 25, 2024

sarahboyce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csirmazbendeguz May 27, 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

csirmazbendeguz commented May 27, 2024

InvalidInterrupt 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

LilyFoote May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csirmazbendeguz commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahboyce May 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csirmazbendeguz commented May 3, 2024 •

edited

csirmazbendeguz May 27, 2024 •

edited

LilyFoote May 28, 2024 •

edited

sarahboyce May 28, 2024 •

edited