Using an Interface inline fragment inside of a Union is awkward to unpack

[alex-torok]

When using an interface to select common fields from multiple types in a Union query, I expected genqlient to define an interface type that I could use to access my fields. Instead, it generated interface types for each type in the union and gave me no easy method to access my common interface fields.

Example Schema:

type Query {
    foo(): FooUnion
}

Union FooUnion: A | B | C

type A implements ColorInterface { ... }
type B implements ColorInterface { ... }
type C implements ColorInterface { ... }

interface ColorInterface {
    color: String
    relatedFoo: FooUnion
}

Example Query:

query query() {
   foo() {
      ... on ColorInterface {
         color
      }
   }
}

In this case, if I want to write code to pull the color value out, I need to type assert across A, B, and C (or write a Getter interface and insert a runtime check). This is a bit of a pain, but understandable

Unfortunately, the problem compounds itself when my FooUnion items contain links to other FooUnion items that I'd like to get the color for:

query query() {
   foo() {
      ... on ColorInterface {
         color
         relatedFoo {
             ... on ColorInterface {
                color
             }
         }
      }
   }
}

This creates a pretty awkward unpacking situation where I need to have a decent bit of nested boilerplate to access the fields in my query.

Describe the solution you'd like
It would be great if golang interface types were generated when performing an interface inline fragment on a union type.

In this case, it seems like a new interface type would need to be generated and applied to the QueryFooA, QueryFooB, and QueryFooC types that were generated for the Union. I poked around in the generate/convert.go code, but I didn't find an obvious place where a change to support this would be made.

This seems tricky to do, as the logic would need to know information about the interface inline fragment when generating the types for the union definition.

I'd consider working on this and opening a PR, but I'd like to hear from a maintainer to see how hard you think this would be (especially for someone who's never worked in the repo).

** Alternatives **

I don't have control over the schema, so changing the foo query to not return a union is not possible.

Using the shurcooL/graphql library made this query trivial, as it was able to unmarshal the json directly into my query object. I'd prefer to have genqlient's type protections and schema validation on my query, but this particular usecase requires so much boilerplate as to not make it worth it.

[benjaminjkraft]

Thanks for the detailed report. I guess it's kinda the same thing that led us to #126 (which was the summation of several conversations outside the issue tracker).

First of all, a simpler workaround is to type-assert directly to the interface type you want. I was thinking you could get genqlient to generate the right interface for you via a named fragment, but there's an extra layer of indirection (and the implementsMyFragment prevents using the interface as a duck type); but you can define it yourself (the methods are generated for you -- see #126). Then you can do resp.Foo.(YourColorInterface).GetColor(); the assertion should always succeed unless the schema changes.

The other workaround, which kinda goes against the genqlient way but may be practical if you just want this in a few places, is to use the bind option to specify your own type. Then you can just define a struct type, if that's all you ever need (You might even be able to use a named fragment and bind directly to that to force genqlient to do what you want -- although it's a bit sketchy and I'm not sure if you'll run into trouble.)

I'm definitely open to improving this. There are a few ways:

1. Make struct: true work in this case. (It currently doesn't accept fragments; but it could allow them as long as they apply to all types in the context -- and so on recursively.) My guess is this is not too hard, in fact from a quick glance you might just need to update the validation (see [here(https://github.com/Khan/genqlient/blob/8dc71037926c32fe163ffd297dc27bba1eb1a64a/generate/convert.go#L382)). It may also be the closest do "do what I mean" in your case? In general it's a useful option that we should probably publicize more (I kinda forgot about it honestly).
2. Make flatten: true work in this case. (Again, the rule would change from "the field type must implement the fragment type" to "all implementations of the field type must implement the fragment type".) This is kinda the better version of the workaround: you would do foo { ...ColorFragment }, and then flatten makes it so the struct field you get is directly Foo ColorFragment (instead of Foo FooUnion, and then all implementations of FooUnion in fact embed ColorFragment). I think from there you can do everything you want. This is also probably just a matter of changing the validation, although the flatten code is a bit more complex so I'm less sure of whether you'd have to do anything else.
3. Just add the fields to the interface, as you say. (Concretely, I think it's something like, if a fragment applies to all union members, add its fields to the union's generated interface.) This is a bit harder probably -- in theory maybe you can just change something in convertSelectionSet to make sharedFields have what you want, but the interface code can get pretty gnarly.

One worry I have is compatibility: if someone changes the schema to add a new type to the union that doesn't satisfy the interface, then your code fails. There's no way completely around that. In the workarounds it will just fail at runtime. I guess any of the three approaches described can at least fail at compile time? The thing I like about (1) and (2) is they are a bit less likely to cause that trouble to someone who doesn't expect it -- we can mention it in the options' documentation -- whereas in (3) you might just unthinkingly use the getter not knowing it's a bit sketchy. (On a related note, an advantage of (2) is it can continue to work if in the future you want to change your query to select some fragment that doesn't always apply. In general we try to make adding fields never break existing ones, but struct: true already ignores that, with a similar warning.)

Arguably, we could add a scarier version of the option(s), brittle_flatten: true, although perhaps there's no need. (Or maybe we should add a toplevel option ignore_schema_compat_issues: true that would unlock this stuff, for users who understand and accept the implications.)

One final thought is that Go may someday get real sum types. I don't think it makes sense to think too hard about what we'd do then -- indeed it may be we just have an option to represent them either way. But I imagine we might be able to do something better.

BTW, if you do make a PR, here's the test case against the existing schema I was using to play with this.

diff --git generate/testdata/queries/ComplexInlineFragments.graphql generate/testdata/queries/ComplexInlineFragments.graphql
index 456973d..a6da5c8 100644
--- generate/testdata/queries/ComplexInlineFragments.graphql
+++ generate/testdata/queries/ComplexInlineFragments.graphql
@@ -15,6 +15,9 @@ query ComplexInlineFragments {
     ... on Content { name }          # (4a) abstract spread in abstract scope, I == J
     ... on HasDuration { duration }  # (4d) abstract spread in abstract scope, neither implements the other
   }
+  randomLeaf {
+    ... on Content { name }
+  }
   repeatedStuff: randomItem {
     id
     id

[alex-torok]

Thank you for such a detailed response!

Then you can do resp.Foo.(YourColorInterface).GetColor(); the assertion should always succeed unless the schema changes.

This works well until you try to get the relatedFoo's color. All of the types for the relatedFoo will be different golang types due to being subtypes of each of the union implementation types (QueryFooARelatedFoo, QueryFooBRelatedFoo, etc...). Since golang interfaces work based on function signatures, the YourColorInterface can't implement all of the GetRelatedFoo methods at the same time. A small example of an interface not applying due to a concrete return type not matching the interface return type exactly - https://go.dev/play/p/GDaDSDqt5xP.

One worry I have is compatibility: if someone changes the schema to add a new type to the union that doesn't satisfy the interface, then your code fails.

The way that I was thinking about this was have it be something like "if any union member implements the interface fragment, add those fields to the generated interface. If no union member implements the interface, error out at codegen time saying that the fragment is invalid". There could be a world where a union has members where only some of them implement the interface. (I believe this is allowed and queryable in graphql, but tbh I don't use it often so not sure).

This addresses the concern of new types being added to the union and breaking existing uses. I tried implementing this logic by making the union handling logic (around here) create types for the implementationTypes and a new interfaceTypes (all interfaces that could apply to all implementation types of the union), but hit issues for generating the definitions for those interface types. That approach seems to be contrary to existing code handling how graphql interfaces apply to concrete types and are converted into golang types by the convertDefinition function.

If it were possible to implement, what would you think of the approach of having union definitions also expose golang types for all interfaces that could apply to any of the implementation types? (Perhaps hide it behind a directive since it is a somewhat niche usecase that could explode the amount of generated code?)

--

I do agree that option 1 or 2 seems easier to implement since it isn't breaking with a bunch of existing assumptions. I'll poke around a bit to see if I can get it working with a trivial PR. My current usecase of the shurcooL library is probably good enough for where I need this, so no promises on actually getting a PR up.

Thanks for the testing diff!

[benjaminjkraft]

The way that I was thinking about this was have it be something like "if any union member implements the interface fragment, add those fields to the generated interface. If no union member implements the interface, error out at codegen time saying that the fragment is invalid". There could be a world where a union has members where only some of them implement the interface. (I believe this is allowed and queryable in graphql, but tbh I don't use it often so not sure).

Yeah, the trouble is where only some implement the interface. The rule in GraphQL validation is that you can have a fragment as long as it applies to at least one of the concrete types (i.e., at least one of the union members implements the interface). So I think the last case is potentially a common one, and the source of trouble.

If it were possible to implement, what would you think of the approach of having union definitions also expose golang types for all interfaces that could apply to any of the implementation types? (Perhaps hide it behind a directive since it is a somewhat niche usecase that could explode the amount of generated code?)

I'm confused how this would work. What does it do when the interface doesn't apply? Does that even compile? Maybe I'm misunderstanding what you mean. Or if you mean only interfaces that apply to all implementations, then I think that's probably ok behind a flag, but I would suggest behind a flag because of the compat danger.