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

Premarshal struct does not maintain JSON tags (omitempty) #263

Closed
kevinmichaelchen opened this issue Apr 19, 2023 · 6 comments
Closed

Premarshal struct does not maintain JSON tags (omitempty) #263

kevinmichaelchen opened this issue Apr 19, 2023 · 6 comments
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

@kevinmichaelchen
Copy link
Contributor

kevinmichaelchen commented Apr 19, 2023

Describe the bug

When you use custom type bindings, a premarshal struct gets generated.

However, the premarshal struct won't have omitempty tags.

I think this is resulting in some buggy behavior when using Hasura.

To Reproduce

I tried to do a minimal reproduction here in this repo.

Specifically, in this comment, I noticed that things work fine when I don't use custom type bindings.

Expected behavior

I'm not sure if this is expected behavior or not. Should the premarshal struct not have the same JSON tags as the original struct?

genqlient version

Using the latest v0.5.0.

@kevinmichaelchen kevinmichaelchen added the bug Something isn't working label Apr 19, 2023
@benjaminjkraft
Copy link
Collaborator

Thanks for the clear repro! It took me a while to understand the subtle case even looking at all the code so the fact that it was all there was very helpful.

I think you're right that we should just propagate the json tags. It's happening in the first place because the premarshal types are generated in a separate code path (regular types, premarshal types), which arguably we could change. But anyway that's probably not necessary right now, probably we can just add a check for omitempty in the premarshal code.

I'm hoping to have some time to work on genqlient this weekend and if I do will try to get to this, but if you are excited to, I think this would actually be a good first PR; the code change is probably one line (in the premarshal types linked above; I think the data we need is already available) and the main thing is just adding some unit and integration tests to show that it works.

@benjaminjkraft benjaminjkraft added good first issue Good for newcomers help wanted Issues that anyone could pick up and implement if useful to them labels May 4, 2023
@kevinmichaelchen
Copy link
Contributor Author

kevinmichaelchen commented May 4, 2023

Hey @benjaminjkraft — glad my repro was helpful!

Me, taking a guess

If I had to take a crack at it, I'd say we want to add

{{if .Omitempty -}},omitEmpty{{end}}

in the necessary places.

For example, we'd change this:

    {{.GoName}} {{repeat .GoType.SliceDepth "[]"}}{{ref "encoding/json.RawMessage"}} `json:"{{.JSONName}}"`
    {{else}}
    {{.GoName}} {{.GoType.Reference}} `json:"{{.JSONName}}"`

to something like this:

    {{.GoName}} {{repeat .GoType.SliceDepth "[]"}}{{ref "encoding/json.RawMessage"}} `json:"{{.JSONName}}{{if .Omitempty -}},omitEmpty{{end}}"`
    {{else}}
    {{.GoName}} {{.GoType.Reference}} `json:"{{.JSONName}}{{if .Omitempty -}},omitEmpty{{end}}"`

where Omitempty comes from goStructField.

Contributing

If that sounds like the correct approach, I can try to put up a PR and look into adding some test cases where needed.

@benjaminjkraft
Copy link
Collaborator

Yep I think that's right! Good catch that we need the change in both places.

@kevinmichaelchen
Copy link
Contributor Author

kevinmichaelchen commented May 4, 2023

Hey @benjaminjkraft — threw up a PR here: #267

Still need to sign the CLA and add a changelog entry.

Should I add/modify documentation anywhere?

@kevinmichaelchen
Copy link
Contributor Author

kevinmichaelchen commented May 4, 2023

In the meantime, I'll try to run my fork and see if it generates Hasura sub-sub-fields in the expected way.

Update: I ran it against nested BoolExp structs, and it looks like I'm still missing something!

benjaminjkraft pushed a commit that referenced this issue May 5, 2023
This PR addresses a bug described in #263

Basically, whenever a struct involves a custom genqlient binding, a
secondary "premarshal" struct gets generated.

The bug was that this "premarshal" struct was not propagating the
`omitempty` JSON tags, which was resulting in unexpected behavior.

The fix involved a few changed lines in the Go template, and a few
changes in the unit tests.

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [x] Included a link to the issue fixed, if applicable
- [x] Included documentation, for new features (n/a)
- [x] Added an entry to the changelog
@benjaminjkraft
Copy link
Collaborator

#267

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

No branches or pull requests

2 participants