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.

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

Improve error reporting for union member types #2472

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Improve error reporting for union member types #2472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 42 additions & 1 deletion src/type/__tests__/validation-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ describe('Type System: Union types must be valid', () => {
]);
});

it('rejects a Union type with non-Object members types', () => {
it('rejects a Union type with non-Object member types', () => {
let schema = buildSchema(`
type Query {
test: BadUnion
Expand Down Expand Up @@ -807,6 +807,47 @@ describe('Type System: Union types must be valid', () => {
]);
}
});

it('rejects a Union type with duplicate non-Object member types', () => {
let schema = buildSchema(`
type Query {
test: BadUnion
}

type TypeA {
field: String
}

union BadUnion =
| Int
| String
| TypeA
| Int
| String
`);

schema = extendSchema(schema, parse('extend union BadUnion = Int'));

expectJSON(validateSchema(schema)).toDeepEqual([
{
message:
'Union type BadUnion can only include Object types, it cannot include Int.',
locations: [
{ line: 11, column: 11 },
{ line: 14, column: 11 },
{ line: 1, column: 25 },
],
},
{
message:
'Union type BadUnion can only include Object types, it cannot include String.',
locations: [
{ line: 12, column: 11 },
{ line: 15, column: 11 },
],
},
]);
});
});

describe('Type System: Input Objects must have fields', () => {
Expand Down
20 changes: 13 additions & 7 deletions src/type/validate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,20 @@ function validateUnionMembers(
}

const includedTypeNames = new Set<string>();
const includedInvalidTypes = new Set<string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just changed this to a single Set, visitedTypeNames?

for (const memberType of memberTypes) {
if (!isObjectType(memberType)) {
const typeString = inspect(memberType);
if (!includedInvalidTypes.has(typeString)) {
context.reportError(
`Union type ${union.name} can only include Object types, ` +
`it cannot include ${inspect(memberType)}.`,
getUnionMemberTypeNodes(union, String(memberType)),
);
includedInvalidTypes.add(typeString);
}
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

this reminds me of #4181 => what do you think about changing the error message string to something that references both errors, at least if there are repetitions?

'Something like Union type can only include unique Object types, ....'

What do you think?

}
if (includedTypeNames.has(memberType.name)) {
context.reportError(
`Union type ${union.name} can only include type ${memberType} once.`,
Expand All @@ -489,13 +502,6 @@ function validateUnionMembers(
continue;
}
includedTypeNames.add(memberType.name);
if (!isObjectType(memberType)) {
context.reportError(
`Union type ${union.name} can only include Object types, ` +
`it cannot include ${inspect(memberType)}.`,
getUnionMemberTypeNodes(union, String(memberType)),
);
}
}
}

Expand Down
Loading