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

Fix #1811: take limit_choices_to into account with FK #6371

Merged
merged 6 commits into from Jan 8, 2019

Conversation

adrienbrunet
Copy link
Contributor

@adrienbrunet adrienbrunet commented Dec 20, 2018

Closes #1811

I created a small repo to test this fix and it works as expected. It is based on #5358 proposal. (I've done nothing other than formatting the code)

Tests are passing but some more tests for this particular bit should be added, hence the WIP in the title.

(If anyone wants to help...)

@adrienbrunet
Copy link
Contributor Author

Finally I took some time to add a simple test for this.
I don't know if it's enough or if we should add more. It's not really elegant though.
Advices welcome.

Copy link
Collaborator

@xordoquy xordoquy left a comment

Choose a reason for hiding this comment

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

Would it be possible to move the test within the test_relations_pk.py file ?

rest_framework/relations.py Outdated Show resolved Hide resolved
@adrienbrunet
Copy link
Contributor Author

adrienbrunet commented Dec 21, 2018

I moved my tests to test_relations_pk file and updated the code in relations.py to filter only when limit_choices_to is defined. I'm all ears if you have any other requests.

@adrienbrunet adrienbrunet changed the title WIP: Issue #1811: take limit_choices_to into account with FK Issue #1811: take limit_choices_to into account with FK Dec 27, 2018
@adrienbrunet adrienbrunet changed the title Issue #1811: take limit_choices_to into account with FK Fix #1811: take limit_choices_to into account with FK Dec 28, 2018
@tomchristie
Copy link
Member

Thanks for this. Looks like it's working towards a resolution.

@adrienbrunet
Copy link
Contributor Author

Thanks for this. Looks like it's working towards a resolution.

What better feeling than fixing a 5-year old issue ? 😁

What do you think about the latest changes ?

@@ -266,6 +266,9 @@ def get_relation_kwargs(field_name, relation_info):
# If this field is read-only, then return early.
# No further keyword arguments are valid.
return kwargs
limit_choices_to = model_field.get_limit_choices_to()
if limit_choices_to:
kwargs['queryset'] = kwargs['queryset'].filter(**limit_choices_to)
Copy link
Member

Choose a reason for hiding this comment

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

I think this block should probably go above the if has_through_model: switch. (L252) Since there's a couple of cases where the queryset keyword argument gets popped off.

Copy link
Member

Choose a reason for hiding this comment

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

Or else use if queryset in kwargs and limit_choices_to:, which would also do the trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I did not think it through. I just wanted to avoid extra work putting it after the read_only but forgot that queryset can be popped.

Maybe another solution would be to add a check like 'queryset' in kwargs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was too slow to post my comment and did not see yours. :/

@tomchristie
Copy link
Member

Nice one! I think there's one last bit to fix here - making sure that the change still works in cases where the queryset keyword argument doesn't exist anymore.

@tomchristie tomchristie dismissed xordoquy’s stale review January 8, 2019 13:42

Requested change has been made.

@adrienbrunet
Copy link
Contributor Author

I'm switching back my solution to the previous one and adding the queryset in kwargs check. Would you like a rebase to avoid all this back and forth commits ?

@tomchristie
Copy link
Member

@adrianmester No need to switch it - looks good as is.

And no to the rebase - I'll just use Squash and merge.

@tomchristie
Copy link
Member

tomchristie commented Jan 8, 2019

What better feeling than fixing a 5-year old issue ? 😁

Neat, yup. It's the oldest open issue on our list!

https://github.com/encode/django-rest-framework/issues?page=6&q=is%3Aissue+is%3Aopen

@adrienbrunet
Copy link
Contributor Author

Allright. That's good for me. It was a pleasure to be part of the team for the last half hour or so 😃

@tomchristie
Copy link
Member

Boom 💥

@tomchristie tomchristie merged commit e3bd4b9 into encode:master Jan 8, 2019
@adrienbrunet adrienbrunet deleted the issue-1811 branch January 8, 2019 13:50
@ulgens
Copy link

ulgens commented Jan 18, 2019

https://docs.djangoproject.com/en/2.1/ref/models/fields/#django.db.models.ForeignKey.limit_choices_to

It's possible that limit_choices_to can get a Q object, but this PR doesn't consider that and expects for a dictionary.

(boom is denied 😄 )

@ulgens
Copy link

ulgens commented Jan 18, 2019

It's possible that limit_choices_to can get a Q object, but this PR doesn't consider that and expects for a dictionary.

A different version, which consider Q objects: dogukantufekci@607059d

@vasdee vasdee mentioned this pull request Feb 24, 2019
6 tasks
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request 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 this pull request may close these issues.

Make related fields on ModelSerializer respect limit_choices_to
5 participants