Defer to lookup_field/custom id fields #409
Conversation
ghost
changed the title
TODO Test coverage There are so many places where PK is assumed in djangorestframework-jsonapi and so many places/ways to change IDs/URLs in DRF itself, that getting coverage on all the edge cases will be quite a lot of work. Some of that work may be better spent re-factoring things to make the code easier to understand and reduce the number of edge cases in the first place.
ghost
changed the base branch from
Codecov Report
@@ Coverage Diff @@
## master #409 +/- ##
==========================================
- Coverage 91.75% 91.53% -0.23%
==========================================
Files 55 55
Lines 2923 2941 +18
==========================================
+ Hits 2682 2692 +10
- Misses 241 249 +8
Continue to review full report at Codecov.
|
| @@ -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( | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
something went wrong when cherry-picking, sorted now
| 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] |
There was a problem hiding this comment.
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.
|
@mblayman Please merge if you are okay with the changes. |
|
any chance to merge this PR please? |
There was a problem hiding this comment.
I am picking this up from @mblayman
Thanks for your work. It looks good so far. I think a few things need to be done to get this ready for merging though.
Some comments I have added above and here some general points:
- Some test cases need to be added (doesn't have to fully cover all cases but we at least need a test case which covers the new feature you are implementing so we know it works
- Add documentation how this can be used
| @@ -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'): | |||
There was a problem hiding this comment.
couldn't this code block be simplified if we add a lookup_field to ResourceRelatedField defaulting to pk?
| relation_id = relation if resource.get(field_name) else None | ||
| lookup_field = getattr(field, 'lookup_field', None) | ||
| if lookup_field: | ||
| relation_id = getattr( |
There was a problem hiding this comment.
could the lookup field not also be a relation?
| pk = None | ||
| elif 'id' in fields: | ||
| pk = fields['id'].get_attribute(resource_instance) | ||
| elif 'url' in fields: |
There was a problem hiding this comment.
Instead of 'url' api_settings.URL_FIELD_NAME should be used.
|
@Ola-t |
Thsi PR allow support for custom pk #155
There are so many places where PK is assumed in
djangorestframework-jsonapi and so many places/ways to change IDs/URLs
in DRF itself, that getting coverage on all the edge cases will be quite
a lot of work. Some of that work may be better spent re-factoring
things to make the code easier to understand and reduce the number of
edge cases in the first place.