Skip to content

Commit

Permalink
Improve error reporting for union member types
Browse files Browse the repository at this point in the history
First make sure that the included type is an object type,
because that would be the more severe problem. Only then we
can be sure that the type has a name and check for duplicates.
Also, create only one error per different non-object type.
  • Loading branch information
Cito authored and yaacovCR committed Sep 30, 2024
1 parent b09b295 commit d390b94
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 8 deletions.
43 changes: 42 additions & 1 deletion src/type/__tests__/validation-test.ts
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
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>();
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;
}
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

0 comments on commit d390b94

Please sign in to comment.