-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[API] Filter refactor #11073
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
base: master
Are you sure you want to change the base?
[API] Filter refactor #11073
Conversation
- Add OptionalField dataclass - Pass serializer class and kwargs separately
✅ Deploy Preview for inventree-web-pui-preview canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.18% 88.19%
=======================================
Files 1290 1290
Lines 58142 58225 +83
Branches 1969 1969
=======================================
+ Hits 51272 51349 +77
- Misses 6379 6385 +6
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 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