feat(schema): semantic nullability draft #3722
Conversation
Reviewer's Guide by SourceryThis PR introduces semantic nullability support in Strawberry by adding a new Class diagram for new and modified classesclassDiagram
class StrawberryRequired {
<<StrawberryContainer>>
}
class SemanticNonNull {
+list<int> levels
}
class StrawberryAnnotation {
+create_required(evaled_type: Any) StrawberryRequired
+_is_required(annotation: Any) bool
}
class SchemaConverter {
+from_maybe_optional_or_required(type_: Union<StrawberryType, type>) Union<GraphQLNullableType, GraphQLNonNull>
+create_semantic_non_null_directive() SemanticNonNull
}
class StrawberryConfig {
+bool semantic_nullability_beta
}
StrawberryAnnotation --> StrawberryRequired
SchemaConverter --> SemanticNonNull
SemanticNonNull --> StrawberryConfig
note for StrawberryRequired "New class for handling required types"
note for SemanticNonNull "New class for semantic non-null directive"
note for SchemaConverter "Modified to handle semantic nullability"
File-Level Changes
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 @erikwrede - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more test cases to cover edge cases and different combinations of Required/Optional types, particularly around nested types and lists.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| NoneType = type(None) | ||
| if type_ is None or type_ is NoneType: | ||
| return self.from_type(type_) | ||
| elif isinstance(type_, StrawberryOptional): | ||
| return self.from_type(type_.of_type) | ||
| elif isinstance(type_, StrawberryRequired): |
There was a problem hiding this comment.
issue (bug_risk): Missing return statement in StrawberryRequired branch
The code sets strawberry_semantic_required_non_null but doesn't return graphql_core_type, causing it to fall through to the else clause. Add 'return graphql_core_type' after setting the attribute.
| from strawberry.schema.config import StrawberryConfig | ||
|
|
||
|
|
||
| def test_entities_type_when_no_type_has_keys(): |
There was a problem hiding this comment.
issue (testing): Test name doesn't match the actual test content
The test name suggests it's testing entity types without keys, but it appears to be testing semantic nullability behavior. Consider renaming the test to something like test_semantic_nullability_schema_generation or test_semantic_nullability_with_required_and_optional_fields to better reflect its purpose.
| price: Required[int] | ||
| weight: Optional[int] |
There was a problem hiding this comment.
suggestion (testing): Missing test cases for semantic nullability behavior
The test only verifies SDL generation but doesn't test the runtime behavior of Required and Optional fields. Consider adding test cases that verify: 1) Required fields actually enforce non-null values at runtime, 2) Optional fields accept null values, 3) Error cases when null is provided for Required fields.
Suggested implementation:
def test_entities_type_when_no_type_has_keys():
"""Test SDL generation for types with Required and Optional fields"""
=======
def test_required_field_rejects_null():
"""Test that Required fields raise an error when null is provided"""
@strawberry.type
class Product:
upc: str
name: str
price: Required[int]
weight: Optional[int]
schema = strawberry.Schema(
query=Product,
config=StrawberryConfig(semantic_nullability_beta=True)
)
with pytest.raises(ValueError) as exc:
Product(upc="123", name="test", price=None, weight=10)
assert "price cannot be null" in str(exc.value)
def test_optional_field_accepts_null():
"""Test that Optional fields accept null values"""
@strawberry.type
class Product:
upc: str
name: str
price: Required[int]
weight: Optional[int]
# Should not raise any errors
product = Product(upc="123", name="test", price=100, weight=None)
assert product.weight is None
def test_required_field_accepts_value():
"""Test that Required fields accept non-null values"""
@strawberry.type
class Product:
upc: str
name: str
price: Required[int]
weight: Optional[int]
# Should not raise any errors
product = Product(upc="123", name="test", price=100, weight=10)
assert product.price == 100You'll need to:
- Add
import pytestat the top of the file if not already present - Make sure the
RequiredandOptionaltypes are properly imported from wherever they are defined in your codebase - Consider adding more edge cases based on your specific requirements (e.g., testing with different types, testing inheritance behavior, etc.)
|
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature.Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) Here's the tweet text: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3722 +/- ##
==========================================
- Coverage 97.01% 94.98% -2.04%
==========================================
Files 500 499 -1
Lines 33399 32381 -1018
Branches 5601 1664 -3937
==========================================
- Hits 32402 30756 -1646
- Misses 793 1357 +564
- Partials 204 268 +64 |
CodSpeed Performance ReportMerging #3722 will not alter performanceComparing Summary
|
This PR brings experimental support for semantic nullability to strawberry, as discussed with @captbaritone, @patrick91.
Open tasks
[Int]. With levels support, we can also support[Int!]or[Int]!For further info on semantic nullability, please check out this excellent talk by Jordan: https://www.youtube.com/watch?v=kVYlplb1gKk