Skip to content

Conversation

albertisfu
Copy link

According to #40 this PR adds support for filtering fields from nested serializers.

This update enhances the DynamicFieldsMixin to support filtering nested fields in child serializers using the existing fields= and omit= query parameters.

These parameters can be used in combination with top-level fields. For example:

  • ?omit=description,recap_documents__plain_text
  • ?fields=description,recap_documents__plain_text

You can also combine omit and fields in a single request:
?fields=id,name,recap_documents__pacer_doc_id,recap_documents__plain_text&omit=entry_number,recap_documents__plain_text

Here are some examples:
api/rest/v4/docket-entries/?fields=id,recap_documents__page_count
Screenshot 2025-07-09 at 3 33 24 p m

/api/rest/v4/docket-entries/?omit=entry_number,recap_documents__plain_text
Screenshot 2025-07-09 at 3 33 45 p m

Deferred Fields

Another goal of issue #40 was to defer top-level and nested filtered fields in database queries to improve performance.

The initial plan was to include a new mixin, DeferredFieldsMixin, so users could optionally use it in their viewsets to defer filtered fields. However, we found that this mixin would significantly increase the complexity of the project, and most of its logic was tailored to our specific use case. Therefore, we decided it wasn’t a good idea to include it in this PR.

Instead, I added the method get_model_fields_to_defer to the DynamicFieldsMixin, so users can leverage it to implement their own mixins or deferring logic. This method provides knowledge about which fields should be deferred.

It processes the allowed and omitted top-level and nested fields, validates that they correspond to actual model fields, and identifies which fields should be deferred. For allowed fields, it defers all valid model fields with DB columns that are not included in the allowed list.

Multiple tests have been included.

If you agree with these changes, I’d be happy to submit a PR to update the documentation accordingly.

@jtrain
Copy link
Collaborator

jtrain commented Jul 10, 2025

I did raise concerns about the amount of logic added to support this to the existing mixin and that doesn't seem to have been dealt with here.

Normally this is indicative of the wrong approach being taken (logic). We can back up and explore the alternatives to solve it since I cannot see where in the discussion that alternatives were discussed now is the time to do it.

  1. This PR: Parent serializer is responsible for filtering children
  2. Child serializers can filter themselves
  3. Parent serializer is responsible but logic is contained in a new optional mixin (since many wouldn't agree with the exposure to new code that doesn't fulfill any requirement in their codebase)
  4. 2/ but also in a new optional mixin
  5. Some kind of functional approach that could modify an existing serializer to apply the functionality rather than mixin.

Perhaps there are more ideas that you can chime in with @albertisfu especially if you had considered some and rejected them.

I like the idea of 2. Correct me if I'm wrong but this PR would only go a single level deep. And that is a lot of code added to simply reach one extra level to filter. If each Serializer could handle itself then we could reduce complexity and go as many levels deep as people care to nest.

Failing that, i like 3/ . It can be called NestedDynamicFieldsMixin or similar. Containing all this extra code to a new optional mixin that existing drf users can opt into.

I will take a quick play around with your code to see what is possible, and if I can find the reasons behind all this extra logic being added.

@albertisfu
Copy link
Author

Closing in favor of #44

@albertisfu albertisfu closed this Jul 12, 2025
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