Fixed #29843 -- Manage contenttypes and permissions using hooks during makemigrations

[arthurio]

Overview

This is a different approach from #10540. It makes things a bit easier to control and more declarative as the added operation(s) will be in the migration file(s).

CC @taylor-cedar

How it would be used

# library/apps.py
from django.apps import AppConfig
from django.db.migrations.autodetector import (
    register_post_add_operation_hook,
    register_pre_add_operation_hook,
)
from django.db.migrations.operations import AddField, CreateModel, RunPython


class DummyOperation(RunPython):
    def database_forwards(self, app_label, schema_editor, from_state, to_state):
        print("Forward!")

    def database_backwards(self, app_label, schema_editor, from_state, to_state):
        print("Backward...")


def nothing(*args, **kwargs):
    pass


def pre_create_model_hook(operation, app_label, from_state, to_state, dependencies):
    operation = DummyOperation(nothing)
    return [operation]


def post_add_field_hook(operation, app_label, from_state, to_state, dependencies):
    operation = DummyOperation(nothing)
    return [operation]


class LibraryConfig(AppConfig):
    name = "library"

    def ready(self):
        register_pre_add_operation_hook(CreateModel, pre_create_model_hook)
        register_post_add_operation_hook(AddField, post_add_field_hook)

Example of output:

# Generated by Django 4.0 on 2021-04-07 02:48

from django.db import migrations, models
import django.db.models.deletion
import library.apps


class Migration(migrations.Migration):

    dependencies = [
        ('library', '0001_initial'),
    ]

    operations = [
        library.apps.DummyOperation(
            code=library.apps.nothing,
        ),
        migrations.CreateModel(
            name='City',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
            ],
        ),
        library.apps.DummyOperation(
            code=library.apps.nothing,
        ),
        migrations.CreateModel(
            name='Signature',
            fields=[
                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
                ('value', models.CharField(max_length=30)),
                ('author', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, to='library.author')),
            ],
        ),
        library.apps.DummyOperation(
            code=library.apps.nothing,
        ),
        migrations.AddField(
            model_name='city',
            name='author',
            field=models.ManyToManyField(to='library.Author'),
        ),
        library.apps.DummyOperation(
            code=library.apps.nothing,
        ),
    ]

[auvipy]

why no one notices this!

[felixxm]

why no one notices this!

I'm not sure how this comment could be helpful.

[felixxm]

Closed by mistake, wrong button, sorry 🤦

[arthurio]

@felixxm I think @auvipy is really excited about this PR and wondering why nobody is paying attention to it 😄
I think it'd be a cool addition but is definitely not trivial to implement. Please see latest comment on ticket: https://code.djangoproject.com/ticket/29843 for more info about how this approach is different from the previous PR.

[felixxm]

I think it'd be a cool addition but is definitely not trivial to implement. Please see latest comment on ticket: https://code.djangoproject.com/ticket/29843 for more info about how this approach is different from the previous PR.

Personally, I really like this idea, it's much better than using signals which is always error prone. Detailed documentation will be crucial. We also need to remember that folks will push it to the limit, so we need to think what to do about:

• recursive hooks, CreateModel ---hook--> AddField ---hook---> CreateModel ---> 🔥 . Maybe hooks should be forced to return the subclasses of RunSQL and RunPython, and at the same time hooks couldn't be registered for such operations 🤔 This should get rid of many edge cases.
• hooks that return multiple operations while some of them have other hooks registered. Maybe hooks should always return a single operation.
• hooks defined in a different app, etc.

[auvipy]

I think it'd be a cool addition but is definitely not trivial to implement. Please see latest comment on ticket: https://code.djangoproject.com/ticket/29843 for more info about how this approach is different from the previous PR.

Personally, I really like this idea, it's much better than using signals which is always error prone. Detailed documentation will be crucial. We also need to remember that folks will push it to the limit, so we need to think what to do about:

* recursive hooks, `CreateModel ---hook--> AddField ---hook---> CreateModel ---> `fire . Maybe hooks should be forced to return the subclasses of `RunSQL` and `RunPython`, and at the same time hooks couldn't be registered for such operations thinking This should get rid of many edge cases.

* hooks that return multiple operations while some of them have other hooks registered. Maybe hooks should always return a single operation.

* hooks defined in a different app, etc.

He felix no offense :D the comment also includes me :D aswell. i know you are very busy with django 4.0 and 3.2 release :D

[taylor-cedar]

recursive hooks, CreateModel ---hook--> AddField ---hook---> CreateModel ---> 🔥 . Maybe hooks should be forced to return the subclasses of RunSQL and RunPython, and at the same time hooks couldn't be registered for such operations 🤔 This should get rid of many edge cases.

That is a good call out that @arthurio and I thought about. It's really powerful to be able to add hooks based on other hooks, but we don't have anything right now, so I would be OK to not allow recursion for now. I can't think of many use cases for to need CreateModel or AddField, so we can probably get away with RunSQL and RunPython

hooks that return multiple operations while some of them have other hooks registered. Maybe hooks should always return a single operation.

I would prefer to allow it to return multiple operations. Otherwise someone will just be creating some kind of multiple operation themselves to allow reusability

return MultiOperation([
  Operation1(...),
  Operation2(...),
 ]);

[felixxm]

@arthurio Do you have time to keep working on this?

[arthurio]

@felixxm I think it's a good idea to pursue but unless someone is willing to pair on it with me I don't think I can get back to it in the near future.

[felixxm]

@arthurio np, thanks for all your efforts :+1 Feel-free to open a new PR.