Improve support for type inheritance from other mapped types #253
+519
−14
Conversation
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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #253 +/- ##
==========================================
+ Coverage 89.72% 90.16% +0.44%
==========================================
Files 17 18 +1
Lines 1936 2065 +129
Branches 145 159 +14
==========================================
+ Hits 1737 1862 +125
+ Misses 125 123 -2
- Partials 74 80 +6 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #253 will not alter performanceComparing Summary
|
Ckk3
changed the title
|
Thanks for adding the Here's a preview of the changelog: This release improves how types inherit fields from other mapped types using Some examples:
@mapper.type(ModelA)
class ApiA:
pass
@mapper.type(ModelB)
class ApiB(ApiA):
# ApiB inherits all fields declared in ApiA
pass
@mapper.type(ModelA)
class ApiA:
__exclude__ = ["relationshipB_id"]
@mapper.type(ModelB)
class ApiB(ApiA):
# ApiB will have all fields declared in ApiA, except "relationshipB_id"
pass
class ModelA(base):
__tablename__ = "a"
id = Column(String, primary_key=True)
example_field = Column(String(50))
class ModelB(base):
__tablename__ = "b"
id = Column(String, primary_key=True)
example_field = Column(Integer, autoincrement=True)
@mapper.type(ModelA)
class ApiA:
# example_field will be a String
pass
@mapper.type(ModelB)
class ApiB(ApiA):
# example_field will be taken from ModelB and will be an Integer
pass
class ModelA(base):
__tablename__ = "a"
id = Column(String, primary_key=True)
example_field = Column(String(50))
class ModelB(base):
__tablename__ = "b"
id = Column(String, primary_key=True)
example_field = Column(Integer, autoincrement=True)
@mapper.type(ModelA)
class ApiA:
pass
@mapper.type(ModelB)
class ApiB(ApiA):
# example_field will be a Float
example_field: float = strawberry.field(name="exampleField") |
Reviewer's GuideThis PR refactors the mapping conversion logic to better support inheritance of mapped types by extracting resolver handling into a helper, refining annotation merging based on Python version, and updating both documentation and tests to cover predictable conflict resolution between inherited, model-defined, and manually declared fields. Class diagram for mapped type inheritance and conflict resolutionclassDiagram
class ModelA {
+id: String
+common_field: String
}
class ModelB {
+id: String
+common_field: Integer
+extra_field: String
}
class ApiA {
<<mapped type>>
__exclude__: List[str]
+common_field
}
class ApiB {
<<mapped type>>
+common_field
+extra_field: float
}
ModelA <|-- ApiA
ModelB <|-- ApiB
ApiA <|-- ApiB
note for ApiB "common_field comes from ModelB, extra_field is manually overridden as float"
Class diagram for annotation merging and field resolution in convert()classDiagram
class Mapper {
+_handle_columns()
}
class ConvertHelper {
+_get_generated_field_keys(type_, old_annotations) Tuple[List[str], Dict[str, Any]]
+convert(type_)
}
Mapper <.. ConvertHelper : uses
ConvertHelper : - _get_generated_field_keys(type_, old_annotations)
ConvertHelper : - convert(type_)
ConvertHelper : - Handles merging of annotations based on Python version
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Ckk3 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/strawberry_sqlalchemy_mapper/mapper.py:656` </location>
<code_context>
+ def _get_generated_field_keys(type_, old_annotations) -> Tuple[List[str], Dict[str, Any]]:
</code_context>
<issue_to_address>
The helper function mutates its input dictionary, which could lead to side effects.
_get_generated_field_keys modifies old_annotations in place. To prevent unintended side effects, make a shallow copy of old_annotations inside the function.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def _get_generated_field_keys(type_, old_annotations) -> Tuple[List[str], Dict[str, Any]]:
generated_field_keys = set()
=======
def _get_generated_field_keys(type_, old_annotations) -> Tuple[List[str], Dict[str, Any]]:
old_annotations = old_annotations.copy()
generated_field_keys = set()
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `README.md:192` </location>
<code_context>
+class ApiB(ApiA):
+ # Inherits fields from ApiA, except "id"
+ # "common_field" will come from ModelB, not ModelA, so it will be a Integer
+ # "extra_field" will be overrided and will be a float now instead of the String type declared in ModelB:
+ extra_field: float = strawberry.field(name="extraField")
+```
</code_context>
<issue_to_address>
Typo: 'overrided' should be 'overridden'.
Please update the comment to use 'overridden' instead of 'overrided'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# "extra_field" will be overrided and will be a float now instead of the String type declared in ModelB:
=======
# "extra_field" will be overridden and will be a float now instead of the String type declared in ModelB:
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `RELEASE.md:62` </location>
<code_context>
+
[email protected](ModelB)
+class ApiB(ApiA):
+ # example_field will be taken from ModelB and will be a Integer
+ pass
+```
</code_context>
<issue_to_address>
Grammar: 'a Integer' should be 'an Integer'.
Update the comment to use 'an Integer' instead of 'a Integer'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
@mapper.type(ModelB)
class ApiB(ApiA):
# example_field will be taken from ModelB and will be a Integer
pass
=======
@mapper.type(ModelB)
class ApiB(ApiA):
# example_field will be taken from ModelB and will be an Integer
pass
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
@bellini666 @patrick91, when you have a moment, I’d appreciate your review. |
bellini666
approved these changes
Jun 22, 2025
Comment on lines
+824
to
+831
| # For Python versions <= 3.9, only update annotations that don't already exist | ||
| # because this versions handle inherance differently | ||
| if sys.version_info[:2] <= (3, 9): | ||
| for k, v in old_annotations.items(): | ||
| if k not in type_.__annotations__: | ||
| type_.__annotations__[k] = v | ||
| else: | ||
| type_.__annotations__.update(old_annotations) |
There was a problem hiding this comment.
question: what is the exact difference between them? In theory they should be handling that the same
One thing that I know is that in previous versions, __annotations__ could not exist in the class if that class didn't have any annotations in it, something which I had to workaround on strawberry-django.
Is that it or something else?
There was a problem hiding this comment.
Hi @bellini666, sorry for the delay!
The problem isn’t directly related to __annotations__ existance, but to inconsistencies with inherited classes and this annotations.
In Python 3.10 and newer, the __annotations__ no longer includes the annotations from original_type (base class in this case), which caused some resolvers or type hints to be missing in the final class. So I had to add extra logic to extract those annotations manually.
I didn’t investigate this deeply, but I suspect it only happens because we’re using a decorator, which might interfere with how Python handles class inheritance and type resolution at that point.
On the other hand, in Python 3.8 and 3.9, the inherited annotations are already present in the subclass’s __annotations__. So thats why I only add missing keys, since its was raising a error when only using update().
Let me know if you see any better idea to fix this, this one was the most stable solution I found to make it work consistently across versions.
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.
Description
Ensures consistent behavior when extending mapped types, resolving conflicts between inherited and model-defined fields more predictably.
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Improve support for type inheritance by centralizing generated resolver handling, merging annotations appropriately for different Python versions, and updating documentation and tests to cover inheritance, exclusion, and conflict resolution
Bug Fixes:
Enhancements:
Documentation:
Tests: