Opened 3 weeks ago

Closed 5 days ago

#35882 closed Cleanup/optimization (fixed)

Loop on all errors in `InteractiveMigrationQuestioner._ask_default()`

Reported by: Adam Johnson Owned by: Adam Johnson
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When changing some fields, the migration questioner can ask for new field default values entered through a Python prompt.

Yesterday, I made a typo which made the makemigrations process exit. This was a little frustrating as there were several fields to fix, and my work answering for the first fields was lost.

Currently, the questioner loops on SyntaxError and NameError (source). My mistake lead to an AttributeError, hence the crash and lost work. I propose we extend this clause to Exception - there's little harm in displaying the error and looping.

Here's a reproducing example. Say you drop null=True from two model fields:

  • example/models.py

    diff --git example/models.py example/models.py
    index 816fe37..9743877 100644
     
    22
    33
    44class Book(models.Model):
    5     title = models.CharField(max_length=100, null=True)
    6     published_date = models.DateField(null=True)
     5    title = models.CharField(max_length=100)
     6    published_date = models.DateField()

makemigrations asks you about the first one:

$ ./manage.py makemigrations example
It is impossible to change a nullable field 'title' on book to non-nullable without providing a default. This is because the database needs something to populate existing rows.
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Ignore for now. Existing rows that contain NULL values will have to be handled manually, for example with a RunPython or RunSQL operation.
 3) Quit and manually define a default value in models.py.
Select an option: 1
Please enter the default value as valid Python.
The datetime and django.utils.timezone modules are available, so it is possible to provide e.g. timezone.now as a value.
Type 'exit' to exit this prompt
>>> "Unknown book"

Immediately after this, it asks about the second field. Making a typo like .dat() instead of .date() crashes the process:

It is impossible to change a nullable field 'published_date' on book to non-nullable without providing a default. This is because the database needs something to populate existing rows.
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Ignore for now. Existing rows that contain NULL values will have to be handled manually, for example with a RunPython or RunSQL operation.
 3) Quit and manually define a default value in models.py.
Select an option: 1
Please enter the default value as valid Python.
The datetime and django.utils.timezone modules are available, so it is possible to provide e.g. timezone.now as a value.
Type 'exit' to exit this prompt
>>> datetime.dat(1970, 1, 1)
Traceback (most recent call last):
  File "/..././manage.py", line 21, in <module>
    main()
  ...
  File "/.../.venv/.../django/db/migrations/questioner.py", line 162, in _ask_default
    return eval(code, {}, {"datetime": datetime, "timezone": timezone})
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<string>", line 1, in <module>
AttributeError: module 'datetime' has no attribute 'dat'. Did you mean: 'date'?

But if you instead made a typo in the datetime name, it would loop:

>>> datetme.date(1970,1,1)
Invalid input: name 'datetme' is not defined
>>>

Change History (6)

comment:1 by Adam Johnson, 3 weeks ago

Has patch: set

comment:4 by Adam Johnson, 3 weeks ago

Owner: set to Adam Johnson
Status: newassigned

comment:5 by Simon Charette, 3 weeks ago

Triage Stage: UnreviewedAccepted

As long as KeyboardInterrupt(BaseException) is allowed to bubble up (which catching for Exception allows) I agree that not trying to catch a predefined set of exceptions is a better default.

comment:6 by Mariusz Felisiak, 9 days ago

Triage Stage: AcceptedReady for checkin

comment:7 by Sarah Boyce <42296566+sarahboyce@…>, 5 days ago

In 3434fab7:

Refs #35882 -- Added test for migration questioner KeyboardInterrupt.

comment:8 by Sarah Boyce <42296566+sarahboyce@…>, 5 days ago

Resolution: fixed
Status: assignedclosed

In e035db1:

Fixed #35882 -- Made migration questioner loop on all errors.

Note: See TracTickets for help on using tickets.
Back to Top