Skip to content

Allow interface resolveType functions to resolve to child interfaces #3599

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.

Sign up for GitHub

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

Closed
yaacovCR wants to merge 10 commits into from
Closed

Allow interface resolveType functions to resolve to child interfaces #3599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a30daec
Allow interface resolveType functions to resolve to child interfaces
yaacovCR May 19, 2022
d89b1ae
change function name
yaacovCR May 22, 2022
00dbea1
use variables for abstract types in tests
yaacovCR May 22, 2022
7167fcf
fix chain and add test
yaacovCR May 22, 2022
81c5ed9
standardize order
yaacovCR May 22, 2022
6c8ad45
remove unneeded check for cycles
IvanGoncharov May 23, 2022
4dbfe20
review change
IvanGoncharov Jun 1, 2022
5ba43d8
change inner recursive function to completeAbstractValueImpl
yaacovCR Jun 2, 2022
9e0c4d0
re-align error message as possible to original
yaacovCR Jun 2, 2022
225cab9
rename runtimeType(Name) to possibleRuntimeType(Name)
yaacovCR Jun 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 119 additions & 4 deletions src/execution/__tests__/abstract-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ describe('Execute: Handles execution of abstract types', () => {
errors: [
{
message:
'Abstract type "Pet" must resolve to an Object type at runtime for field "Query.pet". Either the "Pet" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.',
'Abstract type "Pet" must resolve to an Object type or an intermediate Interface type at runtime for field "Query.pet". Either the "Pet" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.',
locations: [{ line: 3, column: 9 }],
path: ['pet'],
},
Expand Down Expand Up @@ -610,15 +610,15 @@ describe('Execute: Handles execution of abstract types', () => {
}

expectError({ forTypeName: undefined }).toEqual(
'Abstract type "Pet" must resolve to an Object type at runtime for field "Query.pet". Either the "Pet" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.',
'Abstract type "Pet" must resolve to an Object type or an intermediate Interface type at runtime for field "Query.pet". Either the "Pet" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.',
);

expectError({ forTypeName: 'Human' }).toEqual(
'Abstract type "Pet" was resolved to a type "Human" that does not exist inside the schema.',
);

expectError({ forTypeName: 'String' }).toEqual(
'Abstract type "Pet" was resolved to a non-object type "String".',
'Abstract type "Pet" was resolved to a non-object and non-interface type "String".',
);

expectError({ forTypeName: '__Schema' }).toEqual(
Expand All @@ -629,7 +629,7 @@ describe('Execute: Handles execution of abstract types', () => {
// @ts-expect-error
assertInterfaceType(schema.getType('Pet')).resolveType = () => [];
expectError({ forTypeName: undefined }).toEqual(
'Abstract type "Pet" must resolve to an Object type at runtime for field "Query.pet" with value { __typename: undefined }, received "[]".',
'Abstract type "Pet" must resolve to an Object type or an intermediate Interface type at runtime for field "Query.pet" with value { __typename: undefined }, received "[]".',
);

// FIXME: workaround since we can't inject resolveType into SDL
Expand All @@ -640,4 +640,119 @@ describe('Execute: Handles execution of abstract types', () => {
'Support for returning GraphQLObjectType from resolveType was removed in graphql-js@16.0.0 please return type name instead.',
);
});

it('hierarchical resolveType with Interfaces yields useful error', () => {
const schema = buildSchema(`
type Query {
named: Named
}

interface Named {
name: String
}

interface Animal {
isFriendly: Boolean
}

interface Pet implements Named & Animal {
name: String
isFriendly: Boolean
}

type Cat implements Pet & Named & Animal {
name: String
isFriendly: Boolean
}

type Dog implements Pet & Named & Animal {
name: String
isFriendly: Boolean
}

type Person implements Named {
name: String
}
`);

const document = parse(`
{
named {
name
}
}
`);

function expectError() {
const rootValue = { named: {} };
const result = executeSync({ schema, document, rootValue });
return {
toEqual(message: string) {
expectJSON(result).toDeepEqual({
data: { named: null },
errors: [
{
message,
locations: [{ line: 3, column: 9 }],
path: ['named'],
},
],
});
},
};
}

const namedType = assertInterfaceType(schema.getType('Named'));
// FIXME: workaround since we can't inject resolveType into SDL
namedType.resolveType = () => 'Animal';
expectError().toEqual(
'Interface type "Animal" is not a possible type for "Named".',
);

const petType = assertInterfaceType(schema.getType('Pet'));
// FIXME: workaround since we can't inject resolveType into SDL
namedType.resolveType = () => 'Pet';
petType.resolveType = () => 'Person';
expectError().toEqual(
'Runtime Object type "Person" is not a possible type for "Pet".',
);

// FIXME: workaround since we can't inject resolveType into SDL
namedType.resolveType = () => 'Pet';
petType.resolveType = () => undefined;
expectError().toEqual(
'Abstract type "Pet" must resolve to an Object type or an intermediate Interface type at runtime for field "Query.named". Either the "Pet" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.',
);

// FIXME: workaround since we can't inject resolveType into SDL
petType.resolveType = () => 'Human';
expectError().toEqual(
'Abstract type "Pet" was resolved to a type "Human" that does not exist inside the schema.',
);

// FIXME: workaround since we can't inject resolveType into SDL
petType.resolveType = () => 'String';
expectError().toEqual(
'Abstract type "Pet" was resolved to a non-object and non-interface type "String".',
);

// FIXME: workaround since we can't inject resolveType into SDL
petType.resolveType = () => '__Schema';
expectError().toEqual(
'Runtime Object type "__Schema" is not a possible type for "Pet".',
);

// FIXME: workaround since we can't inject resolveType into SDL
// @ts-expect-error
petType.resolveType = () => [];
expectError().toEqual(
'Abstract type "Pet" must resolve to an Object type or an intermediate Interface type at runtime for field "Query.named" with value {}, received "[]".',
);

// FIXME: workaround since we can't inject resolveType into SDL
petType.resolveType = () => 'Pet';
expectError().toEqual(
'Interface type "Pet" is not a possible type for "Pet".',
);
});
});
125 changes: 74 additions & 51 deletions src/execution/execute.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import type {
} from '../type/definition';
import {
isAbstractType,
isInterfaceType,
isLeafType,
isListType,
isNonNullType,
Expand Down Expand Up @@ -790,106 +791,128 @@ function completeLeafValue(
*/
function completeAbstractValue(
exeContext: ExecutionContext,
returnType: GraphQLAbstractType,
abstractType: GraphQLAbstractType,
fieldNodes: ReadonlyArray<FieldNode>,
info: GraphQLResolveInfo,
path: Path,
result: unknown,
): PromiseOrValue<ObjMap<unknown>> {
const resolveTypeFn = returnType.resolveType ?? exeContext.typeResolver;
const resolveTypeFn = abstractType.resolveType ?? exeContext.typeResolver;
const contextValue = exeContext.contextValue;
const runtimeType = resolveTypeFn(result, contextValue, info, returnType);

if (isPromise(runtimeType)) {
return runtimeType.then((resolvedRuntimeType) =>
completeObjectValue(
exeContext,
ensureValidRuntimeType(
resolvedRuntimeType,
exeContext,
returnType,
fieldNodes,
info,
result,
),
fieldNodes,
info,
path,
result,
),
);
}
const possibleRuntimeTypeName = resolveTypeFn(
result,
contextValue,
info,
abstractType,
);

return completeObjectValue(
return completeAbstractValueImpl(
exeContext,
ensureValidRuntimeType(
runtimeType,
exeContext,
returnType,
fieldNodes,
info,
result,
),
abstractType,
possibleRuntimeTypeName,
fieldNodes,
info,
path,
result,
);
}

function ensureValidRuntimeType(
runtimeTypeName: unknown,
function completeAbstractValueImpl(
exeContext: ExecutionContext,
returnType: GraphQLAbstractType,
abstractType: GraphQLAbstractType,
possibleRuntimeTypeName: PromiseOrValue<string | undefined>,
fieldNodes: ReadonlyArray<FieldNode>,
info: GraphQLResolveInfo,
path: Path,
result: unknown,
): GraphQLObjectType {
if (runtimeTypeName == null) {
): PromiseOrValue<ObjMap<unknown>> {
if (isPromise(possibleRuntimeTypeName)) {
return possibleRuntimeTypeName.then((resolved) =>
completeAbstractValueImpl(
exeContext,
abstractType,
resolved,
fieldNodes,
info,
path,
result,
),
);
}

if (possibleRuntimeTypeName == null) {
throw new GraphQLError(
`Abstract type "${returnType.name}" must resolve to an Object type at runtime for field "${info.parentType.name}.${info.fieldName}". Either the "${returnType.name}" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.`,
`Abstract type "${abstractType.name}" must resolve to an Object type or an intermediate Interface type at runtime for field "${info.parentType.name}.${info.fieldName}". Either the "${abstractType.name}" type should provide a "resolveType" function or each possible type should provide an "isTypeOf" function.`,
Copy link
Contributor

@leebyron leebyron Jul 7, 2022

Choose a reason for hiding this comment

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

What do you think about only altering this error message in the case that an intermediate Interface type actually exists as a potential case?

Looking at the unit test impact, my sense is the primary downside of this change is the additional complication to this error message that makes it slightly harder to understand.

Ideally for the large majority common case where there is no intermediate interface, we don't burden the error message

Copy link
Contributor Author

@yaacovCR yaacovCR Jul 7, 2022

Choose a reason for hiding this comment

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

👍 Tailoring the error message to the interface at hand seems pretty reasonable.

{ nodes: fieldNodes },
);
}

// releases before 16.0.0 supported returning `GraphQLObjectType` from `resolveType`
// TODO: remove in 17.0.0 release
if (isObjectType(runtimeTypeName)) {
if (isObjectType(possibleRuntimeTypeName)) {
throw new GraphQLError(
'Support for returning GraphQLObjectType from resolveType was removed in graphql-js@16.0.0 please return type name instead.',
);
}

if (typeof runtimeTypeName !== 'string') {
if (typeof possibleRuntimeTypeName !== 'string') {
throw new GraphQLError(
`Abstract type "${returnType.name}" must resolve to an Object type at runtime for field "${info.parentType.name}.${info.fieldName}" with ` +
`value ${inspect(result)}, received "${inspect(runtimeTypeName)}".`,
`Abstract type "${abstractType.name}" must resolve to an Object type or an intermediate Interface type at runtime for field "${info.parentType.name}.${info.fieldName}" with ` +
`value ${inspect(result)}, received "${inspect(
possibleRuntimeTypeName,
)}".`,
);
}

const runtimeType = exeContext.schema.getType(runtimeTypeName);
if (runtimeType == null) {
const possibleRuntimeType = exeContext.schema.getType(
possibleRuntimeTypeName,
);
if (possibleRuntimeType == null) {
throw new GraphQLError(
`Abstract type "${returnType.name}" was resolved to a type "${runtimeTypeName}" that does not exist inside the schema.`,
`Abstract type "${abstractType.name}" was resolved to a type "${possibleRuntimeTypeName}" that does not exist inside the schema.`,
{ nodes: fieldNodes },
);
}

if (!isObjectType(runtimeType)) {
if (isInterfaceType(possibleRuntimeType)) {
Copy link
Contributor

@leebyron leebyron Jul 7, 2022

Choose a reason for hiding this comment

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

what about union types?

Copy link
Contributor

@leebyron leebyron Jul 7, 2022

Choose a reason for hiding this comment

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

Nevermind - based on feedback from WG discussion, a union type shouldn't be possible (though does this get potentially complicated by the proposal for union constraints via implementing interfaces?

Copy link
Contributor Author

@yaacovCR yaacovCR Jul 7, 2022

Choose a reason for hiding this comment

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

Yes, if unions ever implement interfaces, they could potentially be a link in the chain!

if (!exeContext.schema.isSubType(abstractType, possibleRuntimeType)) {
throw new GraphQLError(
`Interface type "${possibleRuntimeType.name}" is not a possible type for "${abstractType.name}".`,
{ nodes: fieldNodes },
);
}

return completeAbstractValue(
exeContext,
possibleRuntimeType,
fieldNodes,
info,
path,
result,
);
}

if (!isObjectType(possibleRuntimeType)) {
throw new GraphQLError(
`Abstract type "${returnType.name}" was resolved to a non-object type "${runtimeTypeName}".`,
`Abstract type "${abstractType.name}" was resolved to a non-object and non-interface type "${possibleRuntimeTypeName}".`,
{ nodes: fieldNodes },
);
}

if (!exeContext.schema.isSubType(returnType, runtimeType)) {
if (!exeContext.schema.isSubType(abstractType, possibleRuntimeType)) {
throw new GraphQLError(
`Runtime Object type "${runtimeType.name}" is not a possible type for "${returnType.name}".`,
`Runtime Object type "${possibleRuntimeType.name}" is not a possible type for "${abstractType.name}".`,
{ nodes: fieldNodes },
);
}

return runtimeType;
return completeObjectValue(
exeContext,
possibleRuntimeType,
fieldNodes,
info,
path,
result,
);
}

/**
Expand Down