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

Correctly generate a federated schema when no entity has a @key. #985

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

csilvers
Copy link
Contributor

Normally, when a service is taking part in graphql federation, it will
have at least one type defined with a @key field, so that other
services can link to (that is, have an edge pointing to) the type that
this service provides. The previous federation code assumed that was
the case.

But it's not required: a service could not define @key on any of its
types. It might seem that would mean the service is unreachable,
since there is no possibility of edges into the service, but there are
two edges that can exist even without a @key: top level Query edges
and top level Mutation edges. That is, if a service only provides a
top-level query or top-level mutation, it might not need to define a
@key.

This commit updates the federation code to support that use case.

Describe your PR and link to any relevant issues.

I have:

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

@csilvers
Copy link
Contributor Author

I don't know what the protocol is for marking people who I think would be good to review this, so I'll just do it as a comment: @marwan-at-work I think you're the best one to review this!

@@ -56,9 +56,11 @@ func (f *federation) MutateConfig(cfg *config.Config) error {
"_Any": {
Model: config.StringList{"github.com/99designs/gqlgen/graphql.Map"},
},
"Entity": {
}
if len(entityFields) != 0 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you consider changing it as follows?
if len(entityFields) > 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@csilvers
Copy link
Contributor Author

(I maybe did the wrong thing by amending rather than pushing a new commit, but this is ready for another look!)

Copy link
Contributor

@marwan-at-work marwan-at-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm :)

@csilvers
Copy link
Contributor Author

Anything else I need to do to help get this merged? I don't understand the 'cover' failure -- it's in an area unrelated to my change, and the diff seems non-contentful to me. Hopefully that's not the thing holding things up, since I don't know what to do about it...

@marwan-at-work
Copy link
Contributor

I believe @vektah could help get this merged :)

As for cover, I think you have a failing test. Does go test ./... succeed locally?

@csilvers
Copy link
Contributor Author

I do have a failing test locally:

--- FAIL: TestNameForDir (0.24s)
    imports_test.go:45: 
                Error Trace:    imports_test.go:45
                Error:          Not equal: 
                                expected: "tmp"
                                actual  : "linewrap"
                                
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -tmp
                                +linewrap
                Test:           TestNameForDir

This differs from the failing test in circleci:

--- FAIL: TestPrune (0.04s)
    require.go:157: 
        	Error Trace:	prune_test.go:13
        	Error:      	Not equal: 
        	            	expected: "package testdata\n\nimport _ \"underscore\"\nimport a \"fmt\"\nimport \"time\"\n\ntype foo struct {\n\tTime time.Time `json:\"text\"`\n}\n\nfunc fn() {\n\ta.Println(\"hello\")\n}\n\ntype Message struct {\n\tID        string    `json:\"id\"`\n\tText      string    `json:\"text\"`\n\tCreatedBy string    `json:\"createdBy\"`\n\tCreatedAt time.Time `json:\"createdAt\"`\n}\n"
        	            	actual  : "package testdata\n\nimport (\n\ta \"fmt\"\n\t\"time\"\n\t_ \"underscore\"\n)\n\ntype foo struct {\n\tTime time.Time `json:\"text\"`\n}\n\nfunc fn() {\n\ta.Println(\"hello\")\n}\n\ntype Message struct {\n\tID        string    `json:\"id\"`\n\tText      string    `json:\"text\"`\n\tCreatedBy string    `json:\"createdBy\"`\n\tCreatedAt time.Time `json:\"createdAt\"`\n}\n"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -2,5 +2,7 @@
        	            	 
        	            	-import _ "underscore"
        	            	-import a "fmt"
        	            	-import "time"
        	            	+import (
        	            	+	a "fmt"
        	            	+	"time"
        	            	+	_ "underscore"
        	            	+)
        	            	 
        	Test:       	TestPrune
FAIL

Neither of them should be affected by any code I changed.

Copy link
Collaborator

@vvakame vvakame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csilvers I tried merge master/HEAD into this changes and test it locally. it passed cover job.
please rebase onto master/HEAD about this PR.

Normally, when a service is taking part in graphql federation, it will
have at least one type defined with a `@key` field, so that other
services can link to (that is, have an edge pointing to) the type that
this service provides.  The previous federation code assumed that was
the case.

But it's not required: a service could not define `@key` on any of its
types.  It might seem that would mean the service is unreachable,
since there is no possibility of edges into the service, but there are
two edges that can exist even without a `@key`: top level Query edges
and top level Mutation edges.  That is, if a service only provides a
top-level query or top-level mutation, it might not need to define a
`@key`.

This commit updates the federation code to support that use case.
@csilvers
Copy link
Contributor Author

I merged and the tests are still failing. I really believe the test failures are not due to my code, but some sort of flakiness. This time it's this:

/usr/local/go/pkg/tool/linux_amd64/link: signal: killed
FAIL	github.com/99designs/gqlgen/client [build failed]

I don't know what is causing the linker to be killed -- OOM? -- but it looks like that's what's causing the test failure.

I don't know if it's possible to just force-rerun the tests, or if it even matters -- I guess we can merge even with the test failure, if we're confident this PR isn't responsible.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 63.284% when pulling fa88499 on Khan:no-key-needed into 36aae4a on 99designs:master.

@vvakame
Copy link
Collaborator

vvakame commented Jan 24, 2020

I re-run the tests manually, its passed.
https://stackoverflow.com/questions/52129612/golang-circleci-2-0-test-failing-with-signal-killed
This is a problem I haven't seen much before...
so, test is passed. and LGTM.
thanks to your contributions. and thanks to your review @marwan-at-work !

@vvakame vvakame merged commit f7a6772 into 99designs:master Jan 24, 2020
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
Correctly generate a federated schema when no entity has a `@key`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants