Migration Questioner and String-Type Fields

[Jarek Glowacki]

I made a rant/ticket regarding the hidden usage of `blank` here: #27197.

In short, I don't think that `blank` should dictate whether or not the migration questioner runs.

Building on this, I don't think it should run for for string-type fields at all. If they have `default` set, use that for existing rows. Else if they have `null=True`, set existing rows to `NULL`. Else, set existing rows to empty string.

See linked rant/ticket for some (hopefully) compelling arguments..

Thoughts?

[Tim Graham]

If blank=False, then a new column with a non-blank value means that all existing objects won't pass form validation. Therefore, I don't see why a prompt for a value isn't helpful.

[Jarek Glowacki]

Instances created afterwards, via `MyModel.objects.create()`, with this field unset won't pass form validation either.

The use case is where this field is not expected to appear on a Django form.

[Tim Graham]

Sure, but I don't think that use case should take priority. It's not much work to type an empty string into the questioner if that's what you want. If we remove the prompt, it's significantly more work (editing a migration file or using RunPython) for a developer to set a non-empty value.

[Jarek Glowacki]

Mm, convenience over strict correctness. Perhaps all that's needed is a slight rephrasing of the prompt and we can have both?

Adding field with no `blank=True` and no `null=True`:

You are trying to add a non-nullable field '%s' to %s without a default; we can't do that (the database needs something to populate existing rows). [provide default | cancel]

Removing `null=True` from field:

You are trying to change the nullable field '%s' on %s to non-nullable without a default; we can't do that (the database needs something to populate existing rows). [provide default | fix later | cancel]

Removing `blank` from field:

?? (I only skimmed the code; not sure if this even opens up questioner) 

This kind of implies to the user that they're doing something wrong. Maybe if, for string fields, it read something more along the lines of:

You are adding/altering a field without setting `blank=True`. This will populate existing rows with emptystring despite it being an invalid form input. Are you sure? [yes | let me provide a one-time default for existing rows | cancel]

So basically changing "you can't do this!" (exception-esque) to "are you sure you want this?" (warning-esque).

Anyway I think I'll leave it here. I've exhausted my discussion points now, and you already resolved my particular use case back in the ticket, so I no longer feel so strongly about this to continue trying to push for a change (though am willing to submit a PR if any of the suggested changes are approved).

Cheers,

[Markus Holtermann]

Thank you for your input, Jarek.

  

Assuming I have an existing model, adding

  

   models.CharField(blank=True, max_length=150)

  

to it, doesn't invoke the questioner on current master. Changing this field to

  

   models.CharField(max_length=150)

  

doesn't call the questioner either.

  

Looking at the SQL Django generates, the first change results in

  

   ALTER TABLE "example_mymodel" ADD COLUMN "name2" varchar(150) DEFAULT '' NOT NULL;

   ALTER TABLE "example_mymodel" ALTER COLUMN "name2" DROP DEFAULT;

  

and the second in a noop.

  

I'm trying to understand what change you're proposing in order to figure out if going forward with your proposal is something we should do.

  

Cheers,

  

Markus

[Markus Holtermann]

Thank you for your input, Jarek.

Assuming I have an existing model, adding

models.CharField(blank=True, max_length=150)

to it, doesn't invoke the questioner on current master. Changing this
field to

models.CharField(max_length=150)

doesn't call the questioner either.

Looking at the SQL Django generates, the first change results in

ALTER TABLE "example_mymodel" ADD COLUMN "name2" varchar(150) DEFAULT '' NOT NULL;
ALTER TABLE "example_mymodel" ALTER COLUMN "name2" DROP DEFAULT;

and the second in a noop.

I'm trying to understand what change you're proposing in order to figure
out if going forward with your proposal is something we should do.

Cheers,

Markus

>>>>> <https://code.djangoproject.com/ticket/27197#ticket>.

>>>>>
>>>>> In short, I don't think that `blank` should dictate whether or not the
>>>>> migration questioner runs.
>>>>> Building on this, I don't think it should run for for string-type
>>>>> fields at all. If they have `default` set, use that for existing rows. Else
>>>>> if they have `null=True`, set existing rows to `NULL`. Else, set existing
>>>>> rows to empty string.
>>>>>
>>>>> See linked rant/ticket for some (hopefully) compelling arguments..
>>>>>
>>>>> Thoughts?
>>>>>
>>>>
>

>--
>You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
>To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
>To post to this group, send email to django-d...@googlegroups.com.
>Visit this group at https://groups.google.com/group/django-developers.
>To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/60bf1464-7e70-41c4-b8de-b201af84b4b7%40googlegroups.com.
>For more options, visit https://groups.google.com/d/optout.

[Jarek Glowacki]

Hi Markus,

Sorry for the slow reply, I never received a notification that you'd posted. (Gmail does weird inbox sorting these days).

Assuming I have an existing model, adding

  

   models.CharField(blank=True, max_length=150)

  

to it, doesn't invoke the questioner on current master. Changing this field to

  

   models.CharField(max_length=150)

  

doesn't call the questioner either.

Ok so that is acceptable (I put question marks because I hadn't checked behaviour in that case). But the other two cases I listed:

 - AddField without `blank=True` or `null=True`.

 - AlterField [removing `null=True`] AND [removing `blank=True` OR already not having `blank=True`] 

calls the questioner.

In its current state I don't like this because:

(1) Whether or not the questioner (a backend helper) opens depends on `blank` (a frontend validation option).

(2) The questioner phrases itself as you trying to do something erroneous ('you can't do this'), when it's a valid thing to want to do.

The behaviour in (1) threw me when i first stumbled across it, as I hadn't expected `blank` to have any effect on making migrations. Tim has kind of convinced me to accept it, just because it's the 'usual' scenario and makes things convenient. Plus, fixing this would either mean reverting #23405 (bad option), or dropping the questioner altogether (correctness over convenience; not a great option either). 

Alternatively, could try to address (2) by making the questioner less scary: 'Are you sure?/maybe you'd like to set a one-time default instead?' over 'you can't do this; set a default or quit'.

So I think if anything comes of this, it'd likely just be a slight rewording of the questioner and an additional choice being added. This wouldn't offer any new functionality, as choosing 'yes, I'm sure I want to set existing rows to emptystring' would be equivalent to choosing 'set a one-time default' and entering emptystring manually. But the difference would be that the user doesn't feel like they've done something wrong when they see this. I can say from experience, whenever I saw the questioner in the past, I quickly cancelled it and went back to try and fix up my models, assuming I'd made a mistake.

Cheers,

Jarek