pennersr / django-allauth

Integrated set of Django applications addressing authentication, registration, account management as well as 3rd party (social) account authentication.
https://allauth.org
MIT License
9.12k stars 2.98k forks source link

Migrations are added when social account providers are added or removed #2652

Closed johanneswilm closed 3 years ago

johanneswilm commented 3 years ago

Hey, using Django 3.1, when I add or remove an entry like

"allauth.socialaccount.providers.google",

in INSTALLED_APPS, Django notifies me that I need to run ./manage.py makemigrations and then creates migrations inside of the socialaccount app. The reason for this seems to be that the way the model is written, the list of possible choices is calculated rather than handing over a callable.

This:

provider = models.CharField(verbose_name=_('provider'),
                                max_length=30,
                                choices=providers.registry.as_choices())

Rather than:

provider = models.CharField(verbose_name=_('provider'),
                                max_length=30,
                                choices=providers.registry.as_choices)

I see this has been an issue for a long time and some seem to be able to get around it, but I haven't been able to figure out exactly how they do it.

Any idea why it's like this and what is the best workaround?

iarp commented 3 years ago

What version of allauth? Do you have any allauth based settings set?

johanneswilm commented 3 years ago

django-allauth==0.41.0

########## ALLAUTH CONFIGURATION
ACCOUNT_EMAIL_REQUIRED = True
ACCOUNT_EMAIL_VERIFICATION = "mandatory"
ACCOUNT_UNIQUE_EMAIL = True
ACCOUNT_FORMS = {
    "signup": "common.forms.CustomSignupForm",
}

########## END ALLAUTH CONFIGURATION
iarp commented 3 years ago

Have you tried reinstalling django-allauth? I wonder if a migration was made accidentally at some point and now its causing the trigger. Essentially you should only have these exact four files in the socialaccount migrations.

johanneswilm commented 3 years ago

Hey @iarp That is true. I have those four files. But Django notifies me that something is missing and when I run ./manage.py makemigrations it creates a migration 0004 for socialaccount. This won't work as there might at some time be a migration 0004 that is provided from upstream.

As for reinstalling: The problem is I am on a big project where there are many installations running on other machines run by other devs and servers and such. I cannot just uninstall and reinstall on all these other machines as I don't control them.

As far as I can tell, the problem is that the way the model is written and also the migration, means that the list of providers will be taken by executing providers.registry.as_choices() the first time the migration is run. When then later the list of providers changes, Django will add more migrations to socialaccount if you run ./migrate makemigrations.

To be entirely honest, I also use django allauth on a different project where I have changed the list of providers over time, and it has for some reason not caused any migrations to be created. I cannot figure out why that is.

iarp commented 3 years ago

What version of python?

You can't just remove the brackets because django requires choices to be an iterable and not a callable. as_choices() yields from a dict so I have to think that perhaps the order of the dict is not maintaining between comparison.

johanneswilm commented 3 years ago

Python 3.8.3.

The change I am making is that previously none of the socialaccount providers were enabled. Now Google is enabled.

So the migration Django wants to autocreate looks like this:

# Generated by Django 3.1 on 2020-08-31 13:07

from django.db import migrations, models

class Migration(migrations.Migration):

    dependencies = [
        ('socialaccount', '0003_extra_data_default_dict'),
    ]

    operations = [
        migrations.AlterField(
            model_name='socialaccount',
            name='provider',
            field=models.CharField(choices=[('google', 'Google')], max_length=30, verbose_name='provider'),
        ),
        migrations.AlterField(
            model_name='socialapp',
            name='provider',
            field=models.CharField(choices=[('google', 'Google')], max_length=30, verbose_name='provider'),
        ),
    ]

That seems to be entirely reasonable.

johanneswilm commented 3 years ago

You can't just remove the brackets because django requires choices to be an iterable and not a callable.

Ah I see. My bad.

I wonder if storing this in the model is the right place then. Maybe it would be better to enforce it at the level of the form that is being shown on the admin page?

If you run a site for several years, I think it's fairly common that your list of connected social sites will change over time. Some sites close down, others are getting more important, some change focus, etc. . So it's probably not a good idea to have to lock in on a list right at the beginning of starting a site ro system of sites, right?

I am wondering what others have been doing to avoid this problem.

iarp commented 3 years ago

That's the thing though, there is no locked in list. Since allauth uses as_choices() its building the list of choices for every migration file when django checks over the migration files for changes.

If you're up to it you could zip up your allauth folder and share it, i'll look it over and see if i can see anything.

Beyond that though I'm not sure. I matched every version of django, allauth, and python that you supplied and i can add/remove providers at will and makemigrations never has a problem.

johanneswilm commented 3 years ago

That's the thing though, there is no locked in list. Since allauth uses as_choices() its building the list of choices for every migration file when django checks over the migration files for changes.

I see. Maybe there have been some changes in how Django does that? Or maybe some changes per database? I am using postgresql, if that makes a difference. I have seen the issue has come up before [1], so maybe relying on it not actually storing the choices anywhere doesn't work under some specific setups?

If you're up to it you could zip up your allauth folder and share it, i'll look it over and see if i can see anything.

I don't have a special allauth folder that is different from what you people distribute. I just install allauth using:

pip install django-allauth

So it tries to create a migration file in that place: /home/johannes/.pyenv/versions/3.8.3/lib/python3.8/site-packages/allauth/socialaccount/migrations/0004_blablabla.py

[1] https://github.com/pennersr/django-allauth/issues/749

iarp commented 3 years ago

Migrations do not depend on the database, they are based on the existing migration files and the current models. The model and the initial migration are based on as_choices(). As long as the output of as_choices() at the time makemigrations is called is the exact same in both model and migration file then there is no change detected regardless of adding or removing a provider.

Somehow either one of two things has happened:

There is also a possibility of some sort of obscure issue is changing how providers are being loaded between model initialization and migration checks. For instance if I put print(provider_cls.id, provider_cls.name) just above the yield in the as_choices() method, When I run makemigrations while having google as the only provider, i see google printed 4 times. Once for SocialApp class initialization and again for SocialAccount class initialization, then two more times for the migration of SocialApp and SocialAccount. If you did the same and don't see 4 then something is wrong.

johanneswilm commented 3 years ago

Ah, thanks! I found it. The iwas apparently that I changed the name of the Google Provider in the top level urls.py file:

from allauth.socialaccount.providers.google.provider import GoogleProvider

GoogleProvider.name = "Alphabet"  # Changes name of google button on login page.

This change was then not applied during the initial migration but was applied when checking the model. I have entirely removed this now and just put it in the template instead.

Thanks so much, @iarp for helping with this!

iarp commented 3 years ago

Out of curiosity how did you find that as being the issue ?

johanneswilm commented 3 years ago

I added the print line you mentioned and the output was:

google Google
google Google
google Alphabet
google Alphabet