From 0da639309954e0c0ed2f4c9953a91be0a3d3ab3c Mon Sep 17 00:00:00 2001 From: Erik Sipsma Date: Mon, 11 Dec 2023 17:48:41 -0800 Subject: [PATCH 1/2] check whether object implements interface at runtime The pre-existing implementation of the graphql server required that objects essentially pre-register the interfaces they implement when they are added to the server and would error out if the object is attempted to be used when it wasn't registered as an interface. This makes sense for general graphql use cases, but in ours we don't want users to have to say ahead of time which interfaces they are implementing; they may not even know of the interfaces they could be implementing. While it's possible that we could add support for that in the future, as a baseline we don't want to require it. So this change re-uses the existing logic for checking whether an object implements an interface (normally only called when the schema is initialized) but calls it now at the time the object is returned as an interface, erroring out if it doesn't match. Signed-off-by: Erik Sipsma --- executor.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/executor.go b/executor.go index 97138c31..9e5ede66 100644 --- a/executor.go +++ b/executor.go @@ -796,6 +796,10 @@ func completeAbstractValue(eCtx *executionContext, returnType Abstract, fieldAST runtimeType = unionReturnType.ResolveType(resolveTypeParams) } else if interfaceReturnType, ok := returnType.(*Interface); ok && interfaceReturnType.ResolveType != nil { runtimeType = interfaceReturnType.ResolveType(resolveTypeParams) + // verify the object matches the interface + if err := assertObjectImplementsInterface(&eCtx.Schema, runtimeType, interfaceReturnType); err != nil { + panic(err) + } } else { runtimeType = defaultResolveTypeFn(resolveTypeParams, returnType) } @@ -807,13 +811,6 @@ func completeAbstractValue(eCtx *executionContext, returnType Abstract, fieldAST panic(err) } - if !eCtx.Schema.IsPossibleType(returnType, runtimeType) { - panic(gqlerrors.NewFormattedError( - fmt.Sprintf(`Runtime Object type "%v" is not a possible type `+ - `for "%v".`, runtimeType, returnType), - )) - } - return completeObjectValue(eCtx, runtimeType, fieldASTs, info, path, result) } From 01b09ff0d8a9bacccf8b6104a43989be9e7fd621 Mon Sep 17 00:00:00 2001 From: Erik Sipsma Date: Tue, 12 Dec 2023 10:49:17 -0800 Subject: [PATCH 2/2] fix tests to account for runtime checking of interface implementation Signed-off-by: Erik Sipsma --- abstract_test.go | 4 ++-- executor.go | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/abstract_test.go b/abstract_test.go index 321b3aeb..82121e98 100644 --- a/abstract_test.go +++ b/abstract_test.go @@ -424,7 +424,7 @@ func TestResolveTypeOnInterfaceYieldsUsefulError(t *testing.T) { humanType = graphql.NewObject(graphql.ObjectConfig{ Name: "Human", Fields: graphql.Fields{ - "name": &graphql.Field{ + "job": &graphql.Field{ Type: graphql.String, }, }, @@ -507,7 +507,7 @@ func TestResolveTypeOnInterfaceYieldsUsefulError(t *testing.T) { }, Errors: []gqlerrors.FormattedError{ { - Message: `Runtime Object type "Human" is not a possible type for "Pet".`, + Message: `"Pet" expects field "name" but "Human" does not provide it.`, Locations: []location.SourceLocation{ { Line: 2, diff --git a/executor.go b/executor.go index 9e5ede66..25246520 100644 --- a/executor.go +++ b/executor.go @@ -794,6 +794,12 @@ func completeAbstractValue(eCtx *executionContext, returnType Abstract, fieldAST } if unionReturnType, ok := returnType.(*Union); ok && unionReturnType.ResolveType != nil { runtimeType = unionReturnType.ResolveType(resolveTypeParams) + if !eCtx.Schema.IsPossibleType(returnType, runtimeType) { + panic(gqlerrors.NewFormattedError( + fmt.Sprintf(`Runtime Object type "%v" is not a possible type `+ + `for "%v".`, runtimeType, returnType), + )) + } } else if interfaceReturnType, ok := returnType.(*Interface); ok && interfaceReturnType.ResolveType != nil { runtimeType = interfaceReturnType.ResolveType(resolveTypeParams) // verify the object matches the interface