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

Respect to_field property of ForeignKey relations #2435

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion rest_framework/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,14 @@ def __init__(self, slug_field=None, **kwargs):

def to_internal_value(self, data):
try:
return self.get_queryset().get(**{self.slug_field: data})
# Some tests do not have the full QuerySet API
# But we optimize SlugRelatedFields by only loading what we need
qs = self.get_queryset()
if hasattr(qs, 'only'):
return self.get_queryset().only(self.slug_field).get(**{self.slug_field: data})
else:
return self.get_queryset().get(**{self.slug_field: data})
Copy link
Member

Choose a reason for hiding this comment

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

Let's treat this stuff as a separate PR.

Copy link
Author

Choose a reason for hiding this comment

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

I'll remove it


except ObjectDoesNotExist:
self.fail('does_not_exist', slug_name=self.slug_field, value=smart_text(data))
except (TypeError, ValueError):
Expand Down
13 changes: 12 additions & 1 deletion rest_framework/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,7 @@ class ModelSerializer(Serializer):
models.URLField: URLField,
})
_related_class = PrimaryKeyRelatedField
_related_to_field_class = SlugRelatedField

def create(self, validated_data):
"""
Expand Down Expand Up @@ -1002,8 +1003,18 @@ 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.get('to_field', False)
Copy link
Member

Choose a reason for hiding this comment

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

Can you just do kwargs.pop('to_field', False) here instead of getting it, and then popping it on the next line?

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':
Copy link
Member

Choose a reason for hiding this comment

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

The primary key may not be named 'id' so I'm unsure about this.

Copy link
Author

Choose a reason for hiding this comment

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

Please see 2 (outdated diffs) above

Copy link
Member

Choose a reason for hiding this comment

The 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 AutoField considering it involves the id field?

I understand the point of doing if to_field, not the additional check though.

Copy link
Author

Choose a reason for hiding this comment

The 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.pop('to_field', None)
Copy link
Member

Choose a reason for hiding this comment

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

As we are popping this just a few lines above, the line shouldn't be needed.

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)
Expand Down
5 changes: 4 additions & 1 deletion rest_framework/utils/field_mapping.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Should this be kwargs['slug_field'] = to_field ?

Copy link
Member

Choose a reason for hiding this comment

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

It's being used in serializers.py with the check against id. If we determine that the check isn't actually needed, then I think we could get away with just using slug_field here.

Copy link
Member

Choose a reason for hiding this comment

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

It's being used in serializers.py with the check against id.

Sorry I didn't understand - you might need to link me to that?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

The 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)
Expand Down
20 changes: 20 additions & 0 deletions rest_framework/utils/model_meta.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
'model_field',
'related',
'to_many',
'to_field',
'has_through_model'
])

Expand Down Expand Up @@ -96,10 +97,18 @@ def _get_forward_relationships(opts):
"""
forward_relations = OrderedDict()
for field in [field for field in opts.fields if field.serialize and field.rel]:
# For < django 1.6
if hasattr(field, 'to_fields'):
to_field = field.to_fields[0] if len(field.to_fields) else None
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 that familiar with to_fields, why are we only using the first index here?

Copy link
Author

Choose a reason for hiding this comment

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

I think theres a comment somewhere. Theres an array named to_fields which contains the to_field parameter as the first index. I don't know why they did this, maybe they planned to expand on that feature or had to remain backward compatible but this shows some code from the ForeignKey Class:

# https://docs.djangoproject.com/en/1.7/_modules/django/db/models/fields/related/
class ForeignObject(RelatedField):
    requires_unique_target = True
    generate_reverse_relation = True
    related_accessor_class = ForeignRelatedObjectsDescriptor

    def __init__(self, to, from_fields, to_fields, swappable=True, **kwargs):
        self.from_fields = from_fields
        self.to_fields = to_fields

ForeignKey(ForeignObject):
during __init__:
    super(ForeignKey, self).__init__(to, ['self'], [to_field], **kwargs)

The documentation only refers to to_field and not to_fields.

Copy link
Member

Choose a reason for hiding this comment

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

It'll might be worth linking to the relevant part of the Django source given that it's non-obvious.

else:
to_field = None

forward_relations[field.name] = RelationInfo(
model_field=field,
related=_resolve_model(field.rel.to),
to_many=False,
# to_fields is an array but django lets you only set one to_field
to_field=to_field,
has_through_model=False
)

Expand All @@ -109,6 +118,8 @@ def _get_forward_relationships(opts):
model_field=field,
related=_resolve_model(field.rel.to),
to_many=True,
# manytomany do not have to_fields
to_field=None,
has_through_model=(
not field.rel.through._meta.auto_created
)
Expand All @@ -124,10 +135,17 @@ def _get_reverse_relationships(opts):
reverse_relations = OrderedDict()
for relation in opts.get_all_related_objects():
accessor_name = relation.get_accessor_name()
# For < django 1.6
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 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.

Copy link
Author

Choose a reason for hiding this comment

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

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

if hasattr(relation.field, 'to_fields'):
to_field = relation.field.to_fields[0] if len(relation.field.to_fields) else None
else:
to_field = None

reverse_relations[accessor_name] = RelationInfo(
model_field=None,
related=relation.model,
to_many=relation.field.rel.multiple,
to_field=to_field,
has_through_model=False
)

Expand All @@ -138,6 +156,8 @@ def _get_reverse_relationships(opts):
model_field=None,
related=relation.model,
to_many=True,
# manytomany do not have to_fields
to_field=None,
has_through_model=(
(getattr(relation.field.rel, 'through', None) is not None)
and not relation.field.rel.through._meta.auto_created
Expand Down