5766 Add nested field filtering and ensure defer omitted fields

[albertisfu]

No description provided.

[jtrain]

hi @albertisfu Thanks for your work on this so far.

If I could offer a few thoughts on how I see this?

1. Your view mixin is a separate optional component that people can choose to use if they like. I think that is the correct approach to take
2. Your dynamicfields mixin includes a substantial amount of business logic surrounding the ability to filter out fields that are included via relation __. This happens every time, regardless of whether the customer even needs this ability. I think something like this could exist as another optional component that people can choose. It can inherit from the existing one. This way you keep all your new business logic away from existing code that never needed it and still doesn't. It also reduces the need for any logic (which is one way you may have approached not running the new code unless necessary), because at the time of mixing in we are making the choice to use the original or new style mixin.
3. A final note is that there is a lot of unrelated style changes in the diff (e.g. setup.py) it is unclear what really changed in those files. I'd keep the diff as short as is required to implement the feature. style fixups are fine, in a new PR perhaps?

[jtrain]

Could this have been written as a defaultdict?

[jtrain]

  def to_representation(self, instance):
        """This method prunes filtered fields from a nested serializer."""
        representation = super(DynamicFieldsMixin, self).to_representation(instance)

        # Apply nested omit on dicts and lists of dicts
        for parent, omit_list in getattr(self, "_nested_omit", {}).items():
            if parent not in representation:
                continue
            parent_instance = representation[parent]

            def do_omit(d):
                """Helper to drop fields on a single dict"""
                for child in omit_list:
                    d.pop(child, None)

            if isinstance(parent_instance, dict):
                do_omit(parent_instance)
            elif isinstance(parent_instance, list):
                for item in parent_instance:
                    if isinstance(item, dict):
                        do_omit(item)

I was drawn to this code. Mostly because it introduces a large amount of new logic, but also because of the definition of a function inside of a for-loop. Upon reading it again, it seems to me that the way you are omitting/filtering is by rendering the entire serializer, including the expensive fields, then simply dropping them from the resultant dictionary. The problem is the work has already been done, and now you are doing extra work to drop it. So while the client of this request won't see the omitted fields and will see the filtered fields, they don't get the benefits promised them, because they already paid for the serialization of the missing fields.

The way the original code tackled this was to manipulate the fields list of a serializer. If a field no longer exists on a serializer then it won't be serialized and we don't pay for it.

Is there a way to re-use this existing pattern in your solution? Without using that pattern, I don't think your solution sits within the spirit of what this library is about.

[albertisfu]

Thank you, @jtrain, for your feedback! This PR is still a work in progress, but you're absolutely right that overriding to_representation is not the appropriate approach for pruning the nested serializer. I realized this after testing the draft in our project. I'll make some improvements and let you know once it's ready for review.

[albertisfu]

@jtrain I'm closing this PR in favor of #42 which takes a different approach and incorporates the suggestions you made in this PR.