-
-
Notifications
You must be signed in to change notification settings - Fork 509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend query depth limiter allowing for more detailed rules #2505
Conversation
Thanks for adding the Here's a preview of the changelog: This release introduces the new The
Instead, the user should write business logic to determine whether a field should be ignored or not by For example, the following query: """
query {
matt: user(name: "matt") {
email
}
andy: user(name: "andy") {
email
address {
city
}
pets {
name
owner {
name
}
}
}
}
""" can have its depth limited by the following from strawberry.extensions import IgnoreContext
def should_ignore(ignore: IgnoreContext):
return ignore.field_args.get("name") == "matt"
query_depth_limiter = QueryDepthLimiter(should_ignore=should_ignore) so that it effectively becomes: """
query {
andy: user(name: "andy") {
email
pets {
name
owner {
name
}
}
}
}
""" Here's the tweet text:
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2505 +/- ##
==========================================
+ Coverage 93.60% 96.45% +2.85%
==========================================
Files 198 198
Lines 8177 8244 +67
Branches 1478 1500 +22
==========================================
+ Hits 7654 7952 +298
+ Misses 379 184 -195
+ Partials 144 108 -36 |
Linking the previous review here: #2501 (comment) 😁 Edit: @patrick91, thanks for flagging that missing test! It identified a subtle bug within the validation logic! |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…tributeRuleType, specify that FieldArgumentsRuleType is a dict of FieldAttributeRuleType
@patrick91, this PR is in a state to be reviewed :D Edit: anybody? 😭 @DoctorJohn @BryceBeagle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you tell me more about this feature? what's your use case?
Also in terms of API, I wonder if we could use callables instead of field rules. I think that might make our implementation shorter, and potentially the api could be easier to use 🤔
The use case is that described in #2338, which allows to limit the depth of queries depending on the arguments of specific fields in addition to the names of said fields. Let's have a discussion in Discord on the implementation detail of callables instead of FieldRules as I'm not 100% clear on how that would look in the code 😁 I had also hoped to limit based on the sub-fields of a specific field, but this wasn't doable with my implementation. Perhaps callables would help here! |
@patrick91, this PR has been refactored to take into account the callables discussion we had previously. If you have time to review it then it should be ready to merge! |
) | ||
validator = create_validator(max_depth, should_ignore, callback) | ||
else: | ||
warnings.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for adding the deprecation 😊
FieldArgumentType = Union[ | ||
bool, int, float, str, List["FieldArgumentType"], Dict[str, "FieldArgumentType"] | ||
] | ||
FieldArgumentsType = Dict[str, FieldArgumentType] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might want to move these out at some point, or reuse something classes we already have
Enhances the query depth limiter by providing options to specify additional rules for fields within the GraphQL query.
These can be the field name on its own or including the field arguments.
Description
Exposes the new
should_ignore
argument toQueryDepthLimiter
that takes a single argument of typeIgnoreContext
allowing the user greater verbosity when defining their rules. This dataclass can be extended to provide more ignore options by future PRs in response to further feature requests driven by specific use cases.This functionality does not replace previous functionality but simply extends it. So you can still use the
QueryDepthLimiter
extension as it was previously implemented but now you can also use the further verbose functionality. However, the previous implementation is deprecated and will be removed in future releases.Types of Changes
Issues Fixed or Closed by This PR
Checklist