[API] Filter refactor #11073
[API] Filter refactor #11073
Conversation
- Add OptionalField dataclass - Pass serializer class and kwargs separately
✅ Deploy Preview for inventree-web-pui-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the enable_filter functionality to use a new OptionalField dataclass approach for lazy evaluation of optional serializer fields, addressing performance issues caused by premature serializer instantiation.
Key changes:
- Replaced
enable_filter()function withOptionalFielddataclass for declarative field definitions - Refactored
FilterableSerializerMixinto lazily instantiate optional fields only when needed - Updated all serializers across the codebase to use the new
OptionalFieldpattern
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
InvenTree/serializers.py |
Introduced OptionalField dataclass and refactored FilterableSerializerMixin to support lazy field initialization via build_unknown_field() |
InvenTree/test_serializers.py |
Updated tests to use new OptionalField API; removed obsolete enable_filter validation test; corrected spelling from "failiure" to "failure" |
users/serializers.py |
Migrated GroupSerializer fields (permissions, roles, users) from enable_filter to OptionalField |
stock/serializers.py |
Converted multiple optional detail fields including user_detail, template_detail, location_path, part_detail, supplier_part_detail, tests, item_detail to use OptionalField |
part/serializers.py |
Refactored optional fields across CategorySerializer, PartBriefSerializer, PartSerializer, BomItemSerializer, and related serializers to use OptionalField pattern |
order/serializers.py |
Updated order-related serializers including purchase order, sales order, and return order serializers to use OptionalField for detail fields |
company/serializers.py |
Migrated company, manufacturer part, and supplier part serializers' optional fields to OptionalField |
common/serializers.py |
Converted parameter serializer detail fields to use OptionalField |
common/filters.py |
Updated filter helper functions to return OptionalField instances instead of enable_filter wrapped fields |
build/serializers.py |
Refactored build serializer optional fields including part, user, and item detail fields to use OptionalField |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11073 +/- ##
==========================================
- Coverage 88.16% 88.15% -0.01%
==========================================
Files 1290 1290
Lines 58145 58173 +28
Branches 1969 1969
==========================================
+ Hits 51261 51285 +24
- Misses 6393 6397 +4
Partials 491 491
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I might be overlooking something, but are we not completely loosing static introspection with this change? I am not sure if there is much actual runtime performance to be gained by this, have you benchmarked it? |
Benchmarks added to the top comment.
We should not lose any introspection as the fields are still created - they are just defered until we determine they are actually needed. This is most important in the case of deeply nested serializer fields which will never be exposed to the final serializer tree. The |
- Handle case where optional field shadows model property - Consider read_only and write_only fields
- Handle case where optional field shadows model relation
- Request object is only available for the top-level serializer anyway
- Breaks search results
| This is used in conjunction with the `FilterableSerializerMixin` to allow | ||
| dynamic inclusion or exclusion of serializer fields at runtime. |
There was a problem hiding this comment.
This docstrings seem very slim compared to the replaced function
There was a problem hiding this comment.
Yes, I will add more documentation here
| INVENTREE_API_TEXT = """ | ||
| v436 -> 2026-01-03 : https://github.com/inventree/InvenTree/pull/11073 | ||
| - Add OptionalField class for cleaner handling of optional fields in serializers |
There was a problem hiding this comment.
There seem to be other API changes too, should they also be mentioned?
At least in my setups the python LSP seems to not work with this new setup. The kwargs are not validated anymore during startup and you can not rename fields on the serializer automatically because it is not inspectable throught the LSP server anymore. Type checking also seems to be lost on the kwargs. |
- Annotation now performed using OptionalField - Code can be greatly simplified
can you please provide some specific examples of differences, perhaps we can add some additional unit tests? The speed improvements make the UX much snappier, so I'm keen to move this forward but want to make sure you are still happy with it. |
|
As an interesting observation - the speed "penalty" which we are incurring by instantiating optional fields and then later removing them has been in the code base from the beginning, it was not introduced by the |
I am not sure if we really should test against any specific python LSP implementation; this is a fundamental design decision / issue that causes this - not a specific LSP implementation detail Due to only passing a formless dict into this new function, we lose any type inference from the serializer class. If you insist, I can paste a screenshot from VS Code showing no refactor options etc. but showing something not existing is not adding much context.
This diverges from how Django Rest Framework works even more than we already do, I am not sure anyone outside of core devs will grasp how this works in a few weeks. It breaks a lot of development UX and makes the request/response cycle even more difficult to understand. I am not sure it is worth it
A serializer being initialized is part of the basic design of DRF. I purposefully did not touch it when I added enable_filter. If we really want to change the fundamental behavior of DRF we could refactor the whole mechanism similar to https://www.django-rest-framework.org/api-guide/serializers/#dynamically-modifying-fields or https://github.com/khaledsukkar2/drf-shapeless-serializers |
|
I think that this implementation is pretty similar to the concepts in
I do not think that this is any more difficult to understand (from a developer perspective) than the
Deeply nested serializers add a lot of overhead when responding to an API call. Consider building a table for
In current master, these two requests are > 200ms each, which means that this table in particular feels very slow. Additionally, it reduces the number of requests being processed overall, which means that the UI is less snappy in general. After this optimization, we are down to ~40ms + ~30ms in comparative testing. That's ~70ms vs ~450ms to load the table. This is a significant speed up (that is noticable in the UI) which I would like to retain. Maybe there's a more "DRF-ey" approach? |
|
While I understand the possible performance improvements, it should be at least noted that this makes it considerable harder to contribute. Even seasoned Django devs will not know this pattern and it will be in most important API endpoints. One can not jump through the arguments with IntelliSense anymore, one loses type checking on the arguments on the serializers. In my eyes, that is a hard hit as it places more burden on the devs to know what they are doing. I still was not able to reproduce the benchmarked improvements that drastically in my setups; are you using a live environment or one of the test datasets? |
This is with (in both test cases)
And also reproduced with both the django built-in server and gunicorn |
Merging this PR will create unknown performance changes
|
2 participants
This PR is a major refactor of the
enable_filterfunctionality which allows optional fields to be added (or removed) dynamically to API serializers.Problem Description
The old code was written in such a way that the optional serializer class was instantiated on definition.
Consider the following code
The
CategorySerializerclass is instantiated (which is quite expensive) - and then this is also chained down for any multi-level child serializers.In many cases, the fields are later removed, so instantiating them early is a waste of resources.
Also, due to the (wasteful?) way that DRF deep-copies all the fields (multiple times throughout the lifespan of the serializer) this can be very prohibitive.
Some particularly bad API endpoints which had deep nested serializers, saw hundreds of thousands of serializer objects created, and then later deleted.
Solution
The PR introduces an
OptionalFieldapproach, which lazily evaluates the optional fields only after we have decided that they should definitely be included in the serializer:Justification
The new API endpoints are significantly faster, we have been introducing a huge amount of wasted overhead (for years now in the codebase) due to the unnecessary serializer evaluation
Benchmarks
Benchmarking shows that serializers which are deeply nested have the most benefit from this PR. Both GET and OPTIONS requests are improved substantially by deferring serializer instantiation.
Methodology
GET
OPTIONS
Search
/api/search/