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

[jesse-apollo]

This PR is a refresh of https://github.com/99designs/gqlgen/pull/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:

• https://github.com/99designs/gqlgen/issues/2559
• https://github.com/99designs/gqlgen/issues/2632
• https://github.com/99designs/gqlgen/issues/2357
• https://github.com/99designs/gqlgen/issues/1861
• https://github.com/99designs/gqlgen/issues/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
• [x] Write some docs
• [x] Write a test--might need help with this

I have:

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

[coveralls]

coverage: 74.995% (-0.9%) from 75.884% when pulling c839ad56be8063658c8e9f2bfdfe46e6e55916a2 on jesse-apollo:master into e186813e8852a1904d7778d2efd6cd745fced355 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]

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.

[jesse-apollo]

Not that I can think of. Thanks @StevenACoffman

[StevenACoffman]

@jesse-apollo Can you please run go generate ./... and commit + push the results?

[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:

https://github.com/99designs/gqlgen/compare/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.