Add a mechanism to specify options on input-type fields #124

benjaminjkraft · 2021-10-01T01:34:23Z

Summary:

This has been a bit of a thorn since we started using genqlient in
production: just as you might want to specify, say, omitempty on an
argument, you might equally want to specify it on an input-type field.
But there's no obvious syntax to do that, because the input-type field
does not appear in the query (only the schema) so there's nowhere to put
the # @genqlient directive.

This commit, at last, fixes that problem, via a new option, for, which
you use in an option applied to the entire operation (or fragment), and
says, "actually, apply this directive to the given field, not the entire
operation". (It's mainly useful for input types, but I allowed it for
output types too; I could imagine it being convenient if you want to say
you always use a certain type or type-name for a certain field.) It
works basically like you expect: the inline options take precedence over
for take precedence over query-global options.

The implementation was fairly straightforward once I did a little
refactoring, mostly in the directive-parsing and directive-merging
(which are now combined, since merging is now a bit more complicated).
With that in place, and extended to support for, we need only add the
same wiring to input-fields that we have for other places you can put
directives. I did not attempt to solve the issue I've now documented
as #123, wherein conflicting options can lead to confusing behavior;
the new for is a new and perhaps more attractive avenue to cause it
but the issue remains the same and requires nontrivial refactoring
(described in the issue) to solve. (The breakage isn't horrible for the
most part; the option will just apply, or not apply, where you don't
expect it to.)

But while applying that logic, I noticed a problem, which is that we
were inconsistently cascading operation-level options down to
input-object fields. (I think this came out of the fact that initially
I thought to cascade them, then realized that this could cause problems
like #123 and intended to walk them back, but then accidentally only
"fixed" it for omitempty. I guess until this change, operation-level
options were rare enough, and input-field options messy enough, that no
one noticed.) So in this commit I bring things back into consistency,
by saying that they do cascade: with at least a sketch of a path forward
to solve #123 via better validation, I think that's by far the clearest
behavior.

Issue: #14

Test plan:

make check

This has been a bit of a thorn since we started using genqlient in production: just as you might want to specify, say, `omitempty` on an argument, you might equally want to specify it on an input-type field. But there's no obvious syntax to do that, because the input-type field does not appear in the query (only the schema) so there's nowhere to put the `# @genqlient` directive. This commit, at last, fixes that problem, via a new option, `for`, which you use in an option applied to the entire operation (or fragment), and says, "actually, apply this directive to the given field, not the entire operation". (It's mainly useful for input types, but I allowed it for output types too; I could imagine it being convenient if you want to say you always use a certain type or type-name for a certain field.) It works basically like you expect: the inline options take precedence over `for` take precedence over query-global options. The implementation was fairly straightforward once I did a little refactoring, mostly in the directive-parsing and directive-merging (which are now combined, since merging is now a bit more complicated). With that in place, and extended to support `for`, we need only add the same wiring to input-fields that we have for other places you can put directives. I did not attempt to solve the issue I've now documented as #123, wherein conflicting options can lead to confusing behavior; the new `for` is a new and perhaps more attractive avenue to cause it but the issue remains the same and requires nontrivial refactoring (described in the issue) to solve. (The breakage isn't horrible for the most part; the option will just apply, or not apply, where you don't expect it to.) But while applying that logic, I noticed a problem, which is that we were inconsistently cascading operation-level options down to input-object fields. (I think this came out of the fact that initially I thought to cascade them, then realized that this could cause problems like #123 and intended to walk them back, but then accidentally only "fixed" it for `omitempty`. I guess until this change, operation-level options were rare enough, and input-field options messy enough, that no one noticed.) So in this commit I bring things back into consistency, by saying that they do cascade: with at least a sketch of a path forward to fix #123 via better validation, I think that's by far the clearest behavior. Issue: #14 Test plan: make check Reviewers: csilvers, marksandstrom, steve, jvoll, adam, miguel, mahtab

csilvers · 2021-10-01T01:51:09Z

generate/testdata/queries/Pointers.graphql

@@ -1,11 +1,14 @@
 # @genqlient(pointer: true)
+# @genqlient(for: "UserQueryInput.id", pointer: false)
+# @genqlient(for: "User.id", pointer: false)
 query PointersQuery(


I think it would be good to put those example in the docs. All your examples have just one directive and I think it's not necessarily obvious how they combine. This examples makes it very clear.

csilvers

It turns out I need this for one of the mutations I'm converting! So I'm glad you managed to squeeze it in. Even though I'm not super-familiar with the genqlient code this seemed self-contained enough that it made sense to me, so I'm happy to approve it!

csilvers · 2021-10-01T06:13:23Z

generate/genqlient_directive.go

 	var err error
+	forField := ""
+	for _, arg := range graphQLDirective.Arguments {
+		if arg.Name == "for" {


Do you want to check if for is specified multiple times, and error out if so?

csilvers · 2021-10-01T06:18:49Z

generate/genqlient_directive.go

+		if dir.FieldDirectives[typeName] == nil {
+			dir.FieldDirectives[typeName] = make(map[string]*genqlientDirective)
+		}


Can this happen? I guess it doesn't hurt to be defensive, but if all genqlientDirectives are made via newGenqlientDirective then this if will never be true.

Yes -- since it's a map of maps we can only initialize the toplevel one upfront. (It really pains me that Go didn't make the zero value more usable here, but they didn't.)

Ah, I see. Yeah, being forced to call make to make maps is pretty annoying. You don't have to do it for slices!, so...

csilvers · 2021-10-01T06:23:51Z

generate/genqlient_directive.go

+	parent *ast.Definition,
+	operationDirective *genqlientDirective,
+) {
+	var forField *genqlientDirective


This is the for field for some parent element whose for points to us, is that right?

Yes, I'll comment better.

csilvers · 2021-10-01T06:25:17Z

generate/genqlient_directive.go

+// parent need only be set if node is an input-type field; it should be
+// the type containing this field.  (We can get this from gqlparser in other
+// cases, but not input-type fields.)


Do you maybe want to call it parentIfInputField then?

StevenACoffman

It turns out I need this for a mutation I'm doing as well, so very timely!

benjaminjkraft self-assigned this Oct 1, 2021

benjaminjkraft requested review from aberkan, csilvers, dnerdy, jvoll, mahtabsabet, MiguelCastillo and StevenACoffman October 1, 2021 01:34

benjaminjkraft linked an issue Oct 1, 2021 that may be closed by this pull request

Improve options-handling for input types #14

Closed

benjaminjkraft mentioned this pull request Oct 1, 2021

Validate against conflicting options on the same type #123

Open

csilvers reviewed Oct 1, 2021

View reviewed changes

csilvers approved these changes Oct 1, 2021

View reviewed changes

StevenACoffman approved these changes Oct 1, 2021

View reviewed changes

review comments

5b39e19

benjaminjkraft merged commit dd719de into main Oct 1, 2021

benjaminjkraft deleted the benkraft.input-field-options branch October 1, 2021 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a mechanism to specify options on input-type fields #124

Add a mechanism to specify options on input-type fields #124

benjaminjkraft commented Oct 1, 2021 •

edited

Loading

csilvers Oct 1, 2021

csilvers left a comment

csilvers Oct 1, 2021

csilvers Oct 1, 2021

benjaminjkraft Oct 1, 2021

csilvers Oct 1, 2021

csilvers Oct 1, 2021

benjaminjkraft Oct 1, 2021

csilvers Oct 1, 2021

StevenACoffman left a comment

Add a mechanism to specify options on input-type fields #124

Add a mechanism to specify options on input-type fields #124

Conversation

benjaminjkraft commented Oct 1, 2021 • edited Loading

Summary:

Test plan:

Choose a reason for hiding this comment

csilvers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StevenACoffman left a comment

Choose a reason for hiding this comment

benjaminjkraft commented Oct 1, 2021 •

edited

Loading