Add strawberry.cast
#3749
Add strawberry.cast
#3749
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new Sequence diagram for the improved relay node resolutionsequenceDiagram
participant Client
participant GraphQL
participant RelayResolver
participant TypeResolver
Client->>GraphQL: Query node(id: 'PublicUser:123')
GraphQL->>RelayResolver: Resolve node
RelayResolver->>TypeResolver: resolve_type()
TypeResolver-->>RelayResolver: node_type
RelayResolver->>RelayResolver: resolve_node()
Note over RelayResolver: New: Apply strawberry.cast
RelayResolver->>TypeResolver: is_type_of check
Note over TypeResolver: Check __as_strawberry_type__
TypeResolver-->>RelayResolver: Correct type confirmed
RelayResolver-->>GraphQL: Correctly typed node
GraphQL-->>Client: Response
Class diagram for the new cast functionalityclassDiagram
class StrawberryCastObj {
<<Protocol>>
+__as_strawberry_type__: ClassVar[type]
}
class cast {
+cast(type_: type, obj: object) StrawberryCastObj
+cast(type_: type, obj: None) None
}
class Node {
<<interface>>
+resolve_node()
+is_type_of()
}
StrawberryCastObj <|-- Node
note for StrawberryCastObj "New protocol for type-cast objects"
note for cast "New function to safely cast objects to their known type"
State diagram for node type resolution processstateDiagram-v2
[*] --> ReceiveNode
ReceiveNode --> CheckCast: Check if object is cast
CheckCast --> UseCastType: Has __as_strawberry_type__
CheckCast --> StandardResolution: No cast type
StandardResolution --> CheckObjectDefinition
CheckObjectDefinition --> UseOriginType: Has object definition
CheckObjectDefinition --> TypeCheck: No object definition
UseCastType --> [*]: Return correct type
UseOriginType --> [*]: Return type
TypeCheck --> [*]: Return type
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
patrick91
changed the title
strawberry.cast
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @bellini666 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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.
| from strawberry.types.cast import is_strawberry_cast_obj | ||
|
|
||
|
|
||
| def test_cast(): |
There was a problem hiding this comment.
suggestion (testing): Test casting with a Strawberry type
Add a test case where the object being cast is already a Strawberry type to ensure idempotency.
| if should_have_name: | ||
| assert result.data["node"]["name"] == "Strawberry" | ||
| else: | ||
| assert "name" not in result.data["node"] |
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
|
Thanks for adding the Here's a preview of the changelog: The common However, integrations such as Django, SQLAlchemy and Pydantic will not return In case there are more than one possible type defined for that model that is In here we are introducing a new That Here's the tweet text: |
bellini666
force-pushed
the
is_type_of_security_issue
branch
from
eab7f34 to
a3fd1b8
Compare
CodSpeed Performance ReportMerging #3749 will not alter performanceComparing Summary
|
…th multiple possibilities The common `node: Node` used to resolve relay nodes means we will be relying on `is_type_of` to check if the returned object is in fact a subclass of the `Node` interface. However, integrations such as Django, SQLAlchemy and Pydantic will not return the type itself, but instead an alike object that is later resolved to the expected type. In case there are more than one possible type defined for that model that is being returned, the first one that replies `True` to `is_type_of` check would be used in the resolution, meaning that when asking for `PublicUser:123`, strawberry could end up returning `User:123`, which can lead to security issues (such as data leakage). In here we are introducing a new `strawberry.cast`, which will be used to mark an object with the already known type by us, and when asking for `is_type_of` that mark will be used to check instead, ensuring we will return the correct type.
bellini666
force-pushed
the
is_type_of_security_issue
branch
from
0d39a0c to
fd019d3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3749 +/- ##
==========================================
- Coverage 97.28% 97.24% -0.04%
==========================================
Files 502 504 +2
Lines 33375 33461 +86
Branches 5477 5498 +21
==========================================
+ Hits 32469 32540 +71
- Misses 697 703 +6
- Partials 209 218 +9 |
The common
node: Nodeused to resolve relay nodes means we will be relying onis_type_ofto check if the returned object is in fact a subclass of theNodeinterface.However, integrations such as Django, SQLAlchemy and Pydantic will not return the type itself, but instead an alike object that is later resolved to the expected type.
In case there are more than one possible type defined for that model that is being returned, the first one that replies
Truetois_type_ofcheck would be used in the resolution, meaning that when asking forPublicUser:123, strawberry could end up returningUser:123, which can lead to security issues (such as data leakage).In here we are introducing a new
strawberry.cast, which will be used to mark an object with the already known type by us, and when asking foris_type_ofthat mark will be used to check instead, ensuring we will return the correct type.Summary by Sourcery
Bug Fixes: