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

Annotated optional fields with omitempty by using new optional config value #308

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

rmennes
Copy link

@rmennes rmennes commented Feb 9, 2024

It could be useful to annotate optional fields annotate with omitempty when using pointers.
By allowing this option, input fields that get removed (and not used is the executed queries) will not break.
This gives the library an advantage to be more resilient against break API changes.

To introduce this option, a new optional config value is introduced pointer_omitempty.

If optional: pointer_omitempty is set. We generate

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

based on

input task_insert_input {
  id: Int
}

The @genqlient(omitempty) config will override the previous defined config.

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! I think this is pretty close to workable. Unless I'm misunderstanding, there's a tricky edge case still, but I think it should be fairly reasonable to fix.

@@ -262,6 +262,13 @@ func (g *generator) convertType(
// options work, recursing here isn't as connvenient.)
// Note this does []*T or [][]*T, not e.g. *[][]T. See #16.
goTyp = &goPointerType{goTyp}
} else if !options.PointerIsFalse() && (options.GetPointer() || (!typ.NonNull && g.Config.Optional == "pointer_omitempty")) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you may actually need to do this in two cases, one for omitempty and one for pointer. Otherwise, pointer: true overrides the global config, which is a no-op for pointer but actually disables omitempty. I'll comment in the test below where that happens.


Dt json.RawMessage `json:"dt"`

Tz *string `json:"tz"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

e.g. this should I think have omitempty

@rmennes
Copy link
Author

rmennes commented Feb 16, 2024

@benjaminjkraft Thanks for the feedback. I updated the PR accordingly. I hope I understood your suggestion correctly.

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 the updates! This option-checking code is getting confusing so I definitely may be missing something (a clearer test case written from scratch covering each possibility rather than or in addition to cobbling together existing ones might be more convincing). But I think it's wrong for the case where you say pointer: false -- that shouldn't remove the omitempty I don't think. Not sure how much that will come up but to avoid having to break backcompat later it's better if we can to get it correct now and avoid having to add pointer_omitempty_fixed later.

type __premarshal__PointersQueryInput struct {
Query *UserQueryInput `json:"query,omitempty"`

Dt json.RawMessage `json:"dt"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

here's an example, except this example doesn't matter since the RawMessage will never actually be null, but if you put the same config on a different field it would.

# Conflicts:
#	generate/testdata/snapshots/TestInvalidConfigs-InvalidOptional.yaml
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.

None yet

2 participants