ModelBase metaclass implementation prevents addition of model fields via __init_subclass__

[hottwaj]

The current implementation of ModelBase.new prevents addition of model fields via python's init_subclass method (described in PEP 487 and provided since python 3.6).

My understanding is that currently, fields can be added to models outside of a class body by:
i) calling "model_cls.add_to_class(...)" after the definition of model_cls e.g. in module level code after class definition,
ii) using a class decorator to call .add_to_class as in point i), or
iii) using a metaclass to completely customise class creation

Inheritance and init_subclass should in theory provide a relatively straightforward way to customise class creation without using a metaclass (option iii above), as described/encouraged in PEP 487, but Django does not currently support this for Field attributes of subclasses of Model because the ModelBase metaclass does not correctly pickup Fields added to a class during the execution of init_subclass

It seems that init_subclass is called after ModelBase.new collects Fields that require calling of their "contribute_to_class" method, so ModelBase does not do appropriate bookkeeping on fields added in init_subclass and such Fields are then ultimately not "seen" by e.g. the migration builder.

Correctly collecting Fields added by init_subclass in ModelBase.new would allow for easier customisation of model fields outside of a model class body and provide an implementation that works with init_subclass in a way that matches (rather than contrary to) behaviour supported elsewhere in python.

A simple example that currently does not work, but I believe ought to work, is provided below. In this example, the "author" attribute added in BaseBookModel.init_subclassis not collected by current implementation of ModelBase.new so is not created in migrations and not available as might be expected in subclass Book.

from django.db.models import Model, ForeignKey, CASCADE, CharField

class BaseBookModel(Model):
    class Meta:
        abstract = True

    def __init_subclass__(cls, author_model_cls: Type[Model], **kwargs,):
        super().__init_subclass__(**kwargs)
        author = ForeignKey(author_model_cls, on_delete = CASCADE)
        cls.add_to_class('author', author)

class Author(Model):
    name = CharField(max_len = 256, unique = True)

class Book(BaseBookModel, author_model_cls = Author):
    pass

Thanks for reading and appreciate any thoughts!

[Mariusz Felisiak]

Thanks for this ticket, however, it seems that you're rather asking for help in your implementation. Trac is not an appropriate channel for this. You can start a discussion on the DevelopersMailingList or on the ​Django Forum, where you'll reach a wider audience and see what other think.

[hottwaj]

Thanks for taking time to review. I guess I did not phrase the issue very well - I am not looking for help implementing something, but rather think there is an issue with the way the ModelBase metaclass interacts with init_subclass which prevents model fields being added dynamically via the latter.

I have re-written the issue to clarify and would appreciate if you could take a second look. Thanks!

[Simon Charette]

Adapting ModeBase.__new__ to play better with __init_subclass__ and possibly pave the way for some deprecation is one thing but maintaining backward compatibility (or at least providing a bullet proof deprecation path) with the hundreds of third-party applications that rely on the current implementation is-far from trivial. For your particular use case to work, that is cls._meta being already present at __init_subclass__ time, the whole concept of add_to_class would have to be thrown away as object.__new__ (aka super().__new__ in ModeBase.__new__) triggers __init_subclass__.

That's also the reason why contribute_to_class is still relevant today even if recent versions of Python introduced a __set_name__ hook that is a strong contender to replace it.

If you can demonstrate that there is a way to achieve such thing (e.g. a PoC PR) and that there are strong enough benefits to warrant the burden this will incur on third party application that have been using __new__ for years and might need to be adjusted then I believe you might have a stronger case than by asking for this request to be adjusted in its current form.

I'll be closing as wontfix until it can be demonstrated that this is somewhat technically achievable at least.

[hottwaj]

Thanks, I completely agree that this change should not break the existing API in any way.

Please take a look at proposed fix and test added in this PR:
​https://github.com/django/django/pull/16849

[hottwaj]

As I've added a PoC PR and in case it is needed to continue the process I'm reopening - hope I'm not generating too much noise :)
Thanks!

[Mariusz Felisiak]

I appreciate you'd like to reopen the ticket, but please ​follow the triaging guidelines with regards to wontfix tickets and take this to DevelopersMailingList.

[Simon Charette]

Thanks for the PoC, the PR is definitely less invasive than I initially envisioned so my main concerns are addressed. I do agree with Mariusz that this should be brought to the scrutiny of the mailing list though to gather more feedback on the approach and evaluate the appetite of the community for such a change.

[hottwaj]

Great, thanks Simon & Mariusz. I've sent an email to the mailing list as requested, think it is just waiting approval, so will wait to hear on further feedback there. Cheers!

[Carlton Gibson]

Related too is #27880 Use __set_name__ to replace some usages of contribute_to_class, I think