Support for multiple @key directives in federation (reworked) #1723

carldunham · 2021-11-21T22:49:36Z

Reworked support for multiple @key and @requires directives in federation.

Would like feedback in general, and specifically for real use cases.

I have:

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

coveralls · 2021-11-21T22:51:40Z

Coverage increased (+0.06%) to 70.742% when pulling e9c4229 on carldunham:cd/multiple-keys-redux into 2747bd5 on 99designs:master.

StevenACoffman

Looks pretty good to me. Do you feel like you're ready for review at this point? I can ask @MiguelCastillo if he has any more suggestions before merging.

carldunham · 2021-11-22T02:50:43Z

So @MiguelCastillo had some specific issues he found in the last one, so happy to wait for his input. I pinged him on discord about it. I'll remove the "draft" status and we can go from there.

plugin/federation/federation.go

carldunham · 2021-11-22T03:13:01Z

plugin/federation/federation.go

+				//
+				for _, keyField := range r.KeyFields {
+					if len(keyField.Field) == 0 {
+						fmt.Println("skipping field " + keyField.Definition.Name + " in " + r.ResolverName + " in " + e.Def.Name)


Is this output needed?

i'd think so. I think it would be useful to log why we are skipping a @key. tho I would treat this more as an error, but leaving as is for now is good idea.

@carldunham let's add the word key in the message again. It makes it more clear what it is we are skipping. Below in line 217 we include requires to disambiguate.

plugin/federation/federation.gotpl

MiguelCastillo · 2021-11-22T13:22:04Z

So @MiguelCastillo had some specific issues he found in the last one, so happy to wait for his input. I pinged him on discord about it. I'll remove the "draft" status and we can go from there.

Thanks for the ping! I will give it a look later today.

MiguelCastillo · 2021-11-25T14:07:07Z

example/federation/products/graph/generated/federation.go

-			if err != nil {
-				return errors.New(fmt.Sprintf("Field %s undefined in schema.", "id"))
+			entity, err := func() (*model.Product, error) {
+				id0, err := ec.unmarshalNString2string(ctx, rep["manufacturer"].(map[string]interface{})["id"])


This issue is quite tricky. If the query entity was for FindProductByUpc and there is no manufacturer representation, than this will fail because we would be trying to read id from nil.

We definitely want plugin/federation/federation_entityresolver_test.go tests to verify that we can have a nested key and query entities with all the resolvers.

Like here [1] we could add something like

type World @key(fields: "hello { name } foo ") @key(fields: "bar") { foo: String! bar: Int! hello: Hello }

And the tests can have representations like:

{ "__typename": "World", "bar": 1, }, { "__typename": "World", "hello": map[string]interface{}{ "name": "world name - 1", }, "foo": "foo 1", },

gqlgen/plugin/federation/testdata/entityresolver/schema.graphql

Line 8 in 4581fcc

type World @key(fields: "hello { name } foo ") {

https://go.dev/play/p/yLWsp2g0gyR

MiguelCastillo · 2021-11-25T14:23:28Z

plugin/federation/testdata/entities/nokey.yml

@@ -4,3 +4,6 @@ exec:
  filename: testdata/entities/generated/exec.go
 federation:
  filename: testdata/entities/generated/federation.go
+
+autobind:
+  - "github.com/99designs/gqlgen/plugin/federation/test_data/model"


It would be good to have a separate test with autobind. Maybe have a different nokey-autobind.yml so that we can ensure these tests work with model autogen as well as autobinding. WDYT?

MiguelCastillo · 2021-11-25T14:54:59Z

Thanks for tackling this complicated issue. The code itself to enable this looks good to me. The issues are more around the generated code:

Wrapping the resolver in a function needs us to have import logic that make this complicated. Ultimately I think we want to remove this, which I have some thoughts about below.
The process of determining which resolver to call needs some tweaking so that we are not building the arguments for the resolvers and also calling the resolver in that same self executing function. And then calling the next entity resolver if the first one returned nil. This can be non-deterministic behavior because it is possible that the correct entity resolver can return nil for the entity and nil for the error, and in that case we are going to try to call another resolver. And can also be tricky to address the issue I described here Support for multiple @key directives in federation (reworked) #1723 (comment)

I think that we can address both issues together tho! What if we generated a function that for a given type and representation, it would determine what resolver to call.

func entityResolverNameForRepresentation(typename string, rep map[string]interface{}) (string, error) {
   switch typename:
   case "World":
      // Do the logic to determine rep based on the representations
      // what resolver to call.  This is the complicated part that you
      // are currently doing in the self executing functions.  But instead
     // of marshaling, we are just figuring what function to call.
      return "findHelloByUpc", nil
}

Then in resolveEntity the switch statement would look something more like:

   case "World":
      switch entityResolverForType(typename, rep)
        case "findHelloByUpc":
          // the same old marshaling logic we currently have; no more wrapping
          // the entity resolver.

Let me know if this makes sense and or if you would like more specific implementation details.

Again, thanks for tackling this complicated issue!

carldunham · 2021-11-28T16:55:53Z

Smart. Let me take a run at this and see where it goes. Getting rid of the imports would definitely clean things up, and this seems like a good way to do it.

StevenACoffman · 2021-11-30T15:51:50Z

So... @duckbrain are you still working on this, or do you want a re-review as is?

carldunham · 2021-11-30T18:14:55Z

I'm still working on this between other things, should have something to review in the next day or two.

carldunham · 2021-12-01T03:06:51Z

@MiguelCastillo @StevenACoffman (and @duckbrain, if you care 🙂), I have updated this based on comments. Still missing a couple of things, noted in the commit comments. Happy to continue, or address in a follow-on. Also willing to squash and clean up before you merge.

Thanks!

StevenACoffman · 2021-12-01T14:48:14Z

(Oh so sorry duckbrain... I just typed @, the first suggestion was you for some reason, and I just tab completed without thinking).

MiguelCastillo · 2021-12-01T18:00:17Z

@carldunham i am planning on taking another look either today or tomorrow. thank you for your patience and your work on this!

lkendrickd · 2021-12-01T21:17:17Z

@MiguelCastillo Thank you for looking into this. We just adopted gqlgen and I was wondering when I had multiple @key described no entity resolvers would be created after the first initial @key. I believe this also has to do with this open issue from last year.
#1031

superbeeny · 2021-12-06T19:28:03Z

Looking forward to this PR getting merged. Thanks!

LockedThread · 2021-12-06T23:08:54Z

@MiguelCastillo I would appreciate it a lot if you could continue this. This is a huge factor of gqlgen. If you're not able to could you tell me what else needs to be done so I could contribute?

MiguelCastillo · 2021-12-10T15:28:38Z

plugin/federation/federation.go

-			if len(keys) > 1 {
-				// TODO: support multiple keys -- multiple resolvers per Entity
-				panic("only one @key directive currently supported")
+			if len(keys) == 0 {


Food for thought perhaps for later PRs. One of the things that makes this code kinda hard to grok unless you are very familiar with federation internals is the lack of human readable conditionals. Like I'd love to have a helper function here that's perhaps called something is isFederatedEntity or something. And that checks schemeType.Kind == ast.Object and ensure that there are federated keys.

But yeah not for this PR. Just thoughts I have had about how to make this code more readable and accessible to folks.

Went ahead and added this. Hopefully returning the keys isn't more confusing.

Yeah I think its great. The only nit is that functions that start with is usually are just predicate functions. I would have called this instead perhaps getFederatedEntityKeys or something and that can return they keys and ok...

but this is perfect. We can always iterate on this nits.

MiguelCastillo · 2021-12-10T15:31:28Z

plugin/federation/federation.go

+
+			// Let's process custom entity resolver settings.
+			dir := schemaType.Directives.ForName("entityResolver")
+			if dir != nil {


Same sort of idea here. Like a helper function like configureEntityResolverDirective... And configureKeyDirective... These are TODOs in my head. Hoping to get more cycles for this over the break.

Left this for you 🙂

Thank you :)

MiguelCastillo · 2021-12-10T16:03:26Z

plugin/federation/federation.go

+						InputType:    resolverFieldsToGo + "Input",
+					})
+
+					e.Requires = []*Requires{}


I think we want to process requires directives outside of the loop for processing all the keys to avoid reprocessing them.

Yeah, dumb oversight on my part!

all good! these changes were very tricky. and hard to catch in unit tests since the generated should still work correctly. 🤷

MiguelCastillo · 2021-12-10T16:13:28Z

plugin/federation/federation.gotpl

-
-				list[idx[i]] = entity
-				return nil
+					{{ if eq (len .Resolvers) 1 }}


I think it should be OK to always generate the switch statement. There isn't a huge runtime performance penalty and it keep this already giant template more manageable and less error prone since you have to make any template changes in multiple places.

Or is there a reason you'd prefer to having a different generated code when there is only 1 entity resolver vs multiple?

Only that it made the generated code cleaner. But you're right, it comes at the cost of a more complicated template, which will probably need to be comprehended more often. I'm removing the ==1 case.

There were two places where this could be done, hopefully strikes a good balance.

MiguelCastillo · 2021-12-10T16:18:48Z

plugin/federation/federation.gotpl

@@ -163,7 +195,8 @@ func (ec *executionContext) __resolve_entities(ctx context.Context, representati
 					return nil
 				{{ end }}
 			{{ end }}
-		{{- end }}
+		{{ end }}
+	{{- end }}


😅 templates tags are so hard to match up!!! Did an {{ end }} sneak in or is there a misalignment that makes it harder to spot where the opening tag is? I think the new {{ end }} tag is for line 161?

Hopefully, it's a little more aligned and clear now. I think indenting at the case statements might make things clearer? But not doing that yet.

MiguelCastillo · 2021-12-10T16:25:21Z

plugin/federation/federation.gotpl

+var (
+	ErrUnknownType = errors.New("unknown type")
+	ErrTypeNotFound = errors.New("type not found")
+	ErrUnableToResolveEntity = errors.New("unable to resolve Entity")


Is ErrUnableToResolveEntity being used? It seems like it isn't...

Not any more :)

MiguelCastillo · 2021-12-10T16:38:55Z

plugin/federation/federation.gotpl

+{{ range $_, $entity := .Entities }}
+	{{- if .Resolvers }}
+
+		func (ec *executionContext) entityResolverNameFor{{$entity.Name}}(ctx context.Context, rep map[string]interface{}) (string, error) {


From a mental model standpoint, I think it would be a lil better if ec was passed in as an argument to entityResolverNameFor... Not a major thing, so if you feel strongly about keeping it as is then no problem. But in this generated code the execution engine mostly really care about being able to call __resolve__service and __resolve_entities as well as providing an interface for marshaling fields.

Tho - what do you think about not calling any marshaling functions in the generated entityResolverNameFor functions? We are primarily interested in matching up the shapes of rep to determine what resolver to call. We don't care as much about marshaling the values. Especially since rep values are getting marshaled again before calling the entity resolvers. That would do two things: 1. Avoid double marshaling for values. 2. Can allow us to not even pass ec into these generated function.

Thoughts?

The only weird cases would be where an invalid representation in the map[string]interface{} would result in that resolver being chosen, rather than another that had proper reps. Of course, the reverse could happen also, and not be caught. I'll modify it and we can see how it looks.

I think the generated turned out great! We can definitely tweak as we get more use of this new functionality tho.

MiguelCastillo · 2021-12-10T16:41:56Z

@carldunham this is absolute awesome! Im really excited for these changes to land, and so are lots of other folks. 👏
I left you a couple of thoughts for cleaning up. But nothing really major. Overall the flow and the generated code are pretty awesome. Let me know what you think of the comments I left and we can land this!

carldunham · 2021-12-10T16:47:02Z

Thanks, @MiguelCastillo. Great feedback! I'll attack this over the weekend, if not today. It looks like mostly moving and removing, should clean things up nicely!

MiguelCastillo · 2021-12-10T17:04:23Z

@MiguelCastillo I would appreciate it a lot if you could continue this. This is a huge factor of gqlgen. If you're not able to could you tell me what else needs to be done so I could contribute?

Thank you for your offer @LockedThread! 💟 Happily I was able to squeeze in some cycles today for this. I think we are very close to landing. And it's always helpful to have other folks eyes on PRs, so if you go thru the changes and spot anything - call it out! It can only help :)

LockedThread · 2021-12-10T18:00:35Z

@MiguelCastillo I would appreciate it a lot if you could continue this. This is a huge factor of gqlgen. If you're not able to could you tell me what else needs to be done so I could contribute?

Thank you for your offer @LockedThread! 💟 Happily I was able to squeeze in some cycles today for this. I think we are very close to landing. And it's always helpful to have other folks eyes on PRs, so if you go thru the changes and spot anything - call it out! It can only help :)

Alright, I'll stay up-to-date on your changes. Good luck!

@multi

- reworked code generation for federation.go - better checking for missing/incorrect parameters to entity resolver functions - better tests for generated entity resolvers Still missing: - suggested test for autobind vs non-autobind generation - could probably clean up generated code spacing, etc - not sure @multi cases are handled completely (if they apply)

carldunham · 2021-12-13T02:21:30Z

@MiguelCastillo @LockedThread ready for re-review!

MiguelCastillo · 2021-12-13T14:14:26Z

plugin/federation/federation.go

 	}

 	// make sure order remains stable across multiple builds
 	sort.Slice(f.Entities, func(i, j int) bool {
 		return f.Entities[i].Name < f.Entities[j].Name
 	})
 }
+
+func isFederatedEntity(schemaType *ast.Definition) ([]*ast.Directive, bool) {


Ah thank you! Yeah this is is great. We can tweak the function name as we use it more.

MiguelCastillo

Woohoo! This is great, @carldunham. Thank you for all your work with this.

StevenACoffman approved these changes Nov 22, 2021

View reviewed changes

carldunham marked this pull request as ready for review November 22, 2021 02:51

carldunham commented Nov 22, 2021

View reviewed changes

plugin/federation/federation.go Outdated Show resolved Hide resolved

carldunham commented Nov 22, 2021

View reviewed changes

plugin/federation/federation.gotpl Outdated Show resolved Hide resolved

carldunham force-pushed the cd/multiple-keys-redux branch from 4ccca87 to 61de5e9 Compare November 22, 2021 03:20

carldunham force-pushed the cd/multiple-keys-redux branch from 61de5e9 to 26a7283 Compare November 22, 2021 21:38

MiguelCastillo reviewed Nov 25, 2021

View reviewed changes

carldunham force-pushed the cd/multiple-keys-redux branch from 26a7283 to 3407f7a Compare November 29, 2021 03:35

carldunham force-pushed the cd/multiple-keys-redux branch 2 times, most recently from aa2b044 to f46c6bb Compare December 1, 2021 03:02

carldunham force-pushed the cd/multiple-keys-redux branch from f46c6bb to da38c74 Compare December 9, 2021 01:54

Support for multiple @key directives in federation (reworked)

6e7fb59

carldunham force-pushed the cd/multiple-keys-redux branch from da38c74 to 3ad58d5 Compare December 9, 2021 18:29

MiguelCastillo reviewed Dec 10, 2021

View reviewed changes

carldunham force-pushed the cd/multiple-keys-redux branch from 3ad58d5 to e9c4229 Compare December 13, 2021 02:17

MiguelCastillo reviewed Dec 13, 2021

View reviewed changes

MiguelCastillo approved these changes Dec 13, 2021

View reviewed changes

MiguelCastillo merged commit 14cfee7 into 99designs:master Dec 13, 2021

carldunham deleted the cd/multiple-keys-redux branch December 13, 2021 16:56

jclyons52 mentioned this pull request Dec 19, 2022

[Federation] Allow multiple entity keys. #1031

Closed

StevenACoffman added the federation Related to Apollo federation label Jun 14, 2023

StevenACoffman mentioned this pull request Jun 14, 2023

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

Closed

2 tasks

Support for multiple @key directives in federation (reworked) #1723

Support for multiple @key directives in federation (reworked) #1723

Conversation

carldunham commented Nov 21, 2021

coveralls commented Nov 21, 2021 • edited Loading

StevenACoffman left a comment • edited Loading

Choose a reason for hiding this comment

carldunham commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCastillo commented Nov 22, 2021

MiguelCastillo Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCastillo commented Nov 25, 2021 • edited Loading

carldunham commented Nov 28, 2021

StevenACoffman commented Nov 30, 2021

carldunham commented Nov 30, 2021

carldunham commented Dec 1, 2021

StevenACoffman commented Dec 1, 2021

MiguelCastillo commented Dec 1, 2021

lkendrickd commented Dec 1, 2021

superbeeny commented Dec 6, 2021

LockedThread commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCastillo Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCastillo Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCastillo Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MiguelCastillo commented Dec 10, 2021 • edited Loading

carldunham commented Dec 10, 2021

MiguelCastillo commented Dec 10, 2021

LockedThread commented Dec 10, 2021

carldunham commented Dec 13, 2021

Choose a reason for hiding this comment

MiguelCastillo left a comment

Choose a reason for hiding this comment

coveralls commented Nov 21, 2021 •

edited

Loading

StevenACoffman left a comment •

edited

Loading

MiguelCastillo Nov 25, 2021 •

edited

Loading

MiguelCastillo commented Nov 25, 2021 •

edited

Loading

MiguelCastillo Dec 10, 2021 •

edited

Loading

MiguelCastillo Dec 10, 2021 •

edited

Loading

MiguelCastillo Dec 10, 2021 •

edited

Loading

MiguelCastillo commented Dec 10, 2021 •

edited

Loading