On Thu, Jun 7, 2012 at 7:48 PM, Anssi Kääriäinen
<[email protected]> wrote:
> On Jun 7, 11:57 am, Florian Apolloner <[email protected]> wrote:
>> Hi,
>>
>> On Wednesday, June 6, 2012 4:32:02 PM UTC+2, Anssi Kääriäinen wrote:
>>
>> > Still, yet another API idea: [snip]
>>
>> Then, Model.__new__ will replace the SwappableUser class with the
>>
>> > swapped in class. The 'swappable' in model.Meta tells which concrete
>> > model implementation to use.
>>
>> I'd rather not dynamically replace models. The system proposed by Russell
>> is clean and requires no extra magic. The only thing which might look a bit
>> odd is inheriting from a swapped model, eg like:
>>
>> class MyUserProxy(get_user_model()):
>> class Meta:
>> proxy = True
>>
>> etc… (Which obviously can get a bit nicer by using User = get_user_model()
>> and then inheriting from that).
>
> I just don't see it as a good idea to hard-code meta.swappable to be a
> settings variable in user-visible way, without any option to use
> anything else. I see I am in minority here, so I will let this issue
> be.
>
> Review issues spotted:
> - Queries using a swapped-out model should be prevented (probably at
> "execute sql" time).
Good idea. I haven't tried to see what it actually does; I'm guessing
you're going to get Table Does Not Exist errors. A nicer error
wouldn't hurt.
> - For documentation: It should be suggested that the example MyUser
> should define class Meta: swappable = 'AUTH_USER_MODEL'. Otherwise it
> will be synced to the database even if it is not the chosen user model
> for the project, which is likely not wanted. Maybe AbstractBaseUser
> should define swappable = 'AUTH_USER_MODEL' so that anybody who
> inherits from that gets the setting automatically?
Ah - I think I see the problem now. The swappable Meta option isn't
visible to to end user -- ever -- unless they go spelunking in the
source code for auth.models.
If I define MyUser, I *dont'* need to define it as swappable. I just
need to define AUTH_USER_MODEL in my settings file to point at my
replacement model definition. The only place where swappable is
defined is on the base model that is being swapped out -- in this
case, auth.User.
I'm expecting that there will be *no* documentation of the swappable
option (unless, at some point in the future, we decide to make
swappable models an official feature). It exists purely to make the
internals clean, and avoid making auth.User a special case in code.
> - Creating a "class UserDerived(User)" will not succeed if the User
> is swapped out. So, you can't make a swappable derived class.
> Documenting this limitation is IMO enough.
Documentation would be one option; another would be to treat a
reference to a swapped out base class as an abstract class. The
handling is identical in every other sense (i.e., an unsynchronized
model). Better error messages would be another option; I'm having
difficulty thinking of a case where you'd want to have a subclass of a
model that isn't being used in the rest of your system.
> - Create model UserProxy(auth.User), swap out auth.User, and make a
> foreign key to UserProxy - you get an error that "DatabaseError:
> relation "auth_user" does not exist" instead of a mention that
> UserProxy is swapped out. The error should probably say something like
> "UserProxy derives from swapped out model auth.User - can't reference
> this model.", as telling to point to settings.AUTH_USER_MODEL in this
> case is incorrect.
Agreed -- error messages would be better.
> - The abstract user class defines two fields: password, and
> last_login. Is the last_login really a core requirement?
last_login is at least arguable as an inclusion in the abstract base
model. We certainly *could* drop it from the list of requirements. The
reason to include it is so that all the 'user token' code works -- so
all the password reset views et al work as expected.
An alternative option would be to require that the user model provide
an implementation of a "login_timestamp" attribute (just like the
get_full_name and get_short_name requirements).
The last option -- make last_login completely optional, but document
the fact that the password reset views require the existence of the
last_login attribute.
> - Assume a 3rd party developer was going to use the swappable Meta
> attribute. The developer provides a couple of different swappable
> implementations to use, each defining swappable = 'MY_SETTING'.
> However, by default settings.MY_SETTING is None, and thus each of
> these models will be synced into the DB. How does the 3rd party
> developer designate the default implementation? For auth.user this is
> easy, as global_settings can contain the default setting. A 3rd party
> developer doesn't have this option. (This one isn't important for the
> auth.User case, only for making this feature someday publicly
> available).
If MY_SETTING isn't defined, all the code I've written should be
assuming that the model isn't swapped out (i.e., it's effectively
getattr(settings, 'MY_SETTING', model_label_of_default_model) ). As
proof of concept, it certainly wouldn't hurt to drop out the
AUTH_USER_MODEL setting from globals.
>> > About the ORM capabilities of the interface: It seems there can not be
>> > any assumptions about the swapped in model here: for example
>> > SomeModel.filter(user__is_staff=True) or
>> > SomeModel.order_by('user__lastname', 'user__firstname') works
>> > currently, but there are no such lookups available for the interface.
>> > Maybe there should be some way to order by "full_name" and
>> > "short_name" at least. But, this gets us quickly to virtual fields or
>> > virtual lookups territory...
>>
>> Imo it's fine if we require some fields on a Usermodel to be compatible
>> with admin and friends. (eg a first/last_name field which is nullable [or
>> not required in the form validation sense] shouldn't hurt anyone, the admin
>> could check if the field is filled and if not display something else like
>> the username).
>
> Admin is actually somewhat easy to make work re the ordering, because
> in there you can use get_full_name.admin_order_field =
> 'somemodelfield'. This doesn't work outside Admin. This isn't a major
> issue, it prevents things like ordering comments on the commenter's
> "get_short_name" output. Even then, this is only an issue for reusable
> code - in your project you know the concrete user model you have in
> use, and can thus use the specific lookups your model has.
I can see two approaches here.
The first is the full audit of admin that I've mentioned previously.
That will reveal everything that admin is depending on, and will allow
us to define a full "admin user interface" that any swappable user
must define.
The second is to abstract away the ordering field in the same was as
we've abstracted get_short_name -- i.e., don't specify what the field
needs to be called, just require that the user provide an option that
defines how it can be retrieved. This is similar to what we've done
with get_short_name(), but at a model level.
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.
