Skip to content

Commit

Permalink
Add getter methods to all fields, not just where needed (#126)
Browse files Browse the repository at this point in the history
## Summary:
If your type implements an interface, we add getter methods for the
shared fields, so that those may be accessed via the interface.  But it
turns out occasionally it's useful to have these getter methods when
they don't implement a GraphQL interface, so you can use two
genqlient-generated types in the same function if they have the same
fields.  (This comes up most often when you have a GraphQL union that
maybe should really be an interface, or if you don't yet support
interfaces implementing other interfaces (indeed our parser doesn't
either).  But one can imagine other use cases.)

We can't predict how you want to do that, so we can't generate the
interface, but we can generate the methods, so you can define the
interface and do a type assertion from there.  Since these methods are
pretty simple to generate, we just do it always.  (As with #120, if
binary size becomes an issue we could later add an option to only
generate methods that are truly needed but including them seems like the
better default.)

This also fixes a subtle and rare bug, which would have become much more
common (indeed existing tests caught it).  Specifically, if you have a
query like
```graphql
fragment FragmentOne on T { id }
fragment FragmentTwo on T { id }
query Q {
    f {   # interface type T
        ...FragmentOne
        ...FragmentTwo
    }
}
```
since both `FragmentOne` and `FragmentTwo` request some common field,
say `id`, we generate a method `GetId` on each one.  But since
`FragmentOne` and `FragmentTwo` are both on `T`, we also include their
interfaces in the interface we generate for the type of `f`, `QFT`.  So
`QFT` includes a method `GetId`.  But on the implementations, the two
methods conflict, and neither gets promoted; this causes various code to
fail to compile.  With this change, this would have happened much more
frequently -- even if only one of the two fragments is on `T`, as long
as both request the field.  Anyway, we now generate explicit methods on
each struct for all of its recursively emebedded fields -- using the
logic from #120 to compute them -- so that we don't need to rely on
method-promotion.

## Test plan:
make tesc


Author: benjaminjkraft

Reviewers: csilvers, dnerdy, aberkan, jvoll, mahtabsabet, MiguelCastillo, StevenACoffman

Required Reviewers: 

Approved By: csilvers, dnerdy

Checks: ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint, ✅ Test (1.17), ✅ Test (1.16), ✅ Test (1.15), ✅ Test (1.14), ✅ Lint

Pull Request URL: #126
  • Loading branch information
benjaminjkraft committed Oct 1, 2021
1 parent 9906a7b commit 3a5bf46
Show file tree
Hide file tree
Showing 52 changed files with 2,227 additions and 877 deletions.
4 changes: 4 additions & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@ When releasing a new version:

### New features:

- genqlient now generates getter methods for all fields, even those which do not implement a genqlient-generated interface; this can be useful for callers who wish to define their own interface and have several unrelated genqlient types which have the same fields implement it.

### Bug fixes:

- In certain very rare cases involving duplicate fields in fragment spreads, genqlient would generate code that failed to compile due to duplicate methods not getting promoted; genqlient now generates correct types. (See #126 for a more complete description.)

## v0.3.0

Version 0.3.0 adds several new configuration options, allowing simplification of generated types and configuration of input types, as well as marshalers for all genqlient-generated types.
Expand Down
16 changes: 15 additions & 1 deletion docs/FAQ.md
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,21 @@ query GetMonopolyPlayers {
```
and genqlient will skip the indirection and give the field `Winner` type `MonopolyUser` directly. This is often much more convenient if you put all the fields in the fragment, like the first query did. See the [options documentation](genqlient_directive.graphql) for more details.

**Type names:** Finally, if you always want exactly the same fields, you can use the simpler but more restrictive genqlient option `typename`:
**Interfaces:** For each struct field it generates, genqlient also generates an interface method. If you want to share code between two types which to GraphQL are unrelated, you can define an interface containing that getter method, and genqlient's struct types will implement it. (Depending on your exact query, you may need to do a type-assertion from a genqlient-generated interface to yours.) For example, in the above query you could simply do:
```go
type MonopolyUser interface {
GetId() string
GetName() string
}

func FormatUser(user MonopolyUser) { ... }

FormatUser(resp.Game.Winner)
```

In general in such cases it's better to change the GraphQL schema to show how the types are related, and use one of the other mechanisms, but this option is useful for schemas where you can't do that, or in the meantime.

**Type names:** Finally, if you always want exactly the same fields on exactly the same types, and don't want to deal with interfaces at all, you can use the simpler but more restrictive genqlient option `typename`:

```graphql
query GetMonopolyPlayers {
Expand Down
21 changes: 21 additions & 0 deletions example/generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Large diffs are not rendered by default.

Loading

0 comments on commit 3a5bf46

Please sign in to comment.