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

Improve options-handling for input types #14

Closed
benjaminjkraft opened this issue Apr 22, 2021 · 7 comments · Fixed by #124
Closed

Improve options-handling for input types #14

benjaminjkraft opened this issue Apr 22, 2021 · 7 comments · Fixed by #124
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@benjaminjkraft
Copy link
Collaborator

benjaminjkraft commented Apr 22, 2021

Right now you can't apply an option, like pointer: true, to a field of an input type, because there's nowhere to put it. So we need to figure out a syntax; maybe just pointerfields: ["field1", "field2.field3"] but that will be a bit of an explosion of options.

We also need to decide how to resolve conflicts: this would mean that two references to the same GraphQL input-type are no longer guaranteed to generate the same Go type, which means we have to prefix the names or something. (This is unlikely to come up within a single query, but more likely across queries.)

As a sort of workaround, you can construct the type inline:

query MyQuery(inputField: String!, ...) {
  myField(input: {
    field: $inputField,
    ....
  })
}
@benjaminjkraft benjaminjkraft modified the milestones: OSS "launch", Khan Apr 22, 2021
@benjaminjkraft
Copy link
Collaborator Author

Another related problem: you might want to be able to apply omitempty to a particular, or all, fields of the input object. Right now it only applies to arguments. This will actually require new code to handle!

@csilvers
Copy link
Member

csilvers commented Apr 23, 2021

This is, somewhat surprisingly, turning out to be a non-trivial barrier to genqlient adoption.

I thought of another suggestion, that maybe avoids some of the problems above: allow a user to specify the input fields they're interested in much like (in graphql) they specify the output fields. The code could be something like this:


func QueryMyOperation(...) {
   _ = `# @genqlient(inputFor: "MyOperationName")
   type MyOperationInput {
      email
      # @genqlient(pointer: true)
      validate
   }
   `
   _ = `#genqlient
  query MyOperationName($input: MyOperationInput) {
     user(input: $input) { id }
  `
  genqlient.MyOperationName(...)

Then genqlient would validate that the inputFor is a subset of the actual graphql type MyOperationInput, and that it includes all non-optional fields -- and would generate its own type for this, called MyOperationNameMyOperationInput or something. And that own type would follow the pointer/omitempty/etc directives. And then you could use that type name as an input to QueryMyOperation, or construct it inside QueryMyOperation, or whatever.

@benjaminjkraft
Copy link
Collaborator Author

Oh, that's a neat idea! I'll take a look at how to make it work.

benjaminjkraft added a commit that referenced this issue Aug 27, 2021
Summary:
We had this setting called "scalars", which said: bind this GraphQL type
to this Go type, rather than the one you would normally use.  It's
called that because it's most useful for custom scalars, where "the one
you would normally use" is "error: unknown scalar".  But nothing ever
stopped you from using it for a non-scalar type.  I was planning on
removing this functionality, because it's sort of a rough edge, but a
discussion with Craig found some good use cases, so instead, in this
commit, I document it better and add some slightly nicer ways to specify
it.

Specifically, here are a few potential non-scalar use cases:
- bind a GraphQL enum to a nonstandard type (or even `string`)
- bind an input type to some type that has exactly the fields you want;
  this acts as a sort of workaround for issues #14 and #44
- bind an object type to your own struct, so as to add methods to it
  (this is the use case Craig raised)
- bind an object type to your own struct, so as to share it between
  multiple queries (I believe named fragments will address this case
  better, but it doesn't hurt to have options)
- bind a GraphQL list type to a non-slice type in Go (presumably one
  with an UnmarshalJSON method), or any other different structure
The latter three cases still have the sharp edge I was originally
worried about, which is that nothing guarantees that the fields you
request in the query are the ones the type expects to get.  But I think
it's worth having the option, with appropriate disclaimers.

The main change to help support that better is that you can now specify
the type inline in the query, as an alternative to specifying it in the
config file; this means you might map a given object to a given struct,
but only in some cases, and when you do you have a chance to look at the
list of fields you're requesting.

Additionally, I renamed the config field from "scalars" to "bindings"
(but mentioned it in a few places where you might go looking for how to
map scalars, most importantly the error message you get for an unknown
(custom) scalar).  While I was making a breaking change, I also changed
it to be a `map[string]<struct>` instead of a `map[string]string`,
because I expect to add more fields soon, e.g. to handle issue #38.

Finally, since the feature is now intended/documented, I added some
tests, although it's honestly quite simple on the genqlient side.

Test Plan: make tesc

Reviewers: csilvers, marksandstrom, adam, miguel
benjaminjkraft added a commit that referenced this issue Aug 27, 2021
…pe (#69)

## Summary:
We had this setting called "scalars", which said: bind this GraphQL type
to this Go type, rather than the one you would normally use.  It's
called that because it's most useful for custom scalars, where "the one
you would normally use" is "error: unknown scalar".  But nothing ever
stopped you from using it for a non-scalar type.  I was planning on
removing this functionality, because it's sort of a rough edge, but a
discussion with Craig found some good use cases, so instead, in this
commit, I document it better and add some slightly nicer ways to specify
it.

Specifically, here are a few potential non-scalar use cases:
- bind a GraphQL enum to a nonstandard type (or even `string`)
- bind an input type to some type that has exactly the fields you want;
  this acts as a sort of workaround for issues #14 and #44
- bind an object type to your own struct, so as to add methods to it
  (this is the use case Craig raised)
- bind an object type to your own struct, so as to share it between
  multiple queries (I believe named fragments will address this case
  better, but it doesn't hurt to have options)
- bind a GraphQL list type to a non-slice type in Go (presumably one
  with an UnmarshalJSON method), or any other different structure
The latter three cases still have the sharp edge I was originally
worried about, which is that nothing guarantees that the fields you
request in the query are the ones the type expects to get.  But I think
it's worth having the option, with appropriate disclaimers.

The main change to help support that better is that you can now specify
the type inline in the query, as an alternative to specifying it in the
config file; this means you might map a given object to a given struct,
but only in some cases, and when you do you have a chance to look at the
list of fields you're requesting.

Additionally, I renamed the config field from "scalars" to "bindings"
(but mentioned it in a few places where you might go looking for how to
map scalars, most importantly the error message you get for an unknown
(custom) scalar).  While I was making a breaking change, I also changed
it to be a `map[string]<struct>` instead of a `map[string]string`,
because I expect to add more fields soon, e.g. to handle issue #38.

Finally, since the feature is now intended/documented, I added some
tests, although it's honestly quite simple on the genqlient side.

## Test plan:
make tesc


Author: benjaminjkraft

Reviewers: csilvers, aberkan, dnerdy, MiguelCastillo

Required Reviewers: 

Approved by: csilvers

Checks: ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ⌛ Lint, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Test (1.13), ⌛ Lint

Pull request URL: #69
@benjaminjkraft
Copy link
Collaborator Author

As I mention in the above-linked committ, the new bindings option is also a sort of workaround to this.

@benjaminjkraft
Copy link
Collaborator Author

benjaminjkraft commented Sep 7, 2021

Ugh, I think this is still worth doing, but the syntax is proving to be a mess.

I like Craig's idea above -- which I would modify to saying

fragment InputForMyQuery on MyInput {
  email
  # @genqlient(pointer: true)
  validate
}

# @genqlient(inputs: ["InputForMyQuery"])
query MyQuery(input: $MyInput) { ... }

That has the nice property that it parses, but sadly it doesn't validate (GraphQL doesn't allow fragments on input types, because where would you us them?). We could just disable that validation rule (replacing it with our own less restrictive one, perhaps)? Or we could say you specify it in a comment, like the @genqlient directive itself, but that gets messy because GraphQL has no block-comment syntax. (As does having it be an argument to the genqlient directive, because then it still all needs to be commented.)

Probably the least-bad is to just disable the validator. I don't like diverging from standard GraphQL syntax, but I think it's better than the alterrnatives; if we later find a better way we can add it as an alternative.

@benjaminjkraft
Copy link
Collaborator Author

An amusing bug: this means it's not possible to use the following type in genqlient (you need it to be a pointer):

input RecursiveInput { rec: RecursiveInput }

@benjaminjkraft benjaminjkraft self-assigned this Sep 9, 2021
@benjaminjkraft benjaminjkraft added enhancement New feature or request needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing labels Sep 11, 2021
benjaminjkraft added a commit that referenced this issue Sep 15, 2021
In this commit I add two related features to genqlient:
conflict-detection to avoid generating two distinct types with the same
name, and an option to specify the type-name genqlient should use for
some type.

The conflict-detection was pretty simple once I realized I had already
written all the code to do it in #70.  There was a bunch of wiring,
since we now need to keep track of the GraphQL type/selection-set that
each type corresponds to, but it was pretty straightforward.  This
allows us to:
- detect and reject if you have really sneaky type-names (there are some
  examples documented in `names.go`)
- more clearly crash if genqlient accidentally generates two conflicting
  types, and
- avoid stack-overflow when handing recursive (input) types (although
  sadly the poor support for options on input types (#14) makes them
  difficult to use in many cases; you really need to be able to set
  `pointer: true`)

And with that all set up, the type-naming was also easy!  (It doesn't
have to get into the core of the type-generator, just plug in where we
choose names.  The desire for conflict detection was the main reason I
hadn't set it up already.)  Note that the existing limitation of #70 that
the fields have to be in exactly the same order remains (and is now
documented as #93); it's not deeply hard to fix but it's surprisingly
much work.

Issue: #60
Issue: #12

Test plan: make check

Reviewers: csilvers, marksandstrom, adam, miguel, jvoll, mahtab
benjaminjkraft added a commit that referenced this issue Sep 16, 2021
In this commit I add two related features to genqlient:
conflict-detection to avoid generating two distinct types with the same
name, and an option to specify the type-name genqlient should use for
some type.

The conflict-detection was pretty simple once I realized I had already
written all the code to do it in #70.  There was a bunch of wiring,
since we now need to keep track of the GraphQL type/selection-set that
each type corresponds to, but it was pretty straightforward.  This
allows us to:
- detect and reject if you have really sneaky type-names (there are some
  examples documented in `names.go`)
- more clearly crash if genqlient accidentally generates two conflicting
  types, and
- avoid stack-overflow when handing recursive (input) types (although
  sadly the poor support for options on input types (#14) makes them
  difficult to use in many cases; you really need to be able to set
  `pointer: true`)

And with that all set up, the type-naming was also easy!  (It doesn't
have to get into the core of the type-generator, just plug in where we
choose names.  The desire for conflict detection was the main reason I
hadn't set it up already.)  Note that the existing limitation of #70 that
the fields have to be in exactly the same order remains (and is now
documented as #93); it's not deeply hard to fix but it's surprisingly
much work.

Issue: #60
Issue: #12

Test plan: make check

Reviewers: csilvers, marksandstrom, adam, miguel, jvoll, mahtab
benjaminjkraft added a commit that referenced this issue Sep 16, 2021
## Summary:
In this commit I add two related features to genqlient:
conflict-detection to avoid generating two distinct types with the same
name, and an option to specify the type-name genqlient should use for
some type.

The conflict-detection was pretty simple once I realized I had already
written all the code to do it in #70.  There was a bunch of wiring,
since we now need to keep track of the GraphQL type/selection-set that
each type corresponds to, but it was pretty straightforward.  This
allows us to:
- detect and reject if you have really sneaky type-names (there are some
  examples documented in `names.go`)
- more clearly crash if genqlient accidentally generates two conflicting
  types, and
- avoid stack-overflow when handing recursive (input) types (although
  sadly the poor support for options on input types (#14) makes them
  difficult to use in many cases; you really need to be able to set
  `pointer: true`)

And with that all set up, the type-naming was also easy!  (It doesn't
have to get into the core of the type-generator, just plug in where we
choose names.  The desire for conflict detection was the main reason I
hadn't set it up already.)  Note that the existing limitation of #70 that
the fields have to be in exactly the same order remains (and is now
documented as #93); it's not deeply hard to fix but it's surprisingly
much work.

Issue: #60
Issue: #12

## Test plan:
make check


Author: benjaminjkraft

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

Required Reviewers: 

Approved By: StevenACoffman, jvoll

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: #94
benjaminjkraft added a commit that referenced this issue 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
Copy link
Collaborator Author

Ok, I've put this off for too long, it's time to just choose the least-bad option.

I think the least-bad option in my mind is basically a variant of my original idea, namely:

# @genqlient(for: "MyInput.myField", <options>)
# <maybe more of these to apply to several fields>

If we come up with a better syntax later it shouldn't be hard to support it as an alternative. It'll be a little work to add support for repeated directives, but I think still easier than trying to get validation right for a fragment-like syntax, and it's a lot less ugly than doing pointerfields: <list>.

We will have to say that your directives must match exactly for all queries with the same inputs, unless you also use typename (this applies too all the approaches except fragments which would effectively require typename).

benjaminjkraft added a commit that referenced this issue Sep 17, 2021
Previously, we actually allowed you to put several genqlient directives
on the same node, but the semantics were undocumented (and somewhat
confusing, when it comes to `typename`).  In order to support directives
on input options, we're actually going to be encouraging this usage (see
notes in #14), so it's time to fix it.  To avoid confusion, I just had
conflicting directives be an error, rather than defining which one
"wins".

Test plan:
make check

Reviewers: steve, marksandstrom, jvoll, adam, miguel, mahtab
benjaminjkraft added a commit that referenced this issue 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 issue Sep 23, 2021
Previously, we actually allowed you to put several genqlient directives
on the same node, but the semantics were undocumented (and somewhat
confusing, when it comes to `typename`).  In order to support directives
on input options, we're actually going to be encouraging this usage (see
notes in #14), so it's time to fix it.  To avoid confusion, I just had
conflicting directives be an error, rather than defining which one
"wins".

Test plan:
make check

Reviewers: steve, marksandstrom, jvoll, adam, miguel, mahtab
benjaminjkraft added a commit that referenced this issue 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
benjaminjkraft added a commit that referenced this issue Sep 23, 2021
Previously, we actually allowed you to put several genqlient directives
on the same node, but the semantics were undocumented (and somewhat
confusing, when it comes to `typename`).  In order to support directives
on input options, we're actually going to be encouraging this usage (see
notes in #14), so it's time to fix it.  To avoid confusion, I just had
conflicting directives be an error, rather than defining which one
"wins".

Test plan:
make check

Reviewers: steve, marksandstrom, jvoll, adam, miguel, mahtab
benjaminjkraft added a commit that referenced this issue Sep 24, 2021
Previously, we actually allowed you to put several genqlient directives
on the same node, but the semantics were undocumented (and somewhat
confusing, when it comes to `typename`).  In order to support directives
on input options, we're actually going to be encouraging this usage (see
notes in #14), so it's time to fix it.  To avoid confusion, I just had
conflicting directives be an error, rather than defining which one
"wins".

Test plan:
make check

Reviewers: steve, marksandstrom, jvoll, adam, miguel, mahtab
benjaminjkraft added a commit that referenced this issue Sep 24, 2021
## Summary:
Previously, we actually allowed you to put several genqlient directives
on the same node, but the semantics were undocumented (and somewhat
confusing, when it comes to `typename`).  In order to support directives
on input options, we're actually going to be encouraging this usage (see
notes in #14), so it's time to fix it.  To avoid confusion, I just had
conflicting directives be an error, rather than defining which one
"wins".  The same applies to specifying the same option several
times in one directive.

I also fixed two small bugs:
- `typename` on an operation would incorrectly cascade down to
  all input types in a query (causing conflicts).
- directive parse errors had useless positions, now they're correct

## 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, ⌛ Test (1.17), ⌛ Test (1.16), ⌛ Test (1.15), ⌛ Test (1.14), ⌛ Lint

Pull Request URL: #105
@benjaminjkraft benjaminjkraft removed the needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing label Sep 24, 2021
benjaminjkraft added a commit that referenced this issue Oct 1, 2021
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
@benjaminjkraft benjaminjkraft linked a pull request Oct 1, 2021 that will close this issue
benjaminjkraft added a commit that referenced this issue Oct 1, 2021
## 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


Author: benjaminjkraft

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

Required Reviewers: 

Approved By: csilvers, StevenACoffman

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: #124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants