Input value defaulting behaviour is wrong

[meiamsome]

Describe the bug The current behaviour for default values does not align with the GraphQL spec, or the reference implementation:

1. For a non-null field with a default value, it should be possible to override the value with a null value:

   # Schema
   type Query {
   test(arg: Int = 100): Int
   }
   # Query
   query {
   test(arg: null)
   }

   Currently Juniper takes the default value in this case.

2. A similar issue applies for variables:

   query ($arg: Int) {
   test(arg: $arg)
   }
   // or even with a variable default
   query ($arg: Int = 10) {
   test(arg: $arg)
   }

   Passing an explicit null as the value for $arg should pass through to the test resolver.

3. When declaring a non-optional field as having a default value, Juniper currently downgrades the type to non-null when adding the default. This incorrectly implies that an explicit null is allowable.

E.g.:

#[graphql_object]
impl Query {
    fn non_null_default(#[graphql(default = 100)] arg: i32) -> Option<i32> {
        Some(arg)
    }
}

Results in an exposed schema that is similar to this:

type Query {
  nonNullDefault(arg: Int = 100): Int
}

instead of:

type Query {
  nonNullDefault(arg: Int! = 100): Int
}

The GraphQL Spec

Under Input Coercion section of the spec, it is specified that the default value should only apply if no value is provided:

If no value is provided for a defined input object field and that field definition provides a default value, the default value should be used.

And additionally that an explicit null is considered semantically different to having not provided a value:

If the value null was provided for an input object field, and the field’s type is not a non-null type, an entry in the coerced unordered map is given the value null. In other words, there is a semantic difference between the explicitly provided value null versus having not provided a value.

And similarly for variables, the same is true:

If a variable is provided for an input object field, the runtime value of that variable must be used. If the runtime value is null and the field type is non-null, a field error must be raised. If no runtime value is provided, the variable definition’s default value should be used. If the variable definition does not provide a default value, the input object field definition’s default value should be used.

Comparison to the reference implementation

I have created a repo here with a suite of tests against both Juniper and graphql-js that can be used for comparison: https://github.com/meiamsome/juniper-default-value

The results of the comparision tests are as follows: graphql-js:

> jest index.test.js

 PASS  ./index.test.js
  ✓ default value (3 ms)
  ✓ literal number (1 ms)
  ✓ literal null
  ✓ variable field default value
  ✓ variable default value
  ✓ variable default value explicit null
  ✓ variable number
  ✓ variable null
  ✓ schema for default non null is non null (1 ms)

juniper:

running 9 tests
test test::variable_number ... ok
test test::variable_default_value ... ok
test test::default_value ... ok
test test::variable_field_default_value ... ok
test test::literal_null ... FAILED
test test::variable_null ... FAILED
test test::literal_number ... ok
test test::variable_default_value_explicit_null ... FAILED
test test::schema_for_default_non_null_is_non_null ... FAILED

failures:

---- test::literal_null stdout ----
thread 'test::literal_null' panicked at 'assertion failed: `(left == right)`
  left: `Some(100)`,
 right: `None`', src/lib.rs:82:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- test::variable_default_value_explicit_null stdout ----
thread 'test::variable_default_value_explicit_null' panicked at 'assertion failed: `(left == right)`
  left: `Some(100)`,
 right: `None`', src/lib.rs:146:9

---- test::variable_null stdout ----
thread 'test::variable_null' panicked at 'assertion failed: `(left == right)`
  left: `Some(100)`,
 right: `None`', src/lib.rs:190:9

---- test::schema_for_default_non_null_is_non_null stdout ----
thread 'test::schema_for_default_non_null_is_non_null' panicked at 'assertion failed: `(left == right)`
  left: `Object(Object { key_value_list: {"__schema": Object(Object { key_value_list: {"queryType": Object(Object { key_value_list: {"fields": List([Object(Object { key_value_list: {"name": Scalar(String("test")), "args": List([Object(Object { key_value_list: {"name": Scalar(String("arg")), "type": Object(Object { key_value_list: {"name": Scalar(String("Int")), "kind": Scalar(String("SCALAR")), "ofType": Null} })} })])} }), Object(Object { key_value_list: {"name": Scalar(String("nonNullDefault")), "args": List([Object(Object { key_value_list: {"name": Scalar(String("arg")), "type": Object(Object { key_value_list: {"name": Scalar(String("Int")), "kind": Scalar(String("SCALAR")), "ofType": Null} })} })])} })])} })} })} })`,
 right: `Object(Object { key_value_list: {"__schema": Object(Object { key_value_list: {"queryType": Object(Object { key_value_list: {"fields": List([Object(Object { key_value_list: {"name": Scalar(String("test")), "args": List([Object(Object { key_value_list: {"name": Scalar(String("arg")), "type": Object(Object { key_value_list: {"name": Scalar(String("Int")), "kind": Scalar(String("SCALAR")), "ofType": Null} })} })])} }), Object(Object { key_value_list: {"name": Scalar(String("nonNullDefault")), "args": List([Object(Object { key_value_list: {"name": Scalar(String("arg")), "type": Object(Object { key_value_list: {"name": Null, "kind": Scalar(String("NON_NULL")), "ofType": Object(Object { key_value_list: {"name": Scalar(String("Int")), "kind": Scalar(String("SCALAR"))} })} })} })])} })])} })} })} })`', src/lib.rs:301:9

failures:
    test::literal_null
    test::schema_for_default_non_null_is_non_null
    test::variable_default_value_explicit_null
    test::variable_null

test result: FAILED. 5 passed; 4 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

[tyranron]

@meiamsome thanks!

[LegNeato]

I hope to have time to look at this this week, but I wanted to drop by and say this is an amazing bug report, thank you!

[tyranron]

@LegNeato it's OK, we have this on @ilslv's schedule. He will address this in a near time.

It also makes sense to port all tests from graphql-js before releasing 0.16.0. I guess we have much more cases of spec diversion.

[tyranron]

@meiamsome thank you very much for bringing this up! Hope, with the #1080 being merged, this is fixed finally.