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
On Thu, Jan 9, 2014 at 1:28 AM, Russell Keith-Magee <russ...@keith-magee.com
> wrote:
>
>
>
> On Thu, Jan 9, 2014 at 12:04 AM, Andrew Godwin <and...@aeracode.org>wrote:
>
>> 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.
>>
>
> 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 %-)
>
> --
> 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-developers+unsubscr...@googlegroups.com.
> To post to this group, send email to django-developers@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
> .
>
> For more options, visit https://groups.google.com/groups/opt_out.
>
--
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/CAFwN1uqD7mDPfAbOJCE%3DLF0hkrsPQn-jsQ%3Dv18_OAp%2BDNCnDhA%40mail.gmail.com.
For more options, visit https://groups.google.com/groups/opt_out.
