Inconsistent behavior for defaultValue and nullable on input types

[backbone87]

Describe the Bug
A field in an input type or argument list is set to nullable: true, if a default is provided, even when the global nullable is false. Explicitly specifying nullable: false results in a definition error.

On the behavior regarding argument lists and input types with default values and nullable: graphql/graphql-spec#570

I mentioned this in #498 (comment), but created this as a separate issue since the linked issue also argues about object types (while this one is only about input types) and to increase visibility.

To Reproduce

@InputType()
class MyInput {
  // definition error, but should be `foo: Int! = 0`
  @Field(() => Int, { nullable: false; defaultValue: 0 })
  public foo!: number;

  // with global nullable: false results in `foo: Int = 0`, but should be `foo: Int! = 0`
  @Field(() => Int, { defaultValue: 0 })
  public foo!: number;
}

Expected Behavior

Lets a asume a string argument or input field:

• nullable: true (type-graphql behaves correctly)
  • omitted: resolver gets undefined (this is a "legacy" behavior of graphql-js)
  • passed null: resolver gets null
  • passed a string: resolver gets the string
• nullable: false (type-graphql behaves correctly) (this is the only case where a value must be passed when querying)
  • omitted: error
  • passed null: error
  • passed a string: resolver gets the string
• nullable: true, defaultValue: null (type-graphql behaves correctly)
  • omitted: resolver gets null
  • passed null: resolver gets null
  • passed a string: resolver gets the string
• nullable: true, defaultValue: 'foo' (type-graphql behaves correctly)
  • omitted: resolver gets string "foo"
  • passed null: resolver gets null
  • passed a string: resolver gets the string
• nullable: false, defaultValue: null: invalid definition (type-graphql behaves correctly)
• nullable: false, defaultValue: 'foo' (type-graphql behaves incorrectly: if nullable: false is set explicitly, then type-graphql incorrectly rejects the definition as invalid. if nullable: false is set implicitly through global setting, then type-graphql silently converts the field to a nullable: true field. both cases are incorrect behavior.)
  • omitted: resolver gets string "foo"
  • passed null: error
  • passed a string: resolver gets the string

[MichalLytek]

results in a definition error.

What do you mean? A runtime error while building the schema? An explicitly named error is thrown? Every query results in error? Or the definition is not matching your expectation?

should be foo: Int! = 0

It doesn't make sense. What is the purpose of default value if the field is required and you can't provide null value or omit the arg/field?

TypeGraphQL always convert the field to nullable when the defaultValue is provided for convenience, as otherwise it makes no sense.

[backbone87]

A runtime error while building the schema?

this happens. thrown by type-graphql itself at

type-graphql/src/helpers/types.ts

Line 57 in b45961a

throw new ConflictingDefaultWithNullableError(

It doesn't make sense. What is the purpose of default value if the field is required and you can't provide null value or omit the arg/field?

required is the wrong semantic here. nullable config does not make a direct statement about whether or not a client is required to provide a value. nullable config is about what the resolver is expected to get. whether or not the client has to provide a value (semantic: required value) depends on the configuration of nullable and default value. see also the discussion linked in graphql/graphql-spec#570

example: what is the semantic of nullable: false, defaultValue: 'foo' for a string field/arg?

• the resolver always gets a string, since not nullable
• the query issuer may not provide a value, since default value is available
• if the query issuer does provide a value, it must be a string, since not nullable

example: what is the semantic of nullable: true, defaultValue: 'foo' for a string field/arg?

• the resolver gets a string or null, since nullable
• the query issuer may not provide a value, since default value is available
• if the query issuer does provide a value, it must be a string or null, since nullable

example: what is the semantic of nullable: false for a string field/arg?

• the resolver gets a string, since not nullable
• the query issuer must provide a value and it must be a string, since not nullable and no default value is available

conclusion:

• what the resolver is about to expect depends only on nullable
• what the query issuer is expected to provide depends on nullable and defaultValue

[backbone87]

@MichalLytek can we tackle this? can i help in any way?

[MichalLytek]

@backbone87 Sorry, I'm really busy recently 😞

Looks like you're right, I haven't dig enough into GraphQL spec to understand how it works - I thought it works like the (param: type = value) default params for TypeScript.

The fix shouldn't be too hard, if you want to help - please create a PR with fixes on the tests side, so they have proper assertions and should be red 😉

[backbone87]

regarding implementation, should we introduce a flag to opt into this "corrected behavior", since changing this most likely affects a lot of users.

[MichalLytek]

I think it's not needed in that case. Every bug fix in technically a breaking change as someone could rely on the incorrect behavior of the library. GraphQL specification is the source of truth so if something works in a different way, it has to be changed.

In that case minor release with a proper note in changelog should be sufficient. It's not a drastic change and should not affect normal cases. It would be also reported in emitted schema file, so should be fairly easy to spot.

[MichalLytek]

@backbone87 Please review #806 as I need confirmation if it works now according to the behavior you've described 😉

[ericwooley]

regarding implementation, should we introduce a flag to opt into this "corrected behavior", since changing this most likely affects a lot of users.

I think it's not needed in that case. Every bug fix in technically a breaking change as someone could rely on the incorrect behavior of the library. GraphQL specification is the source of truth so if something works in a different way, it has to be changed.

In that case minor release with a proper note in changelog should be sufficient. It's not a drastic change and should not affect normal cases. It would be also reported in emitted schema file, so should be fairly easy to spot.

I understand that this is the way it was supposed to be, but this is definitely messing us up pretty good. For others following in the upgrade, a flag to keep it the same would be amazing. This change results in around 700 changes in our schema, which causes several hundred typescript breaks in our frontend (graphql-code-generator) and thats a lot to fix all at once. Ideally, we could upgrade 1.x to 2.x while keeping the actual schema the same.

Too late for me, as I am just going to tackle all these changes so we can get this upgrade done, but I do think it would be very helpful to others.