Validate against @stream on different instances of the same field

See discussion graphql/defer-stream-wg#100
graphql · Feb 10, 2025 · c4532e9 · c4532e9
1 parent 1a21a18
commit c4532e9
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 27 deletions.
diff --git a/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts b/src/validation/__tests__/OverlappingFieldsCanBeMergedRule-test.ts
@@ -98,13 +98,22 @@ describe('Validate: Overlapping fields can be merged', () => {
     `);
   });
 
-  it('Same stream directives supported', () => {
-    expectValid(`
+  it('stream directive used on different instances of the 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 overlapping 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', () => {
@@ -116,7 +125,7 @@ describe('Validate: Overlapping fields can be merged', () => {
     `).toDeepEqual([
       {
         message:
-          'Fields "name" conflict because they have differing stream directives. Use different aliases on the fields to fetch both if this was intentional.',
+          'Fields "name" conflict because they have overlapping stream directives. Use different aliases on the fields to fetch both if this was intentional.',
         locations: [
           { line: 3, column: 9 },
           { line: 4, column: 9 },
@@ -134,7 +143,7 @@ describe('Validate: Overlapping fields can be merged', () => {
     `).toDeepEqual([
       {
         message:
-          'Fields "name" conflict because they have differing stream directives. Use different aliases on the fields to fetch both if this was intentional.',
+          'Fields "name" conflict because they have overlapping stream directives. Use different aliases on the fields to fetch both if this was intentional.',
         locations: [
           { line: 3, column: 9 },
           { line: 4, column: 9 },
@@ -152,7 +161,7 @@ describe('Validate: Overlapping fields can be merged', () => {
     `).toDeepEqual([
       {
         message:
-          'Fields "name" conflict because they have differing stream directives. Use different aliases on the fields to fetch both if this was intentional.',
+          'Fields "name" conflict because they have overlapping stream directives. Use different aliases on the fields to fetch both if this was intentional.',
         locations: [
           { line: 3, column: 9 },
           { line: 4, column: 9 },
@@ -170,7 +179,7 @@ describe('Validate: Overlapping fields can be merged', () => {
     `).toDeepEqual([
       {
         message:
-          'Fields "name" conflict because they have differing stream directives. Use different aliases on the fields to fetch both if this was intentional.',
+          'Fields "name" conflict because they have overlapping stream directives. Use different aliases on the fields to fetch both if this was intentional.',
         locations: [
           { line: 3, column: 9 },
           { line: 4, column: 9 },
@@ -188,7 +197,7 @@ describe('Validate: Overlapping fields can be merged', () => {
     `).toDeepEqual([
       {
         message:
-          'Fields "name" conflict because they have differing stream directives. Use different aliases on the fields to fetch both if this was intentional.',
+          'Fields "name" conflict because they have overlapping stream directives. Use different aliases on the fields to fetch both if this was intentional.',
         locations: [
           { line: 3, column: 9 },
           { line: 4, column: 9 },
@@ -206,7 +215,7 @@ describe('Validate: Overlapping fields can be merged', () => {
     `).toDeepEqual([
       {
         message:
-          'Fields "name" conflict because they have differing stream directives. Use different aliases on the fields to fetch both if this was intentional.',
+          'Fields "name" conflict because they have overlapping stream directives. Use different aliases on the fields to fetch both if this was intentional.',
         locations: [
           { line: 3, column: 9 },
           { line: 4, column: 9 },
@@ -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 overlapping 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', () => {

diff --git a/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts b/src/validation/rules/OverlappingFieldsCanBeMergedRule.ts
@@ -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 !== undefined) {
+    return [[responseName, overlappingStreamReason], [node1], [node2]];
   }
 
   // The return type for each field.
@@ -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 overlapping stream directives. See https://github.com/graphql/defer-stream-wg/discussions/100';
+    }
+    return 'they have overlapping stream directives';
   }
   // fields have a mix of stream and no stream
-  return false;
+  return 'they have overlapping stream directives';
 }
 
 // Two types conflict if both types could not apply to a value simultaneously.