Skip to content

Defer to lookup_field/custom id fields #409

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
wants to merge 4 commits into from
Closed

Defer to lookup_field/custom id fields #409

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
679a272
Issue 155 Defer to lookup_field/custom id fields
rpatterson Feb 21, 2017
50fdfe0
minor fixes
atarkowska Feb 18, 2018
232d580
test if relation is not empty
atarkowska Oct 5, 2017
1b0e853
relation must have either links, data or meta in relationship
atarkowska Nov 20, 2017
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
54 changes: 43 additions & 11 deletions rest_framework_json_api/renderers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,8 @@ def extract_relationships(cls, fields, resource, resource_instance):
relation_data.append(
OrderedDict([
('type', relation_type),
('id', encoding.force_text(related_object.pk))
('id', encoding.force_text(getattr(
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's happening with this getattr? If my understanding of getattr is correct, then something seems off here. Let's say fields.lookup_field = 'foo'. Wouldn't this getattr call be equivalent to related_object.pk.foo?

Copy link
Author

Choose a reason for hiding this comment

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

something went wrong when cherry-picking, sorted now

related_object, field.lookup_field)))
])
)

Expand Down Expand Up @@ -150,18 +151,24 @@ def extract_relationships(cls, fields, resource, resource_instance):
if isinstance(
field, (relations.PrimaryKeyRelatedField, relations.HyperlinkedRelatedField)
):
resolved, relation = utils.get_relation_instance(
resource_instance, '%s_id' % source, field.parent
)
if not resolved:
continue
relation_id = relation if resource.get(field_name) else None
lookup_field = getattr(field, 'lookup_field', None)
if lookup_field:
relation_id = getattr(
Copy link
Member

Choose a reason for hiding this comment

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

could the lookup field not also be a relation?

getattr(resource_instance, source),
lookup_field, None)
else:
resolved, relation = utils.get_relation_instance(
resource_instance, '%s_id' % source, field.parent
)
if not resolved:
continue
relation_id = relation if resource.get(field_name) else None
relation_data = {
'data': (
OrderedDict([
('type', relation_type), ('id', encoding.force_text(relation_id))
])
if relation_id is not None else None)
if relation_id is not None else {'data': None})
}

if (
Expand Down Expand Up @@ -216,9 +223,15 @@ def extract_relationships(cls, fields, resource, resource_instance):
utils.get_resource_type_from_instance(nested_resource_instance)
)

if hasattr(field.child_relation, 'lookup_field'):
Copy link
Member

Choose a reason for hiding this comment

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

couldn't this code block be simplified if we add a lookup_field to ResourceRelatedField defaulting to pk?

pk = getattr(nested_resource_instance,
field.child_relation.lookup_field)
else:
pk = nested_resource_instance.pk

relation_data.append(OrderedDict([
('type', nested_resource_instance_type),
('id', encoding.force_text(nested_resource_instance.pk))
('id', encoding.force_text(pk))
]))
data.update({
field_name: {
Expand All @@ -244,14 +257,23 @@ def extract_relationships(cls, fields, resource, resource_instance):
if isinstance(serializer_data, list):
for position in range(len(serializer_data)):
nested_resource_instance = resource_instance_queryset[position]
nested_resource_data = serializer_data[position]
nested_resource_instance_type = (
relation_type or
utils.get_resource_type_from_instance(nested_resource_instance)
)

instance_pk_name = (
getattr(field, 'lookup_field', None) or
nested_resource_instance._meta.pk.name)
if instance_pk_name in nested_resource_data:
pk = nested_resource_data[instance_pk_name]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you change this to use a consistent name? Higher in this diff, the variable passed to force_text was _id. Now it's pk. I think we should pick one and stick with it.

else:
pk = nested_resource_instance.pk

relation_data.append(OrderedDict([
('type', nested_resource_instance_type),
('id', encoding.force_text(nested_resource_instance.pk))
('id', encoding.force_text(pk))
]))

data.update({field_name: {'data': relation_data}})
Expand Down Expand Up @@ -463,9 +485,19 @@ def build_json_resource_obj(cls, fields, resource, resource_instance, resource_n
# Determine type from the instance if the underlying model is polymorphic
if force_type_resolution:
resource_name = utils.get_resource_type_from_instance(resource_instance)
if resource_instance is None:
pk = None
elif 'id' in fields:
pk = fields['id'].get_attribute(resource_instance)
elif 'url' in fields:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of 'url' api_settings.URL_FIELD_NAME should be used.

pk = getattr(resource_instance, fields['url'].lookup_field)
else:
# Check if the primary key exists in the resource by getting the
# primary keys attribute name.
pk = resource_instance.pk
resource_data = [
('type', resource_name),
('id', encoding.force_text(resource_instance.pk) if resource_instance else None),
('id', encoding.force_text(pk)),
('attributes', cls.extract_attributes(fields, resource)),
]
relationships = cls.extract_relationships(fields, resource, resource_instance)
Expand Down