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

[noamcattan]

…error

Looks like this is relevant for the following issues:
#3223
#2669

I realized that when the variable validation failes, the variables are not set on the operation context. Since I use the operation context for logging and tracing errors in an extension, this makes it really hard to debug issues, because I lack visibility on what's was passed by the client.

Another workaround I found is to inject the variables in a OperationParameterMutator extension, but this feels like a more correct way.

Thanks in advance for the review!

I have:

• Added tests covering the bug / feature (see testing)
• Updated any relevant documentation (see docs)

[a8m]

Looks good to me.

@StevenACoffman, any objection to merging this?

[a8m]

cc @giautm

[StevenACoffman]

It looks like there are currently test failures:

=== FAIL: graphql/executor TestExecutor/validates_operation/invalid_variables (0.00s)

=== FAIL: graphql/executor TestExecutor/validates_operation (0.00s)

=== FAIL: graphql/executor TestExecutor (0.00s)

panic: assignment to entry in nil map [recovered]
	panic: assignment to entry in nil map

goroutine 35 [running]:
testing.tRunner.func1.2({0x7fbda0, 0xabf740})
	testing/testing.go:1734 +0x3eb
testing.tRunner.func1()
	testing/testing.go:1737 +0x696
panic({0x7fbda0?, 0xabf740?})
	runtime/panic.go:792 +0x132
github.com/99designs/gqlgen/graphql/executor_test.TestExecutor.func2.3.variable.1(0xc0000a41c0)
	github.com/99designs/gqlgen/graphql/executor/executor_test.go:295 +0x74
github.com/99designs/gqlgen/graphql/executor_test.query(0xc000172510, {0x0, 0x0}, {0x854e99, 0x25}, {0xc0000ade88, 0x1, 0x4b868c?})
	github.com/99designs/gqlgen/graphql/executor/executor_test.go:311 +0x285
github.com/99designs/gqlgen/graphql/executor_test.TestExecutor.func2.3(0xc00008a540)
	github.com/99designs/gqlgen/graphql/executor/executor_test.go:42 +0x113
testing.tRunner(0xc00008a540, 0xc00009e0b0)
	testing/testing.go:1792 +0x226
created by testing.(*T).Run in goroutine 18
	testing/testing.go:1851 +0x8f3

DONE 1412 tests, 3 failures in 60.900s

[giautm]

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.

[giautm]

Cc @noamcattan / @a8m

[StevenACoffman]

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.

[StevenACoffman]

@noamcattan I think this is extremely valuable, and I would really like you to continue to work on this. However, I think it would be better to create and return a custom error with the values you need to access.

[noamcattan]

@StevenACoffman sure thing, only that now I don't have an ETA for this. I'll ping you when I'll have an update