Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limits the number of related items added to the choices dictionary on RelatedField #3330

Closed
wants to merge 2 commits into from

Conversation

rubendura
Copy link
Contributor

Refs #3329

Solves a memory issue when evaluating very large querysets

@tomchristie
Copy link
Member

As presented I think this would actually break the html_cutoff displays, by not including the 'Over 1000 items' text. I'm also not sure how keen I am on introducing the cutoff to .choices itself.

An alternative might be a get_choices(cutoff=...) method. The .choices property could call it directly without a cutoff, and iter_options could call it, with the cutoff+1?...

@rubendura
Copy link
Contributor Author

It seems to be working fine on my side with a dropdown using html_cutoff, but I haven't tested other cases yet.

I'm not sure what you mean with the last paragraph. Why would you want to call that get_choices() with different cutoffs?

@tomchristie
Copy link
Member

What I mean is I'm not totally comfortable with '.choices' always being hard-limited.
We ought to ensure that .html_cutoff really only does apply when rendering to html, rather than affecting any other usages.

(Tho its possible I could be convinced to change my mind on that)

@rubendura
Copy link
Contributor Author

I guess that by doing this we keep the current RelatedField.choices behaviour while allowing iter_options to limit how many choices we have. I know this is lacking tests and we should probably have some for it. I might look into it later.

@tomchristie
Copy link
Member

Looks about right, yeah.

@vstoykov
Copy link
Contributor

vstoykov commented Sep 7, 2015

I also have problems with RelatedField to table with many records. OPTIONS response can be delay to 1 minute which is not good. I also thought about proper solution for this problem and I don't think that limiting .choices is a good idea.

  1. First I'm not sure what will happen with validation
  2. If we can't get all of the choices probably we need to create another API endpoint which will return filtered results based on request (for autocomplete for example). Then if we have such an endpoint why we need choices for that field at all?

Probably we need an argument for that field that will mark the field as "raw_id" like in the django admin. And for this field there will be no select in the HTML view and there will be no choices in the OPTIONS response.

What you think about that?

@tomchristie
Copy link
Member

Probably we need an argument for that field that will mark the field as "raw_id" like in the django admin. And for this field there will be no select in the HTML view and there will be no choices in the OPTIONS response.

I think what we present in OPTIONS responses is a diff issue to what we present in HTML selections. Personally, right now I'd rather just we weren't showing them in OPTIONS at all.

@jathanism
Copy link

I would like to add that I am encountering this issue in my own server (with about 220k related records for a field) which utterly crushes the app.

I tested the patch above and it's working for me.

You also have my vote for removing the enumeration of related choices in OPTIONS!

@tomchristie tomchristie added this to the 3.3.3 Release milestone Jan 27, 2016
@tomchristie
Copy link
Member

You also have my vote for removing the enumeration of related choices in OPTIONS!

Probably a good plan.

@xordoquy xordoquy modified the milestones: 3.3.3 Release, 3.3.4 Release, 3.4.0 Release Feb 11, 2016
@wimglenn
Copy link
Contributor

+1 choices enumeration in an OPTIONS request is making the response HUGE, and the browsable API unusable for me in 3.3.2

@wimglenn
Copy link
Contributor

Thanks! 👍

@tomchristie
Copy link
Member

tomchristie commented Aug 10, 2016

Most welcome. Pleasure to get the triage back on track now that we're (mostly) funded! 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants