Incorrect nullability arguments validation

[olegrok]

On top of a2fc502. The main part is a test. Change in validation rule should be discussed and reviewed.

diff --git a/graphql/rules.lua b/graphql/rules.lua
index 184a80d..274ed69 100644
--- a/graphql/rules.lua
+++ b/graphql/rules.lua
@@ -199,13 +199,19 @@ function rules.uniqueArgumentNames(node, _)
   end
 end
 
+local function is_scalar_argument(kind)
+  return kind ~= 'variable' and kind ~= 'list' and kind ~= 'inputObject'
+end
+
 function rules.argumentsOfCorrectType(node, context)
   if node.arguments then
     local parentField = getParentField(context, node.name.value)
     for _, argument in pairs(node.arguments) do
       local name = argument.name.value
+      local kind = argument.value.kind
       local argumentType = parentField.arguments[name]
-      util.coerceValue(argument.value, argumentType.kind or argumentType)
+      local options = {strict_non_null = is_scalar_argument(kind)}
+      util.coerceValue(argument.value, argumentType.kind or argumentType, nil, options)
     end
   end
 end
diff --git a/test/unit/graphql_test.lua b/test/unit/graphql_test.lua
index 1e10a9a..9ec5fc9 100644
--- a/test/unit/graphql_test.lua
+++ b/test/unit/graphql_test.lua
@@ -1056,4 +1056,6 @@ function g.test_boolean_coerce()
             validate, test_schema, parse([[ { test_boolean(value: 123) } ]]))
     t.assert_error_msg_contains('Could not coerce value "value" with type "string" to type boolean',
             validate, test_schema, parse([[ { test_boolean(value: "value") } ]]))
+    t.assert_error_msg_contains('Expected non-null for "NonNull(Boolean)", got null',
+            validate, test_schema, parse([[ { test_boolean(value: null, non_null_value: null) } ]]))
 end

[Totktonada]

It looks counter-intuitive to depend on whether a type is scalar or composite in the decision, whether to perform nullability validation.

My first thought how it may look was the following:

diff --git a/graphql/rules.lua b/graphql/rules.lua
index c902aca..3f56a58 100644
--- a/graphql/rules.lua
+++ b/graphql/rules.lua
@@ -204,7 +204,8 @@ function rules.argumentsOfCorrectType(node, context)
     for _, argument in pairs(node.arguments) do
       local name = argument.name.value
       local argumentType = parentField.arguments[name]
-      util.coerceValue(argument.value, argumentType.kind or argumentType)
+      util.coerceValue(argument.value, argumentType.kind or argumentType, nil, {
+          strict_non_null = true, skip_variables = true})
     end
   end
 end
diff --git a/graphql/util.lua b/graphql/util.lua
index 5347d0b..b5e5d5e 100644
--- a/graphql/util.lua
+++ b/graphql/util.lua
@@ -83,8 +83,13 @@ local function coerceValue(node, schemaType, variables, opts)
   variables = variables or {}
   opts = opts or {}
   local strict_non_null = opts.strict_non_null or false
+  local skip_variables = opts.skip_variables or false
   local defaultValues = opts.defaultValues or {}
 
+  if node and node.kind == 'variable' and skip_variables then
+      return nil
+  end
+
   if schemaType.__type == 'NonNull' then
     local res = coerceValue(node, schemaType.ofType, variables, opts)
     if strict_non_null and res == nil then
diff --git a/test/unit/graphql_test.lua b/test/unit/graphql_test.lua
index c6d9f2b..ea4491f 100644
--- a/test/unit/graphql_test.lua
+++ b/test/unit/graphql_test.lua
@@ -1057,6 +1057,8 @@ function g.test_boolean_coerce()
             validate, test_schema, parse([[ { test_boolean(value: 123) } ]]))
     t.assert_error_msg_contains('Could not coerce value "value" with type "string" to type boolean',
             validate, test_schema, parse([[ { test_boolean(value: "value") } ]]))
+    t.assert_error_msg_contains('Expected non-null for "NonNull(Boolean)", got null',
+            validate, test_schema, parse([[ { test_boolean(value: null, non_null_value: null) } ]]))
 end
 
 function g.test_util_map_name()

It passes all tests. I verified it on top of 0.1.4.

However really we should verify types of variables in a query against variables in the schema rather than calling util.coerceValue(). It is just different thing for different purpose. It is not surprising that such way to validate types works partially.

[no1seman]

Seems that fix resolves the issue not only for arguments but also for variables.

[DifferentialOrange]

Closed by #64, #63 and #62