Skip to content

Commit

Permalink
Validate against @stream on different instances of the same field
Browse files Browse the repository at this point in the history
  • Loading branch information
robrichard committed Feb 7, 2025
1 parent 1a21a18 commit 603eb2b
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 21 deletions.
28 changes: 23 additions & 5 deletions src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,22 @@ describe('Validate: Overlapping fields can be merged', () => {
`);
});

it('Same stream directives supported', () => {
expectValid(`
it('stream directive used on same field', () => {
expectErrors(`
fragment differentDirectivesWithDifferentAliases on Dog {
name @stream(label: "streamLabel", initialCount: 1)
name @stream(label: "streamLabel", initialCount: 1)
}
`);
`).toDeepEqual([
{
message:
'Fields "name" conflict because they have differing stream directives. See https://github.com/graphql/defer-stream-wg/discussions/100. Use different aliases on the fields to fetch both if this was intentional.',
locations: [
{ line: 3, column: 9 },
{ line: 4, column: 9 },
],
},
]);
});

it('different stream directive label', () => {
Expand Down Expand Up @@ -216,12 +225,21 @@ describe('Validate: Overlapping fields can be merged', () => {
});

it('different stream directive both missing args', () => {
expectValid(`
expectErrors(`
fragment conflictingArgs on Dog {
name @stream
name @stream
}
`);
`).toDeepEqual([
{
message:
'Fields "name" conflict because they have differing stream directives. See https://github.com/graphql/defer-stream-wg/discussions/100. Use different aliases on the fields to fetch both if this was intentional.',
locations: [
{ line: 3, column: 9 },
{ line: 4, column: 9 },
],
},
]);
});

it('Same aliases with different field targets', () => {
Expand Down
33 changes: 17 additions & 16 deletions src/validation/rules/OverlappingFieldsCanBeMergedRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -707,12 +707,14 @@ function findConflict(

const directives1 = node1.directives ?? [];
const directives2 = node2.directives ?? [];
if (!sameStreams(directives1, varMap1, directives2, varMap2)) {
return [
[responseName, 'they have differing stream directives'],
[node1],
[node2],
];
const overlappingStreamReason = hasNoOverlappingStreams(
directives1,
varMap1,
directives2,
varMap2,
);
if (overlappingStreamReason) {

Check failure on line 716 in src/validation/rules/OverlappingFieldsCanBeMergedRule.ts

View workflow job for this annotation

GitHub Actions / ci / Lint source files

Unexpected nullable string value in conditional. Please handle the nullish/empty cases explicitly
return [[responseName, overlappingStreamReason], [node1], [node2]];
}

// The return type for each field.
Expand Down Expand Up @@ -830,28 +832,27 @@ function getStreamDirective(
return directives.find((directive) => directive.name.value === 'stream');
}

function sameStreams(
function hasNoOverlappingStreams(
directives1: ReadonlyArray<DirectiveNode>,
varMap1: Map<string, ValueNode> | undefined,
directives2: ReadonlyArray<DirectiveNode>,
varMap2: Map<string, ValueNode> | undefined,
): boolean {
): string | undefined {
const stream1 = getStreamDirective(directives1);
const stream2 = getStreamDirective(directives2);
if (!stream1 && !stream2) {
// both fields do not have streams
return true;
return;
} else if (stream1 && stream2) {
// check if both fields have equivalent streams
return sameArguments(
stream1.arguments,
varMap1,
stream2.arguments,
varMap2,
);
if (sameArguments(stream1.arguments, varMap1, stream2.arguments, varMap2)) {
// This was allowed in previous alpha versions of `graphql-js`.
return 'they have differing stream directives. See https://github.com/graphql/defer-stream-wg/discussions/100';
}
return 'they have differing stream directives';
}
// fields have a mix of stream and no stream
return false;
return 'they have differing stream directives';
}

// Two types conflict if both types could not apply to a value simultaneously.
Expand Down

0 comments on commit 603eb2b

Please sign in to comment.