[Django] #25306: Should be an easier way to restrict the choices for a ForeignKey
Django
-------------------------------+--------------------
Reporter: CuriousG102 | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25306>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: CuriousG102 | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: 1.8
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:
How about using `AdminSite.form` and customizing the foreign key's
queryset in `form.__init__()` as described in
[https://docs.djangoproject.com/en/1.8/topics/forms/modelforms/#changing-
the-queryset Changing the queryset].
--
Ticket URL: <https://code.djangoproject.com/ticket/25306#comment:1>
Django
Reporter: CuriousG102 | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by CuriousG102):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25306#comment:2>
Django
Reporter: CuriousG102 | Owner: nobody
Type: New feature | Status: new
Component: contrib.admin | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by CuriousG102):
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25306#comment:3>
Django
-----------------------------+------------------------------------
Reporter: CuriousG102 | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* component: contrib.admin => Forms
* version: 1.8 => master
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25306#comment:4>
Django
Reporter: Miles Hutson | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Matthijs Kooijman):
* cc: Matthijs Kooijman (added)
Comment:
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
[https://github.com/django/django/blob/58c1acb1d6054dfec29d0f30b1033bae6ef62aec/django/forms/models.py#L307
BaseModelForm.__init__] through
[https://github.com/django/django/blob/58c1acb1d6054dfec29d0f30b1033bae6ef62aec/django/forms/models.py#L97
apply_limit_choices_to_to_formfield]. This leaves one path through
[https://github.com/django/django/blob/58c1acb1d6054dfec29d0f30b1033bae6ef62aec/django/forms/models.py#L182
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.
[https://github.com/django/django/blob/58c1acb1d6054dfec29d0f30b1033bae6ef62aec/django/db/models/fields/related.py#L907
ForeignKey.validate] has the instance, so it can simply pass that on. The
last path is through
[https://github.com/django/django/blob/58c1acb1d6054dfec29d0f30b1033bae6ef62aec/django/db/models/fields/__init__.py#L848
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25306#comment:5>
Django
------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Petr Dlouhý):
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25306#comment:6>
Django
------------------------------+------------------------------------
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by Simon Charette):
I thought I'd bring it the following since
[https://discord.com/channels/856567261900832808/1205482292580192367/1208377208780095558
this was discussed on the Discord].
About 15 years ago [https://github.com/charettes/django-dynamic-choices 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
{{{#!python
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
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25306#comment:7>
Django
------------------------------+------------------------------------
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by Sven R. Kunze):
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 👍
--
Ticket URL: <https://code.djangoproject.com/ticket/25306#comment:8>