Skip to content
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

Merged
merged 2 commits into from
Jan 27, 2023

Conversation

mstephano
Copy link
Contributor

@mstephano mstephano commented Jan 26, 2023

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:

  • basic alias Rune
  • defined type for []byte
  • defined type for []rune

I have:

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

@coveralls
Copy link

Coverage Status

Coverage: 75.517% (-0.9%) from 76.375% when pulling 705d919 on mstephano:fix/byte into e6114a2 on 99designs:master.

@mstephano
Copy link
Contributor Author

Should I add some integration tests too to increase the coverage?

Copy link
Contributor

@roneli roneli left a 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.

@mstephano
Copy link
Contributor Author

@roneli I just tried your code, can you replace map[string]string by map[string]NamedStringType in your marshal/unmarshal func and let me know if that works.

@roneli
Copy link
Contributor

roneli commented Jan 26, 2023

@roneli I just tried your code, can you replace map[string]string by map[string]NamedStringType in your marshal/unmarshal func and let me know if that works.

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.

@mstephano
Copy link
Contributor Author

@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 Byte for anyone who use it.

@StevenACoffman StevenACoffman merged commit 5b85e93 into 99designs:master Jan 27, 2023
@StevenACoffman
Copy link
Collaborator

Thanks for the fix, and I really appreciate you continuing to work on these issues!

StevenACoffman added a commit that referenced this pull request Mar 20, 2023
StevenACoffman added a commit that referenced this pull request Mar 20, 2023
StevenACoffman added a commit that referenced this pull request Mar 20, 2023
Signed-off-by: Steve Coffman <steve@khanacademy.org>
StevenACoffman added a commit that referenced this pull request Mar 20, 2023
Signed-off-by: Steve Coffman <steve@khanacademy.org>
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.

BasicTypes "Byte" Binding for Scalars fails
4 participants