fix validation skipping for null on scalars (#342) (#529)

This PR fixes an inconsistent behavior when using `null` in a variable for a required type (e.g. `String!`) closes #519 Co-authored-by: Patric Vormstein <pvormstein@googlemail.com>
wundergraph · May 9, 2023 · 12e258d · 12e258d
1 parent 4998581
commit 12e258d
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 7 deletions.
diff --git a/pkg/engine/resolve/inputtemplate.go b/pkg/engine/resolve/inputtemplate.go
@@ -104,13 +104,11 @@ func (i *InputTemplate) renderObjectVariable(ctx context.Context, variables []by
 func (i *InputTemplate) renderContextVariable(ctx *Context, segment TemplateSegment, preparedInput *fastbuffer.FastBuffer) (variableWasUndefined bool, err error) {
 	value, valueType, offset, err := jsonparser.Get(ctx.Variables, segment.VariableSourcePath...)
 	if err != nil || valueType == jsonparser.Null {
-		undefined := false
 		if err == jsonparser.KeyPathNotFoundError {
-			undefined = true
+			preparedInput.WriteBytes(literal.NULL)
+			return true, nil
 		}
-
-		preparedInput.WriteBytes(literal.NULL)
-		return undefined, nil
+		return false, segment.Renderer.RenderVariable(ctx.Context(), value, preparedInput)
 	}
 	if valueType == jsonparser.String {
 		value = ctx.Variables[offset-len(value)-2 : offset]

diff --git a/pkg/engine/resolve/inputtemplate_test.go b/pkg/engine/resolve/inputtemplate_test.go
@@ -56,8 +56,11 @@ func TestInputTemplate_Render(t *testing.T) {
 		t.Run("json object as graphql object", func(t *testing.T) {
 			runTest(t, renderer, `{"foo":{"bar":"baz"}}`, []string{"foo"}, `{"type":"object","properties":{"bar":{"type":"string"}}}`, false, `{"bar":"baz"}`)
 		})
+		t.Run("json object as graphql object with null on required type", func(t *testing.T) {
+			runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":["string"]}`, true, ``)
+		})
 		t.Run("json object as graphql object with null", func(t *testing.T) {
-			runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":"string"}`, false, `null`)
+			runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":["string","null"]}`, false, `null`)
 		})
 		t.Run("json object as graphql object with number", func(t *testing.T) {
 			runTest(t, renderer, `{"foo":123}`, []string{"foo"}, `{"type":"integer"}`, false, `123`)
@@ -102,6 +105,34 @@ func TestInputTemplate_Render(t *testing.T) {
 		t.Run("nested string", func(t *testing.T) {
 			runTest(t, renderer, `{"foo":{"bar":"value"}}`, []string{"foo", "bar"}, `{"type":"string"}`, false, `"value"`)
 		})
+		t.Run("on required scalars", func(t *testing.T) {
+			t.Run("error on required string scalar", func(t *testing.T) {
+				runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":"string"}`, true, ``)
+			})
+			t.Run("error on required int scalar", func(t *testing.T) {
+				runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":"integer"}`, true, ``)
+			})
+			t.Run("error on required float scalar", func(t *testing.T) {
+				runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":"number"}`, true, ``)
+			})
+			t.Run("error on required boolean scalar", func(t *testing.T) {
+				runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":"boolean"}`, true, ``)
+			})
+		})
+		t.Run("on non-required scalars", func(t *testing.T) {
+			t.Run("null on non-required string scalar", func(t *testing.T) {
+				runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":["string","null"]}`, false, `null`)
+			})
+			t.Run("null on non-required int scalar", func(t *testing.T) {
+				runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":["integer","null"]}`, false, `null`)
+			})
+			t.Run("null on non-required float scalar", func(t *testing.T) {
+				runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":["number","null"]}`, false, `null`)
+			})
+			t.Run("null on non-required boolean scalar", func(t *testing.T) {
+				runTest(t, renderer, `{"foo":null}`, []string{"foo"}, `{"type":["boolean","null"]}`, false, `null`)
+			})
+		})
 	})
 
 	t.Run("array with csv render string", func(t *testing.T) {
@@ -330,7 +361,7 @@ func TestInputTemplate_Render(t *testing.T) {
 							SegmentType:        VariableSegmentType,
 							VariableKind:       ContextVariableKind,
 							VariableSourcePath: []string{"x"},
-							Renderer:           NewJSONVariableRendererWithValidation(`{"type":"string"}`),
+							Renderer:           NewJSONVariableRendererWithValidation(`{"type":["string","null"]}`),
 						},
 						{
 							SegmentType: StaticSegmentType,

diff --git a/pkg/graphql/execution_engine_v2_test.go b/pkg/graphql/execution_engine_v2_test.go
@@ -947,6 +947,56 @@ func TestExecutionEngineV2_Execute(t *testing.T) {
 		expectedResponse: `{"data":{"heroes":[]}}`,
 	}))
 
+	t.Run("execute operation with null variable on required type", runWithError(ExecutionEngineV2TestCase{
+		schema: func(t *testing.T) *Schema {
+			t.Helper()
+			schema := `
+			type Query {
+				hero(name: String!): String!
+			}`
+			parseSchema, err := NewSchemaFromString(schema)
+			require.NoError(t, err)
+			return parseSchema
+		}(t),
+		operation: func(t *testing.T) Request {
+			return Request{
+				OperationName: "MyHero",
+				Variables:     []byte(`{"heroName": null}`),
+				Query: `query MyHero($heroName: String!){
+						hero(name: $heroName)
+					}`,
+			}
+		},
+		dataSources: []plan.DataSourceConfiguration{
+			{
+				RootNodes: []plan.TypeField{
+					{TypeName: "Query", FieldNames: []string{"hero"}},
+				},
+				Factory: &graphql_datasource.Factory{},
+				Custom: graphql_datasource.ConfigJson(graphql_datasource.Configuration{
+					Fetch: graphql_datasource.FetchConfiguration{
+						URL:    "https://example.com/",
+						Method: "POST",
+					},
+				}),
+			},
+		},
+		fields: []plan.FieldConfiguration{
+			{
+				TypeName:  "Query",
+				FieldName: "hero",
+				Path:      []string{"hero"},
+				Arguments: []plan.ArgumentConfiguration{
+					{
+						Name:       "name",
+						SourceType: plan.FieldArgumentSource,
+					},
+				},
+			},
+		},
+		expectedResponse: ``,
+	}))
+
 	t.Run("execute operation and apply input coercion for lists without variables", runWithoutError(ExecutionEngineV2TestCase{
 		schema: inputCoercionForListSchema(t),
 		operation: func(t *testing.T) Request {

diff --git a/pkg/graphqljsonschema/jsonschema_test.go b/pkg/graphqljsonschema/jsonschema_test.go
@@ -63,6 +63,20 @@ func TestJsonSchema(t *testing.T) {
 			`nope`,
 		},
 	))
+	t.Run("string (required)", runTest(
+		`scalar String type Query { rootField(str: String!): String! }`,
+		`query ($input: String!){ rootField(str: $input) }`,
+		`{"type":["string"]}`,
+		[]string{
+			`"validString"`,
+		},
+		[]string{
+			`false`,
+			`true`,
+			`nope`,
+			`null`,
+		},
+	))
 	t.Run("id", runTest(
 		`scalar ID input Test { str: String }`,
 		`query ($input: ID){}`,