Fix: complexity case selection

[mbranch]

While working on limits to complexity, we found an issue with how code generation builds the Complexity func. Basically, the Complexity func was receiving field not as GoFieldName, but as the GraphQL name.

It's possible this isn't the correct solution (and field param should be a Go name), but based on what's happening in complexity_test.go, I think this is correct. I'll try to add tests this week.

Describe your PR and link to any relevant issues.

I have:

• Added tests covering the bug / feature (see testing)
• Updated any relevant documentation (see docs)

[vektah]

Theres a bit of legacy on this one, and probably a missing test.

We switched to using go field names because _ is a valid graphql identifier - #473

#376 and #622 followed on from there to fix a few of the issues with the current approach.

[mbranch]

Theres a bit of legacy on this one, and probably a missing test.

We switched to using go field names because _ is a valid graphql identifier - #473

#376 and #622 followed on from there to fix a few of the issues with the current approach.

Okay. I'm not sure if you mean this is the wrong solution or not, but let me know and I'm happy to spend time revising.

[vektah]

Yeah its not quite right. The complexity funcs are set and deduped based on the go field name, not the graphql one to avoid the listed bugs.

Heres a failing that hits on the issue (drop it into testserver/complexity_test.go):

func TestComplexityFuncs(t *testing.T) {
	resolvers := &Stub{}
	cfg := Config{Resolvers: resolvers}
	cfg.Complexity.OverlappingFields.Foo = func(childComplexity int) int { return 1000 }
	cfg.Complexity.OverlappingFields.NewFoo = func(childComplexity int) int { return 5 }

	srv := httptest.NewServer(handler.GraphQL(NewExecutableSchema(cfg), handler.ComplexityLimit(10)))
	c := client.New(srv.URL)

	resolvers.QueryResolver.Overlapping = func(ctx context.Context) (fields *OverlappingFields, e error) {
		return &OverlappingFields{
			Foo:    2,
			NewFoo: 3,
		}, nil
	}

	t.Run("with high complexity limit will not run", func(t *testing.T) {
		ran := false
		resolvers.OverlappingFieldsResolver.OldFoo = func(ctx context.Context, obj *OverlappingFields) (i int, e error) {
			ran = true
			return obj.Foo, nil
		}

		var resp struct {
			Overlapping interface{}
		}
		err := c.Post(`query { overlapping { oneFoo, twoFoo, oldFoo, newFoo, new_foo } }`, &resp)

		require.EqualError(t, err, `http 422: {"errors":[{"message":"operation has complexity 1009, which exceeds the limit of 10"}],"data":null}`)
		require.False(t, ran)
	})

	t.Run("with low complexity will run", func(t *testing.T) {
		ran := false
		resolvers.QueryResolver.Overlapping = func(ctx context.Context) (fields *OverlappingFields, e error) {
			ran = true
			return &OverlappingFields{
				Foo:    2,
				NewFoo: 3,
			}, nil
		}

		var resp struct {
			Overlapping interface{}
		}
		c.MustPost(`query { overlapping { newFoo } }`, &resp)

		require.True(t, ran)
	})

	t.Run("with multiple low complexity will not run", func(t *testing.T) {
		ran := false
		resolvers.QueryResolver.Overlapping = func(ctx context.Context) (fields *OverlappingFields, e error) {
			ran = true
			return &OverlappingFields{
				Foo:    2,
				NewFoo: 3,
			}, nil
		}

		var resp interface{}
		err := c.Post(`query { 
			a: overlapping { newFoo },
			b: overlapping { newFoo },
			c: overlapping { newFoo },
		}`, &resp)

		require.EqualError(t, err, `http 422: {"errors":[{"message":"operation has complexity 1009, which exceeds the limit of 10"}],"data":null}`)
		require.False(t, ran)
	})
}

Note that the schema for this has two graphql fields that with underscores removed and case normalised to fit go standards would collide:

type OverlappingFields {
  # ... snip ...
  newFoo: Int!
  new_foo: Int!
}

edit: I think this PR probably correct, but the complexity switch shouldn't be deduping, only the function side should.

[mbranch]

@vektah This is revised to handle overlapping fields. The solution I came up with was to ensure that all possible field names are accounted for in the complexity switch statement. Here's the generated code difference.

[DBL-Lee]

What do you think about excluding __typename in complexity calculation by default?
I am encountering this problem because client generated code queries typename by default and any multiplicative complexity functions will cause this to blow up. And custom complexity functions cannot be defined on this field.

[mbranch]

What do you think about excluding __typename in complexity calculation by default?
I am encountering this problem because client generated code queries typename by default and any multiplicative complexity functions will cause this to blow up. And custom complexity functions cannot be defined on this field.

I'm not a maintainer of this library, but I think __typename having a base-cost of 1 is reasonable. Perhaps your complexity calculation should be multiplying larger numbers at the query/mutation level so that adding a handful extra fields doesn't exceed your complexity limit.