Skip to content

Conversation

tomoikey
Copy link
Contributor

@tomoikey tomoikey commented Jun 27, 2025

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 Rules for GraphQL are managed in a global scope shared across the entire library. However, with the recent addition of features like Replace Rule, these global rules can now be modified at runtime.

This implementation can cause the following issues in a concurrent environment:

  • Race conditions can occur, leading to unintended modifications of the rules.
  • As a result, the set of rules used for validation can become unpredictable, causing unstable behavior.

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:

  • Introduces the new Rules struct.
  • Creates a new core directory and places the Rules struct there to avoid package import cycles.

Future Plans

Once this pull request is successfully merged, the next step is to add new Validate and LoadQuery 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:

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

@coveralls
Copy link

coveralls commented Jun 27, 2025

Coverage Status

coverage: 87.41% (-0.3%) from 87.677%
when pulling 462cef3 on tomoikey:refactor/rules2
into 9e14766 on vektah:master.

@StevenACoffman
Copy link
Collaborator

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.

@tomoikey
Copy link
Contributor Author

tomoikey commented Jun 27, 2025

@StevenACoffman

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.
I considered the impact of this on the existing implementation and concluded it's not a problem. This is because the order was not guaranteed in the previous implementation either, as the Rules were added during the init process. Furthermore, each Rule is independent and has no dependencies on others.

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?

dda8e1b

@StevenACoffman
Copy link
Collaborator

I would remove the deprecated tag for now, if that's ok. We can revisit that in a follow-up.

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Jun 27, 2025

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.

@tomoikey
Copy link
Contributor Author

OK, I'll soon modify them when I got home.

@tomoikey
Copy link
Contributor Author

@StevenACoffman

I've fixed the lint errors. This commit has made CI pass. Please review.

6a9e550

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants