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

Revert "Fix marshaling interfaces and union types (#3211)" #3289

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

StevenACoffman
Copy link
Collaborator

@StevenACoffman StevenACoffman commented Sep 17, 2024

This reverts commit 3556475.

Got a report that since #3211 was merged, the generation seems to be broken.
The name of Go elements are no longer equal to the name of GQL types.
For instance, generating code with GQL type as:

union CliTargetSettingsResult @goModel(model: "github.com/raito-io/appserver/lambda/appserver/services/cli.CliTargetSettingsResult") = CliTargetSettings | PermissionDeniedError

type CliTargetSettings @goModel(model: "github.com/raito io/appserver/lambda/appserver/services/cli.TargetSettings")
...

Will result in generated code like this:

if len(graphql.CollectFields(ec.OperationContext, sel, []string{"CliTargetSettingsResult", "TargetSettings"})) == 0 {
	return graphql.Empty{}
}

As CliTargetSettingsResult and TargetSettings are Go types and not a GQL types, this will always be 0 and return graphql.Empty{}.

On top of that, this could lead to invalid JSON as a response. As Empty{} will be marshaled as an empty string I'm able to generate response data like

{"__typename":"xxxx","zzz":{"__typename":"yyy","id":"lR0QWfLtP7TsZcphexSvx","cliSettings":}}

Which panics on the JSON marshaling.

@coveralls
Copy link

Coverage Status

coverage: 59.582% (+0.004%) from 59.578%
when pulling a95cbd9 on revert_3211
into 26da8a6 on master.

@StevenACoffman StevenACoffman merged commit ab68eca into master Sep 17, 2024
17 of 18 checks passed
@StevenACoffman StevenACoffman deleted the revert_3211 branch September 17, 2024 12:38
@StevenACoffman
Copy link
Collaborator Author

@krupyansky If you can try this again while addressing the unforeseen problems that this caused, I would appreciate it.

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.

2 participants