Premarshal struct does not maintain JSON tags (omitempty)

[kevinmichaelchen]

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.

[benjaminjkraft]

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.

[kevinmichaelchen]

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]

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

[kevinmichaelchen]

Hey @benjaminjkraft — threw up a PR here: https://github.com/Khan/genqlient/pull/267

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

Should I add/modify documentation anywhere?

[kevinmichaelchen]

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]

267