From a4e105e78ac7b5fd94e0ca35584d3dd76ff0fb3f Mon Sep 17 00:00:00 2001 From: Chris Lenfest Date: Fri, 28 Jun 2024 12:53:28 -0500 Subject: [PATCH] Don't put join__enumValue onto an EnumValue if all subgraphs who have the enum have the value (#3054) --- .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, 149 insertions(+), 8 deletions(-) create mode 100644 .changeset/breezy-rocks-crash.md diff --git a/.changeset/breezy-rocks-crash.md b/.changeset/breezy-rocks-crash.md new file mode 100644 index 000000000..7326528f4 --- /dev/null +++ b/.changeset/breezy-rocks-crash.md @@ -0,0 +1,5 @@ +--- +"@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 12ed3f760..609f64c0d 100644 --- a/composition-js/src/__tests__/compose.test.ts +++ b/composition-js/src/__tests__/compose.test.ts @@ -44,6 +44,11 @@ describe('composition', () => { } union U = S | T + + enum AnotherEnum { + E1 + E2 + } ` } @@ -61,6 +66,12 @@ describe('composition', () => { V1 V2 } + + enum AnotherEnum { + E1 + E2 + E3 + } ` } @@ -91,11 +102,20 @@ 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 @join__enumValue(graph: SUBGRAPH2) - V2 @join__enumValue(graph: SUBGRAPH2) + V1 + V2 } input join__ContextArgument { @@ -161,6 +181,12 @@ describe('composition', () => { const [_, api] = schemas(result); expect(printSchema(api)).toMatchString(` + enum AnotherEnum { + E1 + E2 + E3 + } + enum E { V1 V2 @@ -255,8 +281,8 @@ describe('composition', () => { enum E @join__type(graph: SUBGRAPH2) { - V1 @join__enumValue(graph: SUBGRAPH2) - V2 @join__enumValue(graph: SUBGRAPH2) + V1 + V2 } input join__ContextArgument { diff --git a/composition-js/src/merging/merge.ts b/composition-js/src/merging/merge.ts index 57e9b618a..f0196f417 100644 --- a/composition-js/src/merging/merge.ts +++ b/composition-js/src/merging/merge.ts @@ -2213,8 +2213,10 @@ class Merger { } } } + + const sourcesWithEnum = sources.filter(s => s !== undefined).length; for (const value of dest.values) { - this.mergeEnumValue(sources, dest, value, usage); + this.mergeEnumValue(sources, dest, value, usage, sourcesWithEnum); } // We could be left with an enum type with no values, and that's invalid in graphQL @@ -2231,6 +2233,7 @@ 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: @@ -2239,7 +2242,7 @@ class Merger { const valueSources = sources.map(s => s?.value(value.name)); this.mergeDescription(valueSources, value); this.recordAppliedDirectivesToMerge(valueSources, value); - this.addJoinEnumValue(valueSources, value); + this.addJoinEnumValue(valueSources, value, sourcesWithEnum); const inaccessibleInSupergraph = this.mergedFederationDirectiveInSupergraph.get(this.inaccessibleSpec.inaccessibleDirectiveSpec.name); const isInaccessible = inaccessibleInSupergraph && value.hasAppliedDirective(inaccessibleInSupergraph.definition); @@ -2290,7 +2293,11 @@ class Merger { } } - private addJoinEnumValue(sources: (EnumValue | undefined)[], dest: EnumValue) { + 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; + } 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 43b65e309..7d8b65918 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( - `"6dc1bde2b9818fabec62208c5d8825abaa1bae89635fa6f3a5ffea7b78fc6d82"`, + `"522c6ad4638c2b8d6f2139f176b9c82c3e9ccd82cc0bdcab4e811d0781eb4c16"`, ); // 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 6685a7974..b54a474c5 100644 --- a/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts +++ b/internals-js/src/__tests__/extractSubgraphsFromSupergraph.test.ts @@ -927,3 +927,106 @@ 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(); +});