Repository navigation
Conversation
Test ReportTest Results
Code Coverage (Java 25)
Changed Class Coverage (1 class)
|
| if (variableValue == null && isNonNull(type)) { | ||
| throw new NonNullableValueCoercedAsNullException(type); | ||
| } | ||
| return variableValue; |
There was a problem hiding this comment.
Ye old nullability on variables - this has long been a challenge around is a value present but null versus null because its absent.
I am not sure this is correct or that we can tell here if a value is present and was set explicitly to null versus being null
We may have to use graphql.execution.CoercedVariables#containsKey to distinguish
I would like @andimarek to comment on this PR
There was a problem hiding this comment.
fair concern, but i don't think the distinction changes the outcome at either site.
externalValueToInternalValueForVariables only puts a key into CoercedVariables when the variable was supplied (explicit null included) or has a default, so get() returning null here means either an explicit null or an absent variable with no default. for a non-null target both are invalid: graphql-js valueFromAST rejects a missing variable in a non-null list item the same way it rejects an explicit null. the one place absent has to behave differently is an input field with a default, and that branch already goes through containsKey (hasValue) and runs before the line i changed, so an absent variable still picks up the field default.
i ran the combinations against this branch to check:
query($v: String = "x") { list(arg: [$v]) } no variables -> [x]
query($v: String = "x") { list(arg: [$v]) } {"v": null} -> Coerced Null value for NonNull type 'String!'
query($v: String) { nullableItems(arg: [$v]) } no variables -> [null] (unchanged)
query($v: String) { nullableItems(arg: [$v]) } {"v": null} -> [null] (unchanged)
query($v: String) { obj(arg: {a: $v}) } a: String! = "d" no variables -> [a:d]
query($v: String) { obj(arg: {a: $v}) } a: String! = "d" {"v": null} -> Field 'a' has coerced Null value for NonNull type 'String!'
an absent variable without a default can only reach the list item check with validation bypassed (a nullable variable with no default isn't allowed in a [String!] position, and a non-null variable fails variable coercion first), and rejecting it there still matches graphql-js. happy to switch the list item check to containsKey if you'd rather see the absent case spelled out, but the result would be the same.
with
type Query { f(arg: [String!]!): String }, a nullable variable with a default is allowed in the non-null list item position (IsVariableUsageAllowed), so this passes validation:ValuesResolverConversion.literalToInternalValueImplreturnscoercedVariables.get(name)for a variable reference without looking at the target type, so the explicit null ends up inside a[String!]!list. same inside input objects,{items: [$v]}foritems: [String!]. the input object field case,f(arg: {a: $v})witha: String!, reachesassertShouldNeverHappeninstead:both now throw
NonNullableValueCoercedAsNullException, which is what the argument level and variables map paths already do. graphql-jsvalueFromASTrejects the same input (variableValue === null && isNonNullType(type)). the two newValuesResolverTestcases fail before the change.