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-08T03:59:24Z

I have managed to test these operations (add, alter, remove serial field) in the schema.tests, I'm not sure if more tests are necessary?

ngnpope

We will need to address the backward compatibility with pre-Django 4.1 AutoField on PostgreSQL to avoid breaking many old projects that didn't manually migrate to identity columns.

Edit: This might not be too much of a problem after all, but we should carefully consider all the places where things could break, e.g. migrations. We should likely have a test that starts out with a pre-Django 4.1 AutoField to make sure that it is handled correctly.

django/db/backends/postgresql/introspection.py

django/db/backends/postgresql/operations.py

docs/ref/checks.txt

csirmazbendeguz · 2024-05-09T14:34:31Z

@ngnpope , thank you for reviewing my changes ! 🙏 I have pushed a fix for the issues you mentioned.

csirmazbendeguz · 2024-05-14T06:09:27Z

@ngnpope , I think all concerns has been addressed, let me know what you think.

LilyFoote

I'm loving the progress you've made here. I have a few minor suggestions, but overall looks good to me!

django/contrib/postgres/fields/serial.py

django/db/backends/postgresql/schema.py

csirmazbendeguz · 2024-05-16T05:04:52Z

Thanks a lot @LilyFoote ! I addressed your suggestions.

sarahboyce

Thank you @csirmazbendeguz
I haven't reviewed this in depth but will hope to review again soon, as a first look through, this looks nice

sarahboyce · 2024-05-24T15:23:54Z

django/contrib/postgres/fields/serial.py

+        if self.null:
+            return [
+                checks.Error(
+                    "SerialFields do not accept null values.",


Suggested change

"SerialFields do not accept null values.",

f"{self.__class__.__name__} must not have ``null=True``.",

Saying "must not have null=True" aligns more to fields.E007.

Also this is currently documented as

``<field>`` does not accept null values.

But always says SerialFields, maybe we should do something similar to fields.E010 and update the error message depending on the field. If so, update each of these, otherwise I would document the check to be:

SerialFields does not accept null values.

@sarahboyce , I was following fields.E110 which has:

``BooleanField``\s do not accept null values

It was @ngnpope 's idea to make this error message general, and I agree with it, because this way we wouldn't need to define a new error code every time a new field doesn't accept a null value.

If you would like to make this specific to serial fields, I believe we should choose a different error code.

I agree with using the class name instead of "SerialFields" - I adjusted it.

django/contrib/postgres/fields/serial.py

docs/ref/contrib/postgres/fields.txt

docs/releases/5.1.txt

docs/ref/contrib/postgres/fields.txt

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

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

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

csirmazbendeguz force-pushed the ticket_27452 branch 2 times, most recently from 0fc425d to d4f4219 Compare May 6, 2024 11:22

csirmazbendeguz marked this pull request as ready for review May 6, 2024 11:24

csirmazbendeguz force-pushed the ticket_27452 branch 4 times, most recently from 4c086eb to 51ed545 Compare May 6, 2024 12:03

ngnpope suggested changes May 9, 2024

View reviewed changes

django/db/backends/postgresql/introspection.py Outdated Show resolved Hide resolved

django/db/backends/postgresql/operations.py Outdated Show resolved Hide resolved

docs/ref/checks.txt Outdated Show resolved Hide resolved

LilyFoote approved these changes May 15, 2024

View reviewed changes

django/contrib/postgres/fields/serial.py Show resolved Hide resolved

django/db/backends/postgresql/schema.py Outdated Show resolved Hide resolved

django/db/backends/postgresql/schema.py Show resolved Hide resolved

django/db/backends/postgresql/schema.py Outdated Show resolved Hide resolved

sarahboyce reviewed May 24, 2024

View reviewed changes

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

csirmazbendeguz added 2 commits May 25, 2024 13:13

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

69e55a4

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

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 8, 2024 •

edited

ngnpope left a comment •

edited

csirmazbendeguz commented May 9, 2024

csirmazbendeguz commented May 14, 2024

LilyFoote left a comment

csirmazbendeguz commented May 16, 2024

sarahboyce left a comment

sarahboyce May 24, 2024

csirmazbendeguz May 25, 2024 •

edited

csirmazbendeguz commented May 25, 2024

	"SerialFields do not accept null values.",
	f"{self.__class__.__name__} must not have ``null=True``.",

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

LilyFoote May 4, 2024

Choose a reason for hiding this comment

csirmazbendeguz May 5, 2024

Choose a reason for hiding this comment

csirmazbendeguz commented May 5, 2024

LilyFoote commented May 5, 2024

csirmazbendeguz commented May 8, 2024 • edited

ngnpope left a comment • edited

Choose a reason for hiding this comment

csirmazbendeguz commented May 9, 2024

csirmazbendeguz commented May 14, 2024

LilyFoote left a comment

Choose a reason for hiding this comment

csirmazbendeguz commented May 16, 2024

sarahboyce left a comment

Choose a reason for hiding this comment

sarahboyce May 24, 2024

Choose a reason for hiding this comment

csirmazbendeguz May 25, 2024 • edited

Choose a reason for hiding this comment

csirmazbendeguz commented May 25, 2024

csirmazbendeguz commented May 3, 2024 •

edited

csirmazbendeguz commented May 8, 2024 •

edited

ngnpope left a comment •

edited

csirmazbendeguz May 25, 2024 •

edited