fix(fastapi): give each filter config its own dependency signature - #784
Open
ruicleite96 wants to merge 3 commits into
Open
fix(fastapi): give each filter config its own dependency signature#784ruicleite96 wants to merge 3 commits into
ruicleite96 wants to merge 3 commits into
Conversation
`_create_filter_aggregate_function_fastapi` assigned `__signature__` to the
module-level `_aggregate_filter_function` and returned that same object, so
every call to `provide_filters` overwrote the parameters of every dependency
built before it.
Two routers with different configs therefore both served whichever config was
built last. Given `provide_filters({"search": "name"})` for one router and
`provide_filters({"created_at": True})` for another, both expose only
`createdBefore` and `createdAfter`: the first silently loses the search
parameter it was configured with, and gains date parameters it never asked for.
The per-config cache masks this whenever a process happens to build only one
config.
Builds a fresh function per config instead, delegating to the shared
implementation. The cache still returns one object per identical config.
`provide_filters` takes a `FilterConfig` TypedDict, and mypy accepts a dict literal there but not a `dict[str, Any]` variable — nor `dict(config)`, which degrades a TypedDict to `dict[str, object]`. Build the config in a factory returning `FilterConfig`. Each call still yields a distinct, equal dict, so the test goes on exercising the cache by value rather than by identity.
The cache keyed on the config alone, but `dep_defaults` shapes the generated signature just as much — it supplies the parameter names and the default page size. Two callers passing the same config with different defaults therefore shared one dependency, and whichever was built first won for the whole process: a caller asking for `DEFAULT_PAGINATION_SIZE = 100` silently served 20. This is the same failure this branch already fixes one level up, so fix it here rather than leave a second way for a router to serve a config it never asked for. The key uses the values rather than the instance, so two equal `DependencyDefaults` still share one dependency and the cache stays useful.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_create_filter_aggregate_function_fastapiassigns__signature__to the module-level_aggregate_filter_functionand returns that same object (providers.py:763-768). Every call toprovide_filterstherefore rewrites the parameters of every dependency built before it — they are all the same function.Reproducer on current
main:/authorswas configured withsearchand exposes no search parameter at all, while gaining two date parameters it never asked for. Whichever config is built last wins for the whole process.This is quiet: the app starts, the schema looks plausible, and the filters simply do not do what the router asked for. It is masked in any process that only ever builds one config — which is why the existing tests do not catch it.
Change
Build a fresh function per config, delegating to the shared implementation, instead of mutating one shared object:
The per-config
DependencyCacheis untouched, so repeated identical configs still return one shared object.After the fix:
A second way the same thing went wrong
The cache key was
hash((_CACHE_NAMESPACE, make_hashable(config)))— the config alone. Butdep_defaultsshapes the generated signature just as much: it supplies the filter parameter namesand
DEFAULT_PAGINATION_SIZE. Two callers passing the same config with different defaults thereforeshared one dependency, and whichever was built first won for the whole process:
Reversing the build order flips it: the caller who wanted the stock 20 gets 100 instead.
That is the same failure this branch already fixes one level up — a router silently served a
configuration it never asked for — so it is fixed here too rather than left as a second route to it.
The key is built from the defaults' values, not the instance, so two equal
DependencyDefaultsstill share one dependency and the cache stays useful.
Tests
tests/unit/test_extensions/test_fastapi/test_provider_isolation.py:dep_defaultsgets two dependencies, each with its own page size;DependencyDefaultsstill share one dependency.test_create_filter_dependencies_cache_misspinned the old key formula, so it now builds itsexpected key through the same
_filter_cache_keyhelper the provider uses, rather than restatingthe formula and being free to drift from it.
The full
tests/unit/test_extensions/test_fastapi/suite passes;ruff check,ruff formatandmypyare clean.Note
Of the two problems above, only the second reaches the Litestar provider. I checked both by building
its dependencies rather than by reading:
provide_filtersis defined inside_create_filter_aggregate_function(providers.py:745), so it is already a fresh function perconfig —
{"search": "name"}and{"created_at": True}yield['search_filter']and['created_filter']respectively.create_filter_dependencieskeys onhash((_CACHE_NAMESPACE, make_hashable(config)))(providers.py:322) while reading eightdep_defaultsattributes, so a caller passingDEFAULT_PAGINATION_SIZE = 100gets the cachedpage_sizeof 20, exactly as FastAPI did.I have not touched it here to keep the diff scoped to
fastapi— happy to follow up with thecache-key fix, in this PR or a separate one, whichever you prefer.