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

feat: always set omitempty on pointer fields #287

Closed
wants to merge 2 commits into from

Conversation

obitech
Copy link

@obitech obitech commented Jul 27, 2023

When optional: "pointer" is configured, omitempty is needed for input fields for the generated code to be semantically correct. Without omitempty an empty pointer would generate an "empty value" which is incorrect.

Consider the following graphql schema generated by Hasura:

input task_insert_input {
  id: Int
}

Without this fix, the following golang struct will be generated:

type Task_insert_input struct {
  Id: *int `json:"id"`
}

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up!

I think my comment in the original PR that we'll need this flagged somehow is still relevant:

I think as-is this is in some sense a breaking change, although perhaps anyone who cares about this prefers the new behavior? It also seems confusing to do this only and automatically for optional: pointer, which right now is I think otherwise equivalent to # @genqlient(pointer: true) on every optional field. Maybe we add a new config optional: pointer_and_omitempty and handle both issues that way?

(Edit: Thinking more, doing a new config doesn't strictly handle the second issue -- you could want this for non-pointer: true fields. But I guess you can always annotate those individually, and probably you never want it for non-pointer fields globally (since that means false gets omitted). So I guess it's still maybe the best compromise.)

I'm not sure if it makes more sense to do a completely separate option, or a separate value for optional, or what, but we'll need one or the other! Eventually maybe we change the default here but I'd like to see more of how optional: generic (and generally Go style around how to represent Optional[T]) develops, if nothing else, so I think for now it's best to avoid that. (Plus, really, what we need is a Hasura template -- at which point it doesn't matter what the defaults are. A reminder to comment on #272 if you have any thoughts!)

And it still needs some tests and documentation for the new behavior :-) .

@StevenACoffman StevenACoffman removed their request for review October 12, 2023 18:39
@justonia
Copy link

What's the status on this? I'm working with the Linear API and it's broken if omitempty isn't supported.

@staugaard
Copy link

I'm also waiting for this feature.

@benjaminjkraft
Copy link
Collaborator

Closing in favor of #308 which uses a config option

jmhobbs added a commit to chromaui/genqlient that referenced this pull request Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants