-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix #2524 basic alias Byte was not binded properly #2528
Conversation
Should I add some integration tests too to increase the coverage? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, will add as per my comment if we define something like this:
scalar StringMap
type SomeStruct {
field: StringMap
}
func MarshalStrMap(val map[string]string) graphql.Marshaler {
return graphql.WriterFunc(func(w io.Writer) {
err := json.NewEncoder(w).Encode(val)
if err != nil {
panic(err)
}
})
}
func UnmarshalStrMap(v interface{}) (map[string]string, error) {
if m, ok := v.(map[string]string); ok {
return m, nil
}
return nil, fmt.Errorf("%T is not a map", v)
}
And we have a struct that is
type NamedStringType string
type SomeStruct struct{
Field map[string]NamedStringType
}
The generated go will try to use the unrmarshal of the string map but it won't work, since we need to convert the named type to pass to the unmarshaler.
I am trying to figure out in the generator template how to check and fix it.
We can maybe do this in a different PR / I will open an issue about this later today.
@roneli I just tried your code, can you replace |
Yes that will work, but then I need to create an un/marshal for every named type. What I did was force a resolver on the field and convert there. Basically this error will occur if its an external map and you auto bind. (I am doing a lot of auto binding with many many go structs). In that case my simple StringMap isn't enough and I need to create a special map per type. |
@roneli Can you create a new issue for the map and describe the improvement you need with code example? I will take a look into it and we can keep our conversation there. This PR is really to fix for |
Thanks for the fix, and I really appreciate you continuing to work on these issues! |
Signed-off-by: Steve Coffman <steve@khanacademy.org>
Issue from v0.17.24+ with binding bytes type
Fixes #2524 for basic alias
Byte
(proposition from @roneli). Also added tests to make sure binding works properly for:Rune
I have: