Skip to content

Added support to filter nested fields #43

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jtrain
Copy link
Collaborator

@jtrain jtrain commented Jul 10, 2025

#42 replacement for this PR

@albertisfu here is an alternative approach where we do allow each serializer the ability to filter itself instead of orchestrating the filtering from the parent.

Does this approach work for your use-case? You can see that this cuts down on the logic spread throughout the fields method, and contains the hard work of finding the fields to filter / omit in some helper functions - I'm sure these could be simplified further - they are just gpt code. (for example I don't like while statements as a rule)

albertisfu and others added 2 commits July 9, 2025 15:27
Introduces the new code in a separate serializer that inherits from the
existing one so that dynamic field enjoyers can opt into the new
functionality.

However the new field seems to be 100% backwards compatible with the old
one, save for some performance hit since we are doing extra work.
@albertisfu
Copy link

Thank you, @jtrain. I’ve tested this approach, and it works for our use case.

I only needed to apply a simple fix in the compute_level method, because the level computation was failing when serializing multiple instances, for example:

serializer = self.SchoolSerializer([school, school_1], many=True, context={"request": request})

Tweaking the logic to avoid incrementing the level for a ListSerializer resolves the issue.

def compute_level(serializer):
    level = 0
    current = serializer

    while getattr(current, "parent", None) is not None:
        parent = current.parent
        current = parent
        if not isinstance(parent, serializers.ListSerializer):
            level += 1
    return level

We should also update and extend the tests to cover this scenario.

I think it’s fine that you removed all the logic for determining which fields to defer—we can derive that from the request and handle it in our codebase, which makes this PR simpler.

I agree with this approach. If you’d like, I can refine the logic further and improve the tests (I might need to open a separate PR based on this one due to permissions).

Let me know what you think.

@jtrain
Copy link
Collaborator Author

jtrain commented Jul 10, 2025

@albertisfu great! yes please if you like you can use this PR as your base to make the improvements you've suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants