0.17.14 generate getters for interfaces problem

[argoyle]

What happened?

When upgrading to 0.17.14 my code generation started to fail due to the new generation of getters for interfaces. It's due to interfaces returning other interfaces I think.

What did you expect?

Code generation to result in valid code

Minimal graphql.schema and models to reproduce

Reproduction repo: https://github.com/argoyle/gqlgen-implements

versions

• go run github.com/99designs/gqlgen version? 0.17.14
• go version? 1.19

[argoyle]

I can of course work around this by removing a couple of the interfaces (Node, Edge, Connection). Looking at it right now they don't really give me any value. 🙈

[StevenACoffman]

@neptoess Can you look at the https://github.com/argoyle/gqlgen-implements repo and investigate what is happening with your change? I'm traveling and this is hard to look at on a phone.

[neptoess]

This is an interesting one. Looking at the schema (https://github.com/argoyle/gqlgen-implements/blob/main/graph/schema.graphqls), we have UsersConnection implements Connection, but... it doesn't look like it actually does. The edges field on UsersConnection is [UsersEdge!]!, but the edges field in the Connection interface is [Edge!]!.

Even though UsersEdge implements Edge is true, I'm pretty sure [UsersEdge!]! implements [Edge!]! isn't, i.e., even if every element in edges is a UsersEdge, I'm fairly positive the type on edges still needs to be [Edge!]!.

I can see if the GraphQL spec calls this out.

EDIT: It does https://spec.graphql.org/October2021/#sec-Objects. What you have here @argoyle is valid GraphQL. I can modify the getter generation logic for this scenario.

[MichaelMure]

I have a similar issue, but #2332 doesn't solve it. It happens when I try to upgrade gqlgen in https://github.com/MichaelMure/git-bug. Repro repo: https://github.com/MichaelMure/gqlgen-issue

In short:

• I have a bunch of struct in the core of the code. Some of them implement the sign-post that gqlgen generate because why not, it's not hard to do.
• I have a layer that does lazy loading and wrap some of the previous types. It also change a bit the interface and add errors (as the lazy loading can fail)
• I ask gqlgen to bind on the lazy loading types, which also transitively carry some of the normal types
• until 0.17.13, gqlgen was happy with that. With 0.17.14, gqlgen generate an additional getter in the Authored interface, which I can't implement in the normal types as that would create a circular dependency (--> lazy --> normal --> lazy).
• I get the error merging type systems failed: unable to bind to interface: gqlgen-issue/graph/model/types.Bug does not satisfy the interface gqlgen-issue/graph/model.Authored

In that scenario, shouldn't gqlgen generate a resolver? If so, I could easily wrap the normal type in the lazy loading one, but gqlgen just fail.

Am I doing something wrong? Any advises?

[neptoess]

I have a similar issue, but #2332 doesn't solve it. It happens when I try to upgrade gqlgen in https://github.com/MichaelMure/git-bug. Repro repo: https://github.com/MichaelMure/gqlgen-issue

In short:

• I have a bunch of struct in the core of the code. Some of them implement the sign-post that gqlgen generate because why not, it's not hard to do.
• I have a layer that does lazy loading and wrap some of the previous types. It also change a bit the interface and add errors (as the lazy loading can fail)
• I ask gqlgen to bind on the lazy loading types, which also transitively carry some of the normal types
• until 0.17.13, gqlgen was happy with that. With 0.17.14, gqlgen generate an additional getter in the Authored interface, which I can't implement in the normal types as that would create a circular dependency (--> lazy --> normal --> lazy).
• I get the error merging type systems failed: unable to bind to interface: gqlgen-issue/graph/model/types.Bug does not satisfy the interface gqlgen-issue/graph/model.Authored

In that scenario, shouldn't gqlgen generate a resolver? If so, I could easily wrap the normal type in the lazy loading one, but gqlgen just fail.

Am I doing something wrong? Any advises?

@MichaelMure
It's kind of one or the other. gqlgen generates the entire type, or none of it. In your case, your Bug and Comment implementation don't actually implement the Authored interface. Fixing this is fairly straightforward though. Simply adding this to https://github.com/MichaelMure/gqlgen-issue/blob/master/graph/model/types/bug.go fixed the issue in your repro repo.

func (b Bug) IsAuthored()
func (b Bug) GetAuthor() *Identity { return &b.Author }
func (c Comment) IsAuthored()
func (c Comment) GetAuthor() *Identity { return &c.Author }

EDIT: Just remembered that this is actually demonstrated in the Star Wars example (https://github.com/99designs/gqlgen/blob/master/_examples/starwars/models/model.go). I had to update that when I first added getter generation.

[MichaelMure]

@neptoess ha sorry, I didn't realize that my toy example was binding on the wrong types, which was making the problem go away.

I updated it accordingly and noticed 3 different issues:

1. Implementing the generated interface create a cyclic dependency. It's also really wrong the lower level to know about the upper level.
2. The generated interface can't return an error. That only leaves the possibility of panicking?
3. gqlgen doesn't recognize embedded interfaces. Most likely unrelated to the other problems, but worth noting.

I feel like this generated interface can be useful in some scenario, but also break down and force bad design in others. Am I doing something wrong? Or is there a way to disable that getter generation?

[StevenACoffman]

@MichaelMure I re-opened this issue because it looks like there may be some more rough edges to iron out.

If we can't easily make the new generating getters for interfaces transparently painless for existing users, I'd like to default it to off and make a config option to allow people to opt-in to this new behavior while we continue to refine it. What do you think of that @neptoess ?

[neptoess]

I wouldn’t have an issue hiding this behind a config flag. I can look at Michael’s issue more in depth tonight or tomorrow morning, but, from what I see so far on my phone, it looks like, again, the custom implementation for the Go type isn’t actually implementing the generated Go interface.

Out of curiosity, do you remember anything like this coming up when interface generation went from just aliasing interface{} to adding Is{interfaceName}() functions?

[MichaelMure]

Out of curiosity, do you remember anything like this coming up when interface generation went from just aliasing interface{} to adding Is{interfaceName}() functions?

FWIW it did feel weird to me at that time but the workaround was easy enough to implement (just add the empty sign post function). Now that comes with enforced types and no error, which can really mess with and existing code architecture.

[neptoess]

Out of curiosity, do you remember anything like this coming up when interface generation went from just aliasing interface{} to adding Is{interfaceName}() functions?

FWIW it did feel weird to me at that time but the workaround was easy enough to implement (just add the empty sign post function). Now that comes with enforced types and no error, which can really mess with and existing code architecture.

Yeah I see, in your case, why this is frustrating. For GraphQL clients, when they call a query that returns an interface, they can select the fields defined in that interface, without knowing what type is actually implementing that interface. The inspiration for adding the getters in Go was to allow those same kind of semantics, i.e. reading the values of fields defined in the interface, without unboxing to the concrete type.

But your code is doing a lot more than just returning a field. The star wars example in the gqlgen repo has the same thing going on with fields like FriendsConnection. My “fix” there was to just turn those getters into what are effectively stubs (they just return nil / the default value for the type). But, if your code does a lot of this, it’s probably less headache to just not have the getters in the generated interface at all. That’s why I like having a config flag for them. The question is: should it default on or off? @StevenACoffman suggested off by default, to maintain historical behavior. This makes sense to me, since v0.17.14 is arguably a bug fix release, it shouldn’t break anyone’s code. I might suggest a future major/minor release, e.g. v0.18.0 or v1.0.0, change it to default to on though, as I think the getters would more commonly be seen as a convenience than a nuisance.

I may be biased though 😛

[MichaelMure]

Yeah I see, in your case, why this is frustrating.

At the end of the day, I very much appreciate you guy's work and consideration.

[StevenACoffman]

The more I think about it, if we added a config option at this point we could default it to on by default, so we can let people opt out if it is problematic for them, but it would help encourage better practices for new people.

I wonder how people who upgrade to the latest version and encounter a problem as @MichaelMure did would discover the solution/workaround.

The compile time bugs like import cycle not allowed from go vet don't really allow us the ability to give a helpful action item. So perhaps some mention in the documentation that we link to in the release announcement discussion is the best we can do?

[MichaelMure]

As I understand, generating getters is for convenience (?), but could you explain why those sign post functions are necessary?

[neptoess]

The sign posts were added quite some time ago to the Go interfaces generated by gqlgen to give the Go compiler some way to enforce type checking #335. Without getters, or the signpost functions, the interfaces were effectively just aliasing interface{}, meaning any type could be used as that interface, and the compiler would have no way to know that it wasn't valid.

In theory, with getter generation enabled, the signposts are no longer needed for GraphQL interfaces. They would still be needed for GraphQL unions though. And, personally, I think it's pretty convenient having the signposts. Something worth pointing out is that the GraphQL and Go type systems have some pretty glaring differences, and, at least for me, it can be hard to know which mental model to reason with when working with Go code generated from a GraphQL schema. Since GraphQL requires explicitly stating that a type implements an interface in order for it to be used as that interface type, I think it does make sense to leave the signposts, even though Go does the whole implicit interface implementation thing.

[StevenACoffman]

Something worth pointing out is that the GraphQL and Go type systems have some pretty glaring differences, and, at least for me, it can be hard to know which mental model to reason with when working with Go code generated from a GraphQL schema.

That's actually an excellent point. I'm not sure we've articulated the common friction points between the Go and GraphQL type systems, which might go a long way to explaining why we do some things the way we do. Do you mind listing some of the things you stumbled on? I'm worried I've gotten used to it for too long to remember all of what I found surprising.

[neptoess]

Do you mind listing some of the things you stumbled on?

Not at all:

• Unions exist in GraphQL, but not Go, so they are somewhat mocked through Go interfaces
  • The Is{UnionName}() signpost functions leverage the Go compiler to help alleviate some of the confusion here
  • Without the signposts, a programmer implementing a query resolver would need likely need to jump to the GraphQL schema to figure out what types are included in the union
• Interfaces in GraphQL differ from Go interfaces in two key ways
  • A GraphQL interface definition includes only fields, where Go interfaces contain methods
    • Getter functions are generated to preserve the fields defined in the GraphQL interface
  • GraphQL types must explicitly declare that they're implementing an interface to be used as that interface type, where Go does this implicitly, i.e. if a type has the same methods that an interface defines, it can be used as that interface type
    • The signpost functions prevent Go types from inadvertently implementing GraphQL interfaces
      • As an example, imagine two different GraphQL interfaces that only contain ID: ID!
• GraphQL makes you use special input types if you need structured input to a query or mutation
  • input types can't be used as returns, solely as inputs
  • Go's type system has no analog, and no great way to enforce this
• GraphQL types have the notion of "required" fields, e.g. Int!, where Go only has pointers
  • For primitives, it is somewhat convenient to just use the type directly for required fields, and pointers for optional fields
  • For structures, this becomes a hard decision, since using a structure type directly in Go will force copies to be created
    • The compromise here is that structure fields are always generated as pointer in Go, and, if they're marked as required in the GraphQL schema, the resolver has to enforce that the field in the Go structure is non-nil at runtime

That's what I have off the top of my head. I haven't done anything too terribly exotic with gqlgen though (other than filling in schema.resolvers.go, it's just using whatever gqlgen generates), so I'm sure others have stumbled on bigger footguns.

[StevenACoffman]

I asked some co-workers the same question and they added these:

• GraphQL only has Int (=int32) and Float (=float64), notably there is officially no int64/uint64
• Fragments aren't a part of GraphQL's type system per se, but are sort of related and don't have any equivalent in Go; and the fragment embedding rules (legal any time the two types share an implementation) are a bit surprising and different from the Go embedding rules (the embedder necessarily implements the embeddee)
• GraphQL types aren't really just one type, because in different contexts you have different fields of that type. So while both GraphQL schemas and Go are both nominally typed, GraphQL response types kinda feel a bit less nominally typed in that there are many kinds of "User" depending on which fields you asked for. This is especially relevant when it comes to idiomatic usage of the types; in code using GraphQL it's idiomatic to use row polymorphism (a.k.a. inexact types, e.g. using a function of {id: Int} on a value of type {id: Int, <other fields>}) such that you can add some fields to a query and existing code doesn't care. But Go has no way to do row polymorphism (at least not without introducing interfaces everywhere), and so you have to do things a bit differently especially if you have multiple overlapping queries. (Ed.: this isn't a good explanation, if it doesn't make sense let me know and I can take another stab where I actually properly explain my terms.)
  One way to explain the difference in interfaces is that Go interfaces are effectively structurally typed (there are subtle cases where this isn't true, but mostly) whereas GraphQL interfaces are nominally typed, and so we emulate nominally-typed interfaces via sentinel methods (this is probably only a useful explanation for people who already know what those words mean though).

[StevenACoffman]

Ok, I think I'm happy with where things are, but I would like a config option to disable generating getters for interfaces for people who run into similar inconvenience when upgrading.

[neptoess]

Ok, I think I'm happy with where things are, but I would like a config option to disable generating getters for interfaces for people who run into similar inconvenience when upgrading.

I can probably get around to that in the next week or two

[StevenACoffman]

Thanks! I cut an interim release v0.17.15 that includes the latest refinement, and by the time you get a config option landed I can then cut a v0.17.16. I'm going to leave this issue open until then.

[ddevault]

One last note: this should have been more prominent in the release notes because it is a breaking change. Also probably should have justified a minor version bump.

[darkLord19]

@StevenACoffman @neptoess slightly unrelated question but is it possible to generate getters for common fields for graphql union types or some way to tell gqlgen to add getters in union interface generated by gqlgen

[neptoess]

It would be pretty tricky. But, if a field is common among all types in a union, why not make an interface for them instead?

[darkLord19]

@neptoess I am trying to stitch two graphql servers and one server is already having types defined as union and lots of old code is written around it so not feasible to change it rn and due to type inconsistencies, graphql stitching will fail.