Optionally render entity requires populator function for advanced @requires use cases

[jesse-apollo]

This PR is a refresh of #2676

This new PR utilizes a config flag that makes it optional:

federation:
   filename: graph/federation.go
   package: graph
   options:
     explicit_requires: true

If explicit_requires is enabled a new file called federation.requires.go will be rendered with one or more entity resolver functions populated like:

// PopulateTodoRequires is the requires populator for the Todo entity.
func (ec *executionContext) PopulateTodoRequires(ctx context.Context, entity *model.Todo, reps map[string]interface{}) error {
	panic(fmt.Errorf("not implemented: PopulateTodoRequires"))
}

These are only rendered for entities that have an @requires directive on one or more fields on the entity, example:

type Todo @key(fields:"id") {
  id: ID!
  text: String! @requires(fields: "users { name }")
  done: Boolean! @requires(fields: "user { name }")
  user: User!
  users: [User]
  cost: Int  @provides(fields: "profit { usd }")
  profit: Profit @external
}

type User @key(fields: "id") {
  id: ID!
  name: String!
}

This PR addresses several issues:

• Generated Federation code for fields with @external directive applied on a list which is used in a @requires field is wrong. #2559
• gqlgen generates non compilable federation.go code when schema uses @requires directive with attribute referring inside array for dependency  #2632
• non-nullable @external field that are not part of @key fieldset blows up federation #2357
• Federation: requiring a field with a list of objects generates invalid code #1861
• Support for nested selection sets on _FieldSet (on @requires annotation) #1138

We (the Apollo team) think this approach is more flexible and future proof as it handles much more complex use cases like @requires nesting in repeated fields. Our initial approach was to try and extend the existing requires functionality using recursive template rendering but this proved to be complex and difficult to test.

cc @brh55 @dariuszkuc

Todo:

• Squash commits
• Write some docs
• Write a test--might need help with this

I have:

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

[coveralls]

coverage: 74.995% (-0.9%) from 75.884%
when pulling c839ad5 on jesse-apollo:master
into e186813 on 99designs:master.

[StevenACoffman]

Making this opt-in removes my primary objection to the original that it breaks a lot of existing users who are fine with the current level of support.

[jesse-apollo]

Added fixes and tests from @ldebruijn's forked branch.

[ldebruijn]

Hey! I wanted to contribute to this after not following up on the previos PR attempt.

I can't seem to add commits to this PR, but I've spend some time getting the tests green, this patch contains the changes.
fix__Fix_tests_for_entity_requires.patch

[jesse-apollo]

Hi @ldebruijn. Thanks for working on those tests, I was just starting that when I saw your reply. I'm wondering if we should make a new test tree for this instead of reusing testdata/entityresolver?

[jesse-apollo]

@ldebruijn I've reverted this from your proposed change as it breaks the implementation and the test code generation.

[jesse-apollo]

I'm not sure what this failure in running the tests locally means:

    --- FAIL: TestGenerate/default (1.49s)
        generate_test.go:56: Generate() error = merging type systems failed: unable to build object definition: unable to find type: github.com/99designs/gqlgen/api/testdata/default/graph/model.Todo, wantErr false
    --- FAIL: TestGenerate/federation2 (1.57s)
        generate_test.go:56: Generate() error = merging type systems failed: unable to build object definition: unable to find type: github.com/99designs/gqlgen/api/testdata/federation2/graph/model.Todo, wantErr false

@StevenACoffman any ideas where to look?

[ldebruijn]

Hi @ldebruijn. Thanks for working on those tests, I was just starting that when I saw your reply. I'm wondering if we should make a new test tree for this instead of reusing testdata/entityresolver?

Yes, seems like the better way to go indeed.

[StevenACoffman]

I'm not sure what this failure in running the tests locally means:

    --- FAIL: TestGenerate/default (1.49s)
        generate_test.go:56: Generate() error = merging type systems failed: unable to build object definition: unable to find type: github.com/99designs/gqlgen/api/testdata/default/graph/model.Todo, wantErr false
    --- FAIL: TestGenerate/federation2 (1.57s)
        generate_test.go:56: Generate() error = merging type systems failed: unable to build object definition: unable to find type: github.com/99designs/gqlgen/api/testdata/federation2/graph/model.Todo, wantErr false

@StevenACoffman any ideas where to look?

It seems like from the next few comments, you are going to take a different approach, so I haven't bothered to point out why this is happening. Correct me if I'm wrong.

[jesse-apollo]

It seems like from the next few comments, you are going to take a different approach, so I haven't bothered to point out why this is happening. Correct me if I'm wrong.

This failure is after the new approach.

[ldebruijn]

What is left to pull this PR across the finish line?

[StevenACoffman]

@ldebruijn The same as every PR! For it to pass the existing tests and not break backwards compatibility. While I am in favor of this effort, I'm not driving this forward as I'm consumed with my day job, and the existing federation support is sufficient for my purposes.

[ldebruijn]

@StevenACoffman don’t get me wrong! I wasn’t insinuating you to be more involved. It was an honest ask for the left over work to get the PR merged.

As far as I can see only the linting job is still failing, is that correct ? @jesse-apollo is that something you’re planning to tackle? I can take a look and supply another changeset, sadly I cannot push commits to this PR.

[ldebruijn]

From what I can see this is probably the last bit that is still failing the tests. They fail as they observe changes in the generated federation.requires.go file. This is due to the generated resolver functions not having consistent ordering over generate runs.

I've added a sort on the populators to ensure ordering is consistent. The failing checks now pass on my local machine.

fix(federation)__ensure_consistent_ordering_of_generated_resolver_functions.patch

[jesse-apollo]

Thanks @ldebruijn I'll get this patch applied and pushed ASAP.

[StevenACoffman]

Thanks for getting to all tests passing, and it is only failing for the regenerate after you merged master.

It looks like this does not break backwards compatibility for anyone. Is there anything else you would like to do (besides the go generate) before I merge this?

[StevenACoffman]

Thanks for sticking with this and avoiding breaking backwards compatibility while continuing to support more custom use cases!

[mihirpmehta]

When are we planning to merge this in new version ?

[StevenACoffman]

@mihirpmehta You do not need to wait for a release. Please do this and immediately use the new functionality:

go mod edit -replace github.com/99designs/gqlgen=github.com/99designs/gqlgen@15cef76f18afec4c5352911360e33f4042500697

I would like a few people to use this functionality and report whether it needs any further improvement before I drop a new release, as this is a significant new feature.

[ldebruijn]

I've been running it on PRO over the weekend, works perfectly. Adopting the change was a breeze as well. Currently have no suggestions for improvements.

[ericbock]

I'm seeing two issues:

The generated file, in the graph package, is still referencing the package name for types in the package (entity *graph.Boundary here)

package graph

import (
	"context"
	"fmt"
)

// PopulateBoundaryRequires is the requires populator for the Boundary entity.
func (ec *executionContext) PopulateBoundaryRequires(ctx context.Context, entity *graph.Boundary, reps map[string]interface{}) error {
	panic(fmt.Errorf("not implemented: PopulateBoundaryRequires"))
}

[ericbock]

If I remove the graph. in the method signature I run into the second issue.

I'm using the config setting resolvers_always_return_pointers: false

So the modified generated file:

package graph

import (
	"context"
	"fmt"
)

// PopulateBoundaryRequires is the requires populator for the Boundary entity.
func (ec *executionContext) PopulateBoundaryRequires(ctx context.Context, entity *Boundary, reps map[string]interface{}) error {
	panic(fmt.Errorf("not implemented: PopulateBoundaryRequires"))
}

produces the error:

# gitlab.com/armed-atk/development/microservices/system-boundaries/graph
graph/federation.gen.go:100:44: cannot use entity (variable of type Boundary) as *Boundary value in argument to ec.PopulateBoundaryRequires

I think federation.gen.go would need to be updated to account for the resolvers_always_return_pointers setting.

[StevenACoffman]

PRs are welcome, as are a repository that demonstrates the problem.

[ericbock]

Thanks. I've created https://github.com/ericbock/gqlgen_explicit_requires_issue to demonstrate both issues.

[ericbock]

The generated file, in the graph package, is still referencing the package name for types in the package (entity *graph.Boundary here)

It looks like this is happening when the models are generated into the package (graph) where the federation.requires.go file lives.

[StevenACoffman]

Interesting! I generally separate the models into a different package than other generated files. We can update the documentation to make this an explicit requirement or add more logic to the code generation.

[ericbock]

I'm not sure how we landed on our configuration here to dump the models into the same package, but we have it in a lot of projects. I'm working on a small patch for the code generation logic which should help.

[jesse-apollo]

@ericbock I'm at company kickoff right now but I'll look at creating a PR to address this case as soon as we get back.

[ericbock]

@jesse-apollo It looks like this is all that's involved with respecting the package that the model lives in:

master...ericbock:gqlgen:models_in_pkg#diff-fa028f5dfc54acb372fd0d727ec7e799b93d8dbb34b2769918b7a55bbf051910R123

I haven't been able to get the resolvers_always_return_pointers: false fix done yet though.

[StevenACoffman]

@ericbock @jesse-apollo If possible, I would like a PR from either of you soon, as this is holding up releasing the next version.

[ericbock]

@StevenACoffman I just created #2965 to address the "models generated in same directory" issue. I haven't had a chance to go back to look at the remaining resolvers_always_return_pointers: false issue, but I don't think it warrants holding back a release.

[jesse-apollo]

Hi @ericbock & @StevenACoffman just back in the saddle today. I've opened a PR for addressing the resolvers_always_return_pointers: false #2966

Let me know what you think, it's very straightforward.

[ericbock]

Looks good to me @jesse-apollo. I was trying to figure it out somehow from the entity itself and go/types, but it's much more straightforward the way you pulled in the config value directly.

[StevenACoffman]

Thanks! @jesse-apollo @ericbock I have merged both of your PRs, and released a new v0.17.45 that includes them.

Now that #2357 is closed, the Apollo Federation Subgraph Compatibility here https://github.com/apollographql/apollo-federation-subgraph-compatibility/blob/ff24a67687fe96b76edab303f0e2854ea2b9b863/implementations/gqlgen/graph/schema.graphqls#L68 needs to be updated. I made an initial PR apollographql/apollo-federation-subgraph-compatibility#606 that doesn't yet update the gqlgen config nor the populator function, but I'm probably not going to get more time to work on it.

If either of you (or others) find any new Federation-related problems, fast-follow PRs would be very much appreciated. Thanks again!

[suiz0]

QQ: How could I access Resolver dependency within this method?