-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Respect to_field property of ForeignKey relations #2435
Changes from 7 commits
067360c
8263048
d47f29a
49ecaa7
6369f93
6007463
f808b6c
77eeefa
3dd5ddd
6ccc45e
64c49f8
a496c52
8096997
54e16f4
5ca9841
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -303,7 +303,7 @@ def __init__(self, slug_field=None, **kwargs): | |
|
||
def to_internal_value(self, data): | ||
try: | ||
return self.get_queryset().get(**{self.slug_field: data}) | ||
return self.get_queryset().only(self.slug_field).get(**{self.slug_field: data}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty sure the |
||
except ObjectDoesNotExist: | ||
self.fail('does_not_exist', slug_name=self.slug_field, value=smart_text(data)) | ||
except (TypeError, ValueError): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -727,6 +727,7 @@ class ModelSerializer(Serializer): | |
models.URLField: URLField, | ||
}) | ||
_related_class = PrimaryKeyRelatedField | ||
_related_to_field_class = SlugRelatedField | ||
|
||
def create(self, validated_data): | ||
""" | ||
|
@@ -1002,8 +1003,16 @@ def get_fields(self): | |
field_cls = self._get_nested_class(depth, relation_info) | ||
kwargs = get_nested_relation_kwargs(relation_info) | ||
else: | ||
field_cls = self._related_class | ||
kwargs = get_relation_kwargs(field_name, relation_info) | ||
to_field = kwargs.pop('to_field', None) | ||
# it seems that some tests/django initializers are setting | ||
# to_field where it is totally unnecessary | ||
if to_field and to_field != 'id': | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The primary key may not be named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see 2 (outdated diffs) above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add some clarification on this? The comment is a bit vauge about what the issue actually is, though I assume it has to do with I understand the point of doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it also has to do with what gets into that to_field and makes it to that point. I would remove any 'id' entries in compat/get_to_field making this check here unnecessary. Maybe not the best solution but the to_field irritates me for now. I think for plain ForeignKeys it is safe but for anything else... i dont know. I added some links to the core issue of that 2 posts above. Thats what i meant. |
||
# using the slug field for now | ||
kwargs['slug_field'] = to_field | ||
field_cls = self._related_to_field_class | ||
else: | ||
field_cls = self._related_class | ||
# `view_name` is only valid for hyperlinked relationships. | ||
if not issubclass(field_cls, HyperlinkedRelatedField): | ||
kwargs.pop('view_name', None) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -194,7 +194,7 @@ def get_relation_kwargs(field_name, relation_info): | |
""" | ||
Creates a default instance of a flat relational field. | ||
""" | ||
model_field, related_model, to_many, has_through_model = relation_info | ||
model_field, related_model, to_many, to_field, has_through_model = relation_info | ||
kwargs = { | ||
'queryset': related_model._default_manager, | ||
'view_name': get_detail_view_name(related_model) | ||
|
@@ -203,6 +203,9 @@ def get_relation_kwargs(field_name, relation_info): | |
if to_many: | ||
kwargs['many'] = True | ||
|
||
if to_field: | ||
kwargs['to_field'] = to_field | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's being used in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry I didn't understand - you might need to link me to that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/tomchristie/django-rest-framework/pull/2435/files#diff-80f43677c94659fbd60c8687cce68eafR1007 and the line with your comment below it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I named it to_field because mentally i am still treating it like a ForeignKey object at that point. Pointing it to a slug_field afterwards was just handy |
||
|
||
if has_through_model: | ||
kwargs['read_only'] = True | ||
kwargs.pop('queryset', None) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have to work on that one. Reading some more of that old code.