Skip to content

Respect to_field property of ForeignKey relations #2435

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.

Sign up for GitHub

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

Closed
Harper04 wants to merge 15 commits into from
Closed

Respect to_field property of ForeignKey relations #2435

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
067360c
Added support for the to_field on ForeignKey Fields.
Harper04 Jan 19, 2015
8263048
* many to many relationship do not have to_fields
Harper04 Jan 19, 2015
d47f29a
Fixed the rest of the test cases.
Harper04 Jan 19, 2015
49ecaa7
Add support for Django <1.6 where appearently no to_field existed
Harper04 Jan 19, 2015
6369f93
* Removed 2 unnecessary lines
Harper04 Jan 19, 2015
6007463
Merge branch 'master' of https://github.com/tomchristie/django-rest-f…
Harper04 Jan 19, 2015
f808b6c
* Removed SlugRelatedField optimization
Harper04 Jan 20, 2015
77eeefa
Merge branch 'master' of https://github.com/tomchristie/django-rest-f…
Harper04 Jan 20, 2015
3dd5ddd
Merge branch 'master' of https://github.com/tomchristie/django-rest-f…
Harper04 Jan 20, 2015
6ccc45e
Ups. Removed the wrong line. (optimizer)
Harper04 Jan 20, 2015
64c49f8
From this i believe it is field_name pre django 1.6. I still don’t ha…
Harper04 Jan 20, 2015
a496c52
to_field is rel.field_name at <1.6. love pdb and pip!
Harper04 Jan 20, 2015
8096997
Merge branch 'master' of https://github.com/tomchristie/django-rest-f…
Harper04 Apr 16, 2015
54e16f4
Merge branch 'master' into model-tofield-serializer
Harper04 Apr 16, 2015
5ca9841
* updated patch to latest DRF revision
Harper04 Apr 16, 2015
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
Prev Previous commit
Next Next commit
Add support for Django <1.6 where appearently no to_field existed
  • Loading branch information
@Harper04
Harper04 committed Jan 19, 2015
commit 49ecaa7d9e727dfe0a0deaf95fd87298d40ee35f
15 changes: 13 additions & 2 deletions rest_framework/utils/model_meta.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +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
Contributor

@kevin-brown kevin-brown Jan 19, 2015

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

@Harper04 Harper04 Jan 19, 2015

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
Contributor

@lovelydinosaur lovelydinosaur Jan 20, 2015

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=field.to_fields[0] if len(field.to_fields) else None,
to_field=to_field,
has_through_model=False
)

Expand All @@ -130,11 +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
Contributor

@lovelydinosaur lovelydinosaur Jan 20, 2015

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

@Harper04 Harper04 Jan 20, 2015

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=relation.field.to_fields[0] if len(relation.field.to_fields) else None,
to_field=to_field,
has_through_model=False
)

Expand Down