Change Maybe to Some[T] | None instead of Some[T | None] | None
#3961
Conversation
Reviewer's GuideThis PR refactors Strawberry’s Maybe type so that Maybe[T] now represents “Some[T] or None” (no longer allowing explicit null inside Some), and explicit null support is now achieved via Maybe[T | None]. It updates core type definitions, argument and schema conversion logic, adds validation rules to reject nulls for Maybe[T], provides a codemod to migrate code, updates CLI commands, revises documentation, and adjusts tests accordingly. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3961 +/- ##
==========================================
- Coverage 94.41% 94.26% -0.15%
==========================================
Files 528 533 +5
Lines 34371 34863 +492
Branches 1803 1854 +51
==========================================
+ Hits 32450 32864 +414
- Misses 1630 1693 +63
- Partials 291 306 +15 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #3961 will not alter performanceComparing Summary
|
patrick91
force-pushed
the
fix/maybe-none
branch
from
4447be9 to
2a3d6bf
Compare
|
Thanks for adding the Here's a preview of the changelog: This release changes the Breaking Change: The
This provides a cleaner API where See the breaking changes documentation for migration details. Here's the tweet text: |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements a significant refactoring of Strawberry's Maybe type system by changing the type definition from Union[Some[Union[T, None]], None] to Union[Some[T], None]. This change eliminates redundant nesting of None values and provides clearer semantics for handling optional-nullable fields in GraphQL schemas.
The core change affects how developers distinguish between "field not provided" and "field explicitly set to null" scenarios. With the new implementation:
Maybe[T]represents fields that can be absent or contain a value of type T, but cannot be explicitly set to nullMaybe[T | None]represents fields that can be absent, contain a value of type T, or be explicitly set to null
The PR includes comprehensive changes across the codebase:
- Type System: Updated the
Maybetype alias instrawberry/types/maybe.pyto remove nested None handling - Schema Conversion: Modified
schema_converter.pyto properly handle the newStrawberryMaybetype structure - Validation: Added
MaybeNullValidationRuleto enforce type-safe semantics at the GraphQL validation layer - Argument Processing: Updated argument conversion logic to properly wrap values in
Some()containers - Migration Support: Created a
ConvertMaybeToOptionalcodemod to help users migrate existing code - Documentation: Added comprehensive documentation explaining the new Maybe semantics and migration strategies
- Testing: Extensive test coverage for the new behavior patterns
This change addresses issue #3779 by providing a more intuitive and type-safe approach to handling optional fields. The new system ensures that if field is not None consistently means "field was provided" regardless of whether the field allows null values, making the API more predictable and reducing developer errors.
Confidence score: 2/5
- This PR introduces complex breaking changes that could cause significant issues if the migration tooling or validation rules don't work correctly
- Score reflects the high complexity of the type system changes and potential for runtime errors if validation rules fail
- Pay close attention to
strawberry/cli/commands/upgrade/__init__.pywhich has a critical bug preventing the new codemod from being used, andtests/schema/validation_rules/__init__.pywhich accidentally removes essential validation logic
19 files reviewed, 3 comments
| def test_simple_maybe_union_syntax(self) -> None: | ||
| before = """ | ||
| from strawberry import Maybe | ||
|
|
||
| field: Maybe[str] | ||
| """ | ||
|
|
||
| after = """ | ||
| from strawberry import Maybe | ||
| from typing import Union | ||
|
|
||
| field: Maybe[Union[str, None]] | ||
| """ | ||
|
|
||
| self.assertCodemod(before, after, use_pipe_syntax=False) |
There was a problem hiding this comment.
style: Duplicate test - test_simple_maybe_union_syntax has identical logic to test_simple_maybe. Consider removing or differentiating them.
Context Used: Context - In tests involving Pydantic models, ensure that the tests accurately reflect the intended behavior by using the correct instances and values. Avoid copy-pasting code that may lead to confusion. (link)
| # Resolve the actual GraphQL field name using the same logic as NameConverter | ||
| if field_def.graphql_name is not None: | ||
| field_name = field_def.graphql_name | ||
| else: | ||
| # Apply auto_camel_case conversion if enabled (default behavior) | ||
| field_name = to_camel_case(field_def.python_name) |
There was a problem hiding this comment.
logic: The field name resolution logic assumes auto_camel_case is enabled by default, but should handle cases where it's disabled in schema configuration
| def _get_type_name(self, type_: Any) -> str: | ||
| """Get a readable type name for error messages.""" | ||
| if hasattr(type_, "__name__"): | ||
| return type_.__name__ | ||
| # Handle Strawberry types that don't have __name__ | ||
| if hasattr(type_, "of_type") and hasattr(type_.of_type, "__name__"): | ||
| # For StrawberryList, StrawberryOptional, etc. | ||
| return ( | ||
| f"list[{type_.of_type.__name__}]" | ||
| if "List" in str(type_.__class__) | ||
| else type_.of_type.__name__ | ||
| ) | ||
| return str(type_) |
There was a problem hiding this comment.
style: The type name extraction logic could fail for complex nested types. Consider adding more robust handling for generic types and edge cases
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- ConvertMaybeToOptional currently assumes any ‘Maybe’ symbol is from Strawberry—consider enhancing import resolution to avoid transforming unrelated types.
- The new convert_argument logic rearranges StrawberryMaybe and StrawberryOptional handling; adding inline comments or extracting helper methods could clarify the intended branch order and reduce complexity.
- Double-check the insertion point of MaybeNullValidationRule in validate_document to ensure it runs in the correct order relative to other validation rules.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ConvertMaybeToOptional currently assumes any ‘Maybe’ symbol is from Strawberry—consider enhancing import resolution to avoid transforming unrelated types.
- The new convert_argument logic rearranges StrawberryMaybe and StrawberryOptional handling; adding inline comments or extracting helper methods could clarify the intended branch order and reduce complexity.
- Double-check the insertion point of MaybeNullValidationRule in validate_document to ensure it runs in the correct order relative to other validation rules.
## Individual Comments
### Comment 1
<location> `strawberry/codemods/maybe_optional.py:87` </location>
<code_context>
+ def _already_includes_none(self, node: BaseExpression) -> bool:
</code_context>
<issue_to_address>
The _already_includes_none method may not handle all edge cases for complex type expressions.
It currently checks for T | None and Union[..., None], but may not detect Optional[T] or nested unions. Please update the logic or clarify its limitations in the documentation.
</issue_to_address>
### Comment 2
<location> `strawberry/schema/validation_rules/maybe_null.py:121` </location>
<code_context>
+ )
+ )
+
+ def _get_type_name(self, type_: Any) -> str:
+ """Get a readable type name for error messages."""
+ if hasattr(type_, "__name__"):
+ return type_.__name__
+ # Handle Strawberry types that don't have __name__
+ if hasattr(type_, "of_type") and hasattr(type_.of_type, "__name__"):
+ # For StrawberryList, StrawberryOptional, etc.
+ return (
+ f"list[{type_.of_type.__name__}]"
+ if "List" in str(type_.__class__)
+ else type_.of_type.__name__
+ )
+ return str(type_)
+
+
</code_context>
<issue_to_address>
Type name resolution in _get_type_name may produce verbose or unclear names for complex types.
The use of str(type_) for complex types can make error messages harder to interpret. Please refine this logic to generate clearer type names for nested or generic types.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def _get_type_name(self, type_: Any) -> str:
"""Get a readable type name for error messages."""
if hasattr(type_, "__name__"):
return type_.__name__
# Handle Strawberry types that don't have __name__
if hasattr(type_, "of_type") and hasattr(type_.of_type, "__name__"):
# For StrawberryList, StrawberryOptional, etc.
return (
f"list[{type_.of_type.__name__}]"
if "List" in str(type_.__class__)
else type_.of_type.__name__
)
return str(type_)
=======
def _get_type_name(self, type_: Any) -> str:
"""Get a readable type name for error messages, including nested/generic types."""
# Handle built-in types and custom classes
if hasattr(type_, "__name__"):
return type_.__name__
# Handle StrawberryList, StrawberryOptional, StrawberryUnion, etc.
type_class_name = type_.__class__.__name__
if hasattr(type_, "of_type"):
inner_name = self._get_type_name(type_.of_type)
if "List" in type_class_name:
return f"list[{inner_name}]"
if "Optional" in type_class_name or "Maybe" in type_class_name:
return f"Optional[{inner_name}]"
return inner_name
# Handle StrawberryUnion
if hasattr(type_, "types"):
union_names = [self._get_type_name(t) for t in type_.types]
return f"Union[{', '.join(union_names)}]"
# Fallback: try to get a readable name, avoid verbose str(type_)
if hasattr(type_, "_type_definition") and hasattr(type_._type_definition, "name"):
return type_._type_definition.name
# Last resort: use repr to avoid verbose module path
return repr(type_)
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def _get_type_name(self, type_: Any) -> str: | ||
| """Get a readable type name for error messages.""" | ||
| if hasattr(type_, "__name__"): | ||
| return type_.__name__ | ||
| # Handle Strawberry types that don't have __name__ | ||
| if hasattr(type_, "of_type") and hasattr(type_.of_type, "__name__"): | ||
| # For StrawberryList, StrawberryOptional, etc. | ||
| return ( | ||
| f"list[{type_.of_type.__name__}]" | ||
| if "List" in str(type_.__class__) | ||
| else type_.of_type.__name__ | ||
| ) | ||
| return str(type_) |
There was a problem hiding this comment.
suggestion: Type name resolution in _get_type_name may produce verbose or unclear names for complex types.
The use of str(type_) for complex types can make error messages harder to interpret. Please refine this logic to generate clearer type names for nested or generic types.
| def _get_type_name(self, type_: Any) -> str: | |
| """Get a readable type name for error messages.""" | |
| if hasattr(type_, "__name__"): | |
| return type_.__name__ | |
| # Handle Strawberry types that don't have __name__ | |
| if hasattr(type_, "of_type") and hasattr(type_.of_type, "__name__"): | |
| # For StrawberryList, StrawberryOptional, etc. | |
| return ( | |
| f"list[{type_.of_type.__name__}]" | |
| if "List" in str(type_.__class__) | |
| else type_.of_type.__name__ | |
| ) | |
| return str(type_) | |
| def _get_type_name(self, type_: Any) -> str: | |
| """Get a readable type name for error messages, including nested/generic types.""" | |
| # Handle built-in types and custom classes | |
| if hasattr(type_, "__name__"): | |
| return type_.__name__ | |
| # Handle StrawberryList, StrawberryOptional, StrawberryUnion, etc. | |
| type_class_name = type_.__class__.__name__ | |
| if hasattr(type_, "of_type"): | |
| inner_name = self._get_type_name(type_.of_type) | |
| if "List" in type_class_name: | |
| return f"list[{inner_name}]" | |
| if "Optional" in type_class_name or "Maybe" in type_class_name: | |
| return f"Optional[{inner_name}]" | |
| return inner_name | |
| # Handle StrawberryUnion | |
| if hasattr(type_, "types"): | |
| union_names = [self._get_type_name(t) for t in type_.types] | |
| return f"Union[{', '.join(union_names)}]" | |
| # Fallback: try to get a readable name, avoid verbose str(type_) | |
| if hasattr(type_, "_type_definition") and hasattr(type_._type_definition, "name"): | |
| return type_._type_definition.name | |
| # Last resort: use repr to avoid verbose module path | |
| return repr(type_) |
| if input.name is not None: | ||
| return f"Updated to: {input.name.value}" |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if input.name is not None: | ||
| return f"Updated to: {input.name.value}" |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if input.name is not None: | ||
| return f"Updated to: {input.name.value}" |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if input.name is not None: | ||
| return f"Updated to: {input.name.value}" |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else) - Swap if/else branches (
swap-if-else-branches) - Replace if statement with if expression (
assign-if-exp)
| schema = strawberry.Schema(query=Query, mutation=Mutation) | ||
|
|
||
| # Test 1: Valid values for all fields | ||
| query1 = """ |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into function (extract-duplicate-method)
| schema = strawberry.Schema(query=Query, mutation=Mutation) | ||
|
|
||
| # Test 1: Valid lists for both fields | ||
| query1 = """ |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into function (extract-duplicate-method)
| schema = strawberry.Schema(query=Query) | ||
|
|
||
| # Test 1: Valid values for both arguments | ||
| query1 = """ |
There was a problem hiding this comment.
issue (code-quality): Extract duplicate code into function (extract-duplicate-method)
|
|
||
|
|
||
| def test_maybe_graphql_schema_consistency(): | ||
| """Test GraphQL schema generation for Maybe types. |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Move assignment closer to its usage within a block (
move-assign-in-block) - Extract duplicate code into function (
extract-duplicate-method)
Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Refactor the Maybe type definition to avoid redundant nesting of None and update the GraphQL schema converter to handle the new StrawberryMaybe type accordingly
Enhancements:
Summary by Sourcery
Refine strawberry.Maybe to represent Some[T] | None by default, disallow explicit nulls for Maybe[T], and require Maybe[T | None] for nullability. Update conversion logic, schema generation, and validation rules accordingly, provide a codemod for migration, and enrich documentation and tests.
New Features:
Enhancements:
Documentation:
Tests:
Chores: