-
Notifications
You must be signed in to change notification settings - Fork 298
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -272,6 +272,36 @@ def extract_relationships(cls, fields, resource, resource_instance): | |
|
||
return utils.format_keys(data) | ||
|
||
@classmethod | ||
def extract_relation_instance(cls, field_name, field, resource_instance, serializer): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
""" | ||
Determines what instance represents given relation and extracts it. | ||
|
||
Relation instance is determined by given field_name or source configured on | ||
field. As fallback is a serializer method called with name of field's source. | ||
""" | ||
relation_instance = None | ||
|
||
try: | ||
relation_instance = getattr(resource_instance, field_name) | ||
except AttributeError: | ||
try: | ||
# For ManyRelatedFields if `related_name` is not set | ||
# we need to access `foo_set` from `source` | ||
relation_instance = getattr(resource_instance, field.child_relation.source) | ||
except AttributeError: | ||
if hasattr(serializer, field.source): | ||
serializer_method = getattr(serializer, field.source) | ||
relation_instance = serializer_method(resource_instance) | ||
else: | ||
# case when source is a simple remap on resource_instance | ||
try: | ||
relation_instance = getattr(resource_instance, field.source) | ||
except AttributeError: | ||
pass | ||
|
||
return relation_instance | ||
|
||
@classmethod | ||
def extract_included(cls, fields, resource, resource_instance, included_resources): | ||
# this function may be called with an empty record (example: Browsable Interface) | ||
|
@@ -304,19 +334,9 @@ def extract_included(cls, fields, resource, resource_instance, included_resource | |
if field_name not in [node.split('.')[0] for node in included_resources]: | ||
continue | ||
|
||
try: | ||
relation_instance = getattr(resource_instance, field_name) | ||
except AttributeError: | ||
try: | ||
# For ManyRelatedFields if `related_name` is not set we need to access `foo_set` | ||
# from `source` | ||
relation_instance = getattr(resource_instance, field.child_relation.source) | ||
except AttributeError: | ||
if not hasattr(current_serializer, field.source): | ||
continue | ||
serializer_method = getattr(current_serializer, field.source) | ||
relation_instance = serializer_method(resource_instance) | ||
|
||
relation_instance = cls.extract_relation_instance( | ||
field_name, field, resource_instance, current_serializer | ||
) | ||
if isinstance(relation_instance, Manager): | ||
relation_instance = relation_instance.all() | ||
|
||
|