-
-
Notifications
You must be signed in to change notification settings - Fork 527
Annotation-based filtering in FilterSchema #1514
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?
Conversation
* Removed Deprecated Config class (on Schema) support * FilterField instead of Field (+ deprecation warning)
Updated PR by merging with latest
Apologies for the commit noise. Let me know if you want me to squash them on my end before (and if) we proceed. |
Hi @l1b3r - can we make FilterField deprecated for now instead of removing it ? |
Hi @vitalik, I thought that |
@l1b3r - oops, right - I thought that already released, but not yet - so we are good |
Disclaimer: I am the guy who suggested
FilterSchema
in the first place.Problem Statement
There are several issues with how fields are configured for
FilterSchema
:Issue 1: Poor IDE Support
IDEs are unaware of custom arguments placed into a
Field
, which degrades developer experience (no autocompletion, no type checking).Issue 2: Deprecated Pydantic Features
Pydantic V2 has deprecated usage of
**extra
kwarg inField
method which the current solution relies on.Issue 3: Pydantic V2 Behavior Change
Pydantic V2 does not merge
**extra
andjson_schema_extra
if the latter is defined. That means, if you define aField
like this:You will not be able to extract
"q"
fromjson_schema_extra
because the**extra
arguments are ignored whenjson_schema_extra
is present.Issue 4: OpenAPI Schema Pollution
Pydantic V2 suggests using
json_schema_extra
instead of**extra
. While we could use it this way, this would mean that anything placed injson_schema_extra
will end up in youropenapi.json
document. While having extra data in the openapi.json does not break renderers like Swagger UI, exposing internal DB lookup configuration in the public-facing document is undesirable.Suggested Solution
This PR changes the concept of specifying lookup metadata from using
Field
arguments to using a separate annotation class calledFilterLookup
, which is used withinAnnotated
blocks:FilterLookup
supports everything that was supported previously, namely:expression_connector
to specify how to join multiple lookupsignore_none
to control whether to treatNone
s as valid filter valuesAddressing Stated Issues
FilterLookup
is a separate vanilla Python class whose arguments are clearly typed. This enables IDE autocompletion and improved DXFilterLookup
is orthogonal toField
and does not rely on its features anymore. You can useFilterLookup
with or withoutField
, there is no dependency from the former on the latter.Alternative Solutions Considered
We could introduce a
FilterField
function that would wrap around Pydantic'sFilter
function and return an augmentedFieldInfo
. TheFilterField
could be used exactly as a regularField
:The problem of wrapping
Field
lies in how complexlyField
is defined in Pydantic's code base. In order to enable proper static type checking support,FilterField
would need to repeat all those overloads with all those arguments anddjango-ninja
's maintainers would need to make sure the definitions stay aligned with every new version of Pydantic. This, in my opinion, is a considerable amount of technical debt to maintain.Backward Compatibility
FilterSchema
now has a layer of backwards compatibility which allows it to work with newFilterLookup
-annotated fields and with old-styleField
-centric fields, even within a single schema class:The old
Field(q=...)
approach is still functional but considered deprecated and not recommended for new code.Changes in this PR
FilterLookup
class with full typing support and validationFilterSchema
to supportFilterLookup
-based annotations while preserving original behaviorField
approach to "*_deprecated" suffixFilterLookup
, named "*_annotated" suffixFilterSchema
(previously ignored):FilterLookup
toninja/__init__.py
Migration Path
For existing users, no immediate action is required - the old approach continues to work. However, for better IDE support and future compatibility, migration to
FilterLookup
is recommended: