Fix included resources not being included on list #307
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #307 +/- ##
===========================================
+ Coverage 91.55% 91.58% +0.02%
===========================================
Files 49 49
Lines 2310 2318 +8
===========================================
+ Hits 2115 2123 +8
Misses 195 195
Continue to review full report at Codecov.
|
|
@jerel hi! It looks like you are the current maintainer for DRF JA. Do you need some help with maintenance? The issue associated with this PR is killing my app's performance. I'd love to get it fixed and released, but I understand that things happen and people get swamped. If I could lend a bit of a hand to help close some of this stuff out, I'd be happy to do that. |
|
@mblayman I am; I would definitely appreciate help with maintenance. |
|
@rpatterson It seems like you've pushed some commits that are beyond the original scope of this PR. Is this stuff related? If so, can you explain? I think it would be difficult to review if this is a bunch of things all in one branch. |
|
@mblayman Sorry, cleaned up my branches such that |
| relation_model = relation.get_queryset().model | ||
| elif ( | ||
| getattr(relation, 'many', False) and | ||
| hasattr(relation.child, 'Meta') and |
There was a problem hiding this comment.
I'm wondering if these hasattr checks are a bit too much. A serializer doesn't have to be a ModelSerializer so there is no requirement for Meta to exist, but isn't model a required attribute of Meta for a ModelSerializer? It feels slightly too defensive to me to check that model exists.
There was a problem hiding this comment.
I don't quite understand. If the serializer may not be a ModelSerializer, then shouldn't we not assume that Meta has a model?
There was a problem hiding this comment.
Ah, you're right. I thought a vanilla serializer made no use of Meta, but I can see from the source that that is not correct. By all means, please keep model. 😊
rest_framework_json_api/utils.py
Outdated
| hasattr(relation.child.Meta, 'model')): | ||
| # For ManyToMany relationships, get the model from the child | ||
| # serializer of the list serializer | ||
| # TODO Test coverage |
There was a problem hiding this comment.
Could you either add a test to cover this line or delete the TODO comment? A test would be preferable, but understand if you can't. I don't think that TODO comments should be allowed in a master branch. Experience has showed me that they almost always languish and are forgotten until someone comes by later and deletes the comment.
There was a problem hiding this comment.
Removed the TODO comment
rest_framework_json_api/utils.py
Outdated
| meta = getattr(serializer.child, 'JSONAPIMeta', None) | ||
| try: | ||
| return list(serializer.JSONAPIMeta.included_resources) | ||
| return list(meta.included_resources) |
There was a problem hiding this comment.
Slightly contrary to what I stated up above, what do you think of changing this logic to have a hasattr check here? I'd guess that many (most?) serializers will not have included_resources. Relying on an AttributeError for normal flow control of the majority case is bound to be slow. I recognize this would slightly increase the scope of this branch. Would you be ok fixing that while you're in here?
|
I'm excited to get this one fixed for my selfish interest of making my app not hammer my API with requests. I had a couple of minor comments that I think would be worth addressing. I'm happy to merge once they've been considered. 👍 Thanks, @rpatterson! |
|
@mblayman I think I addressed all your feedback. Let me know if I missed anything. |
rest_framework_json_api/utils.py
Outdated
| meta = getattr(serializer.child, 'JSONAPIMeta', None) | ||
| try: | ||
| return list(serializer.JSONAPIMeta.included_resources) | ||
| return list(getattr(meta, 'included_resources', [])) |
There was a problem hiding this comment.
I think the surrounding try/except is no longer needed, right?
|
Dude, you rock! I'll merge it once CI goes green. Thanks again! |
4 participants
No description provided.