-
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 marshaling interfaces and union types #3211
Fix marshaling interfaces and union types #3211
Conversation
@StevenACoffman сan you confirm that these changes are relevant for your library and are accepted? |
Thanks! I appreciate your contributions, but I've been a little slammed at work lately. |
I have a question about this. Since I start using this code, the generation seems to be broken.
This result in generated code like this:
As 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
Which panics on the json marshalling. @StevenACoffman how do you think about this issue? |
@rmennes Just to confirm, you get these problem using v0.17.52 correct? |
This reverts commit 3556475.
@StevenACoffman Yes. I got this issue starting from v0.17.50. But I'm experiencing the same issue with v0.17.52. |
Thanks for your investigation and report @rmennes I reverted this PR and cut a new release https://github.com/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. |
@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. |
@StevenACoffman @rmennes I create pull request #3293 @rmennes help please test out the new version |
} | ||
} | ||
`, &resp) | ||
require.Empty(t, resp.Shapes) |
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.
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).
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
in your example the inline fragment
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!]!
} |
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
and
query {shapes {on ...Square{width length}}}
And resolver can return circles and I get result:
I fixed it