Skip to content
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

fix(utils): prevent race conditions when validating documents #5795

Merged
merged 2 commits into from
Jan 22, 2024

Conversation

shYkiSto
Copy link
Contributor

@shYkiSto shYkiSto commented Dec 27, 2023

Description

Make validateGqlDocuments() validation results more deterministic for cases when documents may include multiple anonymous definition nodes, where previously validateGraphQLDocuments() util would merge definitions in a way that created a risk for a race condition to take place depending on how fast the anonymous nodes get processed (definitions are keyed off Date.now().toString()), and blocked off certain validation rules.

This change essentially merges all definitions into one document as-is, with the exception of FRAGMENT_DEFINITION nodes, which get deduplicated based off their name. In turn this enables certain GraphQL rules, such as lone-anonymous-operation (spec), unique-operation-names (spec).

Note that there's no validation whether fragment nodes are the same, so there may be room for improvement to unblock unique-fragment-names (spec).

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as
    expected)
    • Potentially breaking change, since it will now throw an error if multiple definitions share the same name

Screenshots/Sandbox (if appropriate/relevant):

N/A

How Has This Been Tested?

Verified that a validation error is returned when multiple documents include an anonymous query.

Test Environment:

N/A

Checklist:

  • I have followed the
    CONTRIBUTING doc and the
    style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests and linter rules pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Further comments

N/A

Copy link

changeset-bot bot commented Dec 27, 2023

🦋 Changeset detected

Latest commit: 6360ffb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@graphql-tools/utils Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ardatan
Copy link
Owner

ardatan commented Jan 22, 2024

Thanks!

@ardatan ardatan merged commit f85c093 into ardatan:master Jan 22, 2024
27 checks passed
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.

2 participants