WIP: Refactor federation requires to populate on entity via a function

[jesse-apollo]

The PR is WIP as I'd like to elicit feedback before regenerating the tests, etc.

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

The PR creates a new rendered template with functions that pass the requires representations to a generated function with the entity. From here the user can apply the requires data as needed.

The output of the rendered templates looks like this:

import (
	"context"
	"fmt"

	"github.com/mentat/graph/server/pkg/datasources"
)

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

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

I have:

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

[coveralls]

coverage: 75.949% (+0.01%) from 75.935% when pulling 1c6e304 on jesse-apollo:master into d16f498 on 99designs:master.

[StevenACoffman]

@MiguelCastillo @dnerdy @a8m @giautm @vvakame I would appreciate your views on this PR. I'm leaning in favor, as this seems less messy than a lot of other approaches, and could potentially open up some more flexibility for other extensions (e.g. caching, data loading, Ent style eager loading).

[StevenACoffman]

This PR is intended to address when extending a federated entity with a field of an array type containing object (not primitives/scalars) and then adding a field that requires the array field, gqlgen generate generates invalid code.

@jesse-apollo @dariuszkuc Questions from some other Khan engineers:

The most common issue is why change the paradigm to require people to have a separate function to get the data when the current approach is to read it from the entity?

The prior PRs that @carldunham added did not require changing this paradigm, so why should supporting lists now require such a paradigm change?

See:
https://github.com/99designs/gqlgen/pull/1669/files#diff-44eab2c3801fddd0439e1e18f5620dcc76950e205bd8a5d7b62dd9f7f4412bb8L249
#1684
#1706
#1719
#1723
#1956

[mihirpmehta]

Following the thread

[jesse-apollo]

The most common issue is why change the paradigm to require people to have a separate function to get the data when the current approach is to read it from the entity?

The goal is to offer the most versatility with the minimal amount of generated complexity. I didn't see a way to cleanly utilize the current paradigm for all use cases that @requires supports.

The prior PRs that @carldunham added did not require changing this paradigm, so why should supporting lists now require such a paradigm change?

We initially took this route and built out some code to test it. However, it didn't seem like a viable approach due to the fact that the requires could have multiple fields, some layered under array(s). This would require generating code to do recursive array initialization and pointer to struct initialization to populate those leaf nodes at multiple levels.

See: https://github.com/99designs/gqlgen/pull/1669/files#diff-44eab2c3801fddd0439e1e18f5620dcc76950e205bd8a5d7b62dd9f7f4412bb8L249 #1684 #1706 #1719 #1723 #1956

[MiguelCastillo]

@jesse-apollo thanks for this work! the federation spec is fuzzy about whether or not fieldsets are supported in repeated fields. https://www.apollographql.com/docs/federation/federated-types/federated-directives#requires. But yeah i think it makes sense it's supposed to work.
Anyways - I wont be able to take a closer look at these changes until perhaps for a couple of days; Im out of town traveling. But will do as soon as I can.

At a quick glance it seems like you are generating a function that is called to populate the entity with the correct data right before the entity resolver itself it called? Is that overall the idea here?

[jesse-apollo]

At a quick glance it seems like you are adding a function we call in the generated code to populate the entity field with the correct data? Is that overall the idea here?

Yes, that's correct. The entity is first resolved and then passed to the generated function along with the representations map[string]interface{}. From there the developer can apply the @requires data as they see fit. This also cleanly supports using requires data for computed fields.

[StevenACoffman]

I know it's WIP, but I regenerated and pushed it to your fork as I'm curious to see if this would be sufficient to pass the https://github.com/apollographql/apollo-federation-subgraph-compatibility suite.

Feel free to continue to work on it.

[StevenACoffman]

Suggested change

	if _, ok := requiresImports[imp.ImportPath]; ok {
	// import exists in both places, remove
	delete(requiresImports, imp.ImportPath)
	}
	delete(requiresImports, imp.ImportPath)

You don't need the conditional check, as delete will no-op if the key does not exist

[StevenACoffman]

For users who are happy with the current federation support (those with no need for slices/arrays), I would like for gqlgen to continue to work without configuration changes or any new coding.

I think if people do not opt-in, you could still create a generated "PopulateUserRequires" but for them that it would not just be a stub but would continue to provide the current federation support. This would basically be a slight refactor.

If you want to add a configuration option so people can opt-in to creating a PopulateUserRequires function stub, it will facilitate more complicated use cases.

Also, it would be important to demonstrate an example of taking a stub PopulateUserRequires that would handle a more complicated use case like the current issues with federated GraphQL lists (slices/arrays).

[dariuszkuc]

For users who are happy with the current federation support (those with no need for slices/arrays), I would like for gqlgen to continue to work without configuration changes or any new coding.

While in general I agree that breaking API changes should be avoided. Hiding this improvement behind an opt-in mechanism is IMHO a bad idea as it leads to two different ways in how data is populated and will most likely end up with user errors. Current implementation is broken for list fields and nullable external fields. I guess you could highlight this in the doc but IMHO that will still lead to user confusion, i.e. how users would know whether to opt-in or use the defaults? in most cases they would try it first with default mechanism, encounter some failure (hopefully during testing and before they deploy to prod) and then try to figure out what went wrong. Unless exception message is self explanatory (right now it is not and I don't think it can be) then finding that their underlying issue is because they didn't opt-in to this mechanism may not be as simple as it sounds.

[StevenACoffman]

This PR does not currently provide an example of how to code a PopulateUserRequires method to support list fields and nullable external fields, nor how to code a PopulateUserRequires for the currently working GraphQL federation use cases.

As a result, the current PR is not just a breaking change requiring some minor work to update. This PR change requires a substantial engineering project for every user requiring a deep and nuanced view of both GraphQL and federation. It also substantially increases the scope of work necessary for anyone new who wants to adopt gqlgen with federation support, such as those considering transitioning their backends from other languages.

I am going to be on vacation for the next 9 days. I believe my colleague @MiguelCastillo will be able to spend some time mulling this over and seeing what we can do here.

[mihirpmehta]

In my opinion, any real world complex use case that uses federation, will have use case where nested field @require would exist in schema. Ideally Gqlgen library should take care of it (Implicit parsing or explicit on developer).

In my Opinion this PR fixes a bug, rather than implementing a corner use case. It changes underlying implementation a bit is debatable but there should not be opt-in/opt-out , this is must have functionality for federation to work for any real world use case. Even if someone does not have nested @require fields dependency initially, they may introduce it at later stage (May be after moving to production) ... even if we mention this on doc people do not read Readme file line by line :)

Also we are using this library for our project and due to this bug, we are not able to move forward with it. if this PR merge sooner it would be better for our team.

[StevenACoffman]

Hey, I just merged #2720 which will make the resolver implementation configurable via a new template resolver.gotpl

I wonder if you could similarly facilitate more complicated federation use cases with some sort of optional template?

[ldebruijn]

Two bugs I found with the changeset:

• Requires fields that take arguments foo(limit:4) currently fail code gen because the argument is seen as part of the field name. This can be solved by cleaning field names by splitting from the argument opening character like strings.Split(raw, "(")[0]
• The argument key & value need to be typed without any spaces, otherwise they're seen as separate fields. foo(limit:4) works in codegen, foo(limit: 4) breaks.

I've forked this changeset in order to utilize this feature while waiting for this to be merged. If need by I can push my fixes for these to this PR.

[StevenACoffman]

@ldebruijn can you make a small repository that demonstrates federation using your gqlgen fork?

[parkerroan]

Following the thread here, we at Shipt would love to see this fix get merged. I will try to help if I see an area where we can contribute.

[StevenACoffman]

@parkerroan can you or your colleagues make a demo federation repository that uses this PR to demonstrate how this could possibly work?

[parkerroan]

Yes, we have a demo repo for testing federation features that is pretty lightweight. I will take a look at this PR and test it out, I have not had a chance to really look into the details of this yet to be honest as I just found this PR today. Just reading the thread and description from @jesse-apollo it looks promising.

[mihirpmehta]

Can someone expidite this merge ?

[StevenACoffman]

This PR will break existing federation users, without any examples for how they can fix things.

[StevenACoffman]

Please open a new PR that addresses one or more of the issues without breaking existing users.