Skip to content

Avoid crash when oneOf input variable is not defined #4445

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

Closed
wants to merge 1 commit into from

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Jul 1, 2025

CC @benjie should this be added to the spec?

Fixes #4443

@JoviDeCroock JoviDeCroock requested a review from a team as a code owner July 1, 2025 06:30
@JoviDeCroock JoviDeCroock force-pushed the spec-conformity-allowed-usage branch from cd55555 to 8a16e93 Compare July 2, 2025 07:33
@JoviDeCroock JoviDeCroock added the PR: bug fix 🐞 requires increase of "patch" version number label Jul 2, 2025
Comment on lines +220 to +224
// If the variable definition is missing, skip validation here.
// This should be caught by other validation rules.
if (!definition) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically "Values of Correct Type" in the spec only applies to literals ("For each literal Input Value {value} in the document:"), what you're actually dealing with here is maybe "All Variable Usages Are Allowed"?

But yes, I think in general the spec probably needs all of its validation rules to say "if doesn't exist, return" where that situation has already been covered by a different validation rule explicitly checking that X exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the spec, it looks like we touched up where the validation happened around the time of benjie/graphql-spec#1

In v17, it looks like we accomplished this in #4194

In v16, it looks like we touched up VariablesInAllowedPositions by #4363

So what remains to be done in v16 is to remove the variable bits entirely from ValuesOfCorrectType (created a branch to test around with this: https://github.com/yaacovCR/graphql-js/tree/alternative)

Pain points of multiple long-running development branches.... grumble, grumble

JoviDeCroock added a commit that referenced this pull request Jul 7, 2025
Supersedes #4445
Fixes #4443

`VariablesInAllowedPositionRule` already validates this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validate throws a TypeError: Cannot read properties of undefined (reading 'type')
3 participants