INVALID_FIELD_SHARING: adjust for invalid override values (#26)

.changeset/big-apricots-change.md

@@ -0,0 +1,5 @@
+---
+'@theguild/federation-composition': patch
+---
+
+INVALID_FIELD_SHARING: adjust the check to detect valid override directive

__tests__/supergraph/errors/INVALID_FIELD_SHARING.spec.ts

@@ -118,5 +118,75 @@ testVersions((api, version) => {
         ]),
       }),
     );
+
+    expect(
+      api.composeServices([
+        {
+          name: 'users',
+          typeDefs: graphql`
+            extend schema
+              @link(
+                url: "https://specs.apollo.dev/federation/${version}"
+                import: ["@key", "@shareable"]
+              )
+
+              extend type Query {
+                foo: Foo
+              }
+
+              type Foo @shareable @key(fields: "id") {
+                id: ID!
+                name: String
+              }
+          `,
+        },
+        {
+          name: 'feed',
+          typeDefs: graphql`
+            extend schema
+              @link(
+                url: "https://specs.apollo.dev/federation/${version}"
+                import: ["@key", "@shareable", "@override"]
+              )
+
+              extend type Query {
+                foo: Foo @override(from: "noop")
+              }
+
+              type Foo @shareable @key(fields: "id") {
+                id: ID!
+                name: String
+              }
+          `,
+        },
+        {
+          name: 'noop',
+          typeDefs: graphql`
+            extend schema
+              @link(
+                url: "https://specs.apollo.dev/federation/${version}"
+                import: ["@key", "@shareable"]
+              )
+
+              type Query {
+                noop: String
+              }
+          `,
+        },
+      ]),
+    ).toEqual(
+      expect.objectContaining({
+        errors: expect.arrayContaining([
+          expect.objectContaining({
+            message: expect.stringContaining(
+              `Non-shareable field "Query.foo" is resolved from multiple subgraphs: it is resolved from subgraphs "feed" and "users" and defined as non-shareable in all of them`,
+            ),
+            extensions: expect.objectContaining({
+              code: 'INVALID_FIELD_SHARING',
+            }),
+          }),
+        ]),
+      }),
+    );
   });
 });

src/supergraph/validation/rules/invalid-field-sharing-rule.ts

@@ -26,10 +26,19 @@ export function InvalidFieldSharingRule(
         const fieldHasOverride = field.override;
         const fieldIsUsedAsKey = field.usedAsKey;
 
-        if (fieldIsExternal || fieldHasOverride) {
+        if (fieldIsExternal) {
           continue;
         }
 
+        if (fieldHasOverride) {
+          const overrideGraphId = context.graphNameToId(fieldHasOverride);
+          if (overrideGraphId && fieldState.byGraph.has(overrideGraphId)) {
+            // if a field tries to override some graph, check if it actually exists there.
+            // if it does, exclude it from invalid-field-sharing rule as override is effective.
+            continue;
+          }
+        }
+
         if (objectTypeIsShareable || fieldIsShareable || fieldIsUsedAsKey) {
           resolvableIn.push(graphId);
           continue;

src/supergraph/validation/rules/override-source-has-override.ts

@@ -19,15 +19,14 @@ export function OverrideSourceHasOverrideRule(
 
       for (let i = 0; i < graphsWithOverride.length; i++) {
         const [graph, fieldStateInGraph] = graphsWithOverride[i];
-        // TODO: instead of using toUpperCase here, we should be able to translate subgraph's name to ID
-        const overrideValue = fieldStateInGraph.override.toUpperCase();
-        const graphFromOverride = fieldState.byGraph.get(overrideValue);
+        const overrideValue = context.graphNameToId(fieldStateInGraph.override);
+        const graphFromOverride = overrideValue ? fieldState.byGraph.get(overrideValue) : null;
 
         // We want to first check if the override value points to a graph with an override directive at the same field
         // If not, we want to use the next graph in the list, or the first graph in the list if we're at the end
         const anotherGraphId =
           graphFromOverride && graphFromOverride.override !== null
-            ? overrideValue
+            ? overrideValue!
             : graphsWithOverride[i + 1]
             ? graphsWithOverride[i + 1][0]
             : graphsWithOverride[0][0];

src/supergraph/validation/validation-context.ts

@@ -6,6 +6,12 @@ export type SupergraphValidationContext = ReturnType<typeof createSupergraphVali
 export function createSupergraphValidationContext(subgraphStates: Map<string, SubgraphState>) {
   let reportedErrors: GraphQLError[] = [];
 
+  const subgraphNameToIdMap: Record<string, string | undefined> = {};
+
+  for (const [id, state] of subgraphStates) {
+    subgraphNameToIdMap[state.graph.name] = id;
+  }
+
   return {
     subgraphStates,
     graphIdToName(id: string) {
@@ -17,6 +23,11 @@ export function createSupergraphValidationContext(subgraphStates: Map<string, Su
 
       return found.graph.name;
     },
+    graphNameToId(name: string) {
+      const found = subgraphNameToIdMap[name];
+
+      return found ?? null;
+    },
     reportError(error: GraphQLError) {
       reportedErrors.push(error);
     },