Opened 18 months ago

Closed 18 months ago

Last modified 18 months ago

#34568 closed Bug (fixed)

makemigrations --update should respect the --name option.

Reported by: David Sanders Owned by: Mariusz Felisiak
Component: Database layer (models, ORM) Version: 4.2
Severity: Release blocker Keywords:
Cc: Simon Charette, David Wobrock Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by David Sanders)

This may be intentional behaviour but the docs don't mention this so creating a ticket to update docs or correct behaviour:

If you create a migration with a custom name:

$ ./manage.py makemigrations --name foo
Migrations for 'update_rename':
  update_rename/migrations/0001_foo.py
    - Create model Foo

then running --update will change the name "foo" to the autogenerated one based on the operations:

$ ./manage.py makemigrations --update
Migrations for 'update_rename':
  update_rename/migrations/0001_initial.py
    - Create model Foo
Deleted update_rename/migrations/0001_foo.py

My opinion is that it shouldn't as it violates the principle of least astonishment even though the --name argument wasn't supplied.

EDIT:
This is my first time using --update and here are a few other observations which could indicate that it requires broader discussion:

  • It doesn't utilise the --name argument so it's impossible to customise the name during --update
  • It'd be nice to provide --no-optimize option to --update, here's my use-case: 3-step non-null field addition. After doing nullable step 1, elidable data migration step 2, I want to merge the step 3 non-null update into the migration but --update optimizes this into a single step.

Perhaps --update requires a rethink?

Change History (16)

comment:1 by David Sanders, 18 months ago

<moved to description>

Last edited 18 months ago by David Sanders (previous) (diff)

comment:2 by David Sanders, 18 months ago

Description: modified (diff)

comment:3 by Mariusz Felisiak, 18 months ago

Cc: David Wobrock added

I think you're expecting too much from --update was intended to be an option for updating recently developed changes and don't do anything fancier. Please check the discussion in ​PR.

My opinion is that it shouldn't as it violates the principle of least astonishment even though the --name argument wasn't supplied.

We could document this behavior, but IMO it shouldn't be changed.

It'd be nice to provide --no-optimize option to --update, here's my use-case: 3-step non-null field addition. After doing nullable step 1, elidable data migration step 2, I want to merge the step 3 non-null update into the migration but --update optimizes this into a single step.

It's probably worth adding πŸ€”

comment:4 by David Sanders, 18 months ago

@felix read through that PR, I'm assuming your point about ending up with a name that doesn't match the operations is referring to not being able to tell the difference between a custom name and an auto gen'd name. Here's my response:

  • It's a fair point. Trying to determine custom names is too complex and shouldn't be done.
  • In my anecdotal experience we (myself and colleagues) rarely use the auto gen'd name. An option to --keep-name sounds nice to me with the default to *not keep*.
  • If not, then at the very least supplying --name should raise an exception or honour the name. It definitely shouldn't do *nothing*. My preference is to honour --name 😊

Can we get Simon's take as well since he was part of the thread?

I still think something should change because it's not even fancy… I'd say that turning off optimisations is harder than this.

Version 0, edited 18 months ago by David Sanders (next)

comment:5 by David Sanders, 18 months ago

Also just now thought of another option:

  • An option to print migrations to stdout --stdout (or whatever) then I can redirect the content to my latest migration and edit as necessary πŸ˜…

comment:6 by Mariusz Felisiak, 18 months ago

Cc: Simon Charette added

comment:7 by Natalia Bidart, 18 months ago

After reading the referenced PR and the comments in this ticket, I agree with David that ignoring --name feels like a bug, and I think it's worth fixing. I believe the fix would be as simple as (plus tests):

  • django/core/management/commands/makemigrations.py

    a b class Command(BaseCommand):  
    316316            )
    317317            # Update name.
    318318            previous_migration_path = MigrationWriter(leaf_migration).path
    319             suggested_name = (
     319            suggested_name = self.migration_name or (
    320320                leaf_migration.name[:4] + "_" + leaf_migration.suggest_name()
    321321            )
    322322            if leaf_migration.name == suggested_name:

Regarding optimization, I would have taken a different approach in the original implementation: do not optimize the updated migration, leave that decision to the caller, they can later call optimizemigration. Is it too late for considering that approach?

Lastly, I would not go the route of allowing to keep the current name, the caller can achieve that by passing --name which is already a documented and valid option for the command.

comment:8 by Simon Charette, 18 months ago

I also think that --name should not be ignored when --update is used but updating the name of the updated migration to follow the usual operation fragment stitching otherwise seems like a expected default.

Whether or not we we should add an option not to perform optimization or disable operation fragment usage for migration name entirely seems like distinct feature requests.

in reply to:  8 comment:9 by David Sanders, 18 months ago

Replying to Simon Charette:

Whether or not we we should add an option not to perform optimization or disable operation fragment usage for migration name entirely seems like distinct feature requests.

Yep, though before I create a ticket for that, I might just create a PoC to see whether it helps with the use case I described above.

comment:10 by Mariusz Felisiak, 18 months ago

Severity: Normal β†’ Release blocker
Summary: makemigrations --update changes the name of custom migration name β†’ makemigrations --update should respect the --name option.
Triage Stage: Unreviewed β†’ Accepted
Type: Uncategorized β†’ Bug

I'm happy to treat this as a bug in the new feature. Opt-out optimization is a separate feature request.

comment:11 by David Sanders, 18 months ago

One more thing:

--update is a destructive operation – if you had any customised migration operations or code in your latest migration this will be permanently deleted if you run update *without any warning*.

I'd like to suggest that *at least* one of the following happen:

  1. --update does a confirmation eg "<app>/migrations/0009_last_migration.py will be replaced. Proceed? y/N". Along with this we provide a --no-input. Both of these are consistent with other commands.
  2. we document that it destroys your last migration without warning

My preference is 1. because, to paraphrase FunkyBob, the purpose of any framework is to manage the risky and the tedious.

We should at the very least do 2. if it's decided 1. is a no-go.

This also sounds like another ticket.

comment:12 by Mariusz Felisiak, 18 months ago

Owner: changed from nobody to Mariusz Felisiak
Status: new β†’ assigned

comment:13 by Mariusz Felisiak, 18 months ago

Has patch: set

comment:14 by GitHub <noreply@…>, 18 months ago

Resolution: β†’ fixed
Status: assigned β†’ closed

In c52f429:

Fixed #34568 -- Made makemigrations --update respect --name option.

Thanks David Sanders for the report.

comment:15 by Mariusz Felisiak <felisiak.mariusz@…>, 18 months ago

In cdd970ae:

[4.2.x] Fixed #34568 -- Made makemigrations --update respect --name option.

Thanks David Sanders for the report.
Backport of c52f4295f254e1c14af769d22b1a5f516a941f58 from main

in reply to:  11 comment:16 by Natalia Bidart, 18 months ago

Replying to David Sanders:

One more thing:

--update is a destructive operation – if you had any customised migration operations or code in your latest migration this will be permanently deleted if you run update *without any warning*.

I'd like to suggest that *at least* one of the following happen:

  1. --update does a confirmation eg "<app>/migrations/0009_last_migration.py will be replaced. Proceed? y/N". Along with this we provide a --no-input. Both of these are consistent with other commands.
  2. we document that it destroys your last migration without warning

My preference is 1. because, to paraphrase FunkyBob, the purpose of any framework is to manage the risky and the tedious.

We should at the very least do 2. if it's decided 1. is a no-go.

This also sounds like another ticket.

I agree this should be another ticket, and I also agree overwriting without any warning feels like an antipattern. I would definitely go with (1).

Having said the above, are you positive overwrites are happening? If so, could you please provide the sequence of commands that would trigger that situation in the new ticket? Thanks!

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