Opened 9 years ago

Last modified 5 months ago

#25306 new New feature

Allow a limit_choices_to callable to accept the current model instance

Reported by: Miles Hutson Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: Matthijs Kooijman Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Real life problem: I have a site with two models Person and Storyteller. Storytellers can tell stories (represented by a TextField) about a Person. This relationship is represented by a ForeignKey from Storyteller to Person. An admin can choose a feature story about a Person. This is represented by a foreign key from Person to Storyteller. The change view for a given Person should only display Storytellers that have a foreign key to that person. There isn't an easy way to do this currently.

Change History (8)

comment:1 by Tim Graham, 9 years ago

How about using AdminSite.form and customizing the foreign key's queryset in form.__init__() as described in Changing the queryset.

Version 0, edited 9 years ago by Tim Graham (next)

comment:2 by Miles Hutson, 9 years ago

Thanks for the pointer Tim! I've been digging into the admin code, because I honestly hadn't needed to before. I'm sure that what you mentioned will end up being what I have to do. But part of the point of Django admin seems to be that people don't end up having to dive into the code for an admin interface when they're building a website. Hence why there are options like blank and limit_choices_to on fields. The suggestion in this feature request is that model fields have something similar to limit_choices_to. Perhaps this parameter could be passed a function. When a ForeignKey is populated in a change view, this function could be passed the relevant instance, and could return a queryset to filter on.

Perhaps this could be done by adding the behavior described to limit_choices_to? I believe this would involve modifying the __init__ method of forms.models.BaseModelForm.

Last edited 9 years ago by Tim Graham (previous) (diff)

comment:3 by Miles Hutson, 9 years ago

Anyone who googles this and finds themselves in the same rut, here's a useful stopgap. Override the ModelAdmin's form with a form subclassed off of this.

class LimitChoicesBaseForm(ModelForm):
    limit_choices_methods = []

    def __init__(self, data=None, files=None, auto_id='id_%s', prefix=None,
                 initial=None, error_class=ErrorList, label_suffix=None,
                 empty_permitted=False, instance=None):
        super(LimitChoicesBaseForm, self).__init__(data=data, files=files, auto_id=auto_id, prefix=prefix,
                                                   initial=initial, error_class=error_class, label_suffix=label_suffix,
                                                   empty_permitted=empty_permitted, instance=instance)
        for method in self.limit_choices_methods:
            formfield = self.fields[method['field_name']]
            if hasattr(formfield, 'queryset'):
                filter_q_object = method['method'](instance)
                formfield.queryset = formfield.queryset.filter(filter_q_object)

comment:4 by Tim Graham, 9 years ago

Component: contrib.adminForms
Summary: Should be an easier way to restrict the choices for a ForeignKeyAllow a limit_choices_to callable to accept the current model instance
Triage Stage: UnreviewedAccepted
Version: 1.8master

It might be possible to allow passing the instance to limit_choices_to, but I'm not sure. Accepting on the basis of investigating the idea. See #2445 for related work.

comment:5 by Matthijs Kooijman, 5 years ago

Cc: Matthijs Kooijman added

I'm running into this same problem. The proposed solution of letting limit_choices_to accept a model instance seems reasonable to me, but on closer inspection (see below) probably not feasible, unfortunately.

I guess this will need some compatibility check (to support both the old argumentless version as well as a version with an instance argument). It seems checking this using e.g. utils.inspect.func_supports_parameter() and using a kwarg in the call seems reasonable, or checking with utils.inspect.method_has_no_args() and using a positional argument?

It seems that limit_choices_to is resolved in only three places:
$ git grep callable.*limit_choices_to
contrib/admin/widgets.py: if callable(limit_choices_to):
db/models/fields/related.py: if callable(self.remote_field.limit_choices_to):
forms/models.py: if callable(self.limit_choices_to):

The first one is used by the ForeignKeyRawIdWidget and ManyToManyRawIdWidget. It is probably hard to get the model instance there, but the value is only used to provide a "related" link that shows related objects for a raw id widget. Given this widget is already quite spartan, it might be ok to not have these choices limited (e.g. passing None as the instance and let the callable decide what to do).

The second and third one are part of the get_limit_choices_to() methods on RelatedField and ModelChoiceField

For ModelChoiceField, it seems the instance can be passed by BaseModelForm.__init__ through apply_limit_choices_to_to_formfield. This leaves one path through django.forms.fields_for_model, but only when called with apply_limit_choices_to=True which (though it is the default value) does not seem to happen within the Django codebase (except for tests).

For RelatedField, there are two paths. ForeignKey.validate has the instance, so it can simply pass that on. The last path is through Field.get_choices, which has no access to the model instance (and is again called from a few places without access to an instance AFAICS).

Looking at the above, there are quite some places where there is no instance available, so that would probably require signficant refactoring to move the resolution of limit_choices_to up in the call hierarchy where sufficient context is available (and it would not fix it for get_choices, which is probably a public API).

Alternatively, you could make sure to pass the instance wherever it is available and None elsewhere, as a best effort better-than-nothing approach, but I suspect that that would only lead to inconsistency and the illusion of really supporting this, which might not even be better than nothing.

comment:6 by Petr Dlouhý, 4 years ago

I think, that maybe this could be achieved without need to pass instance to limit_choices_to - it can stay on the database level with some syntax like this:

    storyteller = models.ForeignKey(
        'Storyteller',
        limit_choices_to={'person__id': Value('id')}
    )

I tried that in my project, but I am getting errors like:

  File "site-packages/django/utils/html.py", line 373, in <lambda>
    klass.__str__ = lambda self: mark_safe(klass_str(self))
  File "site-packages/django/forms/boundfield.py", line 33, in __str__
    return self.as_widget()
  File "site-packages/django/forms/boundfield.py", line 96, in as_widget
    renderer=self.form.renderer,
  File "site-packages/django/forms/widgets.py", line 241, in render
    context = self.get_context(name, value, attrs)
  File "site-packages/django/contrib/admin/widgets.py", line 288, in get_context
    'rendered_widget': self.widget.render(name, value, attrs),
  File "site-packages/django/forms/widgets.py", line 241, in render
    context = self.get_context(name, value, attrs)
  File "site-packages/django/forms/widgets.py", line 678, in get_context
    context = super().get_context(name, value, attrs)
  File "site-packages/django/forms/widgets.py", line 639, in get_context
    context['widget']['optgroups'] = self.optgroups(name, context['widget']['value'], attrs)
  File "site-packages/django/forms/widgets.py", line 587, in optgroups
    for index, (option_value, option_label) in enumerate(self.choices):
  File "site-packages/django/forms/models.py", line 1140, in __iter__
    for obj in queryset:
  File "site-packages/django/db/models/query.py", line 346, in _iterator
    yield from self._iterable_class(self, chunked_fetch=use_chunked_fetch, chunk_size=chunk_size)
  File "site-packages/django/db/models/query.py", line 57, in __iter__
    results = compiler.execute_sql(chunked_fetch=self.chunked_fetch, chunk_size=self.chunk_size)
  File "site-packages/django/db/models/sql/compiler.py", line 1157, in execute_sql
    cursor.close()
psycopg2.errors.InvalidCursorName: cursor "_django_curs_140361007625984_sync_1" does not exist

comment:7 by Simon Charette, 9 months ago

I thought I'd bring it the following since this was discussed on the Discord.

About 15 years ago I created a now defunct third-party library that did exactly what is being asked for here, a way to have a callable receive the model instance to perform filtering against the available choices which confirms that this is technically doable.

The interface was analogous to something like

class Player(models.Model):
    name = models.CharField()
    team = models.ForeignKey('Team', limit_choices_to='allowed_teams')

    def allowed_teams(self, queryset):
        if self.captain_for:
            return queryset.filter(pk=self.captain_for.pk)
        return queryset

class Team(models.Model):
    name = models.CharField()
    captain = models.OneToOneField(Player, related_name='captain_for')

That would generate the equivalent of

class PlayerForm(ModelForm):
    class Meta:
       model = Player
       fields = '__all__'

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        if self.instance.pk and self.instance.captain_of:
            team_field = self.fields['team']
            team_field.queryset = team_field.queryset.filter(
                pk=self.instance.captain_of
            )

But without the boilerplate.

The library supported this feature but also a myriad of others that allowed for example to support the common country / state selection problem by hooking up views that would call into the backend to dynamically select fields.

All to say that there might already be existing third packages implementing this feature if someone wants to tinker on how it can be done.

comment:8 by Sven R. Kunze, 5 months ago

Also came across this issue recently.

A bunch of StackOverflow questions could be solved a lot easier when the current instance would be available in limit_choices_to:

https://stackoverflow.com/questions/tagged/limit-choices-to?tab=Active
https://stackoverflow.com/questions/39593577/how-to-get-instance-of-entity-in-limit-choices-to-django
https://stackoverflow.com/questions/232435/how-do-i-restrict-foreign-keys-choices-to-related-objects-only-in-django
https://stackoverflow.com/questions/47774013/django-limit-choices-to-to-current-objects-id
https://stackoverflow.com/questions/28901089/use-field-value-in-limit-choices-to-in-django

Having a db model solution would make the proposed work-arounds obsolete/easier (especially they usually only work for a single form or admin/inline). Also maintaining a single place of the choice restriction is better also in terms of security (hidden/filtered choices due to perms).

I would imagine two possible some solutions:

  • one using an optional request=None, obj=None parameters to the callable (backwards compatible)
  • one using some special InstanceRef() object in a Q object (analogously to OuterRef("pk")

Personally, I would prefer the first solution because it's much more flexible and permission-wise the Request object is sometimes needed to realize more complex filtering.

@Simon nice lib 👍

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