Skip to content

fix: DefaultResolveFn should handle with map[K]V for K that has string as the underlying type #697

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.

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
wants to merge 3 commits into from

Conversation

bkrukowski
Copy link

The current implementation returns the following errors for maps with a custom type for keys:

reflect.Value.MapIndex: value of type string is not assignable to type .*

… `string` as the underlying type

The current implementation returns the following errors for maps with a custom type for keys:

```
reflect.Value.MapIndex: value of type string is not assignable to type .*
```
@coveralls
Copy link

coveralls commented Sep 16, 2024

Coverage Status

coverage: 92.048%. remained the same
when pulling 921fc06 on bkrukowski:custom_maps
into f2b39ca on graphql-go:master.

@bkrukowski
Copy link
Author

@chris-ramon could you please merge that? it fixes a bug

@chris-ramon
Copy link
Member

chris-ramon commented Sep 22, 2024

Hi @bkrukowski - thanks for sending a PR.

I gave a quick look, the change is not needed, since the end result of val is the same as reflect.Value:

val := r.MapIndex(reflect.ValueOf(p.Info.FieldName).Convert(r.Type().Key()))

Could you provide an example of the following error you shared above ? - That is related to the PR ?

reflect.Value.MapIndex: value of type string is not assignable to type .*

… `string` as the underlying type

The current implementation returns the following errors for maps with a custom type for keys:

```
reflect.Value.MapIndex: value of type string is not assignable to type .*
```
… `string` as the underlying type

The current implementation returns the following errors for maps with a custom type for keys:

```
reflect.Value.MapIndex: value of type string is not assignable to type .*
```
@bkrukowski
Copy link
Author

bkrukowski commented Sep 22, 2024

@chris-ramon please take a look, I removed that code without removing the newly added test, and it fails:

image

https://app.circleci.com/pipelines/github/graphql-go/graphql/128/workflows/41b52c89-f893-43a4-a0d4-709dbd0c6fac/jobs/1626

The following code creates a new value of the string type:

reflect.ValueOf(p.Info.FieldName)

It causes an issue for maps that have keys of a custom type, and the underlying type is string, an example in the tests:

type (
	CustomMap map[string]interface{}

	CustomKey  string
	CustomMap2 map[CustomKey]interface{}
)

// ...

	scenarios := []struct {
		query    string
		data     interface{}
		expected interface{}
	}{
		{                                                 // it works
			query: query,
			data: CustomMap{
				"a": "1",
				"b": "2",
			},
			expected: map[string]interface{}{
				"data": map[string]interface{}{
					"a": "1",
				},
			},
		},
		{                                                 // it does not work without the suggested patch
			query: query,
			data: CustomMap2{
				"a": "1",
				"b": "2",
			},
			expected: map[string]interface{}{
				"data": map[string]interface{}{
					"a": "1",
				},
			},
		},
	}

@chris-ramon
Copy link
Member

@bkrukowski as explained before in the comment: #697 (comment) - The change is not needed, going ahead and closing the PR.

@chris-ramon chris-ramon closed this Oct 1, 2024
@bkrukowski
Copy link
Author

@chris-ramon kindly please take a look

TL;DR

The given code panics here, and I think you missed that point.

Full story

@bkrukowski as explained before in the comment: #697 (comment) - The change is not needed, going ahead and closing the PR.

it's incorrect

I gave a quick look, the change is not needed, since the end result of val is the same as reflect.Value:

val := r.MapIndex(reflect.ValueOf(p.Info.FieldName).Convert(r.Type().Key()))

No, it isn't. Without the suggested fix that code panics because we pass a wrong argument to MapIndex, so the val does not exist. Example:

package main

import (
	"github.com/graphql-go/graphql"
)

type MyKey string

type MyMap map[MyKey]any

func main() {
	myMap := MyMap{"key": "value"}

	graphql.DefaultResolveFn(graphql.ResolveParams{Source: myMap})
}

Output:

panic: reflect.Value.MapIndex: value of type string is not assignable to type main.MyKey

goroutine 1 [running]:
reflect.Value.assignTo({0x528da0?, 0x681160?, 0x10?}, {0x555582, 0x16}, 0x528be0, 0x0)
	/usr/local/go-faketime/src/reflect/value.go:3358 +0x299
reflect.Value.MapIndex({0x530fe0?, 0xc0000b0ea0?, 0x41103b?}, {0x528da0, 0x681160, 0x98})
	/usr/local/go-faketime/src/reflect/value.go:1805 +0xe8
github.com/graphql-go/graphql.DefaultResolveFn({{0x530fe0, 0xc0000b0ea0}, 0x0, {{0x0, 0x0}, {0x0, 0x0, 0x0}, 0x0, {0x0, ...}, ...}, ...})
	/tmp/gopath2667206492/pkg/mod/github.com/graphql-go/[email protected]/executor.go:998 +0x365
main.main()
	/tmp/sandbox4170389645/prog.go:16 +0xb0

https://go.dev/play/p/2LiyVcmhqUq

Since that library recovers all the panics, it returns the following error eventually:

reflect.Value.MapIndex: value of type string is not assignable to type main.MyKey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants