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

InputObject field defaults are ignored/overridden #228

Open
kateile opened this issue Oct 13, 2022 · 2 comments
Open

InputObject field defaults are ignored/overridden #228

kateile opened this issue Oct 13, 2022 · 2 comments
Labels
bug Something isn't working
Milestone

Comments

@kateile
Copy link

kateile commented Oct 13, 2022

Describe the bug
Hi geeks, Thanks for this amazing lib. I have the following graphql input with default fields

input FormInput {
    hint: String
    inputType: MwappFormInputType = TEXT
    key: String = "form"
    label: String = "Fill form"
    maxLength: Int = 100
    multiSubmission: Boolean = false
    multiline: Boolean = false
    obscure: Boolean = false
}

input SendMessageInput {
    form: FormInput
}

type Mutation {
    sendMessage(connectionId: ID!, input: SendMessageInput!): String!
}

Which I use in golang like

mw.SendMessage(ConnectionId, mwapp.SendMessageInput{
		Message: "Hi, Please tell us your name. \n you can write it below",
		Form: &mwapp.FormInput{
			Key: FormKeyName.String(),
		},
	})

As you can see above I just use one field Key and ignore the rest. So I expect the default graphql fields specified in schema to be used when send request. But they are sent like null too.

To Reproduce
Create mutation whose input have fields with default fields

Expected behavior
Default fields to be respected when. I believe having pointer with omitempty in the generated code will be the solution. But I don't know how to archive this

genqlient version
v0.5.0

Additional context
My genqclient.yaml is

schema: schema.graphql
package: mwapp
operations:
- operations/*.graphql
generated: generated.go
use_struct_references : true
optional: pointer
bindings:
  Timestamp:
    type: time.Time

Any help on fixing this will be appreciated.

@kateile kateile added the bug Something isn't working label Oct 13, 2022
@benjaminjkraft
Copy link
Collaborator

Huh, I didn't realize input-object fields could have defaults! (For my future self: it's at InputObjectTypeDefinition which refers to InputValueDefinition.)

Does it work to use genqlient's omitempty option? For example you could add at the top of the file # @genqlient(for: "FormInput.inputType", omitempty: true), and so on for each field. (Obviously that's a bit verbose; we could talk about if there's a better way to do it, perhaps by extending #198 to allow an omitempty option.)

@benjaminjkraft benjaminjkraft changed the title Default graphql values are ignored when send request InputObject field defaults are ignored/overridden Dec 3, 2022
@benjaminjkraft benjaminjkraft added this to the v1.0 milestone Jan 26, 2024
@Fazt01
Copy link
Contributor

Fazt01 commented Mar 27, 2024

Hi, yes the omitempty works -> the field is omitted in request and the server takes the default as value for that input field.

(tried with version github.com/Khan/genqlient v0.7.0)

One edge case, though, is when the field with default is non-null, e.g.

type Query {
  someQuery(input: Input!): String
}

input Input {
  inputField: Int! = 10
}

and I try to generate a request

# @genqlient(for: "Input.inputField", pointer: true, omitempty: true)
query MyQuery {
  $input: Input
} {
  someQuery(input: $input)
}

I get omitempty may only be used on optional arguments error, so in this case, I cannot specifically pick input fields that should respect the server default when empty.

I can, however, put omitempty to all fields by removing the for - without the above error

# @genqlient(pointer: true, omitempty: true)
query MyQuery {
  $input: Input
} {
  someQuery(input: $input)
}

which generates a structure as expected

type Input struct {
	InputField *int `json:"inputField,omitempty"`
}

IMO omitempty:false should be allowed to an input field that has a valid default (that is - it is nullable and it has not explicit default, or it has explicit default).
A bit related to #290 (false positive error on omitempty)

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
Projects
None yet
Development

No branches or pull requests

3 participants