-
Notifications
You must be signed in to change notification settings - Fork 135
Refactoring: not to use global rule sets (part 1) #379
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
Conversation
I am a huge fan of this work. I deeply dislike global variables, and really appreciate you spending the time to work on this. I noticed that your part 2 has some test results that are failing, so I'm hesitant to merge this PR until that one appears ready, as I would like both to be included in the same release. |
Thank you. I have fixed the issue in the test code. With my PR change, the Rules are now stored in a map, which means the order of elements is no longer guaranteed. For these reasons, I have determined that this change will not cause any issues. It seems golangci-lint is failing because of a deprecated tag I added. I see three options: remove the deprecated tag, add a nolint directive, or ignore it. Which approach do you prefer? |
I would remove the deprecated tag for now, if that's ok. We can revisit that in a follow-up. |
Honestly, that might be sort of silly. A nolint directive would be better. That way we more clearly signal to library user's our commitment to a direction forward. |
OK, I'll soon modify them when I got home. |
I've fixed the lint errors. This commit has made CI pass. Please review. |
Description
Hello. I'm a regular user and a big fan of this wonderful library.
I would like to propose an improvement.
The Current Problem
Currently, the validation
Rule
s for GraphQL are managed in a global scope shared across the entire library. However, with the recent addition of features likeReplace Rule
, these global rules can now be modified at runtime.This implementation can cause the following issues in a concurrent environment:
Proposed Solution
As a first step to resolve this, I would like to refactor the way rules are managed. Specifically, I propose to stop managing rules with a global variable and instead introduce a new, dedicated
Rules
struct to handle them.This will allow each validation process to have its own isolated set of rules, making it safe to manipulate them in a concurrent environment.
Changes in this Pull Request
As the first step toward implementing the solution above, this pull request introduces the following changes:
Rules
struct.core
directory and places theRules
struct there to avoid package import cycles.Future Plans
Once this pull request is successfully merged, the next step is to add new
Validate
andLoadQuery
functions that do not rely on the global rules. To maintain backward compatibility, these will be provided as new, separate functions.Thank you for your consideration.
relates to
#380
I have: