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

Query complexity calculation and limits #315

Merged
merged 11 commits into from
Aug 31, 2018

Conversation

edsrzf
Copy link
Contributor

@edsrzf edsrzf commented Aug 24, 2018

This PR adds support for calculating query complexity and enforcing a limit on that query complexity.

The core logic for complexity calculation lives in the new complexity package. There is a default calculation, but the calculation can also be customized per-field. To figure out the custom complexity for a field, the package uses the new Complexity method on ExecutableSchema.

Custom complexity function may take into account field arguments and the cost of child selection sets.

A new ComplexityRoot type is generated in the code, similar to DirectiveRoot. It has a Go field for each GraphQL field that is configured to have a custom complexity function. The generated executableSchema now comes with a generated Complexity method that calls these functions when they're non-nil.

Finally, you may configure a complexity limit for your GraphQL handler by using the new ComplexityLimit function.

if e.complexity.{{$object.GoType}}.{{$field.GoFieldName}} == nil {
break
}
{{ if . }}args := map[string]interface{}{} {{end}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is basically copied from args.gotpl. There's probably a better way to do this with less duplication.

In general, it seems like it might be too expensive to be doing all the argument conversion and unmarshaling twice, as well. (Now it will happen once for the complexity calculation, and then again for the actual query execution.)

Copy link
Collaborator

@vektah vektah Aug 24, 2018

Choose a reason for hiding this comment

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

It should be pretty cheap as long as it only happens if there is a complexity function defined? Might be a bunch of extra generated code, but that's OK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What prevents you from calling args.gotpl directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wants to return graphql.Null in error cases, which is the wrong type for this function.

@vektah
Copy link
Collaborator

vektah commented Aug 24, 2018

Looks good! Would be good to run go generate ./... and commit the changes to show the codegen changes here.

@edsrzf
Copy link
Contributor Author

edsrzf commented Aug 27, 2018

I've run go generate ./..., added some tests for the complexity package, and merged master.

I'll write up a new page for the docs. I'm struggling to come up with a clean solution to the args template thing, but I'll keep thinking about it. Anything else?

@vektah
Copy link
Collaborator

vektah commented Aug 27, 2018

Only thought is the config key might not be needed here, currently there are two ways to opt out of adding a complexity func:

  • don't set complextity: true
  • don't set config.Complexity.Foo

@vektah
Copy link
Collaborator

vektah commented Aug 27, 2018

Its worth updating one of the examples to use this codepath too, I tried in the starwars example by doing:

  Starship:
    fields:
      length:
        resolver: true
        complexity: true

and ended up with invalid code

if e.complexity.Starship. == nil {
    break
}

probably even more reason not to have a config key; if everything always gets generated bugs are less likely to creep in?

@edsrzf
Copy link
Contributor Author

edsrzf commented Aug 27, 2018

Pushed some docs and fixed the bug you mentioned, so that the star wars example should work.

I'm not too sure about removing the config key. I agree that it's not good to have two ways to opt out of custom complexity, but always generating code for every single field in every single object doesn't seem right either. (The docs don't currently mention the config key since I'm undecided.)

Maybe we could generate a Complexity interface based on the config instead? You could opt out of implementing the interface completely, but once you've opted in to implementing it, you have to implement every method. Thoughts?

@vektah
Copy link
Collaborator

vektah commented Aug 28, 2018

It looks like there are still bugs here in the starwars example, try adding a complexity function for Mutation.createReview

but always generating code for every single field in every single object doesn't seem right either.

Why not?

By removing all the conditionals in the template I get something like this (exposing a few bugs, probably worth filtering out __ types too):

type ComplexityRoot struct {
	Droid struct {
		Id                func(childComplexity int) int
		Name              func(childComplexity int) int
		Friends           func(childComplexity int) int
		FriendsConnection func(childComplexity int, first *int, after *string) int
		AppearsIn         func(childComplexity int) int
		PrimaryFunction   func(childComplexity int) int
	}
	Human struct {
		Id                func(childComplexity int) int
		Name              func(childComplexity int) int
		Height            func(childComplexity int, unit LengthUnit) int
		Mass              func(childComplexity int) int
		Friends           func(childComplexity int) int
		FriendsConnection func(childComplexity int, first *int, after *string) int
		AppearsIn         func(childComplexity int) int
		Starships         func(childComplexity int) int
	}
}

This looks good to me, it generates no work for the user as the zero val is a bunch of nils. It's also a pretty efficient datastructure, one func pointer per resolver (the nested structs disappear). The user can safely reach into the structs and set exactly what they want with complete type safety:

func New() *Config {
    cfg = &Config{Resolvers: &MyApp{}}
    cfg.Complexity.Human.Name = func(childComplexity int) int {
        return 10
    }
    return cfg
}

Maybe we could generate a Complexity interface based on the config instead? You could opt out of implementing the interface completely, but once you've opted in to implementing it, you have to implement every method. Thoughts?

It's a lot of work to wire up nested interfaces, equivalent code to above:

type MyComplexityRoot struct {}

func (r *MyComplexityRoot) Droid() myHumanComplexityNode {
	return &myHumanComplexityNode{r}
}

type myHumanComplexityNode struct{ }

func (r *myHumanComplexityNode) Name(childComplexity int) int {
	return 10
}

func New() *Config {
    cfg = &Config{
        Resolvers: &MyApp{}
        Complexity: &MyComplexityRoot{}
    }
    return cfg
}

That's pretty hard to digest, but given the resolvers are mandatory we bear it to get compile time safety. Here they are optional so we might as well give the cleaner, optional interface?

Copy link
Collaborator

@vektah vektah left a comment

Choose a reason for hiding this comment

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

This looks awesome, great work 😍

Directives func(childComplexity int) int
}

Type struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These types come from the introspection api, so they can probably be hidden from this struct. (Skip if $object.GQLType starts with "__"), the __ is there, but ToCamel strips it thinking its a delimiter.

@edsrzf
Copy link
Contributor Author

edsrzf commented Aug 28, 2018

I think one more thing I need to figure out is interface fields. The complexity package tests assume that Interface.field can have a completely different cost from Implementation.field, but the generated code doesn't actually allow that.

I'm not sure if that's even the correct approach though. The interface field's costs should probably be the cost of its most expensive implementor? ast.Interface doesn't have a slice of its implementors, though, so it's not easy to loop through them.

@vektah
Copy link
Collaborator

vektah commented Aug 28, 2018

Yeah I think your right, call them all and assume the worst? That gets nasty though, because it ends up recursive, all the a children also need to assume the worst. Schema.GetPossibleTypes will enumerate enums and interfaces for you.

@vektah
Copy link
Collaborator

vektah commented Aug 30, 2018

After thinking about that more, it's not really that bad as its only at one level. All the types returned will be the same, and any extra fields need to be selected with a fragment (inline or otherwise).

@edsrzf
Copy link
Contributor Author

edsrzf commented Aug 30, 2018

Okay, I think I'm going to need to make a parser change to gather up all the interface implementors inside ast.Definition.

@edsrzf
Copy link
Contributor Author

edsrzf commented Aug 30, 2018

Never mind, I see it's already available through ast.Schema.PossibleTypes.

@edsrzf
Copy link
Contributor Author

edsrzf commented Aug 30, 2018

Alright, added the interface logic, merged master, updated PR description. I'm still not 100% happy with the arguments templating but also feel like there are some general improvements that could be made there, outside query complexity. Maybe we could do it in a separate PR?

@vektah
Copy link
Collaborator

vektah commented Aug 31, 2018

I'm still not 100% happy with the arguments templating but also feel like there are some general improvements that could be made there, outside query complexity. Maybe we could do it in a separate PR?

👍 There's probably enough work to justify a separate PR, turning a recursive object graph into a flat function is a little tricky.

@vektah vektah merged commit 7d44dd6 into 99designs:master Aug 31, 2018
@edsrzf edsrzf deleted the query-complexity branch August 20, 2019 02:27
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
Query complexity calculation and limits
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.

2 participants