Migrations and swappable models/AUTH_USER_MODEL

[Andrew Godwin]

So, the last major bug left in migrations that I'm aware of (bar the completion of the GIS backends) is dealing with swappable models - in particular, of course, AUTH_USER_MODEL.

As long as you're not using any third-party apps, then everything works fine, but as soon as third-party apps with migrations come into the picture there's an issue. These apps have to ship with migrations that reference any project's current user model - and the way swapping is currently done means that ForeignKeys end up with concrete references to the user model _at the time the migration was made_.

There are a couple of potential approaches here:

1) Introduce a new value that can be used as a "to" parameter on ForeignKeys which resolves to a swapped model in its internal string-to-model converter. I'm thinking something like "__swapped__.AUTH_USER_MODEL" - I would use settings here, but I can imagine people having an app called settings.

The problem here is I'd then have to make ForeignKey deconstruct to this special value if the model it was pointing to was the replacement model - whether or not it's got settings.AUTH_USER_MODEL in the models.py file (there's no way to tell if it has an actual string or a setting reference in its models.py declaration). This means that some FKs might be too eagerly converted into swapped references.

1b) Like 1), but rather than having the value automatically inserted, require apps to actually use this new string rather than a direct reference to settings.AUTH_USER_MODEL, meaning we can tell if it's swapped or not and deconstruct it appropriately.

2) Change ForeignKey's constructor to automatically point itself to the new swapped-in model if it's pointing to the model that was swapped out (so ForeignKey("auth.User") automatically becomes ForeignKey("myapp.CustomUser") internally.

Now, I prefer the cleanliness of 2), but I suspect there was a reason this wasn't done when swappable models were first introduced; that said, I haven't seen any coding patterns in the wild this would disrupt.

Opinions?

Andrew

[Shai Berger]

Instinctively, I'm almost -1 on 2); I'm not too happy about 1) either. If a
model says it has a FK to auth.User, that shouldn't mean anything other than
auth.User. I don't see that as cleanliness -- it's effectively monkeypatching.

1b seems to me like the most reasonable option.

However, I've had a different thought about this:

> As long as you're not using any third-party apps, then everything works
> fine

No, it doesn't really. You can't change the user model as part of the project
evolution. Supporting this raises a whole set of additional problems -- even
with 1b, not to mention the scenarios where we try to guess swappables from
concrete models that happen to be their sources or targets.

This is a problem that bugs me personally -- I'm involved in a big project
that evolved since Django 1.2. As 1.4 (no swappable user model) is LTS, I'm
certain it's not just me.

Shai.

[Andrew Godwin]

Instinctively, I'm almost -1 on 2); I'm not too happy about 1) either. If a
model says it has a FK to auth.User, that shouldn't mean anything other than
auth.User. I don't see that as cleanliness -- it's effectively monkeypatching.

1b seems to me like the most reasonable option.

It's also the least user-friendly option. I don't see the same problem with 2) - sure, the model says it has an FK to auth.user, and your settings say that auth.user is swapped out in favour of another model. I think it would actually be worse if we kept it like it is - since auth.user is swapped out, it shouldn't have a table made for it, so how are you even going to make an FK to it? (this is how migrations currently fails - there's no auth.user left for it to refer to).

 

However, I've had a different thought about this:

> As long as you're not using any third-party apps, then everything works
> fine

No, it doesn't really. You can't change the user model as part of the project
evolution. Supporting this raises a whole set of additional problems -- even
with 1b, not to mention the scenarios where we try to guess swappables from
concrete models that happen to be their sources or targets.

This is a problem that bugs me personally -- I'm involved in a big project
that evolved since Django 1.2. As 1.4 (no swappable user model) is LTS, I'm
certain it's not just me.

Changing the user model during the project's lifespan is a different task. Migrations will nominally support it - in that it'll change all your FK constraints to immediately point to the new table - but you still have to manage moving the data into that table yourself, and I'm not sure we can do it any other way. Migrations sees you changing the user model as an actionable change, since as far as it's concerned you changed a load of "to" arguments of foreignkeys when you changed the setting, and so it can make migrations for that.

The problem is when the migrations are pre-made - i.e. from third-party apps - and that's the issue I'm trying to solve.

Andrew 

[Marc Tamlyn]

Personally, I like approach 2. It means that if we (or another 3rd party app) were to make a model swappable, then migrations which don't know about this change would continue to work.

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAFwN1uoR0h%3D%2BjOH-12%2BCrMnUH9kqYLpYyt-80SSBEA1T0aB5Nw%40mail.gmail.com.

For more options, visit https://groups.google.com/groups/opt_out.

[Shai Berger]

On Wednesday 08 January 2014 20:00:25 Andrew Godwin wrote:
> > Instinctively, I'm almost -1 on 2); I'm not too happy about 1) either. If
> > a model says it has a FK to auth.User, that shouldn't mean anything
> > other than
> > auth.User. I don't see that as cleanliness -- it's effectively
> > monkeypatching.
> >
> > 1b seems to me like the most reasonable option.
>
> It's also the least user-friendly option. I don't see the same problem with
> 2) - sure, the model says it has an FK to auth.user, and your settings say
> that auth.user is swapped out in favour of another model. I think it would
> actually be worse if we kept it like it is - since auth.user is swapped
> out, it shouldn't have a table made for it, so how are you even going to
> make an FK to it? (this is how migrations currently fails - there's no
> auth.user left for it to refer to).
>

I don't see how "you need to remember that auth.User really means some model
defined in settings" is more user friendly than "you need to remember that
__swappable__.X means that X is swappable". More so, as people already use
auth.get_user_model() -- so, you can monkeypatch *that* when generating
migrations, and let them just keep doing what they do today.

> > However, I've had a different thought about this:
> > > As long as you're not using any third-party apps, then everything works
> > > fine
> >
> > No, it doesn't really. You can't change the user model as part of the
> > project
> > evolution. Supporting this raises a whole set of additional problems --
> > even
> > with 1b, not to mention the scenarios where we try to guess swappables
> > from concrete models that happen to be their sources or targets.
>

> Changing the user model during the project's lifespan is a different task.
> Migrations will nominally support it - in that it'll change all your FK
> constraints to immediately point to the new table - but you still have to
> manage moving the data into that table yourself, and I'm not sure we can do
> it any other way. Migrations sees you changing the user model as an
> actionable change, since as far as it's concerned you changed a load of
> "to" arguments of foreignkeys when you changed the setting, and so it can
> make migrations for that.
>

No, not with any of the three suggestions. The migrations machinery doesn't
introspect the database to find the FK targets -- it re-rolls the migrations in
memory; and if any of the suggestions is taken, then when you run the
migrations with the setting pointing at some model, it will seem like it has
always been that way.

> The problem is when the migrations are pre-made - i.e. from third-party
> apps - and that's the issue I'm trying to solve.
>

The two problems are in conflict -- for evolving the user model, history must
matter; for pre-made migrations, the choice of user model needs to look like
it has always been the way it is "now" (at the time the migrations are run).

Shai.

[Raphaël Barrois]

On Wed, 8 Jan 2014 20:27:54 +0200
Shai Berger <sh...@platonix.com> wrote:

> On Wednesday 08 January 2014 20:00:25 Andrew Godwin wrote:
> > > Instinctively, I'm almost -1 on 2); I'm not too happy about 1)
> > > either. If a model says it has a FK to auth.User, that shouldn't
> > > mean anything other than
> > > auth.User. I don't see that as cleanliness -- it's effectively
> > > monkeypatching.
> > >
> > > 1b seems to me like the most reasonable option.
> >
> > It's also the least user-friendly option. I don't see the same
> > problem with 2) - sure, the model says it has an FK to auth.user,
> > and your settings say that auth.user is swapped out in favour of
> > another model. I think it would actually be worse if we kept it
> > like it is - since auth.user is swapped out, it shouldn't have a
> > table made for it, so how are you even going to make an FK to it?
> > (this is how migrations currently fails - there's no auth.user left
> > for it to refer to).
> >
>
> I don't see how "you need to remember that auth.User really means
> some model defined in settings" is more user friendly than "you need
> to remember that __swappable__.X means that X is swappable". More so,
> as people already use auth.get_user_model() -- so, you can
> monkeypatch *that* when generating migrations, and let them just keep
> doing what they do today.

Actually, people don't use auth.get_user_model() in their models declaration, they use ``settings.AUTH_USER_MODEL``,
as described in the documentation: https://docs.djangoproject.com/en/1.6/topics/auth/customizing/#referencing-the-user-model

The actual user model may not be prepared at the time the ``models.py`` is loaded, thus failing to import.
From a migrations point of view, this means that the ForeignKey is declared exactly as if the user had actually, manually written ``ForeignKey('auth.User')`` instead of ``settings.AUTH_USER_MODEL``.

I think that's why Andrew suggests to either use a special placeholder (``__swapped__.X``) or to automatically detect swapped models from their actual ``class Meta: swappable=True`` declaration.

Actually, you have two problems with a third-party app:

- It needs to have a way to specify "I want a FK to whatever the user has chosen for a AUTH_USER_MODEL", no matter the *type* of its PK field.
=> This mustn't depend on what the third-party-app developer's actual AUTH_USER_MODEL setting was while generating the migration
- Ideally, if the user changes the model, this should be helped by migrations, for instance by put a warning e.g "Looks like you're changing a swappable model, where should I put automatic related migrations ? (these migrations shouldn't go into the app with the FKey to AUTH_USER_MODEL, which may be out of the user's control — e.g third party apps).

Another problem is apps that provide a swappable model whose default is AUTH_USER_MODEL but that may be customized through their own setting,
for instance ``return getattr(settings, 'MY_TARGET_MODEL', settings.AUTH_USER_MODEL)``.


--
Raphaël

[Andrew Godwin]

I don't see how "you need to remember that auth.User really means some model
defined in settings" is more user friendly than "you need to remember that
__swappable__.X means that X is swappable". More so, as people already use
auth.get_user_model() -- so, you can monkeypatch *that* when generating
migrations, and let them just keep doing what they do today.

That's not what people use, they use settings.AUTH_USER_MODEL. See Raphaël's email about that.

No, not with any of the three suggestions. The migrations machinery doesn't
introspect the database to find the FK targets -- it re-rolls the migrations in
memory; and if any of the suggestions is taken, then when you run the
migrations with the setting pointing at some model, it will seem like it has
always been that way.

No, as the setting only exists in the models file - by the time it gets into a migration (the things which are re-run) it's been baked into a real model reference (of whatever the setting was at the time). Option 1b would preserve this behaviour by default; the other two eradicate it and make history changing of AUTH_USER_MODEL invisible in two different ways.

 

> The problem is when the migrations are pre-made - i.e. from third-party
> apps - and that's the issue I'm trying to solve.
>

The two problems are in conflict -- for evolving the user model, history must
matter; for pre-made migrations, the choice of user model needs to look like
it has always been the way it is "now" (at the time the migrations are run).

Yes, and I personally think the pre-made migrations problem is more important. I don't think people should change user model mid-project on a whim - there's a lot of manual work they'd always have to do to port all their user data over losslessly ignoring just this foreign key problem - and if they do, I think we recommend they start again with their migrations.

The only solution that I can see which somehow gives something to both sides - 1b - makes it really, really easy for third-party apps to ship broken migrations and not know about it, and I don't like that. I think we have to sacrifice being able to tell that the user model was changed in favour of having the app ecosystem actually work.

Andrew

[Russell Keith-Magee]

The implementation of swappable models is just exploiting the pre-existing late-binding feature of Django's model tools - a string reference to a model class is always valid, and will be resolved at runtime; settings.AUTH_USER_MODEL is just a string. Migrations notwithstanding, 

We didn't automagically convert auth.User into myapp.CustomUser because this acts as a validation aid. There's no guarantee that myapp.CustomUser adheres to the same user contract as auth.User. If a third party app references user.username, the app will break in production if you supply a user model that doesn't have a username field. A hard reference to a swapped model (either as auth.User *or* as "auth.User") can be identified as a validation issue on startup, flagging that the third party app isn't ready to support swappable user models. An app author can then signal that they're swappable-user ready by making the code change replacing the hard reference with a settings.AUTH_USER_MODEL reference.

So - there's a reason not to do (2) IMHO. 

Functionally, the problem is that ForeignKey(myauth.CustomUser) and ForeignKey(settings.AUTH_USER_MODEL) are both legal foreign key references, and if AUTH_USER_MODEL=CustomUser, then they both point to the same model - but only the second is a "swappable" reference.

In the internals, this resolves to the fact that while auth.User knows that it's been swapped out (and can identify this fact), but FKs to myauth.CustomUser don't know if this particular reference is a hard reference to a particular model, or a swapped in reference.

As an approach, (1b) concerns me a little - we've talking about a feature in the wild that's only been out there for 2 releases, and required third party apps to adapt in response to that change - now we're asking them to adapt again to flag that the reference is swappable. It's a very small change, but it feels like code churn to me, with the potential to upset the wider community.

My preference would be (1), but with a metadata clarification.

We are in a position to make an educated guess that a FK reference was to a swappable model. If the FK definition uses a string, and that string matches the definition of one of the SWAPPABLE meta declarations, then we can say the FK is a reference to a swappable.

This will make an incorrect guess in one specific case -- String-based explicit FK references to the replacement model. (i.e., ForeignKey("myauth.CustomUser") ) 

However, IMHO, this is an edge case. Meta.SWAPPABLE is an undocumented feature; so in practice, we're only talking about User models here. I'm having difficulty imaging why you'd have an explicit reference to a replacement model *and* use it as a swappable.

Given that it's an edge case, lets handle it as one -- assume our guesses will be right, and make this opt-out for the cases where it may be a problem. If you've got an explicit FK reference to a replacement model, and it *isn't* a swappable reference, require a ForeignKey("myauth.CustomUser", swappable=False) definition. Allow swappable=True if someone wants to be explicit; but default to swappable=None, which uses the educated guess approach. This is essentially 1b, but turning the direction around -- given the use case and the existence of historical code, lets assume we can pick the right thing, but provide tools to be explicit.

Then, as a version upgrade check, flag any reference that hits the educated guess marker. The validation framework (soon to hit trunk… promise…) will let us flag this at a low warning level, and provide a way to silence the warning once the update path has been validated.

The only implementation hiccup - there's not currently any way to identify if a FK reference was provided as a string. However, the very first thing that the FK constructor does is check to see if the reference is a model or a string - we just don't persist this metadata. However, even if we ignored this, and just used the resolved model reference, the cross section of affected references would be "explicit references to the replacement model", which is, IMHO, and equally narrow cross section.

Yours,

Russ Magee %-)

[Andrew Godwin]

OK, I like that approach Russ - an inverted 1b. It'll be a bit harder to explain in the docs, but it won't catch anyone out unawares, which is the key thing.

Andrew

--

You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAJxq848VcRg0ZHfptuXzg0n17qNbom79k_2LhhTAPPYggfWJLw%40mail.gmail.com.

[Shai Berger]

On Saturday 11 January 2014 11:45:55 Andrew Godwin wrote:
> OK, I like that approach Russ - an inverted 1b. It'll be a bit harder to
> explain in the docs, but it won't catch anyone out unawares, which is the
> key thing.
>

I like it too.

And foot-in-mouth re referencing swappables notwithstanding, there's two
points I want to raise:

1) This approach is good for the models files; I think a more explicit approach
should be taken for the migration files, where we don't currently have to worry
about backwards compatibility. I'd like to see swappable=None in the model
transformed to swappable=True in the migration, and swappable=None in the
migration treated as an error.

In particular, I don't like Marc's idea about migrations continuing to work
when a target model, which used to be non-swappable, is being swapped out. The
app's "main" code won't continue to work in this scenario, for reasons Russ
has noted; I don't see why migrations should not be treated as part of the
app's code.

2) Swappability of foreign-key targets in migrations may induce dynamic
migration dependencies (if my migration creates a foreign key to the user
model, and that model happens to be in a migrated app, my migration now
depends on some -- not even necessarily the initial -- migration in that app;
we'll only know at migrate time, not at makemigration time). I'd like to make
this more explicit too -- e.g. add some placeholder, or a function call to
calculate the dependency, in the generated list of dependencies. Although in
principle it can be induced from the operations, I think it would make
possible migration dependency issues much easier to troubleshoot.

An alternative would to decree that swappable-replacement models cannot live
in migrated apps; I think that is a very bad option.

Shai.

[Shai Berger]

On Thursday 09 January 2014 01:10:09 Raphaël Barrois wrote:
> On Wed, 8 Jan 2014 20:27:54 +0200
>
> Shai Berger <sh...@platonix.com> wrote:
> > as people already use auth.get_user_model() -- so, you can
> > monkeypatch *that* when generating migrations, and let them just keep
> > doing what they do today.
>
> Actually, people don't use auth.get_user_model() in their models
> declaration, they use ``settings.AUTH_USER_MODEL``, as described in the
> documentation:
> https://docs.djangoproject.com/en/1.6/topics/auth/customizing/#referencing-> the-user-model
>

Yes, I stand corrected.

>
> Actually, you have two problems with a third-party app:
>
> - It needs to have a way to specify "I want a FK to whatever the user has
> chosen for a AUTH_USER_MODEL", no matter the *type* of its PK field. =>
> This mustn't depend on what the third-party-app developer's actual
> AUTH_USER_MODEL setting was while generating the migration

As I've said in response to another mail in this thread, a variation on
Russell's suggestion, where the migration adds a ForeignKey with
swappable=True, should allow this to be done.

> if the user changes the model, this should be helped by migrations, for
> instance by put a warning e.g "Looks like you're changing a swappable
> model, where should I put automatic related migrations ? (these migrations
> shouldn't go into the app with the FKey to AUTH_USER_MODEL, which may be
> out of the user's control — e.g third party apps).
>

I don't think related migrations should be generated automatically. There are
too many issues -- besides third-party apps, there are non-migrated apps.
Editing an app's models under its feet is very bad practice.

> Another problem is apps that provide a swappable model whose default is
> AUTH_USER_MODEL but that may be customized through their own setting, for
> instance ``return getattr(settings, 'MY_TARGET_MODEL',
> settings.AUTH_USER_MODEL)``.

No, such apps should be using the same getattr call in their migrations --
that is, this is one of the cases where editing migration files manually should
be required; and it doesn't matter if the default target model happens to be
swappable. It is enough that a referenced model is decided by settings.

On a related note, this strengthens the case for explicit references to
dynamic dependencies; a migration for such an app should, IMO, look something
like:

from django.db import migrations, models
from django.conf import settings

my_target_model = getattr(settings, 'MY_TARGET_MODEL',
settings.AUTH_USER_MODEL)

class Migration(migrations.Migration):

dependencies = [("migrations", "0001_initial"),
migrations.model_created(my_target_model)]

operations = [
migrations.DeleteModel("Tribble"),
migrations.AddField("MyModel", "target",
models.ForeignKey(my_target_model)),
]

On swappable models, the dynamic dependency could be inferred automatically
from the operations; in this case, where the target model is not (or at least
not necessarily) swappable, the placeholder reference is required.

Shai.

[Andrew Godwin]

Good points. Regarding 1), I can't specially control reconstruction of fields from migrations and raise errors only in that case - it's just calling the normal __init__. Since I'm going to have to write out a call to settings.AUTH_USER_MODEL anyway - the whole point of swappable=True is so the migration serialiser knows to do that - having swappable be True or False makes no difference (the only behaviour it affects is the migration serialiser, for now, and that's never run on re-serialised models). Nevertheless, I'll make sure it's forced to True just in case.

On 2), that's an issue I hadn't thought of. There's special syntax for depending on the first migration of an app which I could inject, but it'd have to be chosen at migration creation time, so not terribly useful. The only concrete way to make sure this works is to have a dependency that's defined based on the setting, e.g.

class Migration(migrations.Migration):

    dependencies = [

        ("comments", "0001_initial"),

        migrations.swappable_dependency(settings.AUTH_USER_MODEL),

    ]

That function would just be lambda value: (value.split(".", 1)[0], "__first__") - we already have the __first__ special migration name which resolves to the start of any app (it's used for depending on apps with no migrations at the moment, in case they gain them in future)

Andrew

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/138953391.ZAfFGT0eZV%40deblack.