Refactor argument-handling to use a struct #103

benjaminjkraft · 2021-09-17T01:18:28Z

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:

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.
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 omitempty generates broken code for structs #43 is actually quite nontrivial otherwise).
Once we have custom unmarshaler support (Allow mapping directly to a type but not using its standard JSON serialization #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

benjaminjkraft · 2021-09-17T17:31:17Z

Hmm, I realized this actually makes some more breaking changes, because my implementation of omitempty had some inconsistencies with JSON around structs. Let me think about this some more.

StevenACoffman

So that was my concern... that it is "breaking"-ish, which I guess if fine before our blog post and initial release version, but I'd like to do that sooner rather than later, and be more sure we don't need to again if we do this.

benjaminjkraft · 2021-09-17T18:54:13Z

Yeah, I think if we had more users I'd be more inclined to live with the existing behavior, but now's a good time to fix the inconsistency before anyone cares. (FWIW, this appears to be something that yaml changed from v1 to v2!) In practice it's not hard to search for potential problems (just find struct-typed fields with # genqlient(omitempty: true)), and webapp has none.

This is useful if you want to bind to a type you don't control (or use for other things) but need different serialization than its default. This is a feature gqlgen has and we've found it very useful. For example, in webapp we want to bind `DateTime` to `time.Time`, but its default serialization is not compatible with Python, so currently we have to bind to a wrapper type and cast all over the place, which is exactly the sort of boilerplate genqlient is supposed to avoid. For unmarshaling, the implementation basically just follows the existing support for abstract types; instead of calling our own generated helper, we now call your specified function. This required some refactoring to abstract the handling of custom unmarshalers generally from abstract types specifically, and to wire in not only the unmarshaler-name but also the `generator` (in order to compute the right import alias). For marshaling, I had to implement all that stuff over again; it's mostly parallel to unmarshaling (and I made a few minor changes to unmarshaling to make the two more parallel). Luckily, after #103 I at least only had to do it once, rather than implementing the same functionality for arguments and for input-type fields. It was still quite a bit of code; I didn't try to be quite as completionist about the tests as with unmarshal but still had to add a few. Issue: #38 Test plan: make check Reviewers: marksandstrom, steve, adam, jvoll, miguel, mahtab

dnerdy

Nice. I don't feel like we need to worry about breaking anyone at this point, plus the change is well documented.

dnerdy · 2021-09-21T02:21:50Z

generate/operation.go.tmpl

-    {{.GoName}} {{.GoType}},
+    {{- if .Input -}}
+    {{- range .Input.Fields -}}
+    {{/* the GraphQL name is here the user-specified variable-name */ -}}


typo? "is here" => "here is"

dnerdy · 2021-09-21T02:35:49Z

graphql/client.go

-	// Typically, GraphQL APIs will accept a JSON payload of the form
+	// `query myQuery { myField }`.  variables contains a JSON-marshalable
+	// value containing the variables to be sent along with the query,
+	// or may be nil if there are none.  Typically, GraphQL APIs wills


Suggested change

// or may be nil if there are none. Typically, GraphQL APIs wills

// or may be nil if there are none. Typically, GraphQL APIs will

dnerdy · 2021-09-21T02:44:12Z

generate/operation.go.tmpl

-        {{if not .Options.GetOmitempty -}}
-        "{{.GraphQLName}}": {{.GoName}},
+    {{- if .Input -}}
+    {{/* __input is not a legal variable name in GraphQL */ -}}


It might be helpful to be more explicit in this comment. Something like "__input won't clash with arguments to this function since all argument names are valid GraphQL variable names and __input isn't a legal variable name in GraphQL".

dnerdy · 2021-09-21T03:15:41Z

generate/convert.go

+		// note prefix is ignored here (see generator.typeName), as is
+		// selectionSet (for input types we use the whole thing).


I agree with the comment in the code below that it's a bit confusing to pass around arguments that aren't used. It may be helpful to explicitly say that selection sets don't apply to input objects, and input types may only be scalars, enums or (recursively) other input objects. A mention of ast.InputObject in convertDefinition might also be helpful.

Also, I think generator.typeName is a dangling reference? (Here and below in convertDefinition.)

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

This is useful if you want to bind to a type you don't control (or use for other things) but need different serialization than its default. This is a feature gqlgen has and we've found it very useful. For example, in webapp we want to bind `DateTime` to `time.Time`, but its default serialization is not compatible with Python, so currently we have to bind to a wrapper type and cast all over the place, which is exactly the sort of boilerplate genqlient is supposed to avoid. For unmarshaling, the implementation basically just follows the existing support for abstract types; instead of calling our own generated helper, we now call your specified function. This required some refactoring to abstract the handling of custom unmarshalers generally from abstract types specifically, and to wire in not only the unmarshaler-name but also the `generator` (in order to compute the right import alias). For marshaling, I had to implement all that stuff over again; it's mostly parallel to unmarshaling (and I made a few minor changes to unmarshaling to make the two more parallel). Luckily, after #103 I at least only had to do it once, rather than implementing the same functionality for arguments and for input-type fields. It was still quite a bit of code; I didn't try to be quite as completionist about the tests as with unmarshal but still had to add a few. Issue: #38 Test plan: make check Reviewers: marksandstrom, steve, adam, jvoll, miguel, mahtab

## Summary: This is useful if you want to bind to a type you don't control (or use for other things) but need different serialization than its default. This is a feature gqlgen has and we've found it very useful. For example, in webapp we want to bind `DateTime` to `time.Time`, but its default serialization is not compatible with Python, so currently we have to bind to a wrapper type and cast all over the place, which is exactly the sort of boilerplate genqlient is supposed to avoid. For unmarshaling, the implementation basically just follows the existing support for abstract types; instead of calling our own generated helper, we now call your specified function. This required some refactoring to abstract the handling of custom unmarshalers generally from abstract types specifically, and to wire in not only the unmarshaler-name but also the `generator` (in order to compute the right import alias). For marshaling, I had to implement all that stuff over again; it's mostly parallel to unmarshaling (and I made a few minor changes to unmarshaling to make the two more parallel). Luckily, after #103 I at least only had to do it once, rather than implementing the same functionality for arguments and for input-type fields. It was still quite a bit of code; I didn't try to be quite as completionist about the tests as with unmarshal but still had to add a few. Issue: #38 ## Test plan: make check Author: benjaminjkraft Reviewers: StevenACoffman, dnerdy, benjaminjkraft, aberkan, jvoll, mahtabsabet, MiguelCastillo Required Reviewers: Approved By: StevenACoffman, 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: #104

benjaminjkraft self-assigned this Sep 17, 2021

benjaminjkraft requested review from aberkan, dnerdy, jvoll, mahtabsabet, MiguelCastillo and StevenACoffman September 17, 2021 01:18

benjaminjkraft linked an issue Sep 17, 2021 that may be closed by this pull request

omitempty generates broken code for structs #43

Closed

StevenACoffman approved these changes Sep 17, 2021

View reviewed changes

benjaminjkraft mentioned this pull request Sep 17, 2021

Add support for binding with a custom marshal/unmarshal function #104

Merged

dnerdy approved these changes Sep 21, 2021

View reviewed changes

benjaminjkraft added 5 commits September 22, 2021 17:11

Fix tests, unskip a test that now passes!

ea8b4ec

Set IsInput correctly (oesn't matter yet, but will soon)

4eec142

Document second breaking change

145821d

Review comments

7794c1b

benjaminjkraft force-pushed the benkraft.argument-refactor branch from d344208 to 7794c1b Compare September 23, 2021 00:13

benjaminjkraft merged commit 5995653 into main Sep 23, 2021

benjaminjkraft deleted the benkraft.argument-refactor branch September 23, 2021 00:16

benjaminjkraft mentioned this pull request Sep 29, 2021

Use a different JSON library #47

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor argument-handling to use a struct #103

Refactor argument-handling to use a struct #103

benjaminjkraft commented Sep 17, 2021 •

edited

Loading

benjaminjkraft commented Sep 17, 2021

StevenACoffman left a comment

benjaminjkraft commented Sep 17, 2021

dnerdy left a comment

dnerdy Sep 21, 2021

dnerdy Sep 21, 2021

dnerdy Sep 21, 2021

dnerdy Sep 21, 2021

	// or may be nil if there are none. Typically, GraphQL APIs wills
	// or may be nil if there are none. Typically, GraphQL APIs will

		// note prefix is ignored here (see generator.typeName), as is
		// selectionSet (for input types we use the whole thing).

Refactor argument-handling to use a struct #103

Refactor argument-handling to use a struct #103

Conversation

benjaminjkraft commented Sep 17, 2021 • edited Loading

Summary:

Test plan:

benjaminjkraft commented Sep 17, 2021

StevenACoffman left a comment

Choose a reason for hiding this comment

benjaminjkraft commented Sep 17, 2021

dnerdy left a comment

Choose a reason for hiding this comment

dnerdy Sep 21, 2021

Choose a reason for hiding this comment

dnerdy Sep 21, 2021

Choose a reason for hiding this comment

dnerdy Sep 21, 2021

Choose a reason for hiding this comment

dnerdy Sep 21, 2021

Choose a reason for hiding this comment

benjaminjkraft commented Sep 17, 2021 •

edited

Loading