feat: fine grained filters - #199
Conversation
📝 WalkthroughWalkthroughAdds fine-grained declared filters with restricted operators and custom SQL application, refactors comparison metadata and field keyword forwarding, and adds unit and integration coverage across supported database schemas. ChangesFine-Grained Filter Pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/strawchemy/schema/filters/__init__.py (1)
47-48: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDuplicate entry
"FilterProtocol"in__all__.Line 47 and 48 both contain
"FilterProtocol". This is harmless but should be deduplicated.♻️ Proposed fix
"EqualityFilter", "FilterProtocol", - "FilterProtocol", "GraphQLComparison",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/strawchemy/schema/filters/__init__.py` around lines 47 - 48, The __all__ list in the filters module contains a duplicate entry for "FilterProtocol" on lines 47 and 48. Remove one of the duplicate "FilterProtocol" entries from the __all__ list to keep only a single occurrence of each exported symbol.src/strawchemy/dto/inspectors/sqlalchemy.py (1)
635-659:⚠️ Potential issue | 🟡 MinorUpdate stale reference in docstring to use new method name.
The verification confirms no external callers use the old
get_field_comparisonname. However, the docstring at line 548 still references the old method name and should be updated toget_comparison.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/strawchemy/dto/inspectors/sqlalchemy.py` around lines 635 - 659, The docstring contains an outdated reference to the old method name `get_field_comparison`. Locate the docstring that references this old method name and update it to use the new method name `get_comparison` to keep the documentation accurate and consistent with the current codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/strawchemy/schema/factories/inputs.py`:
- Around line 288-296: The in-place mutation of `base.__annotations__` causes
issues when the same filter class is reused across multiple factory invocations
(common in tests or hot-reload scenarios). Instead of directly mutating the
user's class annotations, either create a shallow copy of the base class before
modifying its annotations, or add a sentinel attribute to the base class to
detect when it has already been processed and skip the annotation mutation on
subsequent invocations. This prevents the "needs a type annotation" error that
occurs on the second run when annotations have been stripped but filter markers
are still present via getmembers.
In `@tests/integration/types/mysql.py`:
- Line 163: The `_fruit_sweeter_than` function signature uses `Any` as the type
annotation for the `**_ctx` parameter, which violates the ANN401 lint rule.
Replace the `Any` type annotation with `object` for the `**_ctx` parameter in
the function signature. This change must be applied to the `_fruit_sweeter_than`
function in all three files where it appears with identical signatures to ensure
consistency across the codebase and satisfy the ANN401 lint rule.
In `@tests/unit/test_fine_grained_filters.py`:
- Line 53: Replace the use of Any-typed parameters and return annotations in
helper callable signatures to comply with Ruff ANN401. The _apply function at
line 53 and similar helper functions at lines 121, 139, and 299 all use Any in
their type annotations. Either define a properly typed callable alias or
protocol that accurately represents these helpers' expected signatures, or add
targeted # noqa: ANN401 comments to each of these function definitions if the
Any typing is intentional and necessary for their flexibility. Ensure all four
locations (the _apply function and the three other helper functions) are
consistently updated with the same approach.
---
Outside diff comments:
In `@src/strawchemy/dto/inspectors/sqlalchemy.py`:
- Around line 635-659: The docstring contains an outdated reference to the old
method name `get_field_comparison`. Locate the docstring that references this
old method name and update it to use the new method name `get_comparison` to
keep the documentation accurate and consistent with the current codebase.
In `@src/strawchemy/schema/filters/__init__.py`:
- Around line 47-48: The __all__ list in the filters module contains a duplicate
entry for "FilterProtocol" on lines 47 and 48. Remove one of the duplicate
"FilterProtocol" entries from the __all__ list to keep only a single occurrence
of each exported symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 546bb4f3-3cee-433a-86f8-ea28c4ff0d1c
⛔ Files ignored due to path filters (2)
tests/integration/__snapshots__/test_fine_grained_filters.ambris excluded by!**/__snapshots__/**tests/unit/__snapshots__/test_fine_grained_filters/test_fine_grained_schema.gqlis excluded by!**/__snapshots__/**
📒 Files selected for processing (19)
src/strawchemy/__init__.pysrc/strawchemy/dto/backend/strawberry.pysrc/strawchemy/dto/inspectors/sqlalchemy.pysrc/strawchemy/dto/strawberry.pysrc/strawchemy/mapper.pysrc/strawchemy/schema/factories/inputs.pysrc/strawchemy/schema/field.pysrc/strawchemy/schema/filters/__init__.pysrc/strawchemy/schema/filters/fields.pysrc/strawchemy/schema/filters/geo.pysrc/strawchemy/schema/filters/inputs.pysrc/strawchemy/schema/mutation/field_builder.pysrc/strawchemy/transpiler/_transpiler.pysrc/strawchemy/utils/annotation.pytests/integration/test_fine_grained_filters.pytests/integration/types/mysql.pytests/integration/types/postgres.pytests/integration/types/sqlite.pytests/unit/test_fine_grained_filters.py
| if base is not None and declared_filters: | ||
| # Declared filter fields supply only a data type, not a final GraphQL type. | ||
| # Drop their annotations from the base so the strawberry backend does not treat | ||
| # them as verbatim type overrides; the comparison/custom types injected above win. | ||
| base.__annotations__ = { | ||
| annotation_name: annotation_type | ||
| for annotation_name, annotation_type in inspect.get_annotations(base).items() | ||
| if annotation_name not in declared_filters | ||
| } |
There was a problem hiding this comment.
Mutating base.__annotations__ breaks reuse of the same filter class.
This in-place mutation removes declared filter annotations from the user's class. If the factory is invoked twice with the same base (e.g., in tests or during hot-reload development):
- First run: annotations present → works
- Second run: annotations already removed, markers still found via
getmembers→ raises misleading"needs a type annotation"error
Consider operating on a copy or storing processed state to detect re-invocation:
Suggested fix
if base is not None and declared_filters:
- base.__annotations__ = {
- annotation_name: annotation_type
- for annotation_name, annotation_type in inspect.get_annotations(base).items()
- if annotation_name not in declared_filters
- }
+ # Avoid mutating the original class; build a filtered copy for the backend.
+ # The backend receives `base` and merges its annotations, so we create a
+ # subclass with the filtered annotations to preserve the original.
+ filtered_annotations = {
+ k: v for k, v in inspect.get_annotations(base).items() if k not in declared_filters
+ }
+ # Only mutate if this is the first pass (annotations still contain declared fields)
+ if any(name in inspect.get_annotations(base) for name in declared_filters):
+ base.__annotations__ = filtered_annotationsAlternatively, mark the class as processed with a sentinel attribute to detect re-invocation early.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/strawchemy/schema/factories/inputs.py` around lines 288 - 296, The
in-place mutation of `base.__annotations__` causes issues when the same filter
class is reused across multiple factory invocations (common in tests or
hot-reload scenarios). Instead of directly mutating the user's class
annotations, either create a shallow copy of the base class before modifying its
annotations, or add a sentinel attribute to the base class to detect when it has
already been processed and skip the annotation mutation on subsequent
invocations. This prevents the "needs a type annotation" error that occurs on
the second run when annotations have been stripped but filter markers are still
present via getmembers.
| class FruitFilter: ... | ||
|
|
||
|
|
||
| def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and view the specific line
cat -n tests/integration/types/mysql.py | sed -n '160,166p'Repository: gazorby/strawchemy
Length of output: 298
🏁 Script executed:
# Look for lint/flake8 configuration files
fd -i "pyproject\.toml|setup\.cfg|\.flake8|tox\.ini|ruff\.toml" -type fRepository: gazorby/strawchemy
Length of output: 233
🏁 Script executed:
# Search for similar patterns with **_ctx: Any in the codebase
rg "\*\*_ctx:\s*Any" -type pyRepository: gazorby/strawchemy
Length of output: 90
🏁 Script executed:
# Check if there are imports of Any in the file
head -20 tests/integration/types/mysql.py | cat -nRepository: gazorby/strawchemy
Length of output: 745
🏁 Script executed:
# Search for all lint/flake8 configuration files more broadly
find . -maxdepth 3 -name "pyproject.toml" -o -name "setup.cfg" -o -name ".flake8" -o -name "ruff.toml" | head -20Repository: gazorby/strawchemy
Length of output: 125
🏁 Script executed:
# Look for rg with correct syntax - search for similar patterns
rg "\*\*_ctx:\s*Any" --max-count 20Repository: gazorby/strawchemy
Length of output: 810
🏁 Script executed:
# Check if there are other instances with Any in function signatures
rg "def\s+\w+\([^)]*\*\*.*:\s*Any" --max-count 20Repository: gazorby/strawchemy
Length of output: 2607
🏁 Script executed:
# Search for pyproject.toml in root
cat pyproject.toml | head -80Repository: gazorby/strawchemy
Length of output: 2469
🏁 Script executed:
# Check the ruff.toml configuration
cat ruff.tomlRepository: gazorby/strawchemy
Length of output: 2525
🏁 Script executed:
# Search for the context around line 163 in the sqlite and postgres files to understand if they're mirror files
head -20 tests/integration/types/sqlite.py && echo "---" && head -20 tests/integration/types/postgres.pyRepository: gazorby/strawchemy
Length of output: 1148
🏁 Script executed:
# Look for any ignore patterns or comments that might explain the Any usage
rg "ANN401|noqa.*ANN" --max-count 10Repository: gazorby/strawchemy
Length of output: 643
🌐 Web query:
ANN401 Flake8 annotations rule - what does it require for **kwargs typing
💡 Result:
The ANN401 rule in flake8-annotations (and its implementation in Ruff) is designed to disallow the use of typing.Any in function argument annotations [1][2][3]. By default, this rule applies to all function arguments, including *args and kwargs [4][5]. If you use kwargs: Any, the rule will flag it as a violation [4]. However, you can configure the linter to suppress this behavior for variadic arguments [1][2][3]. To allow kwargs: Any (and *args: Any), you must enable the --allow-star-arg-any option [5][6]. Configuration details: - For flake8-annotations: Set --allow-star-arg-any=True in your flake8 configuration file (e.g., setup.cfg, tox.ini, or.flake8) [1][5]. - For Ruff: Use the corresponding configuration in your pyproject.toml: [tool.ruff.flake8-annotations] allow-star-arg-any = true [7] When this option is set to True, the linter will ignore ANN401 violations specifically for *args and kwargs, allowing you to use typing.Any for those parameters without triggering the error [1][5][8].
Citations:
- 1: https://pypi.org/project/flake8-annotations/
- 2: https://pypi.org/project/flake8-annotations/3.0.1/
- 3: https://pypi.org/project/flake8-annotations/3.1.0/
- 4: Exclude
*argsand**kwargsfrom ANN401 sco1/flake8-annotations#135 - 5: https://github.com/sco1/flake8-annotations/blob/main/README.md
- 6: https://github.com/sco1/flake8-annotations
- 7: ANN401: Split rule to resolve conflation between
arg: Anyand**kwargs: Anyastral-sh/ruff#19152 - 8: https://github.com/sco1/flake8-annotations/blob/main/CHANGELOG.md
🏁 Script executed:
# Check if allow-star-arg-any is configured in ruff.toml
rg "allow-star-arg-any" ruff.tomlRepository: gazorby/strawchemy
Length of output: 44
🏁 Script executed:
# Verify the exact context of line 163 and if _ctx is actually used in the function
sed -n '163,170p' tests/integration/types/mysql.py | cat -nRepository: gazorby/strawchemy
Length of output: 550
🏁 Script executed:
# Check if the function signature pattern needs **_ctx at all or if it's unused
ast-grep --pattern 'def _fruit_sweeter_than($_, $_, **_ctx: $_) -> $_ {
$$$
}'Repository: gazorby/strawchemy
Length of output: 44
🌐 Web query:
python typing.Any vs object for **kwargs type annotation best practice
💡 Result:
When annotating kwargs in Python, the choice between Any and object depends on your intent regarding type safety, while modern practices prefer using Unpack with a TypedDict for precise annotations [1][2][3]. 1. Any vs. object - typing.Any: This is a special type that tells static type checkers to disengage. It essentially says, "I don't know the type, and I don't want you to check it." A type checker allows almost any operation on an Any value [1][4][5]. - object: This is a concrete type that represents all possible Python objects. It is type-safe; if you annotate kwargs as object, the type checker will restrict you to only those operations that are valid for all objects (which are very few). It is an excellent choice if you truly mean that the arguments can be anything but you want to maintain type safety [1][6][4]. 2. Best Practices - Precise Typing (Preferred): If you know the expected keyword arguments, the modern standard is to use the Unpack operator with a TypedDict [1][2][7]. This allows you to specify the exact types of the keys and values in the kwargs dictionary, providing full static analysis benefits [2][3]. Example of modern best practice: from typing import TypedDict, Unpack class Options(TypedDict): name: str year: int def my_func(kwargs: Unpack[Options]):... - Fallback for Pass-through: If you are writing a wrapper function that passes kwargs to another function, and you do not wish to (or cannot) define the exact signature, using object is generally considered better practice than Any if you want to avoid disabling type checking entirely [6]. However, Any is often used in practice for simplicity when the complexity of defining a precise interface outweighs the benefit [8]. - When to use which: - Use TypedDict + Unpack: When you want precise, checked, and documented keyword arguments [2][3]. - Use object: When you want to be as permissive as possible while still maintaining type safety, effectively declaring that your code does not depend on specific attributes of the arguments [6][4]. - Use Any: When you need an "escape hatch" because your code is highly dynamic or "magical," and you need the type checker to stop flagging errors [4][9].
Citations:
- 1: https://docs.python.org/3/library/typing.html
- 2: https://peps.python.org/pep-0692/
- 3: https://typing.python.org/en/latest/spec/callables.html
- 4: https://github.com/python/mypy/blob/master/docs/source/dynamic%5Ftyping.rst
- 5: https://github.com/python/typing/blob/main/docs/spec/concepts.rst
- 6: https://adamj.eu/tech/2021/05/07/python-type-hints-use-object-instead-of-any/
- 7: https://rednafi.com/python/annotate-args-and-kwargs/
- 8: https://stackoverflow.com/questions/77807297/what-is-the-best-practice-to-annotate-the-args-and-kwargs-arguments-of-a-wrap
- 9: https://stackoverflow.com/questions/39817081/typing-any-vs-object
🏁 Script executed:
# Check if there are other functions in the same files that accept similar kwargs
rg "def\s+\w+\([^)]*\*\*" tests/integration/types/mysql.py -A 2Repository: gazorby/strawchemy
Length of output: 429
🏁 Script executed:
# Check the strawberry filter API to understand if _ctx is expected
rg "apply=" tests/integration/types/mysql.py -B 2 -A 2Repository: gazorby/strawchemy
Length of output: 329
🏁 Script executed:
# Check if there are any similar patterns with noqa comments
rg "\*\*.*:\s*Any.*noqa" --max-count 10Repository: gazorby/strawchemy
Length of output: 44
🏁 Script executed:
# Check the strawchemy filter API signature to see if **_ctx is required
rg "def filter_field" -A 3 src/ | head -30Repository: gazorby/strawchemy
Length of output: 251
🏁 Script executed:
# Check if removing **_ctx altogether would work (look for other filter functions without it)
rg "def _\w+\(statement:" tests/integration/types/mysql.py -A 2Repository: gazorby/strawchemy
Length of output: 223
🏁 Script executed:
# Check git blame or recent changes to see why **_ctx was added
git log -p --follow -S "**_ctx: Any" -- tests/integration/types/mysql.py | head -50Repository: gazorby/strawchemy
Length of output: 44
🏁 Script executed:
# Check if there are other instances of **_ctx: Any in the same three mirror files
rg "def _fruit_sweeter_than" tests/integration/types/ --max-count 5Repository: gazorby/strawchemy
Length of output: 486
🏁 Script executed:
# Verify that replacing Any with object would satisfy ANN401
# Check documentation or similar patterns using object
rg "\*\*.*:\s*object" --max-count 10Repository: gazorby/strawchemy
Length of output: 144
Replace Any with object in **_ctx parameter to satisfy ANN401 lint rule.
The **_ctx: Any parameter violates the active ANN401 rule. Replacing with object maintains type safety while allowing the function to accept arbitrary keyword arguments without disabling type checking.
Note: This same function exists in tests/integration/types/sqlite.py and tests/integration/types/postgres.py with identical signatures and should be updated similarly.
Suggested patch
-def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]:
+def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: object) -> Select[tuple[Fruit]]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]: | |
| def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: object) -> Select[tuple[Fruit]]: |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 163-163: Dynamically typed expressions (typing.Any) are disallowed in **_ctx
(ANN401)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/types/mysql.py` at line 163, The `_fruit_sweeter_than`
function signature uses `Any` as the type annotation for the `**_ctx` parameter,
which violates the ANN401 lint rule. Replace the `Any` type annotation with
`object` for the `**_ctx` parameter in the function signature. This change must
be applied to the `_fruit_sweeter_than` function in all three files where it
appears with identical signatures to ensure consistency across the codebase and
satisfy the ANN401 lint rule.
Source: Linters/SAST tools
|
|
||
|
|
||
| def test_filter_field_with_apply_captures_callable_and_join() -> None: | ||
| def _apply(statement: Any, *_args: Any, **_kwargs: Any) -> Any: |
There was a problem hiding this comment.
Replace Any-typed helper signatures to satisfy ANN401.
At Line 53, Line 121, Line 139, and Line 299, helper callables use Any in parameter/return annotations and currently trigger Ruff ANN401. This can fail lint depending on repo settings; prefer a typed callable alias/protocol (or targeted # noqa: ANN401 where intentional).
Also applies to: 121-121, 139-139, 299-299
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 53-53: Dynamically typed expressions (typing.Any) are disallowed in statement
(ANN401)
[warning] 53-53: Dynamically typed expressions (typing.Any) are disallowed in *_args
(ANN401)
[warning] 53-53: Dynamically typed expressions (typing.Any) are disallowed in **_kwargs
(ANN401)
[warning] 53-53: Dynamically typed expressions (typing.Any) are disallowed in _apply
(ANN401)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_fine_grained_filters.py` at line 53, Replace the use of
Any-typed parameters and return annotations in helper callable signatures to
comply with Ruff ANN401. The _apply function at line 53 and similar helper
functions at lines 121, 139, and 299 all use Any in their type annotations.
Either define a properly typed callable alias or protocol that accurately
represents these helpers' expected signatures, or add targeted # noqa: ANN401
comments to each of these function definitions if the Any typing is intentional
and necessary for their flexibility. Ensure all four locations (the _apply
function and the three other helper functions) are consistently updated with the
same approach.
Source: Linters/SAST tools
4f31c34 to
951e32b
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
==========================================
+ Coverage 93.21% 93.28% +0.06%
==========================================
Files 72 73 +1
Lines 6474 6675 +201
Branches 862 897 +35
==========================================
+ Hits 6035 6227 +192
- Misses 295 300 +5
- Partials 144 148 +4 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/strawchemy/schema/field.py`:
- Around line 95-98: In the OutputFieldKwargs class, the type annotations for
permission_classes (line 95) and extensions (line 98) are currently using
list[...] which is narrower than the Sequence[...] types accepted by
StrawchemyField.__init__ (referenced at lines 153 and 159). Change both
permission_classes and extensions type annotations from list[...] to
Sequence[...] to match the actual parameter types accepted by
StrawchemyField.__init__ and preserve compatibility with tuple-based call sites
during static type checking.
In `@tests/integration/types/postgres.py`:
- Line 171: The type annotation `**_ctx: Any` in the function
_fruit_sweeter_than violates Ruff's ANN401 rule which disallows dynamically
typed expressions in kwargs. Replace the `Any` type annotation with `object` for
the _ctx parameter to satisfy the linting rule while maintaining the same
functionality.
In `@tests/integration/types/sqlite.py`:
- Line 162: The function _fruit_sweeter_than has a kwargs parameter type
annotation using Any, which violates ANN401. Replace the type annotation in the
**_ctx parameter from Any to object to comply with the codebase convention and
ANN401 requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 151e96cc-b631-4337-b4c2-7247a0c1d03a
⛔ Files ignored due to path filters (2)
tests/integration/__snapshots__/test_fine_grained_filters.ambris excluded by!**/__snapshots__/**tests/unit/__snapshots__/test_fine_grained_filters/test_fine_grained_schema.gqlis excluded by!**/__snapshots__/**
📒 Files selected for processing (19)
src/strawchemy/__init__.pysrc/strawchemy/dto/backend/strawberry.pysrc/strawchemy/dto/inspectors/sqlalchemy.pysrc/strawchemy/dto/strawberry.pysrc/strawchemy/mapper.pysrc/strawchemy/schema/factories/inputs.pysrc/strawchemy/schema/field.pysrc/strawchemy/schema/filters/__init__.pysrc/strawchemy/schema/filters/fields.pysrc/strawchemy/schema/filters/geo.pysrc/strawchemy/schema/filters/inputs.pysrc/strawchemy/schema/mutation/field_builder.pysrc/strawchemy/transpiler/_transpiler.pysrc/strawchemy/utils/annotation.pytests/integration/test_fine_grained_filters.pytests/integration/types/mysql.pytests/integration/types/postgres.pytests/integration/types/sqlite.pytests/unit/test_fine_grained_filters.py
| permission_classes: list[type[BasePermission]] | None | ||
| default: Any | ||
| default_factory: Callable[..., object] | object | ||
| extensions: list[FieldExtension] | None |
There was a problem hiding this comment.
Broaden OutputFieldKwargs collection types to preserve typed API compatibility.
Line 95 and Line 98 currently require list[...], but StrawchemyField.__init__ accepts Sequence[...] (Line 153 and Line 159). This narrows the public typed API and can break existing tuple-based call sites in static type checking.
Proposed fix
class OutputFieldKwargs(StrawberryFieldKwargs, total=False):
@@
- permission_classes: list[type[BasePermission]] | None
+ permission_classes: Sequence[type[BasePermission]] | None
@@
- extensions: list[FieldExtension] | None
+ extensions: Sequence[FieldExtension] | None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/strawchemy/schema/field.py` around lines 95 - 98, In the
OutputFieldKwargs class, the type annotations for permission_classes (line 95)
and extensions (line 98) are currently using list[...] which is narrower than
the Sequence[...] types accepted by StrawchemyField.__init__ (referenced at
lines 153 and 159). Change both permission_classes and extensions type
annotations from list[...] to Sequence[...] to match the actual parameter types
accepted by StrawchemyField.__init__ and preserve compatibility with tuple-based
call sites during static type checking.
| class FruitFilter: ... | ||
|
|
||
|
|
||
| def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ruff check tests/integration/types/postgres.py --select ANN401Repository: gazorby/strawchemy
Length of output: 588
🏁 Script executed:
#!/bin/bash
# Check if changing Any to object resolves ANN401
# First, show the current problematic line
echo "=== Current code (with ANN401 error) ==="
sed -n '171p' tests/integration/types/postgres.py
# Now simulate the fix by creating a test file and checking it
cat > /tmp/test_fix.py << 'EOF'
from typing import Any
from sqlalchemy import Select
class Fruit:
pass
# Original (will fail)
def _fruit_sweeter_than_old(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]:
return statement.where(Fruit.sweetness >= value)
# Proposed fix
def _fruit_sweeter_than_new(statement: Select[tuple[Fruit]], value: int, **_ctx: object) -> Select[tuple[Fruit]]:
return statement.where(Fruit.sweetness >= value)
EOF
echo -e "\n=== Checking with object type ==="
ruff check /tmp/test_fix.py --select ANN401Repository: gazorby/strawchemy
Length of output: 786
🏁 Script executed:
#!/bin/bash
# Verify the fix more clearly by checking only the fixed function
cat > /tmp/test_fix_only.py << 'EOF'
from sqlalchemy import Select
class Fruit:
pass
# Proposed fix - should have NO ANN401 error
def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: object) -> Select[tuple[Fruit]]:
return statement.where(Fruit.sweetness >= value)
EOF
echo "=== Checking fixed version (object instead of Any) ==="
ruff check /tmp/test_fix_only.py --select ANN401Repository: gazorby/strawchemy
Length of output: 147
Replace Any in **_ctx with object to satisfy ANN401.
Line 171 uses **_ctx: Any, which violates Ruff's ANN401 rule (dynamically typed expressions are disallowed in kwargs). Change the type annotation to object:
Suggested patch
-def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]:
+def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: object) -> Select[tuple[Fruit]]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]: | |
| def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: object) -> Select[tuple[Fruit]]: |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 171-171: Dynamically typed expressions (typing.Any) are disallowed in **_ctx
(ANN401)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/types/postgres.py` at line 171, The type annotation
`**_ctx: Any` in the function _fruit_sweeter_than violates Ruff's ANN401 rule
which disallows dynamically typed expressions in kwargs. Replace the `Any` type
annotation with `object` for the _ctx parameter to satisfy the linting rule
while maintaining the same functionality.
Source: Linters/SAST tools
| class FruitFilter: ... | ||
|
|
||
|
|
||
| def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ruff check tests/integration/types/sqlite.py --select ANN401Repository: gazorby/strawchemy
Length of output: 586
🏁 Script executed:
#!/bin/bash
# First, let's verify the suggested patch would fix the ANN401 issue
# We'll create a test to check if object type passes the check
# Search for similar **kwargs patterns in integration tests
rg '\*\*.*:\s*(Any|object)' tests/integration/types/ -A 2 -B 2Repository: gazorby/strawchemy
Length of output: 3386
🏁 Script executed:
#!/bin/bash
# Let's check what the parameter is actually used for in the function
sed -n '162,170p' tests/integration/types/sqlite.pyRepository: gazorby/strawchemy
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Test if the suggested patch with object type would pass ANN401
cat > /tmp/test_patch.py << 'EOF'
from typing import Any
from sqlalchemy import Select
# Original (violates ANN401)
# def _fruit_sweeter_than(statement: Select[tuple], value: int, **_ctx: Any) -> Select[tuple]:
# Suggested fix
def _fruit_sweeter_than(statement: Select[tuple], value: int, **_ctx: object) -> Select[tuple]:
return statement
EOF
# Check if object type satisfies ANN401
python3 << 'PY'
import subprocess
result = subprocess.run(
['rg', '\\*\\*_ctx:', 'tests/integration/types/'],
capture_output=True,
text=True
)
print("Current instances of **_ctx:")
print(result.stdout)
print("\nTotal occurrences:", len(result.stdout.strip().split('\n')) if result.stdout.strip() else 0)
PYRepository: gazorby/strawchemy
Length of output: 538
🏁 Script executed:
#!/bin/bash
# Check if there are any other ANN401 violations in the file
ruff check tests/integration/types/sqlite.py --select ANNRepository: gazorby/strawchemy
Length of output: 586
🏁 Script executed:
#!/bin/bash
# Check if there's a ruff configuration that specifies allowed types for kwargs
find . -name "ruff.toml" -o -name "pyproject.toml" -o -name ".ruff.toml" | head -5Repository: gazorby/strawchemy
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Let's verify that the object type would actually fix the ANN401 violation
# by checking ruff documentation or testing locally
# Check the pyproject.toml for any ANN configuration
cat pyproject.toml | grep -A 10 "^\[tool.ruff" | head -30Repository: gazorby/strawchemy
Length of output: 898
🏁 Script executed:
#!/bin/bash
# Check the ruff.toml file for ANN configuration
cat ruff.tomlRepository: gazorby/strawchemy
Length of output: 2525
🏁 Script executed:
#!/bin/bash
# Verify the suggested fix by checking if object type passes ANN401
# Let's search for similar patterns in the codebase that use object instead of Any for **kwargs
rg '\*\*.*:\s*object' --type pyRepository: gazorby/strawchemy
Length of output: 144
🏁 Script executed:
#!/bin/bash
# Let's verify the fix by checking what happens when we use object instead of Any
# We'll look at the test file that uses object pattern for reference
cat tests/unit/mapping/test_model_field.py | grep -A 5 "_make_class"Repository: gazorby/strawchemy
Length of output: 1740
🏁 Script executed:
#!/bin/bash
# Now let's verify that using object would actually pass the ANN401 check
# by looking at the parameter usage in the function
sed -n '162,165p' tests/integration/types/sqlite.py | cat -ARepository: gazorby/strawchemy
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Check if postgres.py and mysql.py have the same issue
ruff check tests/integration/types/postgres.py --select ANN401
ruff check tests/integration/types/mysql.py --select ANN401Repository: gazorby/strawchemy
Length of output: 1111
Use a non-Any kwargs type in _fruit_sweeter_than.
Line 162 uses **_ctx: Any, which violates ANN401. Change to **_ctx: object to match the pattern used elsewhere in the codebase.
Suggested patch
-def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]:
+def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: object) -> Select[tuple[Fruit]]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: Any) -> Select[tuple[Fruit]]: | |
| def _fruit_sweeter_than(statement: Select[tuple[Fruit]], value: int, **_ctx: object) -> Select[tuple[Fruit]]: |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 162-162: Dynamically typed expressions (typing.Any) are disallowed in **_ctx
(ANN401)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/types/sqlite.py` at line 162, The function
_fruit_sweeter_than has a kwargs parameter type annotation using Any, which
violates ANN401. Replace the type annotation in the **_ctx parameter from Any to
object to comply with the codebase convention and ANN401 requirements.
Source: Linters/SAST tools
951e32b to
a77d556
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/strawchemy/dto/strawberry.py (1)
514-515: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExclude explicit
nullhere. Nullable filter inputs like_notcan flow into thevalue.field_nodebranch and raise at runtime. Keep falsey values such as0/False, but skipNone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/strawchemy/dto/strawberry.py` around lines 514 - 515, Update dto_set_fields to exclude attributes whose value is None in addition to strawberry.UNSET, while retaining falsey valid values such as 0 and False. Keep the existing field-name iteration and return behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/strawchemy/schema/filters/inputs.py`:
- Line 429: Update the _StrawchemyComparison metadata declarations at
src/strawchemy/schema/filters/inputs.py lines 429-429 and 453-453 to register
the specified contains, contained_in, and has_key* operators, including SQLite’s
has_key* variants. Update the corresponding declaration at
src/strawchemy/schema/filters/geo.py lines 65-65 to register contains_geometry,
within_geometry, and is_null so the restriction pipeline recognizes all
operators exposed by those inputs.
In `@tests/integration/test_fine_grained_filters.py`:
- Around line 57-67: Strengthen test_restricted_operator_only_selected by
asserting that the GraphQL validation errors specifically identify the
unsupported contains operator, rather than merely checking that result.errors is
present. Preserve the existing query and ensure unrelated schema errors cannot
satisfy the test.
In `@tests/unit/test_fine_grained_filters.py`:
- Around line 241-252: Update the TicketFilter test around filters_tree() to
declare the filter field with join="in" and assert the resulting CustomFilter
preserves that join strategy, while retaining the existing apply and value
assertions.
---
Outside diff comments:
In `@src/strawchemy/dto/strawberry.py`:
- Around line 514-515: Update dto_set_fields to exclude attributes whose value
is None in addition to strawberry.UNSET, while retaining falsey valid values
such as 0 and False. Keep the existing field-name iteration and return behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a1b87cd-6595-462f-98b2-dab4a26b9055
⛔ Files ignored due to path filters (2)
tests/integration/__snapshots__/test_fine_grained_filters.ambris excluded by!**/__snapshots__/**tests/unit/__snapshots__/test_fine_grained_filters/test_fine_grained_schema.gqlis excluded by!**/__snapshots__/**
📒 Files selected for processing (19)
src/strawchemy/__init__.pysrc/strawchemy/dto/backend/strawberry.pysrc/strawchemy/dto/inspectors/sqlalchemy.pysrc/strawchemy/dto/strawberry.pysrc/strawchemy/mapper.pysrc/strawchemy/schema/factories/inputs.pysrc/strawchemy/schema/field.pysrc/strawchemy/schema/filters/__init__.pysrc/strawchemy/schema/filters/fields.pysrc/strawchemy/schema/filters/geo.pysrc/strawchemy/schema/filters/inputs.pysrc/strawchemy/schema/mutation/field_builder.pysrc/strawchemy/transpiler/_planner.pysrc/strawchemy/utils/annotation.pytests/integration/test_fine_grained_filters.pytests/integration/types/mysql.pytests/integration/types/postgres.pytests/integration/types/sqlite.pytests/unit/test_fine_grained_filters.py
| """ | ||
|
|
||
| __strawchemy_filter__ = JSONFilter | ||
| __strawchemy_comparison__ = _StrawchemyComparison(filter=JSONFilter) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Register class-specific operators when migrating comparison metadata.
The restriction pipeline only recognizes operators listed in _StrawchemyComparison; these comparison types therefore reject operators already exposed by their full inputs.
src/strawchemy/schema/filters/inputs.py#L429-L429: add metadata forcontains,contained_in, and allhas_key*operators.src/strawchemy/schema/filters/inputs.py#L453-L453: add metadata for SQLite’shas_key*operators.src/strawchemy/schema/filters/geo.py#L65-L65: add metadata forcontains_geometry,within_geometry, andis_null.
📍 Affects 2 files
src/strawchemy/schema/filters/inputs.py#L429-L429(this comment)src/strawchemy/schema/filters/inputs.py#L453-L453src/strawchemy/schema/filters/geo.py#L65-L65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/strawchemy/schema/filters/inputs.py` at line 429, Update the
_StrawchemyComparison metadata declarations at
src/strawchemy/schema/filters/inputs.py lines 429-429 and 453-453 to register
the specified contains, contained_in, and has_key* operators, including SQLite’s
has_key* variants. Update the corresponding declaration at
src/strawchemy/schema/filters/geo.py lines 65-65 to register contains_geometry,
within_geometry, and is_null so the restriction pipeline recognizes all
operators exposed by those inputs.
| async def test_restricted_operator_only_selected_exposed(any_query: AnyQueryExecutor) -> None: | ||
| # `contains` is not in the selected ops {eq, like} -> GraphQL validation error. | ||
| query = """ | ||
| { | ||
| fruitsFineGrained(filter: { name: { contains: "x" } }) { | ||
| id | ||
| } | ||
| } | ||
| """ | ||
| result = await maybe_async(any_query(query)) | ||
| assert result.errors |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that validation specifically rejects contains.
The current assertion passes for any GraphQL error, so an unrelated schema failure could mask a regression in operator restrictions.
Proposed test strengthening
result = await maybe_async(any_query(query))
assert result.errors
+ assert any("contains" in error.message for error in result.errors)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def test_restricted_operator_only_selected_exposed(any_query: AnyQueryExecutor) -> None: | |
| # `contains` is not in the selected ops {eq, like} -> GraphQL validation error. | |
| query = """ | |
| { | |
| fruitsFineGrained(filter: { name: { contains: "x" } }) { | |
| id | |
| } | |
| } | |
| """ | |
| result = await maybe_async(any_query(query)) | |
| assert result.errors | |
| async def test_restricted_operator_only_selected_exposed(any_query: AnyQueryExecutor) -> None: | |
| # `contains` is not in the selected ops {eq, like} -> GraphQL validation error. | |
| query = """ | |
| { | |
| fruitsFineGrained(filter: { name: { contains: "x" } }) { | |
| id | |
| } | |
| } | |
| """ | |
| result = await maybe_async(any_query(query)) | |
| assert result.errors | |
| assert any("contains" in error.message for error in result.errors) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/test_fine_grained_filters.py` around lines 57 - 67,
Strengthen test_restricted_operator_only_selected by asserting that the GraphQL
validation errors specifically identify the unsupported contains operator,
rather than merely checking that result.errors is present. Preserve the existing
query and ensure unrelated schema errors cannot satisfy the test.
| @strawchemy.filter(_Ticket, include=["name"]) | ||
| class TicketFilter: | ||
| published_after: datetime = filter_field(apply=_published_after) | ||
|
|
||
| instance = TicketFilter() | ||
| instance.published_after = datetime(2024, 1, 1) # ty: ignore[unresolved-attribute] # noqa: DTZ001 | ||
| _node, query = instance.filters_tree() | ||
|
|
||
| assert any(isinstance(item, CustomFilter) for item in query.and_) | ||
| custom = next(item for item in query.and_ if isinstance(item, CustomFilter)) | ||
| assert custom.apply is _published_after | ||
| assert custom.value == datetime(2024, 1, 1) # noqa: DTZ001 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Exercise non-default join propagation through filters_tree().
The test verifies apply and value, but would still pass if the declared join strategy were discarded. Use "in" and assert it reaches CustomFilter.
Proposed test adjustment
class TicketFilter:
- published_after: datetime = filter_field(apply=_published_after)
+ published_after: datetime = filter_field(apply=_published_after, join="in")
assert custom.apply is _published_after
assert custom.value == datetime(2024, 1, 1) # noqa: DTZ001
+assert custom.join == "in"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @strawchemy.filter(_Ticket, include=["name"]) | |
| class TicketFilter: | |
| published_after: datetime = filter_field(apply=_published_after) | |
| instance = TicketFilter() | |
| instance.published_after = datetime(2024, 1, 1) # ty: ignore[unresolved-attribute] # noqa: DTZ001 | |
| _node, query = instance.filters_tree() | |
| assert any(isinstance(item, CustomFilter) for item in query.and_) | |
| custom = next(item for item in query.and_ if isinstance(item, CustomFilter)) | |
| assert custom.apply is _published_after | |
| assert custom.value == datetime(2024, 1, 1) # noqa: DTZ001 | |
| `@strawchemy.filter`(_Ticket, include=["name"]) | |
| class TicketFilter: | |
| published_after: datetime = filter_field(apply=_published_after, join="in") | |
| instance = TicketFilter() | |
| instance.published_after = datetime(2024, 1, 1) # ty: ignore[unresolved-attribute] # noqa: DTZ001 | |
| _node, query = instance.filters_tree() | |
| assert any(isinstance(item, CustomFilter) for item in query.and_) | |
| custom = next(item for item in query.and_ if isinstance(item, CustomFilter)) | |
| assert custom.apply is _published_after | |
| assert custom.value == datetime(2024, 1, 1) # noqa: DTZ001 | |
| assert custom.join == "in" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_fine_grained_filters.py` around lines 241 - 252, Update the
TicketFilter test around filters_tree() to declare the filter field with
join="in" and assert the resulting CustomFilter preserves that join strategy,
while retaining the existing apply and value assertions.
1 participant
Summary by CodeRabbit
New Features
existsorinbehavior.Bug Fixes
Tests