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

Big refactor to separate operation-traversal from code-generation #51

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

benjaminjkraft
Copy link
Collaborator

Summary:

I've felt for a while that types.go is way too confusing, and as I
started to implement some of the more complex cases of generating
interface-types, the cracks were really starting to show. Luckily, I
also finally realized how to fix it: we need to separate the process of
traversing the GraphQL operation and schema to decide what types to
generate from the process of actually generating those types. This
requires an extra set of intermediate data structures, but I think it
makes things quite a lot easier to understand -- and, importantly, it
means that the code-generation doesn't need to go in the order we
traverse the query/schema.

So in this commit, I did that huge refactor. It's probably best to just
review types.go and traverse.go as if they were new; the old code was
quite hard to understand and the new code will hopefully make a lot more
sense. (And to that end, review comments about what could be organized
better or needs more documentation are very much in order, even for code
that is mostly unchanged.)

This does introduce one bug, sort of, which is that rather than
generating broken code for list-of-interface fields, we generate no code
at all. (A TODO in unmarshal.go describes why.) I'll fix this when I
add support for those fields. (It's all behind the AllowBrokenFeatures
flag, anyway.) Otherwise, the only changes to generated code are that a
few methods are ordered differently, because we now generate the
implements-interface methods with the interface, rather than the
implementations, as it's much simpler that way. (In GraphQL, unlike Go,
we know the list of all possible implementations of each interface, so
this is possible.)

Test plan:

golangci-lint run ./... && go test ./...

I've felt for a while that types.go is way too confusing, and as I
started to implement some of the more complex cases of generating
interface-types, the cracks were really starting to show.  Luckily, I
also finally realized how to fix it: we need to separate the process of
traversing the GraphQL operation and schema to decide what types to
generate from the process of actually generating those types.  This
requires an extra set of intermediate data structures, but I think it
makes things quite a lot easier to understand -- and, importantly, it
means that the code-generation doesn't need to go in the order we
traverse the query/schema.

So in this commit, I did that huge refactor.  It's probably best to just
review types.go and traverse.go as if they were new; the old code was
quite hard to understand and the new code will hopefully make a lot more
sense.  (And to that end, review comments about what could be organized
better or needs more documentation are very much in order, even for code
that is mostly unchanged.)

This does introduce one bug, sort of, which is that rather than
generating broken code for list-of-interface fields, we generate no code
at all.  (A TODO in unmarshal.go describes why.)  I'll fix this when I
add support for those fields.  (It's all behind the AllowBrokenFeatures
flag, anyway.)  Otherwise, the only changes to generated code are that a
few methods are ordered differently, because we now generate the
implements-interface methods with the interface, rather than the
implementations, as it's much simpler that way.  (In GraphQL, unlike Go,
we know the list of all possible implementations of each interface, so
this is possible.)

Test plan: golangci-lint run ./... && go test ./...
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.

I think having an intermediate data structure makes a lot of sense; I found it pretty easy to follow the new code. The feedback I have is pretty superficial:

  • The file name traverse.go made sense once I understood what the code was doing, but it wasn't clear on the initial read that that was the file I needed to go to. Most of the functions in the file have a name that starts with "convert". Naming the file convert.go or something similar could help with discovery.
  • Similarly, comments.go is related to genqlient directives, which happen to be powered by comments, but it wasn't clear to me at first that that's the place I should look for the directive code.
  • Finally (and not at all related to this diff), I was surprised to see that template execution uses runtime.Caller(0) to locate a template. I think this only works when using go run; i.e. this would break if the user only has is a genqlient executable. Using //go:embed would work in both cases: https://pkg.go.dev/embed.

generate/traverse.go Outdated Show resolved Hide resolved

// Reference returns the Go name of this type, e.g. []*MyStruct, and may be
// used to refer to it in Go code.
Reference() string
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about naming this "TypeName" instead of "Reference". "Reference" sounds like this should always be a pointer.

Copy link
Collaborator Author

@benjaminjkraft benjaminjkraft Aug 23, 2021

Choose a reason for hiding this comment

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

The problem is it's not always a type-name either since it might be []*MyStruct. (Or, for goOpaqueType, even map[string]interface{}.) But maybe that's at least closer.

@benjaminjkraft
Copy link
Collaborator Author

* The file name `traverse.go` ...
* Similarly, `comments.go` ...

Good ideas, will do!

* Finally (and not at all related to this diff), I was surprised to see that template execution uses `runtime.Caller(0)` to locate a template. I think this only works when using `go run`; i.e. this would break if the user only has is a genqlient executable. Using `//go:embed` would work in both cases: https://pkg.go.dev/embed.

I believe it works as long as the executable was compiled on the same machine (and the genqlient checkout used to do so has not been moved, etc.) although I haven't tried that. I agree embedding would be a better long-term solution, but we need to either use a third-party package or wait until we drop support for Go 1.13-1.15 (and we definitely can't drop 1.15 yet); see #9.

I didn't rename Reference yet, since I want to think about that a little
more.
@benjaminjkraft benjaminjkraft merged commit 8815d09 into main Aug 25, 2021
@benjaminjkraft benjaminjkraft deleted the benkraft.refactor branch August 25, 2021 19:12
benjaminjkraft added a commit that referenced this pull request Sep 17, 2021
In this commit I refactor the argument-generation logic to move most of
the code out of the template and into the type-generator.  This logic
predates #51, and I didn't think to update it there, but I think it
benefits from similar treatment, for similar reasons.

Specifically, the main change is to treat variables as another struct
type we can generate, rather than handling them inline as a
`map[string]interface{}`.  Users still pass them the same way, but
instead of putting them into a `map[string]interface{}` and JSONifying
that, we generate a struct and put them there.

This turns out to simplify things quite a lot, because we already have a
lot of code to generate types.  Notably, the omitempty code goes from a
dozen lines to basically two, and fixes a bug (#43) in the process,
because now that we have a struct, `json.Marshal` will do our work for
us! (And, once we have syntax for it (#14), we'll be able to handle
field-level omitempty basically for free.)  More importantly, it will
simplify custom marshalers (#38, forthcoming) significantly, since we do
all that logic at the containing-struct level, but will need to apply it
to arguments.

It does require one breaking change, for folks implementing the
`graphql.Client` API (rather than just calling `NewClient`): we now pass
them variables as an `interface{}` rather than a
`map[string]interface{}`.  For most callers, including Khan/webapp, this
is basically a one-line change to the signature of their `MakeRequest`.

Issue: #38
Issue: #43

Test plan:
make check

Reviewers: marksandstrom, steve, jvoll, mahtab, adam, miguel
benjaminjkraft added a commit that referenced this pull request Sep 23, 2021
In this commit I refactor the argument-generation logic to move most of
the code out of the template and into the type-generator.  This logic
predates #51, and I didn't think to update it there, but I think it
benefits from similar treatment, for similar reasons.

Specifically, the main change is to treat variables as another struct
type we can generate, rather than handling them inline as a
`map[string]interface{}`.  Users still pass them the same way, but
instead of putting them into a `map[string]interface{}` and JSONifying
that, we generate a struct and put them there.

This turns out to simplify things quite a lot, because we already have a
lot of code to generate types.  Notably, the omitempty code goes from a
dozen lines to basically two, and fixes a bug (#43) in the process,
because now that we have a struct, `json.Marshal` will do our work for
us! (And, once we have syntax for it (#14), we'll be able to handle
field-level omitempty basically for free.)  More importantly, it will
simplify custom marshalers (#38, forthcoming) significantly, since we do
all that logic at the containing-struct level, but will need to apply it
to arguments.

It does require one breaking change, for folks implementing the
`graphql.Client` API (rather than just calling `NewClient`): we now pass
them variables as an `interface{}` rather than a
`map[string]interface{}`.  For most callers, including Khan/webapp, this
is basically a one-line change to the signature of their `MakeRequest`.

Issue: #38
Issue: #43

Test plan:
make check

Reviewers: marksandstrom, steve, jvoll, mahtab, adam, miguel
benjaminjkraft added a commit that referenced this pull request Sep 23, 2021
## Summary:
In this commit I refactor the argument-generation logic to move most of
the code out of the template and into the type-generator.  This logic
predates #51, and I didn't think to update it there, but I think it
benefits from similar treatment, for similar reasons.

Specifically, the main change is to treat variables as another struct
type we can generate, rather than handling them inline as a
`map[string]interface{}`.  Users still pass them the same way, but
instead of putting them into a `map[string]interface{}` and JSONifying
that, we generate a struct and put them there.

This turns out to simplify things quite a lot, because we already have a
lot of code to generate types.  Notably, the omitempty code goes from a
dozen lines to basically two, and fixes a bug (#43) in the process,
because now that we have a struct, `json.Marshal` will do our work for
us! (And, once we have syntax for it (#14), we'll be able to handle
field-level omitempty basically for free.)  More importantly, it will
simplify custom marshalers (#38, forthcoming) significantly, since we do
all that logic at the containing-struct level, but will need to apply it
to arguments.

It does require two breaking changes:

1. For folks implementing the `graphql.Client` API (rather than just
   calling `NewClient`): we now pass them variables as an `interface{}`
   rather than a `map[string]interface{}`.  For most callers, including
   Khan/webapp, this is basically a one-line change to the signature of
   their `MakeRequest`, and it should be a lot more future-proof.
2. genqlient's handling of the `omitempty` option has changed to match
   that of `encoding/json`, in particular it now never considers structs
   "empty".  The difference was never intentional (I just didn't realize
   that behavior of `encoding/json`); arguably our behavior was more
   useful but I think that's outweighed by the value of consistency with
   `encoding/json` as well as the simpler and more correct
   implementation (fixing #43 is actually quite nontrivial otherwise).
   Once we have custom unmarshaler support (#38), users will be able to
   map a zero value to JSON null if they wish, which is mostly if not
   entirely equivalent for GraphQL's purposes.

Issue: #38
Issue: #43

## Test plan:
make check

Author: benjaminjkraft

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

Required Reviewers: 

Approved By: StevenACoffman, dnerdy

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

Pull Request URL: #103
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.

2 participants