Respect to_field property of ForeignKey relations #2435
Conversation
Uses the SlugRelatedField instead. SlugRelatedField will only get the Slugfield from the ORM as it does not need the other stuff
# Some tests do not have the full QuerySet API # But we optimize SlugRelatedFields by only loading what we need # it seems that some tests/django initializers are setting # to_field where it is totally unnecessary so SlugRelatedField was used… Added checks
rest_framework/serializers.py
Outdated
There was a problem hiding this comment.
Can you just do kwargs.pop('to_field', False) here instead of getting it, and then popping it on the next line?
lovelydinosaur
changed the title
rest_framework/utils/model_meta.py
Outdated
There was a problem hiding this comment.
I'm confused by the For < django 1.6 comment - is the meaning there that to_field is only supported in 1.6+?
We always wrap any version branching behavior in compat.py so you probably want to create a helper function in there named get_to_field(field) or similar, and put this logic there.
There was a problem hiding this comment.
The tests where failing on Travis for django versions below 1.6 and after a quick search i couldnt find to_field there so i assumed it was introduced in Django 1.6.
A little more searching shows that it was there but it is the other way around:
it was named to_field and later on mutated to to_fields as an array
https://github.com/django/django/blob/stable/1.4.x/django/db/models/fields/related.py
https://github.com/django/django/blob/stable/1.7.x/django/db/models/fields/related.py
That makes me also concerned about defaulting to index zero
Casual ForeignKeys shouldnt be a problem. They are giving a single to_field to their super class on initialization. But i found a few testcases in django related to ForeignObjects and Generics
https://github.com/django/django/search?utf8=%E2%9C%93&q=to_fields
|
Some inline comments. Seems to be on the right track! |
* Moved compatibility check of to_fields to compat.py * Expanded the check with a way that should work on django <1.6. Need to check trevis after pushing..
…ramework into model-tofield-serializer
rest_framework/relations.py
Outdated
There was a problem hiding this comment.
Pretty sure the only optimization is unrelated and should be removed.
rest_framework/compat.py
Outdated
There was a problem hiding this comment.
I still have to work on that one. Reading some more of that old code.
…ve django 1.5 installed so it is an educated guess by looking at ManyToOneRel and ForeignKey https://github.com/django/django/blob/stable/1.5.x/django/db/models/fields/related.py#L915
…ramework Conflicts: rest_framework/serializers.py
Conflicts: rest_framework/serializers.py
|
I've updated the pull request as i upgraded to the latest DRF Version and ran into some problems. By the way: |
|
I'll tend to think it's there. Any last minute comment about this one ? |
|
@Harper04 can you rebase your PR with master because there are some conflicts. |
|
Superseded by #3526. |
5 participants
Closes #2426.
This implementation just changes the Field class to SlugRelatedField and passes the to_field around (ultimately saving it to slug_field)
I also added a little optimization to SlugRelatedField i have to use. I get a recursive loop for my little use case because of the validation and loading of foreign models. But i am happy to remove it.
Testcases:
627 passed, 1 warnings in 5.12 seconds
Be gentle - it is my first contribution on github :)