On Tue, Jun 5, 2012 at 3:56 AM, Anssi Kääriäinen <[email protected]> wrote: > On Jun 4, 6:12 pm, Russell Keith-Magee <[email protected]> > wrote: >> * The swapping mechanic is set up using a new Meta option on models >> called 'swappable' that defines the setting where an alternate model >> can be defined. Technically, the swappable option *could* be used for >> any model; however, I'm not proposing that we actually document this. >> I've implemented it as a generic approach purely to make the internals >> cleaner -- mostly to avoid needing to make references to auth.User in >> the model syncdb code, and to make testing possible (i.e., we can do >> validation tests on a dummy 'swappable' model, rather than trying to >> munge a validation test for auth.User specifically). > > The "swappable" meta attribute part of the patch raises a question: > would the following pattern work instead of the meta attribute: > # in auth/models.py > > if settings.AUTH_USER_MODEL: > User = get_model(AUTH_USER_MODEL) > else: > class User(models.Model): > # the traditional auth-user model definition here > > If I understand the patch correctly the "swappable" Meta option > basically just removes the model from the app-cache. This means the > model will not be synced or validated. In addition you can't make > foreign keys to the swapped out model due to other changes in the > patch. Still, the auth.User model exists, and is importable. > > The idea above gets rid of the User model 100% if > settings.AUTH_USER_MODEL is set. In runtime it never exists at any > level. It is very simple in this regard, and of course the > implementation is simple, too. > > Is there some design consideration which prevents the use of the above > mentioned idea?
Two significant reasons: Firstly, if we use the Meta attribute approach, we can raise a specific validation error when the user has an app with a model that references auth.User directly, and User has been swapped out. This is mostly useful as a migration trigger -- as soon as you change AUTH_USER_MODEL, you'll get a bunch of errors about all the apps that haven't been updated and will break as a result. This catches as a development-time validation error the most-likely set of problems that will occur when swappable User models start getting into the wild. Using the code you've provided, changing AUTH_USER_MODEL would silently fail at some point in production, because auth.User still "exists", but is now a very different beast. Secondly, there's some avoidance of nightmares from Django's past. Using the approach you describe, you're making auth.User a dynamic model reference -- the model that is referred to by auth.User is changed at runtime, based on the value of a setting. This means that just because you have a auth.User object, doesn't mean you can know for sure anything about it. This is somewhat comparable to the bad old "magic removal" days of Django. A lot of the problems we experienced back then were caused by the fact that you couldn't be certain that a model existed, or if it did exist, that it was the model that you thought it was, because other mechanisms in the load sequence would swap out models/modules, and/or move them somewhere else. This is a set of headaches that I'd rather not revisit. There's a scaled back alternative of what you describe -- essentially, where you only have the "else" clause from your sample code. This is slightly better, but still leads to the situation where the failure mode is an import failing, rather than a helpful message that points at the model that you shouldn't be referencing. By using the swappable approach, a model reference is always exactly the model that you've imported, unless you've got the reference through a get_user_model() method call, and other validation mechanisms work out whether that model reference is legal. At the end of the day, these validation mechanisms aren't really that complex, either. If you look at the additions to the validation logic, they're essentially just "if f.rel.to.swapped, make noise". The only other complex part is the synchronisation code, but even that is just layering on top of code that's already there. Abstract, proxy and non-managed models all fall in the group of "importable, but not synchronisable" models; swappable just adds one more to that list. There are two other lesser reasons that I've taken this approach. Firstly, I'm not wild about module level if statements. For me, a module should be a container of logic, not an executable unit in itself. I appreciate that there are sometimes valid reasons to make module level code executable, and at some level a def statement is just the execution of a declaration statement, but to my mind it's worth keeping a clear distinction. A good practical example of why this matters is testing -- module level if statements can only be (easily) tested at time of import, so you really only get one chance to test that code. Secondly, I don't like special cases, and a swappable Meta attribute -- even if it's a stealth option -- means that the underlying mechanism is generic, and doesn't require any User-specific references anywhere. Again, testing is a good reason for why this matters. We can test that the mechanics of swappable models work as expected with a suite of non-User swapped/unswapped models. If the mechanism was User specific, we'd only be able to test using User, but the swappability would be determined as an import-time definition, so we'd only be able to test one configuration per test run (unless we got into some messy reload/app cache flushing). As a side effect, it also means that if we were to ever 'bless' swappable models as an official feature, we just need to document what we've already got. In the interim, we have a hidden feature that adventurous members of the community can start playing with. As an unofficial feature, we're not obligated to support or maintain it, but it gives us a way to test out the waters to see whether the idea should be encouraged as broader practice. This approach to API development has precedent in Django. For example, shadow reverse attributes (i.e, related_name="+foo") existed as an undocumented feature for some time before we decided that they weren't actually harmful and added them as official API. Yours, Russ Magee %-) -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
