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

Global omitempty option #260

Open
liron-navon opened this issue Feb 27, 2023 · 13 comments
Open

Global omitempty option #260

liron-navon opened this issue Feb 27, 2023 · 13 comments
Labels
enhancement New feature or request

Comments

@liron-navon
Copy link

liron-navon commented Feb 27, 2023

Let's say a graphql query accepts an input object, which has an optional string in it, if I use the input object, and not give a value to that string, it should be committed, that's just how graphql works.
Unfortunately, this library doesn't work that way.

Is your feature request related to a problem? Please describe.
I have this input for shopify, the non struct fields are not required

Screenshot 2023-02-27 at 11 29 36

The generation works well, but if In don't include the optional fields, genqlient turns them into null (when defining value as pointer) or to an empty string (when defining value as value).

They should just be omitted as I don't want to send them, otherwise shopify will fail to use this.

Using the "@genqlient(omitempty: true)" is impossible, because the import is required, even tho every field on it is optional.

Describe the solution you'd like
add to the settings.yml file an option "omitempty: boolean" that would just cover all the json fields.

Describe alternatives you've considered
Adding manually the "omitempty" directive, but with thousands of fields it's makes it really hard to use, and time consuming and prone to humen errors.

Additional context
What I get generated, why would an optional pointer to a string not have omit empty?

Screenshot 2023-02-27 at 11 28 53

@liron-navon liron-navon added the enhancement New feature or request label Feb 27, 2023
@liron-navon liron-navon changed the title allow to have omitempty in the settings file for all the fields allow to have omitempty in the settings file for all optional the fields? Feb 27, 2023
@benjaminjkraft
Copy link
Collaborator

Check out the optional: pointer setting in genqlient.yaml! If I'm understanding your request correctly that's exactly what it does.

@liron-navon
Copy link
Author

liron-navon commented Mar 30, 2023

@benjaminjkraft why did you close it? you don't understand correctly, the "optional: pointer" doesn't omit empty, it passes "null" instead, this breaks different apis, namely "shopify" and "spacex"

@benjaminjkraft
Copy link
Collaborator

Thanks for clarifying. I guess it's valid GraphQL to treat explicit null differently. It seems like what you are proposing is a global config to turn on omitempty. That seems reasonable to add, although soon we are going to have to figure out how to make all these related config options not explode the config complexity.

@benjaminjkraft benjaminjkraft reopened this Apr 8, 2023
@mujagic-nermin
Copy link

mujagic-nermin commented Apr 12, 2023

Looking at the documentation, I'm trying to achieve what it says here with the global omitempty.
image

Doesn't seem to be working for my code, even though I followed exactly?
image
Where is the ,omitempty json tag??? Below is my genqlient.graphql
image

@dharijanto
Copy link

Thanks for clarifying. I guess it's valid GraphQL to treat explicit null differently. It seems like what you are proposing is a global config to turn on omitempty. That seems reasonable to add, although soon we are going to have to figure out how to make all these related config options not explode the config complexity.

Hi Benjamin, just to clarify, I think the author wants the global omitempty: true only for input parameter. He is probably using Hasura 2.0 (just like I am)

Just to give you a bit more background, in Hasura 2.0, they introduced a breaking change that distinguishes "implicit null" vs. "explicit null".

hasura/graphql-engine#7484 (comment)

@dharijanto
Copy link

Hi @benjaminjkraft I created a draft PR to fix the issue. Would you mind taking a look?
#264

I haven't written/fixed tests associated to it, I figure I'd check with you first if the direction is okay.

@benjaminjkraft
Copy link
Collaborator

benjaminjkraft commented May 4, 2023

Thanks for the context and the PR. I can see why they have ended up wanting the distinction. (For others reading, the changelog entry describes the breaking part of the change, although it sounds like this isn't the only place they draw the distinction.)

I commented in the PR -- for a couple reasons I think a new config option might be useful but I think if we do do that it's a fine way to do things.

At some point we should probably also have a clearer example/FAQ entry for how to use genqlient with hasura, since the hasura-generated schemas seem to have a bunch of quirks that genqlient's defaults don't match with. (But that doesn't have to be now.)

@benjaminjkraft benjaminjkraft changed the title allow to have omitempty in the settings file for all optional the fields? Global omitempty option May 4, 2023
@benjaminjkraft
Copy link
Collaborator

One realization I had on the way home: a workaround is to have your inputs be the fields you want to set, rather than the whole object. For example, instead of:

myQuery(myInput: SomeInputType) {
  field(input: $myInput) { ... }
}

you'd do

myQuery(myField: String!) {
  field(input: {myField: $myField}) { ... }
}

Obviously some pluses and minuses there, so it doesn't obviate the issue, but may be useful until we get this option sorted.

@benjaminjkraft
Copy link
Collaborator

Ah, I realized while trying to consolidate issues that we also have use_struct_references, which is not quite the same but may also be useful to folks in this issue.

@benjaminjkraft
Copy link
Collaborator

Hasura users -- please take a look at #272 and add any thoughts!

@dharijanto
Copy link

dharijanto commented May 9, 2023

Hi Ben,

At some point we should probably also have a clearer example/FAQ entry for how to use genqlient with hasura, since the hasura-generated schemas seem to have a bunch of quirks that genqlient's defaults don't match with. (But that doesn't have to be now.)

Yeah I totally agree. Feels a bit non-intuitive at the moment to get genqlient to work with Hasura.

Thanks for responding to the thread and commenting on the PR. Since you've created a separate issue to consolidate Hasura-related discussions, should I just close the PR?

@skandragon
Copy link

It is also difficult to use Dgraph because IDs are passed as empty strings, and DGraph rejects this.

Dgraph takes the schema submitted and augments it in various ways, such as:

input AddComponentInput {
  type: String!
  name: String!
  version: String!
  publisher: String
  licenses: [ComponentLicenseRef!]
  purl: String
  cpe: String
  vulnerabilities: [VulnerabilityRef!]
}

type AddComponentPayload {
  component(filter: ComponentFilter, order: ComponentOrder, first: Int, offset: Int): [Component]
  numUids: Int
}

type Mutation {
  addComponent(input: [AddComponentInput!]!): AddComponentPayload
}

The issue I have is I want to set things to "omit empty" on at least some of the fields in this Dgraph-generated type list. I am not sure how to do this. Right now, I can modify the generated Go file, but that is quickly a pain.

@GeertJohan
Copy link

I'm also working with shopify and running into this issue. As a workaround, I have a little sed script that runs after genqlient, sharing here in case it's useful for anyone :)

sed -i 's/\(`json:"[^,]*\)"\(.*\)/\1,omitempty"\2/' shopify.gen.go

Now, this works for a lot of cases but ofcourse it's not a proper solution, because suddenly a false boolean isn't sent either, so you'll have to remove omitempty by doing

sed -i 's/\(Enabled bool `json:"enabled\),omitempty/\1/' shopify.gen.go

to make sure the field is actually sent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants