From bc79013a4f85b14ccd74b18db888b6abf9c21112 Mon Sep 17 00:00:00 2001 From: "Sachin D. Shinde" Date: Mon, 8 Jul 2024 12:32:30 -0700 Subject: [PATCH] Revert #3054 (#3063) This PR reverts #3054, as this changes the output of composition for most users and should instead land in `next` instead of `main`. --- .changeset/breezy-rocks-crash.md | 5 - composition-js/src/__tests__/compose.test.ts | 34 +----- composition-js/src/merging/merge.ts | 13 +-- .../__tests__/gateway/lifecycle-hooks.test.ts | 2 +- .../extractSubgraphsFromSupergraph.test.ts | 103 ------------------ 5 files changed, 8 insertions(+), 149 deletions(-) delete mode 100644 .changeset/breezy-rocks-crash.md diff --git a/.changeset/breezy-rocks-crash.md b/.changeset/breezy-rocks-crash.md deleted file mode 100644 index 7326528f4..000000000 --- a/.changeset/breezy-rocks-crash.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@apollo/composition": patch ---- - -Don't put join\_\_enumValue onto an EnumValue if all subgraphs who have the enum have the value diff --git a/composition-js/src/__tests__/compose.test.ts b/composition-js/src/__tests__/compose.test.ts index 609f64c0d..12ed3f760 100644 --- a/composition-js/src/__tests__/compose.test.ts +++ b/composition-js/src/__tests__/compose.test.ts @@ -44,11 +44,6 @@ describe('composition', () => { } union U = S | T - - enum AnotherEnum { - E1 - E2 - } ` } @@ -66,12 +61,6 @@ describe('composition', () => { V1 V2 } - - enum AnotherEnum { - E1 - E2 - E3 - } ` } @@ -102,20 +91,11 @@ describe('composition', () => { directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA - enum AnotherEnum - @join__type(graph: SUBGRAPH1) - @join__type(graph: SUBGRAPH2) - { - E1 - E2 - E3 @join__enumValue(graph: SUBGRAPH2) - } - enum E @join__type(graph: SUBGRAPH2) { - V1 - V2 + V1 @join__enumValue(graph: SUBGRAPH2) + V2 @join__enumValue(graph: SUBGRAPH2) } input join__ContextArgument { @@ -181,12 +161,6 @@ describe('composition', () => { const [_, api] = schemas(result); expect(printSchema(api)).toMatchString(` - enum AnotherEnum { - E1 - E2 - E3 - } - enum E { V1 V2 @@ -281,8 +255,8 @@ describe('composition', () => { enum E @join__type(graph: SUBGRAPH2) { - V1 - V2 + V1 @join__enumValue(graph: SUBGRAPH2) + V2 @join__enumValue(graph: SUBGRAPH2) } input join__ContextArgument { diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index f0196f417..57e9b618a 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -2213,10 +2213,8 @@ class Merger { } } } - - const sourcesWithEnum = sources.filter(s => s !== undefined).length; for (const value of dest.values) { - this.mergeEnumValue(sources, dest, value, usage, sourcesWithEnum); + this.mergeEnumValue(sources, dest, value, usage); } // We could be left with an enum type with no values, and that's invalid in graphQL @@ -2233,7 +2231,6 @@ class Merger { dest: EnumType, value: EnumValue, { position, examples }: EnumTypeUsage, - sourcesWithEnum: number, // count of how many subgraphs have an enum. Allows us to skip join__EnumValue if all subgraphs who have the enum also have the value ) { // We merge directives (and description while at it) on the value even though we might remove it later in that function, // but we do so because: @@ -2242,7 +2239,7 @@ class Merger { const valueSources = sources.map(s => s?.value(value.name)); this.mergeDescription(valueSources, value); this.recordAppliedDirectivesToMerge(valueSources, value); - this.addJoinEnumValue(valueSources, value, sourcesWithEnum); + this.addJoinEnumValue(valueSources, value); const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name); const isInaccessible = inaccessibleInSupergraph && value.hasAppliedDirective(inaccessibleInSupergraph.definition); @@ -2293,11 +2290,7 @@ class Merger { } } - private addJoinEnumValue(sources: (EnumValue | undefined)[], dest: EnumValue, sourcesWithEnumValue: number) { - // if all subgraphs who have the enum also have the value, don't bother with @join__enumValue - if (sources.filter(s => s !== undefined).length === sourcesWithEnumValue) { - return; - } + private addJoinEnumValue(sources: (EnumValue | undefined)[], dest: EnumValue) { const joinEnumValueDirective = this.joinSpec.enumValueDirective(this.merged); // We should always be merging with the latest join spec, so this should exists, but well, in prior versions where // the directive didn't existed, we simply did had any replacement so ... diff --git a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts index 7d8b65918..43b65e309 100644 --- a/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts +++ b/gateway-js/src/__tests__/gateway/lifecycle-hooks.test.ts @@ -149,7 +149,7 @@ describe('lifecycle hooks', () => { // the supergraph (even just formatting differences), this ID will change // and this test will have to updated. expect(secondCall[0]!.compositionId).toMatchInlineSnapshot( - `"522c6ad4638c2b8d6f2139f176b9c82c3e9ccd82cc0bdcab4e811d0781eb4c16"`, + `"6dc1bde2b9818fabec62208c5d8825abaa1bae89635fa6f3a5ffea7b78fc6d82"`, ); // second call should have previous info in the second arg expect(secondCall[1]!.compositionId).toEqual(expectedFirstId); diff --git a/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts b/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts index b54a474c5..6685a7974 100644 --- a/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts +++ b/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts @@ -927,106 +927,3 @@ type U field(a: String @federation__fromContext(field: "$context { prop }")): Int! }`); }); - -test('no join__enumValue on an enum means it appears in all subgraphs', () => { - const supergraph = ` - schema - @link(url: "https://specs.apollo.dev/link/v1.0") - @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) - @link(url: "https://specs.apollo.dev/context/v0.1") -{ - query: Query -} - -directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA - -directive @join__graph(name: String!, url: String!) on ENUM_VALUE - -directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR - -directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION - -directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE - -directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION - -directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE - -directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION - -directive @context(name: String!) repeatable on INTERFACE | OBJECT - -directive @context__fromContext(field: String) on ARGUMENT_DEFINITION - -enum link__Purpose { - """ - \`SECURITY\` features provide metadata necessary to securely resolve fields. - """ - SECURITY - - """ - \`EXECUTION\` features provide metadata necessary for operation execution. - """ - EXECUTION -} - -scalar link__Import - -enum join__Graph { - A @join__graph(name: "A", url: "") - B @join__graph(name: "B", url: "") -} - -scalar join__FieldSet - -scalar join__DirectiveArguments - -scalar join__FieldValue - -input join__ContextArgument { - name: String! - type: String! - context: String! - selection: join__FieldValue! -} - -scalar context__context - -type Query - @join__type(graph: A) - @join__type(graph: B) -{ - a: anEnum! @join__field(graph: A) - b: anEnum! @join__field(graph: B) -} - - enum anEnum - @join__type(graph: A) - @join__type(graph: B) - { - ONE @join__enumValue(graph: A) @join__enumValue(graph: B) - TWO @join__enumValue(graph: A) - THREE - } - `; - - const subgraphs = Supergraph.build(supergraph).subgraphs(); - expect(subgraphs.size()).toBe(2); - const [a,b] = subgraphs.values().map((s) => s.schema); - const aEnum = a.type('anEnum') as any; - expect(aEnum).toBeDefined(); - expect(aEnum?.kind).toBe('EnumType'); - expect(aEnum.values.length).toBe(3); - expect(aEnum.values[0].name).toBe('ONE'); - expect(aEnum.values[1].name).toBe('TWO'); - expect(aEnum.values[2].name).toBe('THREE'); - expect(b.type('anEnum')).toBeDefined(); - - const bEnum = b.type('anEnum') as any; - expect(bEnum).toBeDefined(); - expect(bEnum?.kind).toBe('EnumType'); - expect(bEnum.values.length).toBe(2); - expect(bEnum.values[0].name).toBe('ONE'); - expect(bEnum.values[1].name).toBe('THREE'); - expect(b.type('anEnum')).toBeDefined(); -});