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

Make related fields on ModelSerializer respect limit_choices_to #1811

Closed
pipermerriam opened this issue Aug 30, 2014 · 23 comments · Fixed by #6371
Closed

Make related fields on ModelSerializer respect limit_choices_to #1811

pipermerriam opened this issue Aug 30, 2014 · 23 comments · Fixed by #6371

Comments

@pipermerriam
Copy link
Contributor

The django ForeignKey fields support the keyword argument limit_choices_to, which when present, limits the available choices available by default when a ModelForm is created.

The serializer fields that represent relationships do not respect this keyword argument. I propose they should.

If accepted, it appears the place to do this would be in rest_framework.relations.RelatedField in the initialize method. Open to alternative suggestions on where/how to implement this.

@tomchristie tomchristie added this to the 3.0 Release milestone Sep 1, 2014
@tomchristie
Copy link
Member

Agreed. We should also obsolete this comment... https://github.com/tomchristie/django-rest-framework/blob/2.4.0/rest_framework/serializers.py#L826

Added the 3.0 milestone given the incoming serializer redesign.

@vccabral
Copy link
Contributor

vccabral commented Sep 5, 2014

Any suggestions on approach or considerations?

@vccabral
Copy link
Contributor

vccabral commented Sep 5, 2014

Can we add a kwargs to the queryset creation?

def __init__(self, *args, **kwargs):
    queryset = kwargs.pop('queryset', None)
    self.limit_choices_to = kwargs.pop('limit_choices_to', None)
    self.many = kwargs.pop('many', self.many)
    if self.many:
        self.widget = self.many_widget
        self.form_field_class = self.many_form_field_class

    kwargs['read_only'] = kwargs.pop('read_only', self.read_only)
    super(RelatedField, self).__init__(*args, **kwargs)

    if not self.required:
        # Accessed in ModelChoiceIterator django/forms/models.py:1034
        # If set adds empty choice.
        self.empty_label = BLANK_CHOICE_DASH[0][1]

    self.queryset = queryset.filter(**self.limit_choices_to)

@tomchristie
Copy link
Member

Can we add a kwargs to the queryset creation?

There's two parts to this issue.

Firstly - supporting the extra keyword argument in relational fields.
Secondly - ensuring the automatically generated fields for ModelSerializer have limit_choices_to automatically set.

@pipermerriam
Copy link
Contributor Author

Here are the cases I see. Lemme know if this behavior seems wrong.

  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.

Number 3 is the only one that has any ambiguity to me. It seems like it could be undesirable or unexpected behavior for the queryset to be modified in this case.

@tomchristie
Copy link
Member

I think I'd agree with all that, yup.

@pipermerriam
Copy link
Contributor Author

I'm hoping to take a stab at this sometime this weekend. Tom, those are
the two things I had in mind.
On Sep 6, 2014 12:26 AM, "Tom Christie" notifications@github.com wrote:

Can we add a kwargs to the queryset creation?

There's two parts to this issue.

Firstly - supporting the extra keyword argument in relational fields.
Secondly - ensuring the automatically generated fields for ModelSerializer
have limit_choices_to automatically set.


Reply to this email directly or view it on GitHub
#1811 (comment)
.

@pipermerriam
Copy link
Contributor Author

I opened up #1843 with my first stab at this. Tests passed locally, though I don't have all of the python versions tox wants to test against. Interested in feedback on my approach.

Specifically:

  1. I'm a little unsure about my approach for setting limit_choices_to inside of of the initialize when it needs to be pulled from the parent model. It feels heavy.
  2. I think I've got gaps in my test coverage but I don't know where. I tested with RelatedField but I'm not sure whether I need to do any deeper testing using things like PrimaryKeyRelatedField or HyperlinkedRelatedField
  3. It felt weird to copy over the get_limit_choices_to method from django, but it seemed like an established way to do that so... Not sure if that's bad form.
  4. The additions to the documentation feel thin.

@tomchristie
Copy link
Member

On review it's not clear to me if we need to support limit_choices_to as an initializer itself, or if we should just respect it on automatically created fields, but apply it to the initial queryset that's passed. Thoughts?

@tomchristie
Copy link
Member

Actually given that the Django docs state that limit_choices_to may be re-evaluated multiple times...

  • When instantiating the form.
  • During validation.

I'm not sure if my suggestion is valid or not.

@pipermerriam
Copy link
Contributor Author

I'm not sure I follow your self-rebuttal about it being re-evaluated and how that applies.

I think it's a convenient way to limit the available choices to a field without having to actually provide a queryset. I'm also not sure that convenience is worth the maintenance cost. In general I'm not married to it and don't have a solid enough picture of the future of serializers to make this decision myself. I think I'm +0.

@tomchristie
Copy link
Member

I'm also not sure that convenience is worth the maintenance cost.

Yeah :-| whole lot of code there.

@pipermerriam
Copy link
Contributor Author

What's the best way for us to figure this out. Would you like a second PR that supports limit_choices_to only on generated fields, and does not introduce a new kwarg into RelatedField?

@tomchristie
Copy link
Member

Honestly, I'd say defer this until after 3.0. That won't be all that far off and we'll be in a better position to assess then.

@tomchristie tomchristie modified the milestones: 3.1 Release, 3.0 Release Sep 8, 2014
@pipermerriam
Copy link
Contributor Author

Works for me. I'll try to keep up with 3.0 development.
On Sep 8, 2014 1:15 PM, "Tom Christie" notifications@github.com wrote:

Honestly, I'd say defer this until after 3.0. That won't be all that far
off and we'll be in a better position to assess then.


Reply to this email directly or view it on GitHub
#1811 (comment)
.

@tomchristie
Copy link
Member

Milestoning to 3.0.2 as I don't think it'd be too hard to resolve this.

@pipermerriam
Copy link
Contributor Author

Happy to do the lifting on this if you give me some direction as to how you would prefer this be implemented.

@tomchristie
Copy link
Member

Unless an option in the limit_to dict is a callable it'd make sense to simply apply it as a filter to the initial queryset on the serializer relationship.

Given that, the only case where we'd need to support filter_to as an option on serializer relations would be if the options are callables. To enforce consistency in codebases we should make filter_to on a serializer relationship actually error out if it's not a callable.

We could also do something nifty, like use the same set_context style as we use on default callables. These are class callables that also have a set_context method that is initially called by the serailizer field directly before the callable is invoked. This allows us to eg implement a CurrentUser callable. See: https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/fields.py#L119-127

It's a slightly complex bit of API, but it's then pretty nice that we can do stuff like filter related fields to only be the instances that belong to the requesting user.

@tomchristie
Copy link
Member

Also refs: #1985.

@tomchristie
Copy link
Member

Also note that serializer representations ought to include any filtering clauses.

@tomchristie tomchristie removed this from the 3.0.2 Release milestone Dec 17, 2014
@tomchristie tomchristie modified the milestones: 3.2.0 Release, 3.3.0 Jun 3, 2015
@tomchristie tomchristie removed this from the 3.3.0 Release milestone Jul 31, 2015
@mishu-
Copy link

mishu- commented Mar 5, 2017

This is open since 2014. I just ran into the same problem. What is a definite answer on how to solve this?

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 6, 2017

Someone needs to step in and update #1843

@sergiomb2
Copy link

sergiomb2 commented Jun 5, 2017

IMHO DRF should respect limit_choices_to , is not so uncommon limit a FK to is_active=True the solution provide in #3605 , worked for me i.e. override class PrimaryKeyRelatedField(RelatedField):
with my custom queryset seems that work :

class CustomForeignKey(serializers.PrimaryKeyRelatedField):
    def get_queryset(self):
        return Table.objects.filter(is_active=True)

class Serializer(serializers.ModelSerializer):
    (...)
   table= CustomForeignKey()
   class Meta:
   (...)

even more easy is :

 class Serializer(serializers.ModelSerializer):
 (...)
 table = serializers.PrimaryKeyRelatedField(queryset=Table.objects.filter(is_active=True)) 
 class Meta:
 (...)

tomchristie pushed a commit that referenced this issue Jan 8, 2019
* Fix issue1811: take limit_choices_to into account with FK

* Issue 1811: Add tests to illustrate issue

* Filter queryset only if limit_choices_to exists

* Move test_relations_with_limited_querysets file within test_relations_pk

* move limit_choices_to logic from relations.py to utils/field_mapping.py

* move limit_choices_to above other check to avoid conflicts
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this issue Nov 17, 2020
)

* Fix issue1811: take limit_choices_to into account with FK

* Issue 1811: Add tests to illustrate issue

* Filter queryset only if limit_choices_to exists

* Move test_relations_with_limited_querysets file within test_relations_pk

* move limit_choices_to logic from relations.py to utils/field_mapping.py

* move limit_choices_to above other check to avoid conflicts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants