search

jazzband / django-fsm-log

Automatic logging for Django FSM
https://django-fsm-log.readthedocs.io/en/latest/
MIT License
242 stars 78 forks source link

Fixing Default AutoField for Django 3.2+ #132

Closed alysivji closed 2 years ago

alysivji commented 2 years ago

In Django 3.2, setting:

DEFAULT_AUTO_FIELD = "django.db.models.BigAutoField"

causes a migration to be created for this library.

Per Django docs, we can configure the default for each app by modifying the app config.

codecov[bot] commented 2 years ago

Codecov Report

Merging #132 (2bd064f) into master (42cecd3) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #132   +/-   ##
=======================================
  Coverage   95.04%   95.05%           
=======================================
  Files          25       25           
  Lines         505      506    +1     
=======================================
+ Hits          480      481    +1     
  Misses         25       25
Impacted Files Coverage Δ
django_fsm_log/apps.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 42cecd3...2bd064f. Read the comment docs.

ticosax commented 2 years ago

As far as I understand it, If the project (adopting django-fsm-log) prefers to use django.db.models.BigAutoField, the current behavior is working as intended. The project can own the migration to BigAutoField. It's not up to django-fsm-log library, to decide about which AutoField it should use, that decision belong to the adopting project.

alysivji commented 2 years ago

As far as I understand it, If the project (adopting django-fsm-log) prefers to use django.db.models.BigAutoField, the current behavior is working as intended. The project can own the migration to BigAutoField. It's not up to django-fsm-log library, to decide about which AutoField it should use, that decision belong to the adopting project.

The initial migration uses models.AutoField.

ticosax commented 2 years ago

As far as I understand it, If the project (adopting django-fsm-log) prefers to use django.db.models.BigAutoField, the current behavior is working as intended. The project can own the migration to BigAutoField. It's not up to django-fsm-log library, to decide about which AutoField it should use, that decision belong to the adopting project.

The initial migration uses models.AutoField.

Indeed, as intended, I would say, because it was the default at that time. Apparently your project, is configured to use BigAutoField. My understanding is that you also want to change django-fsm-log.StateLog.id to BigAutoField too ? if yes, is there is a possibility to own that data migration that changes the type of the id to BigAutoField ? just for your project. not for everyone.

What's your end goal here, changing the type of id field or silencing the warning ?

alysivji commented 2 years ago

I started a new Django 3.2 project with the following setting: DEFAULT_AUTO_FIELD = "django.db.models.BigAutoField"

When trying to use this library, it says I have un-applied migrations.

  Your models in app(s): '-----', 'django_fsm_log', '----' have changes that are not yet reflected in a migration, and so won't be applied.
  Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.

When I create migrations, it does the following:

Migrations for 'django_fsm_log':
  /usr/local/lib/python3.8/site-packages/django_fsm_log/migrations/0004_alter_statelog_id.py
    - Alter field id on statelog

There is now a new migration in my site-packages folder.

Running all migrations only runs migrations inside of my project's apps directory, does not run them for packages I pip installed.

This library made its design decision in the initial migration, i.e. the id field is an AutoField. My reasoning for this PR was to codify that decision inside the AppConfig.

Per Django 3.2 Release Notes:

To avoid unwanted migrations in the future ... configure it on a per-app basis:

ticosax commented 2 years ago

Did you tried moving the autogenerated migration file in your own project ?

alysivji commented 2 years ago

If I have to manage migrations independently for third-party packages, I do have a few questions:

0005_description_null (latest migration for this project)
|
|---- 0006_changing_id_to_bigautofield (my migration)
|
|---- 0006_migration (future migration somebody creates for this project)

Going back to your original point,

The project can own the migration to BigAutoField. It's not up to django-fsm-log library, to decide about which AutoField it should use, that decision belong to the adopting project.

django-fsm-log already decided what field to use in the initial migration. If the new decision is to support whatever the Django default is, I think that makes sense... but it should be handled by this library versus users having to manage this change themselves (not sure what that solution looks like).

As an aside, other libraries have implemented fixes to modify the application configuration to account for the design decision to use AutoField.

ticosax commented 2 years ago

You can bring the migration in the namespace of your project and it will be part of the history of your project. To do so, you would need to declare a double dependency in the migration class (dependencies property) https://docs.djangoproject.com/en/4.0/topics/migrations/#migration-files-1

it would look like

class Migration(migrations.Migration):

    dependencies = [
        ('your_project', '00XX_previous_migration'),
        ('django_fsm_log', '0004_auto_20190131_0341'),
    ]

    operations = [
        migrations.AlterField(
            model_name='statelog',
            name='id',
            field=models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID'),
        ),
    ]

edit: it's not tested.

ticosax commented 2 years ago

If the suggestion doesn't work, how about upgrading the django-fsm-log StateLog's id field to BigAutoField, instead of pinning it to AutoField, since it the default field shipped with django3.2 ?

alysivji commented 2 years ago

I don't have a preference for what id field StateLog uses, my opinion is that changing my project's Django settings should not cause migrations to be generated for third-party packages that I have to manage independently.

Let's walk through a hypothetical situation: I have an app running Django 3.0 with django-fsm and django-fsm-log to make my state machines more explicit and auditable. I plan to upgrade to Django 3.2 & update my settings file to use the new BigAutoField. This will result in a django-fsm-log migration that will require me to change the type of my primary key column. Running this migration in production will result in application downtime as the table will be locked during the alter column step. This is not ideal.

Going back to your original point:

The project can own the migration to BigAutoField. It's not up to django-fsm-log library, to decide about which AutoField it should use, that decision belongs to the adopting project.

django-fsm-log has already made a decision in its initial migration to use AutoField. By not hardcoding this decision, it can result in migrations that have downtime.

alysivji commented 2 years ago

I'm gonna explore a solution where I monkeypatch the config class so I don't have to manage migrations for third-party packages.

holvi-mikael commented 2 years ago

First of all, thank you for taking on the maintenance of this excellent package!

I have the same issue with getting "missing migrations" from this package, and would not prefer to add any confusing "fixes" to our system just to silence migration warnings.

Since BigAutoField has been available since Django 1.10, it would not seem excessive if django-fsm-log migrated to that as well.

ticosax commented 2 years ago

Based on this SO thread https://stackoverflow.com/questions/47153776/how-to-store-third-party-apps-migrations-in-django I managed to make it work.

diff --git a/your_project/settings.py b/your_project/settings.py
index 7ba021bf..57ad1756 100644
--- a/your_project/settings.py
+++ b/your_project/settings.py
@@ -58,6 +58,7 @@ class Django(Configuration):  # type: ignore
         'autocompletefilter',
         'taggit',
         'your_project.docs',
+        'your_project.third_parties.django_fsm_log_',
     ]

     MIDDLEWARE = [
@@ -107,7 +108,11 @@ class Django(Configuration):  # type: ignore
         conf['ATOMIC_REQUESTS'] = True
         return value

-    DEFAULT_AUTO_FIELD = 'django.db.models.AutoField'
+    DEFAULT_AUTO_FIELD = 'django.db.models.BigAutoField'
+    MIGRATION_MODULES = {
+        'django_fsm_log_': 'your_project.third_parties.django_fsm_log_.migrations_',
+    }
+

     # Password validation
     # https://docs.djangoproject.com/en/3.0/ref/settings/#auth-password-validators
diff --git a/your_project/third_parties/django_fsm_log_/migrations_/0001_lter_statelog_id.py b/your_project/third_parties/django_fsm_log_/migrations_/0001_lter_statelog_id.py
new file mode 100644
index 00000000..42967cb2
--- /dev/null
+++ b/your_project/third_parties/django_fsm_log_/migrations_/0001_lter_statelog_id.py
@@ -0,0 +1,22 @@
+# Generated by Django 4.0.3 on 2022-03-24 17:24
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    def __init__(self, name, app_label):
+        super().__init__(name, 'django_fsm_log')
+
+    dependencies = [
+        ('django_fsm_log', '0004_auto_20190131_0341'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='statelog',
+            name='id',
+            field=models.BigAutoField(
+                auto_created=True, primary_key=True, serialize=False, verbose_name='ID'
+            ),
+        ),
+    ]
diff --git a/your_project/third_parties/django_fsm_log_/migrations_/__init__.py b/your_project/third_parties/django_fsm_log_/migrations_/__init__.py
new file mode 100644
index 00000000..e69de29b
python manage.py migrate django_fsm_log_

A bit hackish though, maybe there is a better way ?

ticosax commented 2 years ago

This controversial issue, is maybe a sign that django-fsm-log should, maybe, not provide a concrete Model, but only an Abstract Model instead ?

If yes, I think, that's a direction we should explore, while keeping the concrete model for backward compatibility only. We would make lot of projects happy (#34)

holvi-mikael commented 2 years ago

@alysivji, did you figure out how to make the monkey patching approach work?

alysivji commented 2 years ago

@holvi-mikael I created a class that inherits the Django FSM application config and made my changes there. This SO answer is a good guide on the workflow I used.