Skip to content

grapqql/executor: set variables on operation context after validation … #3952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
noamcattan wants to merge 2 commits into from
+42 −21
Closed

grapqql/executor: set variables on operation context after validation … #3952

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
055a6dc
grapqql/executor: setvariables on operation context after validation …
noamcattan Dec 3, 2025
7c27f79
add a test
noamcattan Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions graphql/executor/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ func (e *Executor) CreateOperationContext(
gqlErr, ok := err.(*gqlerror.Error)
if ok {
errcode.Set(gqlErr, errcode.ValidationFailed)
opCtx.Variables = params.Variables
Copy link
Contributor

@giautm giautm Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont like the idea of having invalid variable on the execution context, it should Only and only accept valid variable there - thats the point of validation.

I prefer have the raw input on the gqlError instead.

Copy link
Contributor

@giautm giautm Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cc @noamcattan / @a8m

Copy link
Collaborator

@StevenACoffman StevenACoffman Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel similarly. Generally, a function returns a value OR an error, but not both.
Otherwise, it seems to be splintered failure mode. If we really need to carry around invalid variables for some contextual error reporting, then we could make a custom error type for that purpose.

return opCtx, gqlerror.List{gqlErr}
}
}
Expand Down
62 changes: 41 additions & 21 deletions graphql/executor/executor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,33 @@
exec := testexecutor.New()

t.Run("calls query on executable schema", func(t *testing.T) {
resp := query(exec, "", "{name}")
resp, _ := query(exec, "", "{name}")
assert.JSONEq(t, `{"name":"test"}`, string(resp.Data))
})

t.Run("validates operation", func(t *testing.T) {
t.Run("no operation", func(t *testing.T) {
resp := query(exec, "", "")
resp, _ := query(exec, "", "")
assert.Empty(t, string(resp.Data))
assert.Len(t, resp.Errors, 1)
assert.Equal(t, errcode.ValidationFailed, resp.Errors[0].Extensions["code"])
})

t.Run("bad operation", func(t *testing.T) {
resp := query(exec, "badOp", "query test { name }")
resp, _ := query(exec, "badOp", "query test { name }")
assert.Empty(t, string(resp.Data))
assert.Len(t, resp.Errors, 1)
assert.Equal(t, errcode.ValidationFailed, resp.Errors[0].Extensions["code"])
})

t.Run("invalid variables", func(t *testing.T) {
resp, oc := query(exec, "", "query test($id: Int!) {find(id: $id)}", variable("id", "invalid"))

Check failure on line 42 in graphql/executor/executor_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint (1.25) 

File is not properly formatted (golines)
assert.Empty(t, string(resp.Data))
assert.Len(t, resp.Errors, 1)
assert.Equal(t, errcode.ValidationFailed, resp.Errors[0].Extensions["code"])
assert.NotNil(t, oc)
assert.Equal(t, "invalid", oc.Variables["id"])
})
})

t.Run("invokes operation middleware in order", func(t *testing.T) {
Expand All @@ -54,7 +63,7 @@
},
)

resp := query(exec, "", "{name}")
resp, _ := query(exec, "", "{name}")
assert.JSONEq(t, `{"name":"test"}`, string(resp.Data))
assert.Equal(t, []string{"first", "second"}, calls)
})
Expand All @@ -74,7 +83,7 @@
},
)

resp := query(exec, "", "{name}")
resp, _ := query(exec, "", "{name}")
assert.JSONEq(t, `{"name":"test"}`, string(resp.Data))
assert.Equal(t, []string{"first", "second"}, calls)
})
Expand All @@ -94,7 +103,7 @@
},
)

resp := query(exec, "", "{name}")
resp, _ := query(exec, "", "{name}")
assert.JSONEq(t, `{"name":"test"}`, string(resp.Data))
assert.Equal(t, []string{"first", "second"}, calls)
})
Expand All @@ -110,7 +119,7 @@
return next(ctx)
})

resp := query(exec, "", "{name}")
resp, _ := query(exec, "", "{name}")
assert.JSONEq(t, `{"name":"test"}`, string(resp.Data))
assert.Equal(t, []string{"first", "second"}, calls)
})
Expand All @@ -129,7 +138,7 @@
return nil
},
})
resp := query(exec, "", "{name}")
resp, _ := query(exec, "", "{name}")
assert.JSONEq(t, `{"name":"test"}`, string(resp.Data))
assert.Equal(t, []string{"param", "context"}, calls)
})
Expand All @@ -146,7 +155,7 @@
},
)

resp := query(exec, "", "invalid")
resp, _ := query(exec, "", "invalid")
assert.Empty(t, string(resp.Data))
assert.Len(t, resp.Errors, 1)
assert.Len(t, errors1, 1)
Expand All @@ -160,7 +169,7 @@
qry := `query Foo {name}`

t.Run("cache miss populates cache", func(t *testing.T) {
resp := query(exec, "Foo", qry)
resp, _ := query(exec, "Foo", qry)
assert.JSONEq(t, `{"name":"test"}`, string(resp.Data))

cacheDoc, ok := cache.Get(ctx, qry)
Expand All @@ -173,7 +182,7 @@
require.NoError(t, err)
cache.Add(ctx, qry, doc)

resp := query(exec, "Bar", qry)
resp, _ := query(exec, "Bar", qry)
assert.JSONEq(t, `{"name":"test"}`, string(resp.Data))

cacheDoc, ok := cache.Get(ctx, qry)
Expand All @@ -186,7 +195,7 @@
func TestExecutorDisableSuggestion(t *testing.T) {
t.Run("by default, the error message will include suggestions", func(t *testing.T) {
exec := testexecutor.New()
resp := query(exec, "", "{nam}")
resp, _ := query(exec, "", "{nam}")
assert.Empty(t, string(resp.Data))
assert.Equal(
t,
Expand All @@ -198,7 +207,7 @@
t.Run("disable suggestion, the error message will not include suggestions", func(t *testing.T) {
exec := testexecutor.New()
exec.SetDisableSuggestion(true)
resp := query(exec, "", "{nam}")
resp, _ := query(exec, "", "{nam}")
assert.Empty(t, string(resp.Data))
assert.Len(t, resp.Errors, 1)
assert.Equal(
Expand All @@ -208,7 +217,7 @@
)

// check if the error message is displayed correctly even if an error occurs multiple times
resp = query(exec, "", "{nam}")
resp, _ = query(exec, "", "{nam}")
assert.Empty(t, string(resp.Data))
assert.Len(t, resp.Errors, 1)
assert.Equal(
Expand Down Expand Up @@ -272,28 +281,39 @@
},
)

resp := query(exec, "", "{name}")
resp, _ := query(exec, "", "{name}")
assert.Equal(t, "null", string(resp.Data))
assert.Len(t, errors1, 1)
assert.Len(t, errors2, 1)
})
}

func query(exec *testexecutor.TestExecutor, op, q string) *graphql.Response {
type paramOption func(*graphql.RawParams)

func variable(name string, v any) paramOption {
return func(p *graphql.RawParams) {
p.Variables[name] = v
}
}

func query(exec *testexecutor.TestExecutor, op, q string, opts ...paramOption) (*graphql.Response, *graphql.OperationContext) {
ctx := graphql.StartOperationTrace(context.Background())
now := graphql.Now()
rc, err := exec.CreateOperationContext(ctx, &graphql.RawParams{
params := &graphql.RawParams{
Query: q,
OperationName: op,
ReadTime: graphql.TraceTiming{
Start: now,
End: now,
},
})
}
for _, opt := range opts {
opt(params)
}
rc, err := exec.CreateOperationContext(ctx, params)
if err != nil {
return exec.DispatchError(ctx, err)
return exec.DispatchError(ctx, err), rc
}

resp, ctx2 := exec.DispatchOperation(ctx, rc)
return resp(ctx2)
return resp(ctx2), rc
}
Loading