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

Add getter methods to all fields, not just where needed #126

Merged
merged 2 commits into from
Oct 1, 2021

Conversation

benjaminjkraft
Copy link
Collaborator

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

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

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

Reviewers: steve, marksandstrom, csilvers, jvoll, adam, miguel, mahtab
Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

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

Seems easy enough! Hopefully all these extra methods don't bulk up the generated code size too much.

Copy link
Contributor

@dnerdy dnerdy left a comment

Choose a reason for hiding this comment

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

Nice reuse of FlattenedFields. Looks good to me!

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.

3 participants