`MustParseSchema` panics with nullable argument Directives

[ssko1]

Expected

The following schema parses with graphql.MustParseSchema()

directive @withNullableArgument(aNullableArgument: String) on FIELD_DEFINITION
type Query {
  a: String! @withNullableArgument()
}

Actual

graphql.MustParseSchema panics with a nil pointer dereference

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x11a7d24]

Basic Test Case

package graphql_test

import (
    "context"
    "fmt"
    "testing"

    "github.com/graph-gophers/graphql-go"
)

const s = `
directive @withNullableArgument(aNullableArgument: String) on FIELD_DEFINITION
type Query {
  a: String! @withNullableArgument()
}
`

type WithNullableArgumentDirective struct {
    ANullableArgument *string
}

func (_ *WithNullableArgumentDirective) Validate(_ context.Context, _ interface{}) error {
    return nil
}

func (_ *WithNullableArgumentDirective) ImplementsDirective() string {
    return "withNullableArgument"
}

func TestResolverReturnsInterface(t *testing.T) {
    opts := graphql.Directives(&WithNullableArgumentDirective{})
    s := graphql.MustParseSchema(s, &DirectiveQueryResolver{}, opts)
    exec := s.Exec(context.Background(), `{ a }`, "", map[string]interface{}{})
    fmt.Println(exec.Errors)
    fmt.Println(string(exec.Data))
}

type DirectiveQueryResolver struct{}

func (_ *DirectiveQueryResolver) A() string {
    return "a"
}

[ssko1]

The panic occurs when resolvable.go tries to deserialize the directive's nullable argument L652-654

for _, arg := range d.Arguments {
    args[arg.Name.Name] = arg.Value.Deserialize(nil)
}

[pavelnikolov]

@ssko1 Thank you for reporting this. I'll try to reproduce and fix it.

[EverWinter23]

I would like to contribute to this project @pavelnikolov. May I take this one? What should be the expected behavior?

[ssko1]

"There should be no panic" would be the ideal behavior at a high level.

A bit underneath the hood, it seems that resolvable.go#L567-569 should be checking for directive arguments that were not passed (recall that explicit nulls is different than no argument). The code shouldn't attempt to pack an argument value that does not exist.

The test case that I provided above will show the line that panics.

[pavelnikolov]

I haven't had a chance to test and review this, however, at first glance it seems that @ssko1 has given enough information. My wild guess is that we iterate over the argument definition instead of iterating over the passed arguments. The definition will always be a supper-set and some args might be missing (which is exactly what seems to be happening in the failing example).

@EverWinter23 the argument definition list is what you have in your schema - we should not iterate over that. Instead we should iterate over the arguments that are passed in the incoming query and create a packer only for those. I'll probably be able to check this out over the weekend. In the meantime PRs would be welcome.

[EverWinter23]

@pavelnikolov @ssko1 I have raised a PR. Could you please go through it add add your thoughts? I was able to address the issue, however I am unsure about the correctness.