BREAKING: Fix AddRule and ReplaceRule methods behavior and add documentation

[tomoikey]

Description

This PR modifies the Rules struct 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

• Add some docs
• 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

---

• Added tests covering the bug / feature
• Updated any relevant documentation

[coveralls]

coverage: 87.336% (-0.03%) from 87.364%
when pulling 69d2633 on tomoikey:fix/rules
into 22fa045 on vektah:master.

[StevenACoffman]

After this PR the names of the functions more accurately describe the (new) implementation's behavior.
However, I think the change should be highlighted in a way that for anyone who relied on the old 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 sync.Map as described in sync.Map might be a good precaution?

[tomoikey]

@StevenACoffman

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 go test ./... command passed on my local environment.
https://github.com/99designs/gqlgen/pull/3751/files

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?
If I've misunderstood anything about this issue, please don't hesitate to correct me.

[StevenACoffman]

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.