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

# @genqlient(for: "Input.nonOptionalField", omitempty: false) not working #290

Closed
Anas737 opened this issue Jul 28, 2023 · 4 comments · Fixed by #338
Closed

# @genqlient(for: "Input.nonOptionalField", omitempty: false) not working #290

Anas737 opened this issue Jul 28, 2023 · 4 comments · Fixed by #338
Labels
bug Something isn't working help wanted Issues that anyone could pick up and implement if useful to them
Milestone

Comments

@Anas737
Copy link

Anas737 commented Jul 28, 2023

Describe the bug
Hey all 👋

We are having a use-case where we want to apply omitempty on all fields except a mandatory graphql field.

We've tried the below code both at the top of the query and at the top of the $input line:
# @genqlient(omitempty: true)
# @genqlient(for: "Input.nonOptionalField", omitempty: false)

But we got the error:
omitempty may only be used on optional arguments

Shouldn't omitempty be considered for non optional graphql fields when set to false?

When using # @genqlient(omitempty: true) only, it works on all fields including non optional graphql fields and adds the json:"fieldname,omitempty" to them.

To Reproduce
Schema:

input Input {
    requiredField: Type!
    optionalField: Type
}

Query:

# @genqlient(omitempty: true)
# @genqlient(for: "Input.requiredField", omitempty: false)
mutation Name(
  $input: Input!
) {
  name(input: $input) {
     ...
  }
}

Happens at code generation running:
go run -mod=readonly github.com/Khan/genqlient@v0.5.0

Expected behavior
When using the below code at the top of the query:
# @genqlient(omitempty: true)
# @genqlient(for: "Input.nonOptionalField", omitempty: false)

I would expect omitempty to be added to all fields except the Input.nonOptionalField one.
I tested and it worked when using only optional fields.

genqlient version
Tested on both 0.5.0 and 0.6.0

@Anas737 Anas737 added the bug Something isn't working label Jul 28, 2023
@benjaminjkraft
Copy link
Collaborator

benjaminjkraft commented Aug 1, 2023

Thanks for raising this. I agree this is a bug. We can probably just change the validation (here and a few
similar places in the same file) to only reject omitempty: true, not omitempty: false on non-nullable fields. Or, more correct (although probably more work) would be to just change the behavior so a global omitempty only applies to non-nullable fields; I don't see how that behavior as-is could be useful (and in most cases it just doesn't matter -- only if you try to pass the Go zero value which I assume is where you are finding trouble).

@Anas737
Copy link
Author

Anas737 commented Aug 3, 2023

Thank you for looking into it

@benjaminjkraft benjaminjkraft added the help wanted Issues that anyone could pick up and implement if useful to them label Aug 3, 2023
@mwajeeh
Copy link

mwajeeh commented Sep 4, 2023

I want it to add omitempty to everything but # @genqlient(omitempty: true) is throwing omitempty may only be used on optional arguments. Here is my mutaion:

# @genqlient(omitempty: true)
mutation ProvisionOrPortModalProvisionNumberMutation($input: ProvisionNumberInput!) {
    provisionNumber(input: $input) {
        errorMessage
        success
    }
}

@benjaminjkraft
Copy link
Collaborator

@mwajeeh you want to do:

# @genqlient(omitempty: true)
mutation ProvisionOrPortModalProvisionNumberMutation(
    $input: ProvisionNumberInput!,
) {
    provisionNumber(input: $input) {
        errorMessage
        success
    }
}

(See #151.)

@benjaminjkraft benjaminjkraft added this to the v1.0 milestone Jan 26, 2024
benjaminjkraft pushed a commit that referenced this issue Jun 7, 2024
Fixes #290 and
#228 (comment)
(for latter, only the comment of mine, not the whole issue)

This is only changing where the `omitempty` is allowed and forbidden -
it does not change where is the `omitempty` actually generated or not in
generated code.

Separately each line of changelog:
- allow `omitempty` on non-nullable input field, if the field has a
default (pretty much
#228 (comment))
- added a `&& field.DefaultValue == nil` to the `"omitempty may only be
used on optional arguments"` error
- allow `omitempty: false` on an input field, even when it is
non-nullable (#290)
  - `fieldDir.Omitempty != nil` changed to `fieldOptions.GetOmitempty()`
- forbid `omitempty: false` (including implicit behaviour) when using
pointer on non-null, no-default input field
- as setting a correct combination of directives (and potentially some
options, which changes implicit pointer and/or omitempty), and this
library promises "Compile-time validation of GraphQL queries: never ship
an invalid GraphQL query again!", I found it fitting to guard against
the most simple case, that can be enforced in Go type system.
- this, however, is a breaking change, so not sure if I should include
it here. No previously present test failed after such change, but for
example
`generate/testdata/errors/DefaultInputsNoOmitPointerForDirective.graphql`
would previously generate following (below - which has a possibility to
send invalid graphql input), but now the generation fails.
 ```
        type InputWithDefaults struct {
        	Field         *string `json:"field"`
        	NullableField string  `json:"nullableField"`
        }
```
- - alternative would be to force omitempty tag in such cases (even if there is no omitempty directive/option) - so that generation would not fail. But I'm not sure if I can afford to do that. That would probably still be breaking change (different generated code for same query), but a bit better. Maybe just setting omitempty flag instead of returning error would be sufficient.
  
In general, I have also moved the omitempty check from directives to the time of creating Go types/tags of field. This seems more consistent, as not all possibilities were caught before (i.e. general `@genqlient(omitemtpy: true)` vs `@genqlient(for: ..., omitempty: true)`). When creating Go type/tags, all the options/directives are already merged, so the final result is being checked. There is minor difference in error message (instead of reference to directive, the error refers to the whole operation, but also includes type.field name)


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
- [ ] Included documentation, for new features
- [x] Added an entry to the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Issues that anyone could pick up and implement if useful to them
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants