Fixed #26739 -- Made reverse RemoveField handle non-nullable columns. #16890

DevilsAutumn · 2023-05-23T14:40:23Z

Ticket #26739
Thanks @MarkusH , @apollo13 and @Gagaro for initial work. (#6870 )

checklist:

Add test for ask_not_null_removal questioner.

django/db/migrations/operations/fields.py

django/db/migrations/autodetector.py

shangxiao · 2023-05-24T09:40:46Z

django/db/migrations/autodetector.py

+            and not field.many_to_many
+            and not (field.blank and field.empty_strings_allowed)
+            and not (
+                isinstance(field, time_fields)


Checking the type is unnecessary. Often duck typing is used in Django; presence of auto_now or auto_now_add is enough.

If I am not wrong fields other than date and time fields don't have auto_now and auto_now_add attribute. won't they throw an error if i remove type checking? It is used even in field addition, see here.

Maybe let's use:

getattr(field, "auto_now", False) or getattr(field, "auto_now_add", False)

This is the same as BaseDatabaseSchemaEditor._effective_default.
I think we have everything from BaseDatabaseSchemaEditor._effective_default, I tested GeneratedField case but these are covered by not field.null.

django/db/migrations/autodetector.py

django/db/migrations/questioner.py

tests/migrations/test_commands.py

shangxiao · 2023-05-24T10:03:43Z

tests/migrations/test_operations.py

+        # MySQL populates NOT NULL cols with implicit defaults instead of raising
+        # an error.


Interesting I did not know this… I don't use MySQL but I'm hardly surprised 😛

I wonder whether this can be utilised in any way.

django/db/migrations/questioner.py

LilyFoote

Looks like a good start to me!

The main thing I spotted is that we don't have any support for db_default which landed recently.

django/db/migrations/autodetector.py

django/db/migrations/operations/fields.py

django/db/migrations/questioner.py

django/db/migrations/operations/fields.py

sarahboyce

Thank you @DevilsAutumn! This is getting really close, got a few more comments 👍

django/db/migrations/questioner.py

docs/releases/5.0.5.txt

docs/releases/5.1.txt

sarahboyce · 2024-04-26T07:01:13Z

django/db/migrations/autodetector.py

+            and not field.many_to_many
+            and not (field.blank and field.empty_strings_allowed)
+            and not (
+                isinstance(field, time_fields)


Maybe let's use:

getattr(field, "auto_now", False) or getattr(field, "auto_now_add", False)

This is the same as BaseDatabaseSchemaEditor._effective_default.
I think we have everything from BaseDatabaseSchemaEditor._effective_default, I tested GeneratedField case but these are covered by not field.null.

django/db/migrations/autodetector.py

tests/migrations/test_operations.py

docs/ref/migration-operations.txt

DevilsAutumn marked this pull request as draft May 23, 2023 14:40

auvipy suggested changes May 23, 2023

View reviewed changes

django/db/migrations/operations/fields.py Outdated Show resolved Hide resolved

DevilsAutumn force-pushed the ticket_26739 branch 2 times, most recently from 4517c00 to 2af2a74 Compare May 23, 2023 17:43

DevilsAutumn marked this pull request as ready for review May 23, 2023 19:17

DevilsAutumn changed the title ~~WIP #26739 -- Made reverse RemoveField handle non-nullable columns.~~ Fixed #26739 -- Made reverse RemoveField handle non-nullable columns. May 23, 2023