Various fixes around @interfaceObject (#2303)

There were a few things that didn't worked as expected or weren't validated correctly in the handling of `@interfaceObject` and this commit fixes those issues. The issues concretely fixed are:
- Proper validation of shareability for fields provided by @interfaceObject, including cases where multiple @interfaceObject "abstract" the same field.
- If `@interfaceObject` is used for some interface `I`, reject if the subgraph also defines some of the implementations of `I` (this may be something that we lift later, but since the `@interfaceObject` abstract the type in question, we should take some time to think about the consequence if we do lift that limitation).
- Rejects `@override` on both `@interfaceObject` fields and object type fields which are otherwise provided by an @interfaceObject in another subgraph. Here again, we can probably lift such limitation later, but we need to think that through.
- Fix handling of external fields on implementation type when those field are provided by an `@interfaceObject`. This was preventing using such fields in `@requires` for instance.
- Rejects `@override` when used on interface fields. This is actually not strictly `@interfaceObject` related, but simply noticed that we were not properly rejecting it (much like we didn't reject `@shareable` on interface fields until a recent fix). The code definitively does not handle `@override` on an interface field (and like for `@shareable`, it's a tad unclear what it would mean really), and accepting it while ignoring it is just confusing.

Author: Sylvain Lebresne

composition-js/CHANGELOG.md

@@ -7,6 +7,10 @@ This CHANGELOG pertains only to Apollo Federation packages in the 2.x range. The
 ## 2.3.0
 
 - Preserves source of union members and enum values in supergraph [PR #2288](https://github.com/apollographql/federation/pull/2288).
+- __BREAKING__: composition now rejects `@override` on interface fields. The `@override` directive was not
+  meant to be supported on interfaces and was not having any impact whatsoever. If an existing subgraph does have a
+  `@override` on an interface field, this will now be rejected, but the `@override` can simply and safely be removed
+  since it previously was ignored.
 
 ## 2.2.0
 

composition-js/src/__tests__/compose.test.ts

@@ -2113,6 +2113,160 @@ describe('composition', () => {
       // (definition and extension) but also that it's properly taking into account.
       assertCompositionSuccess(result);
     });
+
+    describe('@interfaceObject', () => {
+      // An @interfaceObject type provides fields for all the implementation it abstracts, which should impact the shareability
+      // for those concrete impelmentations. That is, if a field is provided by both an @interfaceObject and also by one concrete
+      // implementation in another subgraph, then it needs to be marked @shareable. Those test check this as well as some
+      // variants.
+
+      it.each([
+        {
+          shareableOnConcreteType: false,
+          shareableOnInterfaceObject: false,
+          nonShareableErrorDetail: 'all of them',
+        },
+        {
+          shareableOnConcreteType: true,
+          shareableOnInterfaceObject: false,
+          nonShareableErrorDetail: 'subgraph "subgraphA" (through @interfaceObject field "I.x")',
+        },
+        {
+          shareableOnConcreteType: false,
+          shareableOnInterfaceObject: true,
+          nonShareableErrorDetail: 'subgraph "subgraphB"',
+        },
+        {
+          shareableOnConcreteType: true,
+          shareableOnInterfaceObject: true,
+        },
+      ])(
+        'enforces shareable constraints for field "abstracted" by @interfaceObject and shared (shareable on concrete type: $shareableOnConcreteType, shareable on @interfaceObject: $shareableOnInterfaceObject)',
+        ({ shareableOnConcreteType, shareableOnInterfaceObject, nonShareableErrorDetail}) => {
+          const subgraphA = {
+            typeDefs: gql`
+              type Query {
+                iFromA: I
+              }
+
+              type I @interfaceObject @key(fields: "id") {
+                id: ID!
+                x: Int${shareableOnInterfaceObject ? ' @shareable' : ''}
+              }
+            `,
+            name: 'subgraphA',
+          };
+
+          const subgraphB = {
+            typeDefs: gql`
+              type Query {
+                iFromB: I
+              }
+
+              interface I @key(fields: "id") {
+                id: ID!
+                x: Int
+              }
+
+              type A implements I @key(fields: "id") {
+                id: ID!
+                x: Int${shareableOnConcreteType ? ' @shareable' : ''}
+              }
+            `,
+            name: 'subgraphB',
+          };
+
+          const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
+          if (nonShareableErrorDetail) {
+            expect(result.errors).toBeDefined();
+            expect(errors(result)).toStrictEqual([[
+              'INVALID_FIELD_SHARING',
+              `Non-shareable field "A.x" is resolved from multiple subgraphs: it is resolved from subgraphs "subgraphA" (through @interfaceObject field "I.x") and "subgraphB" and defined as non-shareable in ${nonShareableErrorDetail}`
+            ]]);
+          } else {
+            expect(result.errors).toBeUndefined();
+          }
+        }
+      );
+
+      it.each([
+        {
+          shareableOnI1: false,
+          shareableOnI2: false,
+          nonShareableErrorDetail: 'all of them',
+        },
+        {
+          shareableOnI1: true,
+          shareableOnI2: false,
+          nonShareableErrorDetail: 'subgraph "subgraphA" (through @interfaceObject field "I2.x")',
+        },
+        {
+          shareableOnI1: false,
+          shareableOnI2: true,
+          nonShareableErrorDetail: 'subgraph "subgraphA" (through @interfaceObject field "I1.x")',
+        },
+        {
+          shareableOnI1: true,
+          shareableOnI2: true,
+        },
+      ])(
+        'enforces shareability in a single subgraph with 2 intersecting @interfaceObject (shareable on first @interfaceObject: $shareableOnI1, shareable on second @interfaceObject: $shareableOnI2)',
+        ({ shareableOnI1, shareableOnI2, nonShareableErrorDetail}) => {
+          const subgraphA = {
+            typeDefs: gql`
+              type Query {
+                i1FromA: I1
+                i2FromA: I2
+              }
+
+              type I1 @interfaceObject @key(fields: "id") {
+                id: ID!
+                x: Int${shareableOnI1 ? ' @shareable' : ''}
+              }
+
+              type I2 @interfaceObject @key(fields: "id") {
+                id: ID!
+                x: Int${shareableOnI2 ? ' @shareable' : ''}
+              }
+            `,
+            name: 'subgraphA',
+          };
+
+          const subgraphB = {
+            typeDefs: gql`
+              type Query {
+                i1FromB: I1
+                i2FromB: I2
+              }
+
+              interface I1 @key(fields: "id") {
+                id: ID!
+              }
+
+              interface I2 @key(fields: "id") {
+                id: ID!
+              }
+
+              type A implements I1 & I2 @key(fields: "id") {
+                id: ID!
+              }
+            `,
+            name: 'subgraphB',
+          };
+
+          const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
+          if (nonShareableErrorDetail) {
+            expect(result.errors).toBeDefined();
+            expect(errors(result)).toStrictEqual([[
+              'INVALID_FIELD_SHARING',
+              `Non-shareable field "A.x" is resolved from multiple subgraphs: it is resolved from subgraphs "subgraphA" (through @interfaceObject field "I1.x") and "subgraphA" (through @interfaceObject field "I2.x") and defined as non-shareable in ${nonShareableErrorDetail}`
+            ]]);
+          } else {
+            expect(result.errors).toBeUndefined();
+          }
+        }
+      );
+    });
   });
 
   it('handles renamed federation directives', () => {
@@ -3544,5 +3698,59 @@ describe('composition', () => {
         '[subgraphA] Interface type "I" has a resolvable key (@key(fields: "id")) in subgraph "subgraphA" but that subgraph is missing some of the supergraph implementation types of "I". Subgraph "subgraphA" should define type "C" (and have it implement "I").',
       ]]);
     });
+
+    it('errors if a subgraph defines both an @interfaceObject and some implemenations', () => {
+      const subgraphA = {
+        typeDefs: gql`
+          type Query {
+            iFromA: I
+          }
+
+          interface I @key(fields: "id") {
+            id: ID!
+            x: Int
+          }
+
+          type A implements I @key(fields: "id") {
+            id: ID!
+            x: Int
+            w: Int
+          }
+
+          type B implements I @key(fields: "id") {
+            id: ID!
+            x: Int
+            z: Int
+          }
+        `,
+        name: 'subgraphA',
+      };
+
+      const subgraphB = {
+        typeDefs: gql`
+          type Query {
+            iFromB: I
+          }
+
+          type I @interfaceObject @key(fields: "id") {
+            id: ID!
+            y: Int
+          }
+
+          type A @key(fields: "id") {
+            id: ID!
+            y: Int
+          }
+        `,
+        name: 'subgraphB',
+      };
+
+      const result = composeAsFed2Subgraphs([subgraphA, subgraphB]);
+      expect(result.errors).toBeDefined();
+      expect(errors(result)).toStrictEqual([[
+        'INTERFACE_OBJECT_USAGE_ERROR',
+        '[subgraphB] Interface type "I" is defined as an @interfaceObject in subgraph "subgraphB" so that subgraph should not define any of the implementation types of "I", but it defines type "A"',
+      ]]);
+    });
   });
 });

composition-js/src/__tests__/override.compose.test.ts

@@ -757,4 +757,112 @@ describe("composition involving @override directive", () => {
       `@override cannot be used on field "T.k" on subgraph "Subgraph1" since "T.k" on "Subgraph1" is marked with directive "@external"`,
     ]);
   });
+
+  // At the moment, we've punted on @override support when interacting with @interfaceObject, so the
+  // following tests mainly cover the various possible use and show that it currently correcly raise
+  // some validation errors. We may lift some of those limitation in the future.
+  describe('@interfaceObject', () => {
+    it("does not allow @override on @interfaceObject fields", () => {
+      // We currently rejects @override on fields of an @interfaceObject type. We could lift
+      // that limitation in the future, and that would mean such override overrides the field
+      // in _all_ the implementations of the target subtype, but that would imply generalizing
+      // the handling overriden fields and the override error messages, so we keep that for
+      // later.
+      // Note that it would be a tad simpler to support @override on an @interfaceObject if
+      // the `from` subgraph is also an @interfaceObject, as we can essentially ignore that
+      // we have @interfaceObject in such case, but it's a corner case and it's clearer for
+      // not to just always reject @override on @interfaceObject.
+      const subgraph1 = {
+        name: "Subgraph1",
+        url: "https://Subgraph1",
+        typeDefs: gql`
+          type Query {
+            i1: I
+          }
+
+          type I @interfaceObject @key(fields: "k") {
+            k: ID
+            a: Int @override(from: "Subgraph2")
+          }
+        `,
+      };
+
+      const subgraph2 = {
+        name: "Subgraph2",
+        url: "https://Subgraph2",
+        typeDefs: gql`
+          type Query {
+            i2: I
+          }
+
+          interface I @key(fields: "k") {
+            k: ID
+            a: Int
+          }
+
+          type A implements I @key(fields: "k") {
+            k: ID
+            a: Int
+          }
+        `,
+      };
+
+      const result = composeAsFed2Subgraphs([subgraph1, subgraph2]);
+      expect(result.errors).toBeDefined();
+      expect(errors(result)).toContainEqual([
+        "OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE",
+        '@override is not yet supported on fields of @interfaceObject types: cannot be used on field "I.a" on subgraph "Subgraph1".',
+      ]);
+    });
+
+    it("does not allow @override when overriden field is an @interfaceObject field", () => {
+      // We don't allow @override on a concrete type field when the `from` subgraph has
+      // an @interfaceObject field "covering" that field. In theory, this could have some
+      // use if one wanted to move a field from an @interfaceObject into all its implementations
+      // (in another subgraph) but it's also a bit hard to validate/use because we would have
+      // to check that all the implementations have an @override for it to be correct and
+      // it's unclear how useful that gets.
+      const subgraph1 = {
+        name: "Subgraph1",
+        url: "https://Subgraph1",
+        typeDefs: gql`
+          type Query {
+            i1: I
+          }
+
+          type I @interfaceObject @key(fields: "k") {
+            k: ID
+            a: Int
+          }
+        `,
+      };
+
+      const subgraph2 = {
+        name: "Subgraph2",
+        url: "https://Subgraph2",
+        typeDefs: gql`
+          type Query {
+            i2: I
+          }
+
+          interface I @key(fields: "k") {
+            k: ID
+            a: Int
+          }
+
+          type A implements I @key(fields: "k") {
+            k: ID
+            a: Int @override(from: "Subgraph1")
+          }
+        `,
+      };
+
+      const result = composeAsFed2Subgraphs([subgraph1, subgraph2]);
+      expect(result.errors).toBeDefined();
+      expect(errors(result)).toContainEqual([
+        "OVERRIDE_COLLISION_WITH_ANOTHER_DIRECTIVE",
+        'Invalid @override on field "A.a" of subgraph "Subgraph2": source subgraph "Subgraph1" does not have field "A.a" but abstract it in type "I" and overriding abstracted fields is not supported.',
+      ]);
+    });
+  });
 });

composition-js/src/__tests__/supergraph_reversibility.test.ts

@@ -90,3 +90,43 @@ it('preserves the source of enum values', () => {
 
   composeAndTestReversibility([s1, s2]);
 });
+
+describe('@interfaceObject', () => {
+  it('correctly extract external fields of concrete type only provided by an @interfaceObject', () => {
+    const s1 = {
+      typeDefs: gql`
+        type Query {
+          iFromS1: I
+        }
+
+        interface I @key(fields: "id") {
+          id: ID!
+          x: Int
+        }
+
+        type T implements I @key(fields: "id") {
+          id: ID!
+          x: Int @external
+          y: Int @requires(fields: "x")
+        }
+      `,
+      name: 'S1',
+    };
+
+    const s2 = {
+      typeDefs: gql`
+        type Query {
+          iFromS2: I
+        }
+
+        type I @interfaceObject @key(fields: "id") {
+          id: ID!
+          x: Int
+        }
+      `,
+      name: 'S2',
+    };
+
+    composeAndTestReversibility([s1, s2]);
+  });
+});