[Django] #26810: DATA_UPLOAD_MAX_NUMBER_FIELDS not taken into account in admin mass actions
Django
-------------------------------+--------------------
Reporter: Adys | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
In Django 1.10a1, when selecting all items in an admin view with a lot of
items and clicking "Select all 50+ elements" and proceeding with the
action, if the amount of elements exceeds the new
`DATA_UPLOAD_MAX_NUMBER_FIELDS` setting, a TooManyFieldsSent error will be
raised. The error is not caught by the admin.
Possible resolutions:
* Handle TooManyFieldsSent in admin actions (simple but bad experience)
* Allow a "select everything" that does not use individual POST fields
(hard, but best experience)
* Check DATA_UPLOAD_MAX_NUMBER_FIELDS in the admin, preventing too many
items from being selected (hard, bad experience)
--
Ticket URL: <https://code.djangoproject.com/ticket/26810>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Adys | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I think the idea is that individual apps shouldn't strive to adapt to this
setting but rather projects should either raise the value of setting as
needed and/or customize `handler400` with a more-friendly message. That
said, disabling the "select all items" button if the changelist contains
some really large number of items seem reasonable to me, independent of
the new setting.
With the current default of `DATA_UPLOAD_MAX_NUMBER_FIELDS = 1000`, it
requires selecting more than ~1000 items (minus a few for other fields on
the deletion form) to trigger this, doesn't it?
--
Ticket URL: <https://code.djangoproject.com/ticket/26810#comment:1>
Django
Reporter: Adys | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adys):
Likely correct. I triggered it with 1136 items.
I think the correct solution would be to rewrite the "select absolutely
everything" bit to work with a single POST parameter or for the "select x"
to work with a comma-separated single parameter (disclaimer: Haven't
looked at the code). However, given that 1.10 is already in beta 1, that
might not be an option - if so, adding a TooManyFieldsSent handler for
that particular scenario and failing gracefully in case of a large
selection would be good enough.
--
Ticket URL: <https://code.djangoproject.com/ticket/26810#comment:2>
Django
Reporter: Adys | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adys):
Worth noting that a rewrite of the POST handling could be considered
backwards-incompatible, too.
--
Ticket URL: <https://code.djangoproject.com/ticket/26810#comment:3>
Django
Reporter: Adys | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by vytisb):
I have tried to reproduce this issue and I think that it is unrelated to
"Select all X objects". Admin already uses a single POST parameter to
specify that "Select all X" was clicked. I have seen 4 fixed POST params
and at most one page worth of object IDs.
I only managed to receive the error with non default values of
`list_per_page` and/or `list_max_show_all`. Clicking the corner checkbox
to select the whole page and running an action was sufficient to invoke
the error in this case.
Maybe a system check to notify the user when `max(list_per_page,
list_max_show_all) + 4 > DATA_UPLOAD_MAX_NUMBER_FIELDS` would solve this
issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/26810#comment:4>
Django
Reporter: Adys | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
@vytisb, did you submit the intermediate delete page? That's where the
entire list of IDs is present.
--
Ticket URL: <https://code.djangoproject.com/ticket/26810#comment:5>
Django
Reporter: Adys | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by vytisb):
* cc: vytis.banaitis@… (added)
Comment:
@timgraham, I see it now. Previously, I used a custom action without
intermediate pages.
So this affects the delete action intermediate page and any custom action
intermediate page that mimics the behavior of delete action.
For the delete action a possible solution would be to make the
intermediate page form send the same params as the action form in
changelist.
However, this solution has a problem when new objects are created between
the rendering of confirmation page and user confirming the deletion --
objects would be deleted that the user did not confirm.
The current implementation avoids this for direct objects but not for
related objects that would be cascade-deleted.
--
Ticket URL: <https://code.djangoproject.com/ticket/26810#comment:6>
Django
Reporter: Adys | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cristianocca):
{{{
Allow a "select everything" that does not use individual POST fields
(hard, but best experience)
}}}
Makes the most sense to me.
I'm still not sure why is it currently an issue, considering the page size
should be something between 100 and 1000, above this django will start to
die anyways trying to fetch everything into memory and then the for loop
on the template rendering, so the initial post shouldn't really be getting
an error unless you use an incredibly high page size.
Does the issue come from the second post after the confirmation page? If
this is the case, I think that this page is really bad when the "select
all" option is used. You shouldn't be loading the whole table into memory
and then render it on the preview page, it will get even worse with
cascade deletes and related objects. So I really think the "select all"
behaviour should be improved to use a bulk delete operation and do not
send every selected ID on a post request, neither render everything on the
template.
--
Ticket URL: <https://code.djangoproject.com/ticket/26810#comment:7>
Django
Reporter: Adys | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* component: Uncategorized => contrib.admin
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
On [https://groups.google.com/d/msg/django-
developers/UwkkZ4h0dVA/pbmeOgwjBQAJ django-developers], Tom Christie
suggests:
Most realistic in immediate future: Accept it as a current limitation
(possibly handle TooManyFieldsSent)
Someday later, perhaps: Allow a "select everything" that does not use
individual POST fields
--
Ticket URL: <https://code.djangoproject.com/ticket/26810#comment:8>