Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing Default AutoField for Django 3.2+ #132

Closed
wants to merge 2 commits into from

Conversation

alysivji
Copy link

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.

@alysivji alysivji force-pushed the add-default-autofield branch from bd525a3 to 2bd064f Compare March 21, 2022 16:53
@codecov
Copy link

codecov bot commented Mar 21, 2022

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.

Copy link
Member

@MRigal MRigal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense!

@ticosax
Copy link
Member

ticosax commented Mar 21, 2022

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
Copy link
Author

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
Copy link
Member

ticosax commented Mar 21, 2022

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
Copy link
Author

alysivji commented Mar 21, 2022

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
Copy link
Member

ticosax commented Mar 22, 2022

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

@alysivji
Copy link
Author

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

  • How do you suggest I manage migrations for third-party languages in my project? Is there a folder structure you would recommend?
  • How is managing my own migrations different than forking the project? If this package adds a new migration, what will that mean for my application and my migration graph? To me it sounds like there would be multiple leaf nodes:
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
Copy link
Member

ticosax commented Mar 23, 2022

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
Copy link
Member

ticosax commented Mar 23, 2022

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
Copy link
Author

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
Copy link
Author

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

@alysivji alysivji closed this Mar 23, 2022
@alysivji alysivji deleted the add-default-autofield branch March 23, 2022 16:06
@holvi-mikael
Copy link

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
Copy link
Member

ticosax commented Mar 24, 2022

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
Copy link
Member

ticosax commented Mar 25, 2022

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

  • Would it work ?
  • Do we know other third party django app, that adopted this pattern ?

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
Copy link

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

@alysivji
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants