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

Allow nullable variables for non-null arguments that have defaults; allow scalar variable to list types #3662

Merged
merged 19 commits into from
Oct 16, 2023

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Jul 11, 2023

See:

Sample query which would fail validation:

query ($arg: String) {
  dummy(dummyArg: $arg)
}

where dummyArg was defined as [String]. However, these scenarios worked correctly:

query {
  dummy(dummyArg: "test")
}

and

query ($arg: [String]) {
  dummy(dummyArg: $arg)
}

where variables were:

{
  "arg": "test"
}

Now the original query at the top passes validation and works as described in the GraphQL spec.

Note that the spec is slightly in conflict with the description of rule All Variable Usages Are Allowed.

See:

This PR also fixes some out-of-spec behavior during coercion of arguments that have default values:

# this should pass validation, providing that null is not provided to the arg variable
query ($arg: String = "abc") {
  dummy (nonNullArg: $arg)
}

# also this should pass validation, providing that null is not provided to the arg variable
query ($arg: String) {
  dummy (nonNullArgWithDefaultValue: $arg)
}

# also this should pass validation, providing that null is not provided to the arg variable
query ($arg: String = "abc") {
  dummy (inputArg: { nonNullValue: $arg })
}

# also this should pass validation, providing that null is not provided to the arg variable
query ($arg: String) {
  dummy (inputArg: { nonNullValueWithDefaultValue: $arg })
}

And I added a lot more tests to cover various of these scenarios.

@github-actions github-actions bot added the test Pull request that adds new or changes existing tests label Jul 11, 2023
@Shane32
Copy link
Member Author

Shane32 commented Jul 11, 2023

Todo: double check we have tests that ensure that type validation disallows [Int] matching [[Int]], either as literal, or variable, or with this new feature.

@Shane32 Shane32 added enhancement New feature or request and removed test Pull request that adds new or changes existing tests labels Jul 11, 2023
Comment on lines +107 to +108
{
}

Check warning

Code scanning / CodeQL

Empty branch of conditional, or empty loop body Warning

Empty block without comment.
Comment on lines +328 to +338
if (sup.ResolvedType is NonNullGraphType sup2)
{
// >> - Let {nullableItemLocationType} be the unwrapped nullable type of {itemLocationType}.
// >> - Return {AreTypesCompatible(variableType, nullableItemLocationType)}.
return IsSubtypeOf(maybeSubType, sup2);
}
else
{
// >> - Otherwise, return {AreTypesCompatible(variableType, itemLocationType)}.
return IsSubtypeOf(maybeSubType, sup.ResolvedType!, allowScalarsForLists);
}

Check notice

Code scanning / CodeQL

Missed ternary opportunity Note

Both branches of this 'if' statement return - consider using '?' to express intent better.
@Shane32 Shane32 self-assigned this Oct 10, 2023
@Shane32 Shane32 added this to the 7.7 milestone Oct 10, 2023
@Shane32
Copy link
Member Author

Shane32 commented Oct 11, 2023

Todo: fix final failing test to prevent allowing null for a non-null input object field

@Shane32 Shane32 changed the title Allow single variable to list types Allow nullable variables for non-null arguments that have defaults; allow scalar variable to list types Oct 14, 2023
Comment on lines +295 to +298
[InlineData("query q10 { dummyList(arg: null) }", null,
"Argument 'arg' has invalid value. Expected '!', found null.")]
[InlineData("query q11 { dummyNestedList(arg: null) }", null,
"Argument 'arg' has invalid value. Expected '!', found null.")]
Copy link
Member Author

Choose a reason for hiding this comment

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

These error messages could be cleaned up.

@codecov-commenter
Copy link

Codecov Report

Merging #3662 (3960020) into master (86a3d71) will increase coverage by 0.09%.
Report is 6 commits behind head on master.
The diff coverage is 92.80%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #3662      +/-   ##
==========================================
+ Coverage   84.14%   84.24%   +0.09%     
==========================================
  Files         409      410       +1     
  Lines       18234    18523     +289     
  Branches     2856     2909      +53     
==========================================
+ Hits        15343    15604     +261     
- Misses       2215     2236      +21     
- Partials      676      683       +7     
Files Coverage Δ
src/GraphQL/ExperimentalFeatures.cs 100.00% <100.00%> (ø)
src/GraphQL/Validation/TypeInfo.cs 97.64% <100.00%> (+0.10%) ⬆️
...aphQL/Validation/ValidationContext.GetVariables.cs 98.48% <100.00%> (+0.48%) ⬆️
src/GraphQL/Validation/ValidationContext.cs 89.73% <100.00%> (+0.53%) ⬆️
src/GraphQL/Validation/VariableUsage.cs 64.28% <100.00%> (-35.72%) ⬇️
src/GraphQL/Execution/ExecutionHelper.cs 94.11% <96.00%> (+2.65%) ⬆️
...s/5.8 - Variables/5. VariablesInAllowedPosition.cs 92.45% <92.85%> (+1.88%) ⬆️
src/GraphQL/Validation/Variable.cs 73.68% <71.42%> (-26.32%) ⬇️
src/GraphQL/Validation/Variables.cs 64.86% <77.77%> (-5.14%) ⬇️
src/GraphQL/Extensions/GraphQLExtensions.cs 76.30% <82.35%> (-0.26%) ⬇️

... and 6 files with indirect coverage changes

@Shane32
Copy link
Member Author

Shane32 commented Oct 14, 2023

@sungam3r This PR contains bug fixes for type validation of variables:

  • When a non-null argument (or input object field) has a default value, the variable type should allow a nullable type -- except with the null value is passed as the variable.
  • And, if the value is not specified (with no variable default), it should use the argument default.

It also allows passing String variables to [String] arguments, but since this technically is not part of the spec, it is locked behind an experimental feature flag. See:

I will merge this shortly unless you have comments.

@Shane32 Shane32 merged commit f50aa46 into master Oct 16, 2023
10 checks passed
@Shane32 Shane32 deleted the variables_dont_coerce_properly branch October 16, 2023 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants