BREAKING: Fix AddRule and ReplaceRule methods behavior and add documentation
#381
Conversation
- AddRule now checks if rule already exists before adding - ReplaceRule now only replaces existing rules
Rules methods behavior and add documentation
Rules methods behavior and add documentationAddRule and ReplaceRule methods behavior and add documentation
|
After this PR the names of the functions more accurately describe the (new) implementation's behavior. There is now no direct equivalent of the old implementation which might have been better described as "AddOrReplaceRule"? I'm not sure if adding such a method would help anyone. I'm also having some trouble updating gqlgen to incorporate your recent changes to gqlparser and getting the tests to pass over in 99designs/gqlgen#3751 This appears to be due to data races. I don't think in normal usage people are likely to be making many changes to the rules, so I wonder if switching to using a |
|
Thank you for creating the pull request. I feel that the responsibility for ensuring safe execution in a concurrent environment should lie with the user of gqlparser. Therefore, instead of making the Rules struct internally thread-safe with the sync package, I believe the user should be responsible for handling concurrency safely on their end. This is because gqlparser's concern is limited to parsing and validating GraphQL queries, not how they are executed. I was taking a look, and when I applied the following changes, the diff --git a/graphql/executor/executor.go b/graphql/executor/executor.go
index 6acaa325..8d6beceb 100644
--- a/graphql/executor/executor.go
+++ b/graphql/executor/executor.go
@@ -230,13 +230,13 @@ func (e *Executor) parseQuery(
// swap out the FieldsOnCorrectType rule with one that doesn't provide suggestions
if e.disableSuggestion {
- validator.RemoveRule("FieldsOnCorrectType")
+ currentRules.RemoveRule("FieldsOnCorrectType")
rule := rules.FieldsOnCorrectTypeRuleWithoutSuggestions
- validator.AddRule(rule.Name, rule.RuleFunc)
+ currentRules.AddRule(rule.Name, rule.RuleFunc)
} else { // or vice versa
validator.RemoveRule("FieldsOnCorrectTypeWithoutSuggestions")
rule := rules.FieldsOnCorrectTypeRule
- validator.AddRule(rule.Name, rule.RuleFunc)
+ currentRules.AddRule(rule.Name, rule.RuleFunc)
}
listErr := validator.ValidateWithRules(e.es.Schema(), doc, currentRules)What are your thoughts on this? |
|
Thanks for looking at the gqlgen PR and seeing my mistake. 🤦 I was just too sleep deprived to see what was staring me in the face. 😅 It may still be useful to ensure validation rules are always listed and applied in a deterministic order, otherwise it may cause CI failures in downstream projects that are difficult to diagnose. |
Description
This PR modifies the
Rulesstruct in the validator package by fixing a minor behavioral bug in the rule management methods.I partially implemented AddRule and ReplaceRule methods incorrectly. I apologize for the oversight in my previous implementation.
Changes
AddRule: Now If a rule with the same name already exists, it will not be added.ReplaceRule: Now If no rule with the specified name exists, it does nothing.Relates to
#379
#380