Update federation audit suite and fix some cases (#1588)

Author: enisdenjo

.changeset/wet-bugs-double.md

@@ -0,0 +1,6 @@
+---
+'@graphql-tools/federation': patch
+'@graphql-tools/delegate': patch
+---
+
+Correctly resolve circular @requires in different subgraphs

packages/delegate/src/defaultMergedResolver.ts

@@ -94,13 +94,14 @@ export function defaultMergedResolver(
       if (
         fieldNodesByType?.every((fieldNode) => {
           const responseKey = fieldNode.alias?.value ?? fieldNode.name.value;
-          if (Object.prototype.hasOwnProperty.call(parent, responseKey)) {
-            return true;
-          }
-          return false;
+          return Object.prototype.hasOwnProperty.call(parent, responseKey);
         })
       ) {
         handleResult(parent, responseKey, context, info);
+      } else {
+        // not all fields are present, we need to resolve more leftovers
+        // this can occur when there are circular @requires, see requires-circular audit test
+        handleLeftOver(parent, context, info, leftOver);
       }
       return deferred.promise;
     }
@@ -304,13 +305,18 @@ function handleFlattenedParent<TContext extends Record<string, any>>(
                           nestedTypeName
                         ]?.resolvers.get(subschema);
                       if (resolver) {
+                        const subschemaTypes =
+                          stitchingInfo.mergedTypes[
+                            nestedTypeName
+                          ]!.typeMaps.get(subschema)!;
+                        const returnType = subschemaTypes[nestedTypeName];
                         const res = await resolver(
                           nestedParentItem,
                           context,
                           info,
                           subschema,
                           selectionSet,
-                          info.parentType,
+                          returnType, // returnType
                           info.parentType,
                         );
                         if (res) {
@@ -383,15 +389,48 @@ function handleDeferredResolverResult<TContext extends Record<string, any>>(
   const deferredFields =
     leftOver.missingFieldsParentDeferredMap.get(leftOverParent);
   if (deferredFields) {
+    const stitchingInfo = info.schema.extensions?.[
+      'stitchingInfo'
+    ] as StitchingInfo;
+    const parentTypeName = leftOverParent?.__typename || info.parentType.name;
+    const resolvedKeys = new Set<string>();
     for (const [responseKey, deferred] of deferredFields) {
-      // If the deferred field is resolved, resolve the deferred field
-      if (Object.prototype.hasOwnProperty.call(resolverResult, responseKey)) {
+      // after handleResolverResult, check if the field is now in the parent
+      if (Object.prototype.hasOwnProperty.call(leftOverParent, responseKey)) {
+        // field was added to parent by handleResolverResult
         deferred.resolve(
           handleResult(leftOverParent, responseKey, context, info),
         );
+        resolvedKeys.add(responseKey);
+      } else {
+        // check if the required fields for this deferred field are now satisfied
+        const fieldNodesByType =
+          stitchingInfo?.fieldNodesByField?.[parentTypeName]?.[responseKey];
+        if (fieldNodesByType) {
+          if (
+            fieldNodesByType.every((fieldNode) => {
+              const requiredKey =
+                fieldNode.alias?.value ?? fieldNode.name.value;
+              return Object.prototype.hasOwnProperty.call(
+                leftOverParent,
+                requiredKey,
+              );
+            })
+          ) {
+            // requirements are satisfied, trigger handleLeftOver to fetch this field
+            handleLeftOver(leftOverParent, context, info, leftOver);
+          }
+        }
       }
     }
-    leftOver.missingFieldsParentDeferredMap.delete(leftOverParent);
+    // delete resolved deferred fields
+    for (const key of resolvedKeys) {
+      deferredFields.delete(key);
+    }
+    // and delete map if empty
+    if (deferredFields.size === 0) {
+      leftOver.missingFieldsParentDeferredMap.delete(leftOverParent);
+    }
   }
 }
 

packages/federation/package.json

@@ -60,7 +60,7 @@
     "@apollo/subgraph": "^2.11.3",
     "@types/lodash": "4.17.20",
     "graphql": "^16.9.0",
-    "graphql-federation-gateway-audit": "the-guild-org/graphql-federation-gateway-audit#main",
+    "graphql-federation-gateway-audit": "graphql-hive/federation-gateway-audit#cc224d26da54ad1c2393dfef3346893c315f351d",
     "pkgroll": "2.20.1"
   },
   "sideEffects": false

packages/federation/src/supergraph.ts

@@ -806,10 +806,21 @@ export function getStitchingOptionsFromSupergraphSdl(
                   selection.kind === Kind.FIELD &&
                   selection.arguments?.length
                 ) {
+                  const argsHash = selection.arguments
+                    .map(
+                      (arg) =>
+                        arg.name.value +
+                        // TODO: slow? faster hash?
+                        memoizedASTPrint(arg.value).replace(
+                          /[^a-zA-Z0-9]/g,
+                          '',
+                        ),
+                    )
+                    .join('');
                   // @ts-expect-error it's ok we're mutating consciously
                   selection.alias = {
                     kind: Kind.NAME,
-                    value: '_' + selection.name.value,
+                    value: '_' + selection.name.value + '_' + argsHash,
                   };
                 }
                 if ('selectionSet' in selection && selection.selectionSet) {

packages/federation/tests/federation-compatibility.test.ts

@@ -5,24 +5,20 @@ import {
   GatewayRuntime,
   useCustomFetch,
 } from '@graphql-hive/gateway-runtime';
-import { normalizedExecutor } from '@graphql-tools/executor';
 import {
   ExecutionResult,
   filterSchema,
   getDirective,
   MapperKind,
   mapSchema,
 } from '@graphql-tools/utils';
-import { assertSingleExecutionValue } from '@internal/testing';
 import {
   buildSchema,
   getNamedType,
   GraphQLSchema,
   isEnumType,
   lexicographicSortSchema,
-  parse,
   printSchema,
-  validate,
 } from 'graphql';
 import { createRouter } from 'graphql-federation-gateway-audit';
 import { beforeAll, describe, expect, it } from 'vitest';
@@ -154,47 +150,9 @@ describe('Federation Compatibility', () => {
         );
       });
       tests.forEach((_, i) => {
-        describe(`test-query-${i}`, () => {
-          it('gives the correct result w/ core', async () => {
-            const test = tests[i];
-            if (!test) {
-              throw new Error(`Test ${i} not found`);
-            }
-            const document = parse(test.query, { noLocation: true });
-            const validationErrors = validate(stitchedSchema, document);
-            let result: ExecutionResult;
-            if (validationErrors.length > 0) {
-              result = {
-                errors: validationErrors,
-              };
-            } else {
-              const execRes = await normalizedExecutor({
-                schema: stitchedSchema,
-                document,
-              });
-              assertSingleExecutionValue(execRes);
-              result = execRes;
-            }
-            const received = {
-              data: result.data ?? null,
-              errors: !!result.errors?.length,
-            };
-
-            const expected = {
-              data: test.expected.data ?? null,
-              errors: test.expected.errors ?? false,
-            };
-
-            try {
-              expect(received).toEqual(expected);
-            } catch (e) {
-              result.errors?.forEach((err) => {
-                console.error(err);
-              });
-              throw e;
-            }
-          });
-          it('gives the correct result w/ gateway', async () => {
+        (supergraphName === 'requires-with-argument-conflict' ? it.todo : it)(
+          `test-query-${i}`,
+          async () => {
             const test = tests[i];
             if (!test) {
               throw new Error(`Test ${i} not found`);
@@ -230,8 +188,8 @@ describe('Federation Compatibility', () => {
               });
               throw e;
             }
-          });
-        });
+          },
+        );
       });
     });
   }