Fixed #23521 -- Support changing model bases in migrations#11222
Fixed #23521 -- Support changing model bases in migrations#11222LilyFirefly wants to merge 1 commit intodjango:masterfrom
Conversation
LilyFirefly
force-pushed
the
ticket_23521
branch
from
6646b18 to
d50c3df
Compare
LilyFirefly
force-pushed
the
ticket_23521
branch
from
d50c3df to
f94b8c9
Compare
felixxm
left a comment
felixxm
left a comment
There was a problem hiding this comment.
@ian-foote Thanks for this patch 👍 I left some comments.
@MarkusH @charettes At first glance it looks good 🚀 Can you take a look?
| pass | ||
|
|
||
| def describe(self): | ||
| return "Update %s bases to %s" % (self.model_name, self.bases) |
There was a problem hiding this comment.
I would rename self.model_name -> self.name (like in CreateModel, DeleteModel, or RenameModel).
|
|
||
| def __init__(self, model_name, bases): | ||
| self.model_name = model_name | ||
| self.bases = bases |
There was a problem hiding this comment.
Should we add sanity-check for duplicates in bases, _check_for_duplicates()? (e.g. CreateModel.__init__()).
There was a problem hiding this comment.
name should be used to follow ModelOperation.__init__ signature and super().__init__(name) should be called.
| parts are ``AlterModelBases`` to tell django what the new model hierarchy is | ||
| and ``AlterField`` to convert the implicit ``OneToOneField`` into an implicit | ||
| ``AutoField`` primary key. The ``RenameField` is useful to avoid needing to add | ||
| the ``parent_ptr`` field explicitly to the updated ``Child`` model. |
There was a problem hiding this comment.
I'm not sure if starting from an example is a good way to describe use cases 🤔 AlterModelBases should be used also in other cases e.g. renaming parent.
Maybe:
If you want to change parent models with
:ref:`multi-table inheritance <multi-table-inheritance>` you need to write a
custom migration e.g. to rename a parent model or make a child model
independent. ...
charettes
left a comment
charettes
left a comment
There was a problem hiding this comment.
Something that is problematic with the current approach is that AlterModelBases doesn't implement adding bases or any auto-detector or reduction logic.
In fact the way AlterModelBases is currently implemented is a specialized state only operation that needs manual migration generation by users. It's more of a DisconnectModelBase than anything else.
I think the experience would be better if AlterModelBases could handle or error out on base addition (e.g. raise NotImplementedError) and that the auto-detector was able to generate the appropriate migration operation when the last concrete parent is removed from a child model class and a parent pointer (_ptr) is implicitly turned into an AutoField primary key.
|
|
||
| def __init__(self, model_name, bases): | ||
| self.model_name = model_name | ||
| self.bases = bases |
There was a problem hiding this comment.
name should be used to follow ModelOperation.__init__ signature and super().__init__(name) should be called.
|
This is a slightly aging PR, but I just ran into this while working on refactor work for our https://foundation.mozilla.org project, with no docs or blog posts or even SO questions that explained how to properly perform this task, so I posted https://stackoverflow.com/questions/61665607/renaming-a-django-superclass-model-and-updating-the-subclass-pointers-correctly/61723620, which garnered an answer that pointed to this PR: it would be fantastic if this PR could find its way into the codebase, making that SO question a matter of "This works out of the box as of 3.x.y, just run |
|
I might get back to this sometime, but I'm quite busy with work at the moment, so if someone else wants to take it and get it into shape, that's ok by me. The major work that still needs to be done is integrating with |
|
@felixxm why did this get closed? |
|
Because @ian-foote don't have time to work on it. You can feel-free to continue his work. |
|
Given that the PR is almost done, leaving it open with an invitation to get it finished would certainly be preferable: even if I personally don't have time, I do have money to maybe find someone to pull this over the finishing line, since this is an important PR that would have saved us several days of being blocked. Sponsoring that work is perfectly acceptable. |
|
@Pomax, Hi, it’s me the author of the answer of your question on SO. I would like to work on this. But probably will need some guidance from core dev team. |
|
@felixxm would you know whether someone from the core team would be willing to mentor this issue? |
|
Core team doesn't exist anymore, you can ask for a guidance on the developers mailing list or on forum.
The remaining part is not trivial.
Not really, leaving abandoned PRs open doesn't change anything. Ticket is marked as a ticket with patch that needs improvement, so anyone can finish it.
We don't allow donors to directly sponsor specific feature, see "Can I choose a specific feature or project to support?". |
|
That is unfortunate. @imomaliev it sounds like you'll want to pick up this thread over on https://code.djangoproject.com/ticket/23521 |
5 participants
Also document how to use the new
AlterModelBasesoperation to refactor a concrete model inheritance into a stand-alone model.https://code.djangoproject.com/ticket/23521