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
2 2 3 3 4 4 class 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 , 3 weeks ago
Has patch: | set |
---|
comment:4 by , 3 weeks ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:5 by , 3 weeks ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:6 by , 9 days ago
Triage Stage: | Accepted → Ready for checkin |
---|
As long as
KeyboardInterrupt(BaseException)
is allowed to bubble up (which catching forException
allows) I agree that not trying to catch a predefined set of exceptions is a better default.