feat(schema): remove duplicated field_map on strawchemy types#200
feat(schema): remove duplicated field_map on strawchemy types#200gazorby wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe PR removes the ChangesDTOKey / field_map removal and aliased-column regression fix
pyproject.toml formatting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/strawchemy/schema/factories/inputs.py (1)
320-375:⚠️ Potential issue | 🔴 CriticalThis code will fail with aliased aggregatable fields.
name.field_definition.namereturns the aliased name (e.g., "rating") when an alias is configured, butmodel_fieldsis keyed by the actual model field name (e.g., "score"). The lookup at line 331 will raise aKeyErrorwhen aggregatable columns have aliases. Additionally, line 328 incorrectly usesname.field_definition.nameasmodel_field_nameinstead of the actual model field name.Use
name.field_definition.model_field_nameinstead to access the underlying model field name that matches the keys inmodel_fields.🤖 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 320 - 375, In the _order_by_aggregation_fields method, the code uses name.field_definition.name which returns the aliased field name (e.g., "rating"), but model_fields is keyed by the actual model field name (e.g., "score"). This causes a KeyError when aggregatable columns have aliases. Replace both occurrences of name.field_definition.name with name.field_definition.model_field_name: first when assigning the model_field_name parameter in the FunctionArgFieldDefinition constructor, and second when looking up the field from the model_fields dictionary at the model_fields[name.field_definition.name].model_field access point.
🤖 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 `@pyproject.toml`:
- Line 316: The glob pattern entry for alembic contains two trailing spaces
before the comma that should be removed. Locate the "alembic/*" pattern in the
pyproject.toml file and remove the trailing whitespace so the pattern reads
"alembic/*" without any spaces before the comma.
---
Outside diff comments:
In `@src/strawchemy/schema/factories/inputs.py`:
- Around line 320-375: In the _order_by_aggregation_fields method, the code uses
name.field_definition.name which returns the aliased field name (e.g.,
"rating"), but model_fields is keyed by the actual model field name (e.g.,
"score"). This causes a KeyError when aggregatable columns have aliases. Replace
both occurrences of name.field_definition.name with
name.field_definition.model_field_name: first when assigning the
model_field_name parameter in the FunctionArgFieldDefinition constructor, and
second when looking up the field from the model_fields dictionary at the
model_fields[name.field_definition.name].model_field access point.
🪄 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: 91708b44-7b89-42f7-9040-8cc0e159e88b
📒 Files selected for processing (11)
pyproject.tomlsrc/strawchemy/dto/strawberry.pysrc/strawchemy/repository/strawberry/base.pysrc/strawchemy/schema/factories/_kwargs.pysrc/strawchemy/schema/factories/aggregations.pysrc/strawchemy/schema/factories/base.pysrc/strawchemy/schema/factories/inputs.pysrc/strawchemy/schema/factories/types.pytests/unit/dto/test_dto.pytests/unit/mapping/test_schemas.pytests/unit/schemas/order/order_by_aliased_aggregation.py
💤 Files with no reviewable changes (1)
- tests/unit/dto/test_dto.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
=======================================
Coverage 93.26% 93.27%
=======================================
Files 69 72 +3
Lines 6338 6391 +53
Branches 853 847 -6
=======================================
+ Hits 5911 5961 +50
- Misses 285 292 +7
+ Partials 142 138 -4 ☔ View full report in Codecov by Harness. |
gazorby
force-pushed
the
worktree-centralize-factory-field-map
branch
from
820f5c8 to
9c874b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/dto/strawberry.py`:
- Around line 507-509: The issue is that field.model_field assignment in the
FunctionFieldDefinition handling directly mutates the shared
__dto_field_definitions__ ClassVar, causing stale model_field values to persist
across subsequent OrderByDTO instances. Instead of mutating the shared field
definition, create a copy of the field object before assigning
node.value.model_field to it. This ensures each instance has its own independent
copy of the field with the correct node-specific model context rather than
reusing a mutated shared definition.
🪄 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: fbae2cf6-f1ff-4de4-a7af-5d307cc1b2b3
📒 Files selected for processing (11)
pyproject.tomlsrc/strawchemy/dto/strawberry.pysrc/strawchemy/repository/strawberry/base.pysrc/strawchemy/schema/factories/_kwargs.pysrc/strawchemy/schema/factories/aggregations.pysrc/strawchemy/schema/factories/base.pysrc/strawchemy/schema/factories/inputs.pysrc/strawchemy/schema/factories/types.pytests/unit/dto/test_dto.pytests/unit/mapping/test_schemas.pytests/unit/schemas/order/order_by_aliased_aggregation.py
💤 Files with no reviewable changes (1)
- tests/unit/dto/test_dto.py
| field = self.__dto_field_definitions__[name] | ||
| if isinstance(field, FunctionFieldDefinition) and not field.has_model_field: | ||
| field.model_field = node.value.model_field |
There was a problem hiding this comment.
Copy the field before adding node-specific model context.
__dto_field_definitions__ is a ClassVar, so field.model_field = node.value.model_field mutates shared DTO metadata. After the first traversal sets it, later OrderByDTO instances can reuse a stale parent model_field and build incorrect order-by nodes.
🐛 Proposed fix
- field = self.__dto_field_definitions__[name]
+ field = self.__dto_field_definitions__[name]
if isinstance(field, FunctionFieldDefinition) and not field.has_model_field:
+ field = copy(field)
field.model_field = node.value.model_field🤖 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 507 - 509, The issue is that
field.model_field assignment in the FunctionFieldDefinition handling directly
mutates the shared __dto_field_definitions__ ClassVar, causing stale model_field
values to persist across subsequent OrderByDTO instances. Instead of mutating
the shared field definition, create a copy of the field object before assigning
node.value.model_field to it. This ensures each instance has its own independent
copy of the field with the correct node-specific model context rather than
reusing a mutated shared definition.
1 participant
Summary by CodeRabbit
pyproject.tomlwithout changing values.