Check resource name on included serializer in to_internal_value #306
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #306 +/- ##
===========================================
+ Coverage 76.38% 76.45% +0.06%
===========================================
Files 50 50
Lines 6027 6036 +9
===========================================
+ Hits 4604 4615 +11
+ Misses 1423 1421 -2
Continue to review full report at Codecov.
|
czosel
force-pushed
the
bug_relations_to_internal_value
branch
from
c0bce86 to
76caa82
Compare
|
@mblayman Did you get a chance to take a look at this one yet? Please let me know if there's anything that needs more clarification or work. PS: Thank you for taking over the maintenance for this repo, you're doing a great job! 👍 |
|
@czosel this PR pre-dated my start as a maintainer role so I haven't really looked at it closely. I'll try to get some time to take a look at it soon. Thanks for the contribution! |
|
@mblayman Awesome, thanks for your quick reply! |
| def test_serializer_resource_name_create(self, client, monkeypatch): | ||
| monkeypatch.setattr(serializers.CommentSerializer.Meta, 'resource_name', 'renamed_comments', False) | ||
| monkeypatch.setattr(serializers.EntrySerializer.Meta, 'resource_name', 'renamed_entries', False) | ||
| self.create_data['data']['type'] = 'renamed_comments' |
There was a problem hiding this comment.
I think you should probably deepcopy self.create_data. Otherwise, I think you're modifying the class state for all tests.
| def teardown_method(self, method): | ||
| models.Comment.__bases__ = (models.Comment.__bases__[0],) | ||
| try: |
There was a problem hiding this comment.
I'm not super familiar with these tests. Would you mind explaining why this tear down code is not needed?
rest_framework_json_api/relations.py
Outdated
| return None | ||
|
|
||
| def is_serializer(self, candidate): | ||
| return hasattr(candidate, 'included_serializers') |
There was a problem hiding this comment.
Is this a true test of whether something is a serializer? A serializer could be somethign that donesn't have included_serializers, right? Am I mistaken?
There was a problem hiding this comment.
You're right - i just verified that isinstance(candidate, Serializer) also works in this case, which is certainly cleaner.
|
@czosel This stuff looks pretty good to me. Thanks for your effort. I have a couple of questions which I added as comments. Could I also request that you merge the latest Also, I've heard that features that aren't documented don't exist. Could we find an appropriate place to document this functionality so others might benefit from your work? Thanks! |
|
@mblayman Thanks for your review! I just pushed some minor changes and hope to have answered your questions. The only thing i didn't do was change the documentation, because i think the "setting the resource name chapter" in the docs pretty much says it all - this PR is basically just fixing this feature for two previously broken cases (nested serializers, POST requests). |
|
👍 Thanks for your patience, @czosel! Sorry it took 6 months to get this work merged. |
|
@mblayman No worries, thanks for merging it! 👍 |
Fixes #302 and #303