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

Add omitempty json tags for nullable fields #2436

Merged
merged 1 commit into from
Mar 9, 2023
Merged

Add omitempty json tags for nullable fields #2436

merged 1 commit into from
Mar 9, 2023

Conversation

StevenACoffman
Copy link
Collaborator

@StevenACoffman StevenACoffman commented Nov 25, 2022

Fix #789: Request for omitempty on json struct tag for models

Per @Luscha PR #1477 : Nullable fields generated with json omit empty tag

Nullable fields should be generated with json omit empty tag.
There are cases where the graphql server accepts only one of the field to perform a mutation (an example could be performing a mutation on an union field which expects only one of the type reference set).

This is the case working in particular with Dgraph, which expects a strict format for inputs.

Per @joshpauline in #2325

There is already an open pr for this feature however it had merge conflicts so I would like to fix those and merge this. This feature helps a lot when you are using the generated structs with external services. e.g Saving data to a DB

I have rebased that PR and regenerated it

@StevenACoffman StevenACoffman added the help wanted Extra attention is needed label Dec 3, 2022
@StevenACoffman
Copy link
Collaborator Author

Looks like rebasing isn't enough, and it requires a bit of cleanup, but I'm not able to give it attention right now. Help wanted.

@StevenACoffman StevenACoffman force-pushed the josh branch 2 times, most recently from ce77eb4 to 3eab763 Compare December 21, 2022 00:03
Signed-off-by: Steve Coffman <steve@khanacademy.org>
@coveralls
Copy link

Coverage Status

Coverage: 75.407% (-0.03%) from 75.441% when pulling 590f62b on josh into acbae6f on master.

@StevenACoffman StevenACoffman merged commit 0bbc7f8 into master Mar 9, 2023
@StevenACoffman StevenACoffman deleted the josh branch March 9, 2023 19:52
@koenpunt
Copy link

koenpunt commented Apr 1, 2023

I believe omitempty will also leave out fields which are explicitly set to nil, which isn't correct; in graphql there's a difference between sending in null and omitting a field.

@StevenACoffman
Copy link
Collaborator Author

Hmmm... I won't get a chance to look at this soon, so if you can make a PR, I would appreciate it.

@adomaskizogian
Copy link
Contributor

adomaskizogian commented Dec 13, 2023

I believe omitempty will also leave out fields which are explicitly set to nil, which isn't correct; in graphql there's a difference between sending in null and omitting a field.

Having the same question. Has anyone experienced any issues on live systems?

Update: If anyone stumbles upon this, know that at the moment I wrote this, gqlgen used a custom built gql marshaler that is totally unrelated to encoding/json

type Marshaler interface {
    MarshalGQL(w io.Writer)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request for omitempty on json tag for models
5 participants