Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apollo Federation MVP #851

Merged
merged 11 commits into from Jan 7, 2020
Merged

Apollo Federation MVP #851

merged 11 commits into from Jan 7, 2020

Conversation

@marwan-at-work
Copy link
Contributor

marwan-at-work commented Aug 30, 2019

This PR is mostly to get some eyes on the current architecture and what can be improved/re-written.

Once that looks good, I'll move on to adding unit/e2e tests

Fixes #740

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 30, 2019

Coverage Status

Coverage decreased (-0.7%) to 63.145% when pulling 48dc29c on marwan-at-work:federation into f869f5a on 99designs:master.

Copy link

JulienBreux left a comment

Very nice work! Here a tiny review 😊
Thanks again!

codegen/directives.gotpl Outdated Show resolved Hide resolved
api/generate.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
plugin/federation/federation.go Show resolved Hide resolved
plugin/federation/federation.go Show resolved Hide resolved
plugin/federation/federation.go Outdated Show resolved Hide resolved
plugin/federation/federation.go Show resolved Hide resolved
plugin/federation/federation.go Show resolved Hide resolved
plugin/federation/federation.go Show resolved Hide resolved
Copy link
Collaborator

vektah left a comment

Nice work, the code looks good.

I was hoping for more feedback from people who want to use this. I don't think there are any BC breaks in here though so let's get this ready to ship and iterate.

Might be nice to enumerate the current known limitations in the PR description (eg currently only supports single field simple keys)

codegen/directives.gotpl Outdated Show resolved Hide resolved
@@ -95,6 +95,18 @@ func (b *builder) bindField(obj *Object, f *Field) error {
f.GoReceiverName = "ec"
f.GoFieldName = "introspectType"
return nil
case f.Name == "_entities":

This comment has been minimized.

Copy link
@vektah

vektah Sep 16, 2019

Collaborator

does this generate invalid code if the plugin is missing? perhaps this should be injected by the plugin?

This comment has been minimized.

Copy link
@vvakame

vvakame Oct 8, 2019

Collaborator

It would be nice to have a mechanism that can be extended with plugins.

This comment has been minimized.

Copy link
@marwan-at-work

marwan-at-work Nov 12, 2019

Author Contributor

@vektah maybe if the user has an _entities query without federation...we can possibly check cfg.Federated && f.Name == _entities ... that said, it would be great if the plugin can inject this part, but it looks like there's no way right now...can you confirm? Thanks!

This comment has been minimized.

Copy link
@vektah

vektah Nov 12, 2019

Collaborator

Welcome back!

I wonder if we could add something to TypeMapField in config that replaces all of this logic?

type TypeMapField struct {
	Resolver  bool   `yaml:"resolver"`
	FieldName string `yaml:"fieldName"`

	// new
	GeneratedMethod string `yaml:"-"`
}

which would be set here to reference __resolve_entities

This comment has been minimized.

Copy link
@marwan-at-work

marwan-at-work Dec 7, 2019

Author Contributor

@vektah that's an interesting idea...I'll give it a shot 👍

This comment has been minimized.

Copy link
@marwan-at-work

marwan-at-work Dec 7, 2019

Author Contributor

Actually, TypeMapField is in MutateConfig not MutateSchema where you linked above, can you expand more on how that would work? Thanks!

This comment has been minimized.

Copy link
@vektah

vektah Jan 7, 2020

Collaborator

Lets leave it like this for now and get this on master. Might do some cleanup once it lands to make this more pluggable.

codegen/templates/templates.go Outdated Show resolved Hide resolved
plugin/federation/federation.go Outdated Show resolved Hide resolved
for _, ent := range f.Entities {
union.Types = append(union.Types, ent.Name)
s.AddPossibleType("_Entity", ent.Def)
// s.AddImplements(ent.Name, union) // Do we need this?

This comment has been minimized.

Copy link
@vektah

vektah Sep 16, 2019

Collaborator

This comment has been minimized.

Copy link
@marwan-at-work

marwan-at-work Dec 7, 2019

Author Contributor

@vektah does that mean I won't need to check for the key directive if I add this line?

This comment has been minimized.

Copy link
@marwan-at-work

marwan-at-work Dec 7, 2019

Author Contributor

Nevermind, I uncommented the line and it added _Entity to implementors. This seems to only be used in fragment-related things?


// Entity represents a federated type
// that was declared in the GQL schema.
type Entity struct {

This comment has been minimized.

Copy link
@vektah

vektah Sep 16, 2019

Collaborator

I think I would prefer if this referenced the underlying types where possible, rather than copying them just out, just in case another plugin is modifying them.

  • We already have Def, so Name is redundant
  • FieldName and FieldTypeGQL come from FieldDef, so let's add that instead?

If we apply the same logic to RequireField I think it can be replaced entirely by a codegen.Field

This comment has been minimized.

Copy link
@marwan-at-work

marwan-at-work Dec 7, 2019

Author Contributor

@vektah I did it for Def and FieldDef. However, RequireField would make setEntities create a dummy codegen.Field so I can have a Name to match against in GenerateCode, then GenerateCode will have to replace its codegen.Field with the dummy one which fields a bit ugly. Would you like me to switch that up as well?

@vektah vektah mentioned this pull request Oct 1, 2019
Copy link
Collaborator

vvakame left a comment

If Apollo federation completely separate plugin from gqlgen core code, It's greatful works!
so, plugin system is still developing. this work is very good as a space explorer, I think.

Please tell me anything I can do. 😉

@@ -95,6 +95,18 @@ func (b *builder) bindField(obj *Object, f *Field) error {
f.GoReceiverName = "ec"
f.GoFieldName = "introspectType"
return nil
case f.Name == "_entities":

This comment has been minimized.

Copy link
@vvakame

vvakame Oct 8, 2019

Collaborator

It would be nice to have a mechanism that can be extended with plugins.

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

This comment has been minimized.

Copy link

benjaminjkraft commented Oct 11, 2019

I was hoping for more feedback from people who want to use this.

There are some other users over in #740, but for us (Khan Academy) I can say we're already using this and are very happy to see it merged!

@cliedeman

This comment has been minimized.

Copy link

cliedeman commented Oct 14, 2019

Interested in SchemaMutator to implement a relay plugin

@nurdism

This comment has been minimized.

Copy link

nurdism commented Oct 14, 2019

couple issues I've found while messing with this

there should be a way to define

service:
  filename: "..."
  package: "..."

in the gqlgen.yml (there is a PR for this on your fork)

When generating service.go, you use _Entity interface (which goes in the generated models package), in my setup models are generated on a models package (from gqlgen.yml) , the _Entity interface should be declared in the service.go package (or use a different name). Underscore values are not exported and will cause errors when the packages differ.

When using custom scalars as the key in the @key directive, it should import the package and use the type when casting it
IE

id, ok := rep["id"].(my.pkg/custom/scalar.type)

to

import "my.pkg/custom/scalar"
...
id, ok := rep["id"].(scalar.type)

In my case this is causing validation errors.

(edit: thinking about it, would it make sense for service.go to be part of exec package? Wondering why it was decided to be a separate file, it would solve the first 3 issues I've run into)

should be a way for users to define the federation spec (for external validations or the schema files) and fill in the blanks (of it is not provided)

lastly, more of a nit pick, but the errors for failed type casting on __resolve_entities func in service.go should be more declarative (ie "Failed casting id to scalar.type", rather than it saying "opsies")

Other than that, working pretty well for me. Still doing some testing. I'll report back if I find anything else. Keep up the good work 👍

@sothychan

This comment has been minimized.

Copy link

sothychan commented Nov 7, 2019

This is great. I'm going to try using this now and will provide feedback if any.

@sothychan

This comment has been minimized.

Copy link

sothychan commented Nov 12, 2019

@marwan-at-work @vektah I have a PR that pulls in the latest gqlgen into @marwan-at-work 's federation branch and fixed the merge conflicts. Here it is: marwan-at-work#3

@marwan-at-work

This comment has been minimized.

Copy link
Contributor Author

marwan-at-work commented Nov 12, 2019

@sothychan that's great thank you 🙏 -- i've been incredibly busy the last couple of months (switched companies), but will get back to this ASAP.

@sothychan

This comment has been minimized.

Copy link

sothychan commented Nov 12, 2019

@marwan-at-work I'm running into a weird issue saying "_Entity not found in typemap" when I bind models. I'm going to look into why and submit a PR.

@vvakame

This comment has been minimized.

Copy link
Collaborator

vvakame commented Nov 19, 2019

hi, I tried to implement https://github.com/apollographql/federation-demo by gqlgen with this PR.
It's works fine! 👏

I can make public my repository but federation-demo repository's license is unclear.
I opened apollographql/federation-demo#12 . please wait.

@josemarluedke

This comment has been minimized.

Copy link

josemarluedke commented Dec 4, 2019

@marwan-at-work @vektah

Any new development in this area? We are starting to look for a service-oriented graphql, and the federation would be perfect for that. Any changes to get this moving?

@StevenACoffman

This comment has been minimized.

Copy link

StevenACoffman commented Dec 5, 2019

@marwan-at-work

This comment has been minimized.

Copy link
Contributor Author

marwan-at-work commented Dec 5, 2019

Now that I'm fully onboarded with my new job...I'm very happy to come back to this, I've cleared my Sunday for this PR 👍

Hopefully I can wrap it up within that day but I did notice there are some merge conflicts also when I generate tests a lot of them do fail...although federation itself works okay 😄

@marwan-at-work marwan-at-work force-pushed the marwan-at-work:federation branch from 8fb9130 to 368d546 Dec 7, 2019
@marwan-at-work

This comment has been minimized.

Copy link
Contributor Author

marwan-at-work commented Dec 7, 2019

@vektah all of the comments are addressed except:

  1. field.go (would love to know more about how we can fix that)
  2. making the build/tests pass as I'm not familiar with the codebase enough to know why they're failing :)
@marwan-at-work

This comment has been minimized.

Copy link
Contributor Author

marwan-at-work commented Dec 8, 2019

@vektah I managed to get the builds to pass but federation tests are still super preliminary.

This leaves the field.go which I'd need some guidance as to how to inject into it from the plugin or I can a TODO comment for a follow up PR if that's something that requires more figuring out.

Thanks

@vvakame vvakame requested a review from vektah Dec 19, 2019
@lwc
lwc approved these changes Dec 23, 2019
@jepp2078

This comment has been minimized.

Copy link

jepp2078 commented Jan 6, 2020

Quick question @marwan-at-work. Won't the name _Entity in the generated model give problems for people that use packages for their models, since _Entity isn't exported?

@vektah

This comment has been minimized.

Copy link
Collaborator

vektah commented Jan 7, 2020

I've merged the gqlparser pr and tagged a new release. Lets update the this and get it 🚢'd.

This leaves the field.go which I'd need some guidance as to how to inject into it from the plugin or I can a TODO comment for a follow up PR if that's something that requires more figuring out.

Lets ship this as is and iterate on master.

@marwan-at-work

This comment has been minimized.

Copy link
Contributor Author

marwan-at-work commented Jan 7, 2020

@vektah sounds great, I updated the go.mod file ✌️

@jepp2078 that's a valid point, although I'm not sure in which use case you'd need to programmatically import the _Entity interface. That said, I imagine it would be possible to let the user customize those names just like any other model.

@vektah

This comment has been minimized.

Copy link
Collaborator

vektah commented Jan 7, 2020

I think the mod file still needs a version bump to 1.2.1
d2a13d3#diff-37aff102a57d3d7b797f152915a6dc16L25

@marwan-at-work

This comment has been minimized.

Copy link
Contributor Author

marwan-at-work commented Jan 7, 2020

@vektah done

@vektah vektah merged commit 8218c73 into 99designs:master Jan 7, 2020
6 checks passed
6 checks passed
ci/circleci: cover Your tests passed on CircleCI!
Details
ci/circleci: integration Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage decreased (-0.7%) to 63.145%
Details
@jepp2078

This comment has been minimized.

Copy link

jepp2078 commented Jan 7, 2020

@jepp2078 that's a valid point, although I'm not sure in which use case you'd need to programmatically import the _Entity interface. That said, I imagine it would be possible to let the user customize those names just like any other model.

@marwan-at-work I'm pretty sure it would break as soon as you set the package for exec and model to two different values, since generated.go and service.go needs to use _Entity.

@marwan-at-work

This comment has been minimized.

Copy link
Contributor Author

marwan-at-work commented Jan 7, 2020

@jepp2078 now that this is merged, it would be incredibly helpful to open an issue with reproduction steps of how it would break. I can dig into it afterwards, thanks!

VitaliiLakusta added a commit to VitaliiLakusta/apollo-server that referenced this pull request Jan 23, 2020
gqlgen - one of the most popular graphql Go lib - just recently added support for federation. 
PR to the lib was merged to master Jan 7 - 99designs/gqlgen#851

The lib doesn't have documentation for the feature yet, but it's fully working, as I am already using it in one of the projects.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.