CMS_PERMISSION: optionally use raw id user lookups #1481

Merged
merged 9 commits into from Apr 15, 2013

4 participants

@jimr

The "view restrictions" and "page permissions" inlines on the "page"
admin change forms can cause performance problems where there are many
thousands of users being put into simple select boxes. Added a new
setting (CMS_RAW_ID_USERS) that, if set, forces the inlines on that page
to use standard Django admin "raw ID" lookups rather than select boxes.
The restriction on which users may be selected should still be honoured
by the new widget because we set the 'limit_choices_to' attribute to be
the same as the queryset on the original widget.

Using raw_id_fields in combination with limit_choices_to blows up if you
have many thousands of users. For this reason, we only apply this limit
if the number of users is relatively small (< 500, though that figure is
somewhat arbitrary). If the number of users we need to limit to is
greater than that, we use the usual input field instead unless the user
is a CMS superuser, in which case we bypass the limit. Unfortunately,
this means that non-superusers won't see any benefit from this change.

jimr added some commits Sep 14, 2012
@jimr jimr CMS_PERMISSION: optionally use raw id user lookups
The "view restrictions" and "page permissions" inlines on the "page"
admin change forms can cause performance problems where there are many
thousands of users being put into simple select boxes. Added a new
setting (CMS_RAW_ID_USERS) that, if set, forces the inlines on that page
to use standard Django admin "raw ID" lookups rather than select boxes.
The restriction on which users may be selected should still be honoured
by the new widget because we set the 'limit_choices_to' attribute to be
the same as the queryset on the original widget.

Using raw_id_fields in combination with limit_choices_to blows up if you
have many thousands of users. For this reason, we only apply this limit
if the number of users is relatively small (< 500, though that figure is
somewhat arbitrary). If the number of users we need to limit to is
greater than that, we use the usual input field instead unless the user
is a CMS superuser, in which case we bypass the limit. Unfortunately,
this means that non-superusers won't see any benefit from this change.
4afe5e1
@jimr jimr Removed unused import 1ea6e55
@jimr jimr .count() rather than len() for performance boost 2b1b614
@digi604
Divio AG member

Ok this needs docs and it needs merge with develop. I don't think we gonna release an other 2.2 release for this.

@jimr

OK, I'll add some docs & remove the version bump.

@jimr jimr CMS_RAW_ID_USERS: added default setting + docs
Also removed the previous version bump so it can merge cleanly upstream.
b3183f2
@digi604
Divio AG member

Looks good to me... @ojii could you have a look as well?

@ojii ojii and 1 other commented on an outdated diff Nov 12, 2012
cms/admin/forms.py
page = forms.ModelChoiceField(Page, label=_('user'), widget=HiddenInput(), required=True)
def __init__(self, *args, **kwargs):
super(PagePermissionInlineAdminForm, self).__init__(*args, **kwargs)
user = get_current_user() # current user from threadlocals
- self.fields['user'].queryset = get_subordinate_users(user)
- self.fields['user'].widget.user = user # assign current user
+ sub_users = get_subordinate_users(user)
+
+ limit_choices = True
+ use_raw_id = False
+ if hasattr(settings, 'CMS_RAW_ID_USERS') and settings.CMS_RAW_ID_USERS:
@ojii
ojii added a note Nov 12, 2012

if getattr(settings, 'CMS_RAW_ID_USERS', False):

@jimr
jimr added a note Nov 12, 2012

Yes, that would be clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii ojii and 1 other commented on an outdated diff Nov 12, 2012
cms/admin/permissionadmin.py
@@ -22,6 +22,15 @@ class PagePermissionInlineAdmin(admin.TabularInline):
classes = ['collapse', 'collapsed']
exclude = ['can_view']
+ def __getattribute__(self, name):
@ojii
ojii added a note Nov 12, 2012

is there no other way?!

@jimr
jimr added a note Nov 12, 2012

It's the only way I could find of configuring raw_id_fields based on settings. If there's a better way I'll happily change it.

@ojii
ojii added a note Mar 29, 2013
@property
def  raw_id_fields(self):
    if ....
@jimr
jimr added a note Apr 5, 2013

Unfortunately this isn't possible because of the way Django validates the form (it checks to see whether the raw_id_fields is an instance of list or tuple which would fail if it's a property).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii

I have a few isuses with the patch itself, but on top of that I'm (as we all know by now) not a huge fan of adding new settings.

If this is such a good thing, why don't we just make it the default?

@digi604
Divio AG member

Good point @ojii. Remove the setting and make it an Raw ID field if there are more 500 users? Maybe even 200?

@jimr

I only made it a setting because I didn't want to alter default behaviour. With thousands of user accounts, it's currently infeasible to use CMS_PERMISSION because of the issues addressed by this patch, so making it default would make sense to me.

@FrankBie

Would it not be better to rename the setting from CMS_RAW_ID_USERS to CMS_RAW_ID_USERS_LIMIT_COUNT defaulting to None and otherwise taking a configurable limit amount?

Pro: you can put it to the limit you like without agreeing to the 500.

@digi604
Divio AG member

please remerge with develop and i think too that the configurable limit would be better...

@ojii ojii commented on an outdated diff Mar 29, 2013
cms/conf/global_settings.py
@@ -26,6 +26,9 @@
# Whether to enable permissions.
CMS_PERMISSION = False
+# Whether to use raw ID lookups for users when CMS_PERMISSION is set to True
+CMS_RAW_ID_USERS = False
@ojii
ojii added a note Mar 29, 2013

with regards to the limit, how about making this setting either False (disable the raw ID lookup) or an integer which is the limit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jimr

OK, sounds good. I'll bring my branch up to date & make the limit configurable.

@jimr

@ojii should I put CMS_RAW_ID_USERS in DEFAULTS in cms/utils/conf.py as RAW_ID_USERS? Seems the arrangement of settings has changed rather a lot since I started.

@ojii

yes please

jimr added some commits Apr 5, 2013
@jimr jimr Merge branch 'develop' into raw_id_users
Conflicts:
	cms/conf/global_settings.py
2481bf3
@jimr jimr Only use raw ID above a certain number of users
So the RAW_ID_USERS setting should now be either left as False or set to
a positive integer which will be the threshold for the number of users
above which the raw ID widget will be used.
ec9f8d8
@jimr jimr import typo 61055b9
@ojii ojii and 1 other commented on an outdated diff Apr 5, 2013
cms/admin/permissionadmin.py
@@ -27,6 +27,16 @@ class PagePermissionInlineAdmin(TabularInline):
exclude = ['can_view']
extra = 0 # edit page load time boost
+ def __getattribute__(self, name):
@ojii
ojii added a note Apr 5, 2013

github seems to have lost this discussion, but this works for me:

In [1]: class A:
   ...:     @property
   ...:     def b(self):
   ...:         return []
   ...:     

In [2]: a = A()

In [3]: isinstance(a.b, list)
Out[3]: True

In [4]: isinstance(a.b, (list, tuple))
Out[4]: True
@jimr
jimr added a note Apr 5, 2013

Django checks it on the class, not an instance, otherwise it would indeed work.

https://github.com/django/django/blob/master/django/contrib/admin/validation.py#L267

@ojii
ojii added a note Apr 5, 2013
class classproperty(object):

    def __init__(self, fget):
        self.fget = fget

    def __get__(self, owner_self, owner_cls):
        return self.fget(owner_cls)

## -- End pasted text --

In [2]: class A:
   ...:     @classproperty
   ...:     def b(cls):
   ...:         return []
   ...:     

In [3]: A.b
Out[3]: []
@jimr
jimr added a note Apr 5, 2013

OK, so that would work, but do you think it's better than just using __getattribute__? Up to you, but IMO the original code is easier to understand.

@ojii
ojii added a note Apr 5, 2013

arguably, but I just really am not a fan of __getattribute__. had too many issues with it in the past...

@jimr
jimr added a note Apr 5, 2013

Fair enough. Let me know if this is blocking the merge and I'll change it, but my personal preference would be to leave as-is.

@ojii
ojii added a note Apr 6, 2013

had a chat with @digi604 and @stefanfoulis and we all three prefer classproperty over __getattribute__.

I think the best place to put that helper class is probably cms.utils.helpers, so if you could change this bit to make use of that technique, that would be highly appreciated.

@jimr
jimr added a note Apr 8, 2013

Okie doke.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii ojii commented on the diff Apr 6, 2013
cms/admin/forms.py
page = forms.ModelChoiceField(Page, label=_('user'), widget=HiddenInput(), required=True)
def __init__(self, *args, **kwargs):
super(PagePermissionInlineAdminForm, self).__init__(*args, **kwargs)
user = get_current_user() # current user from threadlocals
- self.fields['user'].queryset = get_subordinate_users(user)
- self.fields['user'].widget.user = user # assign current user
+ sub_users = get_subordinate_users(user)
+
+ limit_choices = True
+ use_raw_id = False
+
+ # Unfortunately, if there are > 500 users in the system, non-superusers
+ # won't see any benefit here because if we ask Django to put all the
+ # user PKs in limit_choices_to in the query string of the popup we're
+ # in danger of causing 414 errors so we fall back to the normal input
+ # widget.
+ if get_cms_setting('RAW_ID_USERS'):
+ if sub_users.count() < 500:
@ojii
ojii added a note Apr 6, 2013

isn't this where it should use the variable setting?

@jimr
jimr added a note Apr 8, 2013

The threshold check to decide whether we should try to use a raw ID field is applied in cms/admin/permissionadmin.py. The "< 500" check applied here is only to prevent the raw ID blowing up with a 414 error, and there's little point making this configurable, IMO (though 500 is a little arbitrary).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jimr jimr Switch from __getattribute__ to classproperty
Added the new classproperty helper in cms.utils.helpers.
9a778f5
@digi604 digi604 merged commit bd166e3 into divio:develop Apr 15, 2013

1 check passed

Details default The Travis build passed
@jimr

Thanks!

@jimr jimr deleted the Maplecroft:raw_id_users branch Apr 15, 2013
@digi604
Divio AG member

Thank YOU!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment