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.

Sign up for GitHub

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

Jump to bottom

omitempty generates broken code for structs #43

Closed
benjaminjkraft opened this issue May 3, 2021 · 2 comments · Fixed by #103
Closed

omitempty generates broken code for structs #43

benjaminjkraft opened this issue May 3, 2021 · 2 comments · Fixed by #103
Labels
bug Something isn't working good first issue Good for newcomers help wanted Issues that anyone could pick up and implement if useful to them

Comments

@benjaminjkraft
Copy link
Collaborator

benjaminjkraft commented May 3, 2021 •
edited
Loading

You get

	var zero_myVar []string
	if myVar != zero_myVar {
		variables["myVar"] = myVar
	}

which is semantically correct but doesn't compile, because arrays aren't comparable. We should instead compare to nil (or check len, depending what we want to do with []string{}) for slices and maps.

As a workaround, since []string(nil) encodes to null, there's usually no need for omitempty on such fields, unless the backend actually makes a distinction between the argument being null and unset.
@benjaminjkraft benjaminjkraft added this to the Khan milestone May 3, 2021
@benjaminjkraft benjaminjkraft reopened this Jun 3, 2021
@benjaminjkraft
Copy link
Collaborator Author

benjaminjkraft commented Jun 3, 2021 •
edited
Loading

This also affects struct { F []string }, which will be a little harder to fix. It's also an issue for non-comparable custom scalar types, but those can probably just be a TODO for now.

benjaminjkraft added a commit that referenced this issue Jun 3, 2021
@benjaminjkraft
Actually build the generated code in tests
dfc9f02 
This revealed one bug already (a redux of #43)!  And will hopefully help
more as unmarshalers get more complicated (thanks to
interfaces/fragments).
@benjaminjkraft benjaminjkraft changed the title omitempty generates broken code for arrays omitempty generates broken code for structs Aug 26, 2021
@benjaminjkraft benjaminjkraft removed this from the Khan milestone Aug 26, 2021
@benjaminjkraft benjaminjkraft added bug Something isn't working good first issue Good for newcomers help wanted Issues that anyone could pick up and implement if useful to them labels Sep 11, 2021
benjaminjkraft added a commit that referenced this issue Sep 17, 2021
@benjaminjkraft
Refactor argument-handling to use a struct
6d654b4 
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 benjaminjkraft mentioned this issue Sep 17, 2021
Merged
@benjaminjkraft benjaminjkraft linked a pull request Sep 17, 2021 that will close this issue
Refactor argument-handling to use a struct #103
Merged
@benjaminjkraft
Copy link
Collaborator Author

This is also incorrect if you use bindings to map some type to a slice.

benjaminjkraft added a commit that referenced this issue Sep 23, 2021
@benjaminjkraft
Refactor argument-handling to use a struct
8ef48f1 
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
@benjaminjkraft
Refactor argument-handling to use a struct (#103)
5995653 
## 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
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers help wanted Issues that anyone could pick up and implement if useful to them
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor argument-handling to use a struct Khan/genqlient
1 participant
@benjaminjkraft