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

Add support for limit_choices_to on related fields. #1843

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/api-guide/relations.md
Expand Up @@ -102,6 +102,7 @@ By default this field is read-write, although you can change this behavior using
* `many` - If applied to a to-many relationship, you should set this argument to `True`.
* `required` - If set to `False`, the field will accept values of `None` or the empty-string for nullable relationships.
* `queryset` - By default `ModelSerializer` classes will use the default queryset for the relationship. `Serializer` classes must either set a queryset explicitly, or set `read_only=True`.
* `limit_choices_to` - Used to filter the `queryset` to a limited set of choices. Either a dictionary, a Q object, or a callable returning a dictionary or Q object can be used.

## HyperlinkedRelatedField

Expand Down
28 changes: 28 additions & 0 deletions rest_framework/relations.py
Expand Up @@ -41,6 +41,8 @@ class RelatedField(WritableField):

def __init__(self, *args, **kwargs):
queryset = kwargs.pop('queryset', None)
self.limit_choices_to = kwargs.pop('limit_choices_to', {})

self.many = kwargs.pop('many', self.many)
if self.many:
self.widget = self.many_widget
Expand All @@ -58,13 +60,39 @@ def __init__(self, *args, **kwargs):

def initialize(self, parent, field_name):
super(RelatedField, self).initialize(parent, field_name)

if self.queryset is None and not self.limit_choices_to:
manager = getattr(self.parent.opts.model, self.source or field_name)
try:
self.limit_choices_to = manager.field.rel.limit_choices_to
except AttributeError:
# GenericForeignKey and their reverse relationships don't have
# a `field` property.
pass

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this code block is for. self.limit_choices_to is only relevant when queryset is not None so why do we need to set it to anything here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the event that the model field is defined as:

class MyModel(models.Model):
    user = models.ForeignKey(User, limit_choices_to={'is_active': True})

it seemed appropriate for DRF to respect that value and filter the default queryset.

Do you think it would be better to only do filtering when limit_choices_to is explicitly declared on the serializer field?

if self.queryset is None and not self.read_only:
manager = getattr(self.parent.opts.model, self.source or field_name)
if hasattr(manager, 'related'): # Forward
self.queryset = manager.related.model._default_manager.all()
else: # Reverse
self.queryset = manager.field.rel.to._default_manager.all()

if self.queryset is not None:
self.queryset = self.queryset.complex_filter(self.get_limit_choices_to())

def get_limit_choices_to(self):
"""
source: `django.db.models.fields.related.RelatedField.get_limit_choices_to`

Returns 'limit_choices_to' for this serializer field.

If it is a callable, it will be invoked and the result will be
returned.
"""
if callable(self.limit_choices_to):
return self.limit_choices_to()
return self.limit_choices_to

# We need this stuff to make form choices work...

def prepare_value(self, obj):
Expand Down
10 changes: 5 additions & 5 deletions rest_framework/serializers.py
Expand Up @@ -822,9 +822,6 @@ def get_related_field(self, model_field, related_model, to_many):

Note that model_field will be `None` for reverse relationships.
"""
# TODO: filter queryset using:
# .using(db).complex_filter(self.rel.limit_choices_to)

kwargs = {
'queryset': related_model._default_manager,
'many': to_many
Expand All @@ -846,6 +843,9 @@ def get_related_field(self, model_field, related_model, to_many):
if model_field.help_text is not None:
kwargs['help_text'] = model_field.help_text

if model_field.rel.limit_choices_to:
kwargs['limit_choices_to'] = model_field.rel.limit_choices_to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not clear about whether it should be related_model instead of model_field.rel here. Thoughts ?


return PrimaryKeyRelatedField(**kwargs)

def get_field(self, model_field):
Expand Down Expand Up @@ -1101,8 +1101,6 @@ def get_related_field(self, model_field, related_model, to_many):
"""
Creates a default instance of a flat relational field.
"""
# TODO: filter queryset using:
# .using(db).complex_filter(self.rel.limit_choices_to)
kwargs = {
'queryset': related_model._default_manager,
'view_name': self._get_default_view_name(related_model),
Expand All @@ -1115,6 +1113,8 @@ def get_related_field(self, model_field, related_model, to_many):
kwargs['help_text'] = model_field.help_text
if model_field.verbose_name is not None:
kwargs['label'] = model_field.verbose_name
if model_field.rel.limit_choices_to:
kwargs['limit_choices_to'] = model_field.rel.limit_choices_to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment.


if self.opts.lookup_field:
kwargs['lookup_field'] = self.opts.lookup_field
Expand Down
8 changes: 8 additions & 0 deletions tests/models.py
Expand Up @@ -134,6 +134,14 @@ class OptionalRelationModel(RESTFrameworkModel):
other = models.ForeignKey('OptionalRelationModel', blank=True, null=True)


# Model for issue #1811
class LimitedChoicesModel(RESTFrameworkModel):
rel = models.ForeignKey(
'ForeignKeyTarget',
limit_choices_to={'name': 'foo'},
)


# Model for RegexField
class Book(RESTFrameworkModel):
isbn = models.CharField(max_length=13)
Expand Down
162 changes: 161 additions & 1 deletion tests/test_relations.py
Expand Up @@ -7,7 +7,7 @@
from django.test import TestCase
from django.utils import unittest
from rest_framework import serializers
from tests.models import BlogPost
from tests.models import BlogPost, LimitedChoicesModel, ForeignKeyTarget


class NullModel(models.Model):
Expand Down Expand Up @@ -147,3 +147,163 @@ def test_blank_option_is_added_to_choice_if_required_equals_false(self):
widget_count = len(field.widget.choices)

self.assertEqual(widget_count, choice_count + 1, 'BLANK_CHOICE_DASH option should have been added')


class LimitChoicesToTest(TestCase):
"""
Test for #1811 "Support `limit_choices_to` on related fields."

1. Fully auto-generated relation field: should limit if model field has
limit_choices_to set
2. Declared relation field with neither queryset or limit_choices_to field
set: should limit if model field has limit_choices_to set
3. Declared relation field with queryset declared and without
limit_choices_to set where the model field has limit_choices_to set: The
user has explicitly declared a queryset so I don't think we should modify
it.
4. Declared relation field with limit_choices_to declared and no queryset
declared: Should limit choices based on the limit_choices_to that was
declared on the serializer field.
5. Declared relation field with both limit_choices_to and queryset
declared: I think that since both were declared, that it makes sense to go
ahead and apply the limit choices to filtering to the provided queryset.
"""
def test_generated_field_with_limit_choices_set_on_model_field(self):
"""
Ensure that for a fully auto-generated serializer field for a model
field which has the `limit_choices_to` value set that the queryset is
filtered correctly on the value from the model field.
"""
# Generate one instance that will match the `limit_choices_to`
ForeignKeyTarget.objects.create(name='foo')
ForeignKeyTarget.objects.create(name='bar')

class LimitChoicesSerializer(serializers.ModelSerializer):
class Meta:
model = LimitedChoicesModel
fields = ('rel',)

serializer = LimitChoicesSerializer()

field = serializer.fields['rel']
queryset = field.queryset

self.assertEqual(
set(queryset.all()),
set(ForeignKeyTarget.objects.filter(name='foo')),
)

def test_declared_related_field_with_limit_choices_set_on_model(self):
"""
Test that a declared `RelatedField` will correctly filter on it's model
field's `limit_choices_to` when neither the queryset nor the local
`limit_choices_to` has been declared.

TODO: is this test necessary?
"""
# Generate one instance that will match the `limit_choices_to`
ForeignKeyTarget.objects.create(name='foo')
ForeignKeyTarget.objects.create(name='bar')

class LimitChoicesSerializer(serializers.ModelSerializer):
rel = serializers.RelatedField(read_only=False)

class Meta:
model = LimitedChoicesModel
fields = ('rel',)

serializer = LimitChoicesSerializer()

field = serializer.fields['rel']
queryset = field.queryset

self.assertEqual(
set(queryset.all()),
set(ForeignKeyTarget.objects.filter(name='foo')),
)

def test_declared_queryset_on_related_field_is_not_effected_by_model_limit_choices_to(self):
"""
Test that when the `queryset` kwarg is declared for a `RelatedField`
that it isn't further filtered when `limit_choices_to` has been
declared on the model field.
"""
ForeignKeyTarget.objects.create(name='foo')
ForeignKeyTarget.objects.create(name='bar')

class LimitChoicesSerializer(serializers.ModelSerializer):
rel = serializers.RelatedField(
queryset=ForeignKeyTarget.objects.all(),
read_only=False,
)

class Meta:
model = LimitedChoicesModel
fields = ('rel',)

serializer = LimitChoicesSerializer()

field = serializer.fields['rel']
queryset = field.queryset

self.assertEqual(
set(queryset.all()),
set(ForeignKeyTarget.objects.all()),
)

def test_limit_choices_to_on_serializer_field_overrides_model_field(self):
"""
Test that when `limit_choices_to` is declared on a serializer field
that it correctly overrides the value declared on the model field.
"""
ForeignKeyTarget.objects.create(name='foo')
ForeignKeyTarget.objects.create(name='bar')

class LimitChoicesSerializer(serializers.ModelSerializer):
rel = serializers.RelatedField(limit_choices_to={'name': 'bar'}, read_only=False)

class Meta:
model = LimitedChoicesModel
fields = ('rel',)

serializer = LimitChoicesSerializer()

field = serializer.fields['rel']
queryset = field.queryset

self.assertEqual(
set(queryset.all()),
set(ForeignKeyTarget.objects.filter(name='bar')),
)

def test_serializer_field_with_both_limit_choices_to_and_queryset_is_filtered(self):
"""
Test that when both the `limit_choices_to` and `queryset` are declared
for a serializer field that the provided queryset is subsequently
filtered using the provided `limit_choices_to`.
"""
ForeignKeyTarget.objects.create(name='foo')
only_choice = ForeignKeyTarget.objects.create(name='bar')
ForeignKeyTarget.objects.create(name='baz')
to_exclude = ForeignKeyTarget.objects.create(name='bar')

class LimitChoicesSerializer(serializers.ModelSerializer):
rel = serializers.RelatedField(
limit_choices_to={'name': 'bar'},
queryset=ForeignKeyTarget.objects.exclude(pk=to_exclude.pk),
read_only=False,
)

class Meta:
model = LimitedChoicesModel
fields = ('rel',)

serializer = LimitChoicesSerializer()

field = serializer.fields['rel']
queryset = field.queryset

self.assertEqual(
set(queryset.all()),
set(ForeignKeyTarget.objects.filter(pk=only_choice.pk)),
)