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

Setting omit empty or pointer on nested structs #187

Closed
Laurence-Caraccio opened this issue Apr 26, 2022 · 5 comments
Closed

Setting omit empty or pointer on nested structs #187

Laurence-Caraccio opened this issue Apr 26, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@Laurence-Caraccio
Copy link

Laurence-Caraccio commented Apr 26, 2022

Is your feature request related to a problem? Please describe.

Hi guys, really like the project and intending to adopt it, one sticking point that would make life even better.

We're using GraphQL with App sync on the front and have a number of list queries that take a filter which is itself a nested struct (see additional context for details)

The struct at the bottom level has what are essentially mutually exclusive fields where a max 1 of them can be provided, if more than one are the requests will fail.

As they are themselves simple types setting use_struct_references doesn't help. The fields in TableStringFilterInput will not have omitempty set or be pointers.

As they are more than one layer down you can't set them to be omitted or pointers with decorators.
i.e. # @genqlient(for: "filter.name.eq", omitempty: true, pointer: true) gives genqlient.graphql:8: for must be of the form "MyType.myField"

Is there any way to get the fields on TableStringFilterInput to have omitempty set?

If not any suggestions about what would be the best way to achieve this? Open to having a go at making any changes required.

Describe the solution you'd like

A way to tell genqlient to set omitempty on all fields for a type via the yaml file.

Describe alternatives you've considered

Passing all the fields from the filter through individually and then setting them to be omit empty using the decorator, not great design as every caller will have to pass N structs with only 1 being set rather than 1 struct with the relevant field set.

i.e.

# @genqlient(for: "TableStringFilterInput.eq", omitempty: true, pointer: true)
# @genqlient(for: "TableStringFilterInput.ne", omitempty: true, pointer: true)
# @genqlient(for: "TableStringFilterInput.contains", omitempty: true, pointer: true)
# @genqlient(for: "TableStringFilterInput.notContains", omitempty: true, pointer: true)
# @genqlient(for: "TableStringFilterInput.beginsWith", omitempty: true, pointer: true)
# @genqlient(for: "TableStringFilterInput.in", omitempty: true, pointer: true)
query ListCats(
		$breed: TableStringFilterInput,
                $name: TableStringFilterInput,
		$limit: Int,
		$nextToken: String
) {
    listCats(filter: {name: $name, breed: $breed}, limit: $limit, nextToken: $nextToken) {
        items {
            ...CatFragment
        }
        nextToken
    }
}

The decorators would be required for every list query and would mean the caller has to pass in a filter for every field even when only trying to query by one of them.

Additional context

Example GraphQL schema:

scalar AWSTimestamp
directive @aws_subscribe(mutations: [String!]!) on FIELD_DEFINITION
directive @aws_api_key on OBJECT
directive @aws_cognito_user_pools on OBJECT

# Self-described.
type Query @aws_api_key @aws_cognito_user_pools {
	# Get list of cats query (DynamoDB).
	listCats(
		# DynamoDB filters for cats. Optional.
		filter: TableCatFilterInput,
		# Max count of items to scan. Optional.
		limit: Int,
		# Next batch pointer. Optional.
		nextToken: String
	): CatConnection
}

# Filter for DynamoDB String type field.
input TableStringFilterInput {
	# Begins with. Optional.
	beginsWith: String
	# Contains string. Optional.
	contains: String
	# Equal. Optional.
	eq: String
	# In defined list. Optional.
	in: [String]
	# Not Equal. Optional.
	ne: String
	# Not contains string. Optional.
	notContains: String
}

input TableCatFilterInput {
	name: TableStringFilterInput
	breed: TableStringFilterInput
}

type CatConnection @aws_api_key @aws_cognito_user_pools {
	items: [Cat]
	nextToken: String
}

type Cat @aws_api_key @aws_cognito_user_pools {
	name: String!
    breed: String
}

Example of generated code using said schema
cat_test.zip

Note: The TableStringFilterInput is an appsync + dynamo thing, for more see - https://docs.aws.amazon.com/appsync/latest/devguide/provision-from-schema.html

@Laurence-Caraccio Laurence-Caraccio added the enhancement New feature or request label Apr 26, 2022
@benjaminjkraft
Copy link
Collaborator

Interesting. I think what you're really hoping for is a way to say "use pointers for optional fields" at the type level, similar to what #178 proposes globally. Or maybe you'd be fine with a global option too? Perhaps if someone has time to implement #178 you could see if that works for you, and if not we can use that logic to implement what you want; I'm guessing it won't be too complex to add thereafter.

In the meantime I want to point out a fix to the workaround you tried. We do allow for to apply to nested types, you just have to specify it as TypeName.fieldName ignoring the nesting. So it will look like:

# @genqlient(for: "TableStringFilterInput.eq", omitempty: true, pointer: true)

and that will apply even when that TableStringFilterInput is a field of an argument, not an argument in and of itself. So you don't need the filter: {name: $name, breed: $breed} stuff, you can just use the same decorators you had there, but passing directly filter: $filter.

Another way we could solve this is to have a way to add a global for option in your genqlient.yaml. Then you could put those 6 decorators in your genqlient.yaml once, and be done. Not sure if that would be a better idea; I'll make a separate issue to consider it.

@benjaminjkraft
Copy link
Collaborator

Oh, one more thing to add is: if you have control over the schema, you might want to consider using a union and/or enum type here! I think the more canonical way to represent a filter like this in GraphQL would be

type TableStringEqFilterInput { val: String! }
type TableStringInFilterInput { val: [String!]! }
# (etc.)

union TableStringFilterInput =
  | TableStringEqFilterInput
  | TableStringInFilterInput
  # (etc.)

Obviously that adds some indirection, but it'll save you some validation work, and make tools better able to see what's going on.

@Laurence-Caraccio
Copy link
Author

Hey, thanks for the reply. Knowing how to set pointer and omitempty on subfields is super helpful thanks and will certainly solve our specific problem.

I'll put a PR in to expand on the FAQs with an example of how to set them on subfields.

It's a bit of a workaround solution though, the decorators only need to be specified once, but, it must be on the query with the first usage of the filter, decorators on later filters for the same type will be ignored, this would be easy to mess up with a refactor as all it would take is moving a query above whichever has the decorators to break things.

#178 may resolve the problem but we'd want to be able to set omitempty on the fields as well as making them pointers which might not make sense to do for all optional types.

I like the idea of being able to specify an equivalent to the decorators in the yaml file, this would resolve any issue of query ordering in the schema and allow more fine grained control than simply making all optional fields pointers and omitempty.

On the union types my understanding is GraphQL won't allow a union in an input type which the FilterInputs end up being part of. There's quite possibly a better way to express mutually exclusive fields though, will have to try some of the suggestions from graphql/graphql-spec#488.

@benjaminjkraft
Copy link
Collaborator

Thanks for the replies -- it seems like global decorators (#190) are probably the best route here. I may or may not have time to take a look at that next week, or if not then later in the month; feel free to take a stab at it as well if you'd like. (I would guess it won't be too hard to implement -- in theory all the parsing and option-precedence code exists and just needs new wiring -- but I'm not certain.)

the decorators only need to be specified once, but, it must be on the query with the first usage of the filter, decorators on later filters for the same type will be ignored, this would be easy to mess up with a refactor as all it would take is moving a query above whichever has the decorators to break things.

Ah, yes, the idea is that you should specify them on all queries that use the filter, but obviously it's easy to miss one. (See also #123.)

#178 may resolve the problem but we'd want to be able to set omitempty on the fields as well as making them pointers which might not make sense to do for all optional types.

Ah, thanks for pointing that out, yeah. This makes the "global decorators" idea (#190) seem more compelling. Or, if we explicitly let you set it at the type level (and allow both pointers and omitempty) -- but then maybe global decorators are just a simpler way to do that.

On the union types my understanding is GraphQL won't allow a union in an input type

Ah, of course, I forgot we were talking about input types! My bad; I don't know of a better way then (at least in the current spec).

@Laurence-Caraccio
Copy link
Author

I'm going to close this, we've got a workable solution and #190 can explore cleaner options.

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

2 participants