From ccf2b7520464b1cb3b10f62c9ccf0ffcf0c2fd02 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Wed, 22 May 2024 13:52:53 -0400 Subject: [PATCH] fix: stabilize makemigrations when SITE_ID != 1 (#34787) Some models in third_party_auth used settings.SITE_ID as a field default, which caused Django to say migrations were out of sync whenever settings.SITE_ID happened to be anything other than 1 for any developer: Your models in app(s): 'third_party_auth' 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. This could easily happen if a developer is testing out site configuration or site-specific theming and ends up with a SITE_ID other than 1. The fix, inspired by a StackOverflow answer [1], is to simply create a wrapper function for the dynamic default value. The wrapper function, rather than the current value of SITE_ID, will be serialized to the migraiton file. This commit includes a migration file, but from a database perspective, the migration is a no-op. [1] https://stackoverflow.com/a/12654998 --- .../0013_default_site_id_wrapper_function.py | 36 +++++++++++++++++++ common/djangoapps/third_party_auth/models.py | 22 ++++++++++-- 2 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 common/djangoapps/third_party_auth/migrations/0013_default_site_id_wrapper_function.py diff --git a/common/djangoapps/third_party_auth/migrations/0013_default_site_id_wrapper_function.py b/common/djangoapps/third_party_auth/migrations/0013_default_site_id_wrapper_function.py new file mode 100644 index 0000000000..250e537085 --- /dev/null +++ b/common/djangoapps/third_party_auth/migrations/0013_default_site_id_wrapper_function.py @@ -0,0 +1,36 @@ +# Generated by Django 4.2.13 on 2024-05-13 19:22 + +import common.djangoapps.third_party_auth.models +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('sites', '0002_alter_domain_unique'), + ('third_party_auth', '0012_alter_ltiproviderconfig_site_and_more'), + ] + + operations = [ + migrations.AlterField( + model_name='ltiproviderconfig', + name='site', + field=models.ForeignKey(default=common.djangoapps.third_party_auth.models._get_site_id_from_settings, help_text='The Site that this provider configuration belongs to.', on_delete=django.db.models.deletion.CASCADE, related_name='%(class)ss', to='sites.site'), + ), + migrations.AlterField( + model_name='oauth2providerconfig', + name='site', + field=models.ForeignKey(default=common.djangoapps.third_party_auth.models._get_site_id_from_settings, help_text='The Site that this provider configuration belongs to.', on_delete=django.db.models.deletion.CASCADE, related_name='%(class)ss', to='sites.site'), + ), + migrations.AlterField( + model_name='samlconfiguration', + name='site', + field=models.ForeignKey(default=common.djangoapps.third_party_auth.models._get_site_id_from_settings, help_text='The Site that this SAML configuration belongs to.', on_delete=django.db.models.deletion.CASCADE, related_name='%(class)ss', to='sites.site'), + ), + migrations.AlterField( + model_name='samlproviderconfig', + name='site', + field=models.ForeignKey(default=common.djangoapps.third_party_auth.models._get_site_id_from_settings, help_text='The Site that this provider configuration belongs to.', on_delete=django.db.models.deletion.CASCADE, related_name='%(class)ss', to='sites.site'), + ), + ] diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index 1ea265e868..6d244d96ed 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -88,6 +88,24 @@ class AuthNotConfigured(SocialAuthBaseException): ) +def _get_site_id_from_settings() -> int: + """ + Simply return SITE_ID from settings. + + We define this function so the current SITE_ID can be used as the default value on a + couple fields in this module. We can't use `settings.SITE_ID` directly in the model class, + because then the migration file will contain whatever the value of SITE_ID was when the + migration was created. This value is usually 1, but in the event that a developer is + running a non-default site, they would get a value like 2 or 3, which will result in + a dirty migration state. By defining this wrapper function, the name + `_get_site_id_from_settings` will be serialized to the migration file as the default, + regardless of what any developer's active SITE_ID is. + + Reference: https://docs.djangoproject.com/en/dev/ref/models/fields/#default + """ + return settings.SITE_ID + + class ProviderConfig(ConfigurationModel): """ Abstract Base Class for configuring a third_party_auth provider @@ -143,7 +161,7 @@ class ProviderConfig(ConfigurationModel): ) site = models.ForeignKey( Site, - default=settings.SITE_ID, + default=_get_site_id_from_settings, related_name='%(class)ss', help_text=_( 'The Site that this provider configuration belongs to.' @@ -455,7 +473,7 @@ class SAMLConfiguration(ConfigurationModel): KEY_FIELDS = ('site_id', 'slug') site = models.ForeignKey( Site, - default=settings.SITE_ID, + default=_get_site_id_from_settings, related_name='%(class)ss', help_text=_( 'The Site that this SAML configuration belongs to.'