Opened 17 months ago

Last modified 17 months ago

#26810 new Cleanup/optimization

DATA_UPLOAD_MAX_NUMBER_FIELDS not taken into account in admin mass actions

Reported by: Jerome Leclanche Owned by: nobody
Component: contrib.admin Version: 1.10
Severity: Normal Keywords:
Cc: vytis.banaitis@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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)

Change History (8)

comment:1 Changed 17 months ago by Tim Graham

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?

comment:2 Changed 17 months ago by Jerome Leclanche

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.

comment:3 Changed 17 months ago by Jerome Leclanche

Worth noting that a rewrite of the POST handling could be considered backwards-incompatible, too.

comment:4 Changed 17 months ago by Vytis Banaitis

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?

comment:5 Changed 17 months ago by Tim Graham

@vytisb, did you submit the intermediate delete page? That's where the entire list of IDs is present.

comment:6 Changed 17 months ago by Vytis Banaitis

Cc: vytis.banaitis@… added

@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.

comment:7 Changed 17 months ago by Cristiano Coelho

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.

comment:8 Changed 17 months ago by Tim Graham

Component: Uncategorizedcontrib.admin
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

On 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

Note: See TracTickets for help on using tickets.
Back to Top