-
Notifications
You must be signed in to change notification settings - Fork 299
Added nested included serializer support for remapped relations #347
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.
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
Added nested included serializer support for remapped relations #347
Conversation
Hey, @sliverc, this branch now has some conflicts because of the work you did on the flake8 branch. Could you fix that up and also add a CHANGELOG entry for this too? I'm not sure I'll have time to review this today, but I will get to it when I can. Thanks!
03f7943 to
2d0303b
Compare
@mblayman Solved merge conflicts and added CHANGELOG entry
Codecov Report
@@ Coverage Diff @@ ## develop #347 +/- ## =========================================== + Coverage 76.86% 77.13% +0.27% =========================================== Files 50 50 Lines 6013 6036 +23 =========================================== + Hits 4622 4656 +34 + Misses 1391 1380 -11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a docstring which provides a conceptual overview of what's going on here? I'm finding the four nested levels to be really intense. If you could use PEP 257 style, that would be great.
"""Extract ... <relevant details that might help understand what's going on here> """
This many levels of exceptions makes me wonder if there might performance challenges here (because of the cost of going through the exception stack over and over). I don't think it's fair to ask you for a performance test so I'm mostly wondering "out loud" if this is too much exception handling as a way to do control flow. It does feel like a bit of a code smell. Maybe that's the cost of working with a spec that's so dynamic ̄\(ツ)/ ̄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a docstring.
Considering the levels of exceptions I also think this is certainly not ideal but I cannot think of an alternative way to do it though.
For example calling getattr with default argument None wouldn't work, as an attribute could actually be None. Calling hasattr wouldn't make things better either as internally it also does a try-except (https://docs.python.org/3/library/functions.html#hasattr).
It could be argued whether the serializer method case is really needed but otherwise I guess we will need to live with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for thinking about it. The only alternative that I can think of is to have multiple returns. What do you think of something like the following:
try: return getattr(resource_instance, field_name) except AttributeError: pass try: # For ManyRelatedFields if `related_name` is not set # we need to access `foo_set` from `source` return getattr(resource_instance, field.child_relation.source) except AttributeError: pass <and so on...>
Is that any "better?" Or is it worse? It would remove the deep nesting even if the logic is functionally the same.
Thanks, @sliverc. I think this will work well. I appreciate the contribution and dialog.
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
this is a split from the `Channel` model, while the server reconfigures to conform to JSON:API and use DRF to manage its endpoints. The key for `image` is different (was `logoImage`) due to a bug in the JSON:API extension to DRF. Included nested serialized resources at a different key than its source throws an error. There was an issue that was closed that was supposed to fix it, but it didn't in my experience. django-json-api/django-rest-framework-json-api#347
Uh oh!
There was an error while loading. Please reload this page.
In ResourceRelatedField it is possible to rename/remap a field so it can be represented in the api with a different name using the source attribute. This works well however if I request to include this relationship in
includedit won't be returned.I have added a test which should make it clearer what issue this PR addresses.