Add support for optional connections #3707
Conversation
Enable nullable Connection types in the connection field decorator by updating type checking logic and adding validation for inner types. Update documentation and add tests to ensure compatibility with permission extensions and different nullable syntax. New Features: - Support nullable Connection types in the connection field decorator in strawberry.relay.fields. Enhancements: - Update type checking logic to handle Optional[Connection[T]] and Connection[T] | None annotations. Documentation: - Update documentation to reflect that connection fields can now be nullable. Tests: - Add tests to verify nullable connection fields work correctly with permission extensions and both Optional[Connection[T]] and Connection[T] | None syntax. Resolves #3703
for more information, see https://pre-commit.ci
Reviewer's Guide by SourceryThis PR adds support for optional Relay connections in Strawberry GraphQL, allowing connections to be nullable. The implementation modifies the connection field handling to support Optional types and includes type checking adjustments to accommodate this change. Updated class diagram for Relay connection handlingclassDiagram
class Connection {
<<interface>>
}
class Node {
<<interface>>
}
class StrawberryField {
+type: Any
}
class StrawberryOptional {
+of_type: Any
}
class RelayField {
+apply(field: StrawberryField) void
+resolve() void
+connection_type: Type[Connection[Node]]
}
RelayField --> StrawberryField
RelayField --> StrawberryOptional
StrawberryField --> Connection
StrawberryField --> Node
note for RelayField "Handles optional connection types"
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
CodSpeed Performance ReportMerging #3707 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3707 +/- ##
=======================================
Coverage 96.97% 96.97%
=======================================
Files 503 504 +1
Lines 33708 33768 +60
Branches 5627 5639 +12
=======================================
+ Hits 32689 32748 +59
- Misses 809 810 +1
Partials 210 210 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @patrick91 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue 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.
| @@ -253,13 +256,18 @@ def apply(self, field: StrawberryField) -> None: | |||
| None, | |||
| ) | |||
|
|
|||
| if is_union(resolver_type): | |||
There was a problem hiding this comment.
issue (bug_risk): The current union type handling is unsafe as it assumes the first type argument is the correct one
Consider implementing proper validation of the union type structure to ensure we're handling optional types correctly. The current approach could lead to runtime errors if the assumptions about the type structure don't hold.
| @@ -138,7 +138,7 @@ async def subscribe( | |||
| middleware_manager, | |||
| execution_context_class, | |||
| ) | |||
| # GrapQL-core might return an initial error result instead of an async iterator. | |||
| # GraphQL-core might return an initial error result instead of an async iterator. | |||
There was a problem hiding this comment.
nitpick (typo): Fix typo in comment: 'GrapQL' should be 'GraphQL'
There was a problem hiding this comment.
Hrm, isn't this suggestion correct? 🤔
There was a problem hiding this comment.
Yes, the suggestion is correct. The original comment had a typo, "GrapQL," which should indeed be corrected to "GraphQL." This change ensures clarity and accuracy in the code comments. Let me know if you have any other questions or need further clarification!
|
Thanks for adding the Here's a preview of the changelog: This release adds support for making Relay connection optional, this is useful Example: import strawberry
from strawberry import relay
from strawberry.permission import BasePermission
class IsAuthenticated(BasePermission):
message = "User is not authenticated"
# This method can also be async!
def has_permission(
self, source: typing.Any, info: strawberry.Info, **kwargs
) -> bool:
return False
@strawberry.type
class Fruit(relay.Node):
code: relay.NodeID[int]
name: str
weight: float
@classmethod
def resolve_nodes(
cls,
*,
info: strawberry.Info,
node_ids: Iterable[str],
):
return []
@strawberry.type
class Query:
node: relay.Node = relay.node()
@relay.connection(
relay.ListConnection[Fruit] | None, permission_classes=[IsAuthenticated()]
)
def fruits(self) -> Iterable[Fruit]:
# This can be a database query, a generator, an async generator, etc
return all_fruits.values()Here's the tweet text: |
strawberry/relay/fields.py
Outdated
| # TODO: actually check if is optional and get correct type | ||
| resolver_type = get_args(resolver_type)[0] |
There was a problem hiding this comment.
suggestion: we can probably use is_optional to check and get_optional_annotation in this line from strawberry.utils.typing
Co-authored-by: Thiago Bellini Ribeiro <[email protected]>
Closes #3703
Summary by Sourcery
Add support for optional Relay connections to enhance flexibility in permission handling and update tests and documentation accordingly.
New Features:
Documentation:
Tests: