Repository: MichalLytek/type-graphql (ts)

Issue:
Inconsistent behavior for defaultValue and nullable on input types
**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: https://github.com/graphql/graphql-spec/issues/570

I mentioned this in https://github.com/MichalLytek/type-graphql/issues/498#issuecomment-723673596, 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**
```ts
@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


Pull request description:
Properly emit types nullability when `defaultValue` is provided 
Fixes #751

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8ed4fd89..85e59075 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@ ### Features
 - allow defining directives for interface types and theirs fields, with inheritance for object types fields (#744)
 - allow deprecating input fields and args (#794)
 ### Fixes
+- **Breaking Change**: properly emit types nullability when `defaultValue` is provided and remove `ConflictingDefaultWithNullableError` error (#751)
 - allow defining extension on field resolver level for fields also defined as a property of the class (#776)
 - fix throwing error when schema with dynamic default value was built again (#787)
 - fix converting inputs with fields of nested array type (#801)
diff --git a/src/errors/ConflictingDefaultWithNullableError.ts b/src/errors/ConflictingDefaultWithNullableError.ts
deleted file mode 100644
index febbbb4a..00000000
--- a/src/errors/ConflictingDefaultWithNullableError.ts
+++ /dev/null
@@ -1,17 +0,0 @@
-import { NullableListOptions } from "../decorators/types";
-
-export class ConflictingDefaultWithNullableError extends Error {
-  constructor(
-    targetName: string,
-    propertyName: string,
-    defaultValue: any,
-    nullable: boolean | NullableListOptions,
-  ) {
-    super(
-      `Wrong nullable option set for ${targetName}#${propertyName}. ` +
-        `You cannot combine default value '${defaultValue}' with nullable '${nullable}'.`,
-    );
-
-    Object.setPrototypeOf(this, new.target.prototype);
-  }
-}
diff --git a/src/errors/index.ts b/src/errors/index.ts
index 583960af..6aa10f91 100644
--- a/src/errors/index.ts
+++ b/src/errors/index.ts
@@ -3,7 +3,6 @@ export * from "./CannotDetermineGraphQLTypeError";
 export * from "./ForbiddenError";
 export * from "./GeneratingSchemaError";
 export * from "./ConflictingDefaultValuesError";
-export * from "./ConflictingDefaultWithNullableError";
 export * from "./InterfaceResolveTypeError";
 export * from "./InvalidDirectiveError";
 export * from "./MissingSubscriptionTopicsError";
diff --git a/src/helpers/types.ts b/src/helpers/types.ts
index ca43e14e..01ecaee3 100644
--- a/src/helpers/types.ts
+++ b/src/helpers/types.ts
@@ -12,7 +12,7 @@ import { TypeOptions } from "../decorators/types";
 import { GraphQLTimestamp } from "../scalars/timestamp";
 import { GraphQLISODateTime } from "../scalars/isodate";
 import { BuildContext } from "../schema/build-context";
-import { WrongNullableListOptionError, ConflictingDefaultWithNullableError } from "../errors";
+import { WrongNullableListOptionError } from "../errors";
 
 export function convertTypeIfScalar(type: any): GraphQLScalarType | undefined {
   if (type instanceof GraphQLScalarType) {
@@ -50,17 +50,6 @@ export function wrapWithTypeOptions<T extends GraphQLType>(
   ) {
     throw new WrongNullableListOptionError(target.name, propertyName, typeOptions.nullable);
   }
-  if (
-    typeOptions.defaultValue !== undefined &&
-    (typeOptions.nullable === false || typeOptions.nullable === "items")
-  ) {
-    throw new ConflictingDefaultWithNullableError(
-      target.name,
-      propertyName,
-      typeOptions.defaultValue,
-      typeOptions.nullable,
-    );
-  }
 
   let gqlType: GraphQLType = type;
 
@@ -73,10 +62,9 @@ export function wrapWithTypeOptions<T extends GraphQLType>(
   }
 
   if (
-    typeOptions.defaultValue === undefined &&
-    (typeOptions.nullable === false ||
-      (typeOptions.nullable === undefined && nullableByDefault === false) ||
-      typeOptions.nullable === "items")
+    typeOptions.nullable === false ||
+    (typeOptions.nullable === undefined && nullableByDefault === false) ||
+    typeOptions.nullable === "items"
   ) {
     gqlType = new GraphQLNonNull(gqlType);
   }


Test patch:
diff --git a/tests/functional/default-values.ts b/tests/functional/default-values.ts
index e1b392c3..0e52989a 100644
--- a/tests/functional/default-values.ts
+++ b/tests/functional/default-values.ts
@@ -1,5 +1,5 @@
 import "reflect-metadata";
-import { IntrospectionSchema } from "graphql";
+import { GraphQLSchema, IntrospectionSchema, printType } from "graphql";
 import {
   Arg,
   buildSchema,
@@ -46,4 +46,146 @@ describe("default values", () => {
       await expect(buildSchema({ resolvers: [sampleResolver] })).resolves.not.toThrowError();
     });
   });
+
+  describe("with nullable settings", () => {
+    describe("when `nullable: false` and defaultValue is provided", () => {
+      let schema: GraphQLSchema;
+
+      beforeEach(async () => {
+        getMetadataStorage().clear();
+        @InputType()
+        class SampleInput {
+          @Field({ defaultValue: "stringDefaultValue", nullable: false })
+          inputField: string;
+        }
+
+        @Resolver()
+        class SampleResolver {
+          @Query()
+          sampleQuery(@Arg("input") input: SampleInput): string {
+            return "sampleQuery";
+          }
+        }
+        schema = await buildSchema({ resolvers: [SampleResolver] });
+      });
+
+      it("should emit field of type non-nullable string with default value", async () => {
+        const sampleInputType = schema.getType("SampleInput")!;
+        const sampleInputSDL = printType(sampleInputType);
+
+        expect(sampleInputSDL).toMatchInlineSnapshot(`
+          "input SampleInput {
+            inputField: String! = \\"stringDefaultValue\\"
+          }"
+        `);
+      });
+    });
+
+    describe("when `nullable: true` and defaultValue is provided", () => {
+      let schema: GraphQLSchema;
+
+      beforeEach(async () => {
+        getMetadataStorage().clear();
+        @InputType()
+        class SampleInput {
+          @Field({ defaultValue: "stringDefaultValue", nullable: true })
+          inputField: string;
+        }
+
+        @Resolver()
+        class SampleResolver {
+          @Query()
+          sampleQuery(@Arg("input") input: SampleInput): string {
+            return "sampleQuery";
+          }
+        }
+        schema = await buildSchema({ resolvers: [SampleResolver] });
+      });
+
+      it("should emit field of type nullable string with default value", async () => {
+        const sampleInputType = schema.getType("SampleInput")!;
+        const sampleInputSDL = printType(sampleInputType);
+
+        expect(sampleInputSDL).toMatchInlineSnapshot(`
+          "input SampleInput {
+            inputField: String = \\"stringDefaultValue\\"
+          }"
+        `);
+      });
+    });
+
+    describe("when `nullableByDefault: true`", () => {
+      describe("when defaultValue is provided", () => {
+        let schema: GraphQLSchema;
+
+        beforeEach(async () => {
+          getMetadataStorage().clear();
+          @InputType()
+          class SampleInput {
+            @Field({ defaultValue: "stringDefaultValue" })
+            inputField: string;
+          }
+
+          @Resolver()
+          class SampleResolver {
+            @Query()
+            sampleQuery(@Arg("input") input: SampleInput): string {
+              return "sampleQuery";
+            }
+          }
+          schema = await buildSchema({
+            resolvers: [SampleResolver],
+            nullableByDefault: true,
+          });
+        });
+
+        it("should emit field of type nullable string with default value", async () => {
+          const sampleInputType = schema.getType("SampleInput")!;
+          const sampleInputSDL = printType(sampleInputType);
+
+          expect(sampleInputSDL).toMatchInlineSnapshot(`
+            "input SampleInput {
+              inputField: String = \\"stringDefaultValue\\"
+            }"
+          `);
+        });
+      });
+
+      describe("when `nullable: false` and defaultValue is provided", () => {
+        let schema: GraphQLSchema;
+
+        beforeEach(async () => {
+          getMetadataStorage().clear();
+          @InputType()
+          class SampleInput {
+            @Field({ defaultValue: "stringDefaultValue", nullable: false })
+            inputField: string;
+          }
+
+          @Resolver()
+          class SampleResolver {
+            @Query()
+            sampleQuery(@Arg("input") input: SampleInput): string {
+              return "sampleQuery";
+            }
+          }
+          schema = await buildSchema({
+            resolvers: [SampleResolver],
+            nullableByDefault: true,
+          });
+        });
+
+        it("should emit field of type non-nullable string with default value", async () => {
+          const sampleInputType = schema.getType("SampleInput")!;
+          const sampleInputSDL = printType(sampleInputType);
+
+          expect(sampleInputSDL).toMatchInlineSnapshot(`
+            "input SampleInput {
+              inputField: String! = \\"stringDefaultValue\\"
+            }"
+          `);
+        });
+      });
+    });
+  });
 });
diff --git a/tests/functional/resolvers.ts b/tests/functional/resolvers.ts
index 3f49af1c..a6cd43a1 100644
--- a/tests/functional/resolvers.ts
+++ b/tests/functional/resolvers.ts
@@ -45,7 +45,6 @@ import {
   PubSubEngine,
   ClassType,
   ConflictingDefaultValuesError,
-  ConflictingDefaultWithNullableError,
   WrongNullableListOptionError,
   createParamDecorator,
   CannotDetermineGraphQLTypeError,
@@ -406,13 +405,20 @@ describe("Resolvers", () => {
         expect(inputArgInnerType.name).toEqual("SampleInput");
       });
 
-      it("should generate nullable string arg type with defaultValue for object field method", async () => {
+      it("should generate non-nullable string arg type with defaultValue for object field method", async () => {
         const inputArg = argMethodField.args.find(arg => arg.name === "defaultStringArg")!;
         const defaultValueStringArgType = inputArg.type as IntrospectionNamedTypeRef;
 
         expect(inputArg.defaultValue).toBe('"defaultStringArgDefaultValue"');
-        expect(defaultValueStringArgType.kind).toEqual(TypeKind.SCALAR);
-        expect(defaultValueStringArgType.name).toEqual("String");
+        expect(defaultValueStringArgType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
       });
     });
 
@@ -429,18 +435,25 @@ describe("Resolvers", () => {
         )! as IntrospectionInputObjectType;
       });
 
-      it("should generate nullable string arg type with defaultValue for input object field", async () => {
+      it("should generate non-nullable string arg type with defaultValue for input object field", async () => {
         const defaultValueStringField = sampleInputType.inputFields.find(
           arg => arg.name === "defaultStringField",
         )!;
         const defaultValueStringFieldType = defaultValueStringField.type as IntrospectionNamedTypeRef;
 
         expect(defaultValueStringField.defaultValue).toBe('"defaultStringFieldDefaultValue"');
-        expect(defaultValueStringFieldType.kind).toEqual(TypeKind.SCALAR);
-        expect(defaultValueStringFieldType.name).toEqual("String");
-      });
-
-      it("should generate nullable string arg type with implicit defaultValue for input object field", async () => {
+        expect(defaultValueStringFieldType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
+      });
+
+      it("should generate non-nullable string arg type with implicit defaultValue for input object field", async () => {
         const implicitDefaultValueStringField = sampleInputType.inputFields.find(
           arg => arg.name === "implicitDefaultStringField",
         )!;
@@ -449,8 +462,15 @@ describe("Resolvers", () => {
         expect(implicitDefaultValueStringField.defaultValue).toBe(
           '"implicitDefaultStringFieldDefaultValue"',
         );
-        expect(implicitDefaultValueStringFieldType.kind).toEqual(TypeKind.SCALAR);
-        expect(implicitDefaultValueStringFieldType.name).toEqual("String");
+        expect(implicitDefaultValueStringFieldType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
       });
 
       it("should overwrite defaultValue in child input object", async () => {
@@ -460,8 +480,15 @@ describe("Resolvers", () => {
         const defaultValueStringFieldType = defaultValueStringField.type as IntrospectionNamedTypeRef;
 
         expect(defaultValueStringField.defaultValue).toBe('"defaultValueOverwritten"');
-        expect(defaultValueStringFieldType.kind).toEqual(TypeKind.SCALAR);
-        expect(defaultValueStringFieldType.name).toEqual("String");
+        expect(defaultValueStringFieldType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
       });
 
       it("should overwrite implicit defaultValue in child input object", async () => {
@@ -473,8 +500,15 @@ describe("Resolvers", () => {
         expect(implicitDefaultValueStringField.defaultValue).toBe(
           '"implicitDefaultValueOverwritten"',
         );
-        expect(implicitDefaultValueStringFieldType.kind).toEqual(TypeKind.SCALAR);
-        expect(implicitDefaultValueStringFieldType.name).toEqual("String");
+        expect(implicitDefaultValueStringFieldType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
       });
 
       it("should inherit field with defaultValue from parent", async () => {
@@ -484,8 +518,15 @@ describe("Resolvers", () => {
         const inheritDefaultFieldType = inheritDefaultField.type as IntrospectionNamedTypeRef;
 
         expect(inheritDefaultField.defaultValue).toBe('"inheritDefaultFieldValue"');
-        expect(inheritDefaultFieldType.kind).toEqual(TypeKind.SCALAR);
-        expect(inheritDefaultFieldType.name).toEqual("String");
+        expect(inheritDefaultFieldType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
       });
     });
 
@@ -501,15 +542,22 @@ describe("Resolvers", () => {
         expect(stringArgInnerType.name).toEqual("String");
       });
 
-      it("should generate nullable type arg with defaultValue from args object field", async () => {
+      it("should generate non-nullable type arg with defaultValue from args object field", async () => {
         const argsQuery = getQuery("argsQuery");
         const defaultStringArg = argsQuery.args.find(arg => arg.name === "defaultStringArg")!;
         const defaultStringArgType = defaultStringArg.type as IntrospectionNamedTypeRef;
 
         expect(defaultStringArg.name).toEqual("defaultStringArg");
         expect(defaultStringArg.defaultValue).toEqual('"defaultStringArgDefaultValue"');
-        expect(defaultStringArgType.kind).toEqual(TypeKind.SCALAR);
-        expect(defaultStringArgType.name).toEqual("String");
+        expect(defaultStringArgType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
       });
 
       it("should overwrite defaultValue in child args object field", async () => {
@@ -519,8 +567,15 @@ describe("Resolvers", () => {
 
         expect(defaultStringArg.name).toEqual("defaultStringArg");
         expect(defaultStringArg.defaultValue).toEqual('"defaultValueOverwritten"');
-        expect(defaultStringArgType.kind).toEqual(TypeKind.SCALAR);
-        expect(defaultStringArgType.name).toEqual("String");
+        expect(defaultStringArgType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
       });
 
       it("should overwrite implicit defaultValue in child args object field", async () => {
@@ -532,8 +587,15 @@ describe("Resolvers", () => {
 
         expect(implicitDefaultStringArg.name).toEqual("implicitDefaultStringArg");
         expect(implicitDefaultStringArg.defaultValue).toEqual('"implicitDefaultValueOverwritten"');
-        expect(implicitDefaultStringArgType.kind).toEqual(TypeKind.SCALAR);
-        expect(implicitDefaultStringArgType.name).toEqual("String");
+        expect(implicitDefaultStringArgType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
       });
 
       it("should inherit defaultValue field from parent args object field", async () => {
@@ -543,11 +605,18 @@ describe("Resolvers", () => {
 
         expect(inheritDefaultArg.name).toEqual("inheritDefaultArg");
         expect(inheritDefaultArg.defaultValue).toEqual('"inheritDefaultArgValue"');
-        expect(inheritDefaultArgType.kind).toEqual(TypeKind.SCALAR);
-        expect(inheritDefaultArgType.name).toEqual("String");
-      });
-
-      it("should generate nullable type arg with implicit defaultValue from args object field", async () => {
+        expect(inheritDefaultArgType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
+      });
+
+      it("should generate non-nullable type arg with implicit defaultValue from args object field", async () => {
         const argsQuery = getQuery("argsQuery");
         const implicitDefaultStringArg = argsQuery.args.find(
           arg => arg.name === "implicitDefaultStringArg",
@@ -558,8 +627,15 @@ describe("Resolvers", () => {
         expect(implicitDefaultStringArg.defaultValue).toEqual(
           '"implicitDefaultStringArgDefaultValue"',
         );
-        expect(implicitDefaultStringArgType.kind).toEqual(TypeKind.SCALAR);
-        expect(implicitDefaultStringArgType.name).toEqual("String");
+        expect(implicitDefaultStringArgType).toEqual({
+          kind: "NON_NULL",
+          name: null,
+          ofType: {
+            kind: "SCALAR",
+            name: "String",
+            ofType: null,
+          },
+        });
       });
 
       it("should generate nullable type arg from args object field", async () => {
@@ -1076,34 +1152,6 @@ describe("Resolvers", () => {
         }
       });
 
-      it("should throw error when default value set with non-nullable option", async () => {
-        expect.assertions(3);
-
-        try {
-          @InputType()
-          class SampleInput {
-            @Field({ defaultValue: "stringDefaultValue", nullable: false })
-            inputField: string;
-          }
-
-          @Resolver()
-          class SampleResolver {
-            @Query()
-            sampleQuery(@Arg("input") input: SampleInput): string {
-              return "sampleQuery";
-            }
-          }
-          await buildSchema({ resolvers: [SampleResolver] });
-        } catch (err) {
-          expect(err).toBeInstanceOf(Error);
-          expect(err).toBeInstanceOf(ConflictingDefaultWithNullableError);
-          const error = err as ConflictingDefaultWithNullableError;
-          expect(error.message).toMatchInlineSnapshot(
-            `"Wrong nullable option set for SampleInput#inputField. You cannot combine default value 'stringDefaultValue' with nullable 'false'."`,
-          );
-        }
-      });
-
       it("should throw error when list nullable option is combined with non-list type", async () => {
         expect.assertions(3);
 
diff --git a/tests/functional/typedefs-resolvers.ts b/tests/functional/typedefs-resolvers.ts
index 6c0ec287..e2dfcfdd 100644
--- a/tests/functional/typedefs-resolvers.ts
+++ b/tests/functional/typedefs-resolvers.ts
@@ -306,7 +306,15 @@ describe("buildTypeDefsAndResolvers", () => {
 
       expect(sampleInput.kind).toBe(TypeKind.INPUT_OBJECT);
       expect(sampleInput.inputFields).toHaveLength(2);
-      expect(sampleInputDefaultStringFieldType.name).toBe("String");
+      expect(sampleInputDefaultStringFieldType).toEqual({
+        kind: "NON_NULL",
+        name: null,
+        ofType: {
+          kind: "SCALAR",
+          name: "String",
+          ofType: null,
+        },
+      });
       expect(sampleInputDefaultStringField.defaultValue).toBe('"sampleInputDefaultStringField"');
     });