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 marshaling interfaces and union types #3211

Merged

Conversation

krupyansky
Copy link
Contributor

@krupyansky krupyansky commented Aug 5, 2024

I added unit tests for query concrete types that I not insert in the query, but they return in a resolver.

Earlier, I can have same types

type Shape interface

type Square struct {
  radius float64
}

type Circle struct {
  width, lenght int
}

type Query {
  shapes: []Shape
}

and query {shapes {on ...Square{width length}}}

And resolver can return circles and I get result:

shapes{{}, {}, {width: 10, lenght: 50}}

I fixed it

@coveralls
Copy link

Coverage Status

coverage: 74.71% (+0.001%) from 74.709%
when pulling 8e43a85 on krupyansky:feature/fix-marshaling-interfaces
into 564e2dc on 99designs:master.

@krupyansky
Copy link
Contributor Author

@StevenACoffman сan you confirm that these changes are relevant for your library and are accepted?

@StevenACoffman StevenACoffman merged commit 3556475 into 99designs:master Aug 15, 2024
17 checks passed
@StevenACoffman
Copy link
Collaborator

Thanks! I appreciate your contributions, but I've been a little slammed at work lately.

@rmennes
Copy link

rmennes commented Sep 17, 2024

I have a question about this. Since I start using this code, the generation seems to be broken.
The name of my go elements are not equal to the name of my GQL types.
I'm generating the 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")
...

This 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 unvalid json as a response. As Empty{} will be marshelled 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 marshalling.

@StevenACoffman how do you think about this issue?

@StevenACoffman
Copy link
Collaborator

@rmennes Just to confirm, you get these problem using v0.17.52 correct?

@rmennes
Copy link

rmennes commented Sep 17, 2024

@StevenACoffman Yes. I got this issue starting from v0.17.50. But I'm experiencing the same issue with v0.17.52.

StevenACoffman added a commit that referenced this pull request Sep 17, 2024
@StevenACoffman
Copy link
Collaborator

Thanks for your investigation and report @rmennes

I reverted this PR and cut a new release https:/99designs/gqlgen/releases/tag/v0.17.53

Please verify that it resolves your problem.

@krupyansky If you can make another attempt at this PR now that you know what unintentional problems to watch out for, I would appreciate it. If so, consider mentioning @rmennes as he might be willing to try it out for you.

@rmennes
Copy link

rmennes commented Sep 17, 2024

@StevenACoffman thanks for the quick response. The issue seems to be resolved in v0.17.53.

@krupyansky happy to help. Let me know if I can help with anything or test out the new version.

@krupyansky
Copy link
Contributor Author

@StevenACoffman @rmennes I create pull request #3293

@rmennes help please test out the new version

}
}
`, &resp)
require.Empty(t, resp.Shapes)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats not compliant with the GraphQL spec. The resolver must return two empty objects (for the two circles) here. I also write the comment on the issue. Its not a but or inconvenience if the graphql server is returning empty objects in union or interface types.

Thats part of the specification. https://spec.graphql.org/October2021/#sec-Unions

and here is one quote:

interfaces must return the set of interfaces that an object implements (if none, interfaces must return the empty set).

@werjo-gridfuse
Copy link

werjo-gridfuse commented Sep 19, 2024

I already wrote it in the Pull Request but this issue is no issue. Interface types and Union types must return empty objects in this case.

https://spec.graphql.org/October2021/#sec-Unions

interfaces must return the set of interfaces that an object implements (if none, interfaces must return the empty set).

in your example the inline fragment

...on Rectangle is not telling the server to only return Rectangles. Its telling the server what field it should resolve if the type is a Rectangle. Thats why its absolutely correct if the resolver returns empty objects for all types that are not rectangles.

fragments are not filters!

If you want a resolver that filters, you have to create one with an inpit paramter like this:

type Query {
  shapes(filter: [String!]!): [Shape!]!
}

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.

5 participants