Fix of Max Choices bug #32
Conversation
…he standard text input will be used
…rwise, the standard text input will be used" This reverts commit cc19130.
… distinct choices. This allows max_choices to be the maximum unique choices.
This commit changes the way the URLs are declared to avoid RemovedInDjango110Warning messages. The old way, using django.conf.urls.patterns(), is deprecated and will be removed in Django 1.10
|
Hi @pjpassa, thanks for the contribution! This fix makes sense but, I'm worried about potentially harmful performance if the query will return a very large number of values. What if we Additionally, I would add a more specific test case, for example leaving the existing name with identical 5 names, and asserting the view returns the 1 distinct name. Later, randomizing the names (or using ID field), and asserting the max limit is reached. |
| @@ -69,8 +69,8 @@ def get(self, request, model=None, field_name=None): | |||
| logger.debug('No choices calculated for field %s of type %s', | |||
| field, type(field)) | |||
| else: | |||
| choices = set(model_obj.objects.values_list(field.name, flat=True)) | |||
| if len(choices) <= max_choices: | |||
| choices = model_obj.objects.values_list(field.name, flat=True).distinct() | |||
There was a problem hiding this comment.
Hope you don't mind me commenting while you work, I think order_by explicitly here is really a must, since the model may have default ordering that differs from our field.name, which may break the distinct query, as noted here.
... if you use a values() query to restrict the columns selected, the columns used in any order_by() (or default model ordering) will still be involved and may affect uniqueness of the results.
| choices = model_obj.objects.values_list(field.name, flat=True) | ||
| if choices.count() < max_choices: | ||
| choices = set(choices) | ||
| choices = model_obj.objects.values_list(field.name, flat=True).distinct() |
There was a problem hiding this comment.
I think we should add an explicit order_by in case the model's default queryset manager doesn't order_by a field outside of our values list. This is explained in distinct() docs - first note.
Max choices used to only send data to the front end if the total number of instances of the model was less than the max choices setting.
I refactored the code so that the set was created earlier. This way the view will return data if the number of unique values is less than the max choices setting.