From 22cd8c3d686c6cba1679205ad67452adf00e31b1 Mon Sep 17 00:00:00 2001 From: trevor-anderson <43518091+trevor-anderson@users.noreply.github.com> Date: Fri, 7 Feb 2025 11:42:55 -0500 Subject: [PATCH 1/3] fix: update visitor logic in `topologicalSortAST` to fix Issue #528 Currently, there is a problem described in Issue #528 whereby "const" output can have some variables used before they're defined. There were two related issues in the `visit` invocation in `topologicalSortAST` that caused the problem. These changes resolve "use before defined" errors in generated output and enhance compatibility with custom scalars and circular type definitions. - Recursively unwrap GraphQL types (List/NonNull) via `getNamedType` to correctly register dependencies. - Add `INTERFACE_TYPE_DEFINITION` to the list of target kinds for proper dependency handling. Additionally, this commit updates the `topsort` algo to iterate over all nodes (`g.nodes().forEach(visit)`) instead of just sinks, ensuring self-referential and cyclic dependencies (e.g. self-circular inputs) are processed. Since these changes affect output, impacted tests have had their expect-values updated as well (just re-ordering of certain type declarations). --- src/graphql.ts | 103 +++++++++++++++++++++++++----------------- tests/graphql.spec.ts | 8 ++-- tests/myzod.spec.ts | 12 ++--- tests/yup.spec.ts | 13 +++--- tests/zod.spec.ts | 13 +++--- 5 files changed, 85 insertions(+), 64 deletions(-) diff --git a/src/graphql.ts b/src/graphql.ts index d5f6eeca..aff04b3b 100644 --- a/src/graphql.ts +++ b/src/graphql.ts @@ -14,12 +14,23 @@ import type { import { Graph } from 'graphlib'; import { isSpecifiedScalarType, + Kind, visit, } from 'graphql'; -export const isListType = (typ?: TypeNode): typ is ListTypeNode => typ?.kind === 'ListType'; -export const isNonNullType = (typ?: TypeNode): typ is NonNullTypeNode => typ?.kind === 'NonNullType'; -export const isNamedType = (typ?: TypeNode): typ is NamedTypeNode => typ?.kind === 'NamedType'; +/** + * Recursively unwraps a GraphQL type until it reaches the NamedType. + * + * Since a GraphQL type is defined as either a NamedTypeNode, ListTypeNode, or NonNullTypeNode, + * this implementation safely recurses until the underlying NamedTypeNode is reached. + */ +function getNamedType(typeNode: TypeNode): NamedTypeNode { + return typeNode.kind === Kind.NAMED_TYPE ? typeNode : getNamedType(typeNode.type); +} + +export const isListType = (typ?: TypeNode): typ is ListTypeNode => typ?.kind === Kind.LIST_TYPE; +export const isNonNullType = (typ?: TypeNode): typ is NonNullTypeNode => typ?.kind === Kind.NON_NULL_TYPE; +export const isNamedType = (typ?: TypeNode): typ is NamedTypeNode => typ?.kind === Kind.NAMED_TYPE; export const isInput = (kind: string) => kind.includes('Input'); @@ -48,44 +59,50 @@ export function InterfaceTypeDefinitionBuilder(useInterfaceTypes: boolean | unde export function topologicalSortAST(schema: GraphQLSchema, ast: DocumentNode): DocumentNode { const dependencyGraph = new Graph(); const targetKinds = [ - 'ObjectTypeDefinition', - 'InputObjectTypeDefinition', - 'EnumTypeDefinition', - 'UnionTypeDefinition', - 'ScalarTypeDefinition', + Kind.OBJECT_TYPE_DEFINITION, + Kind.INPUT_OBJECT_TYPE_DEFINITION, + Kind.INTERFACE_TYPE_DEFINITION, + Kind.SCALAR_TYPE_DEFINITION, + Kind.ENUM_TYPE_DEFINITION, + Kind.UNION_TYPE_DEFINITION, ]; visit(ast, { enter: (node) => { switch (node.kind) { - case 'ObjectTypeDefinition': - case 'InputObjectTypeDefinition': { + case Kind.OBJECT_TYPE_DEFINITION: + case Kind.INPUT_OBJECT_TYPE_DEFINITION: + case Kind.INTERFACE_TYPE_DEFINITION: { const typeName = node.name.value; dependencyGraph.setNode(typeName); if (node.fields) { node.fields.forEach((field) => { - if (field.type.kind === 'NamedType') { - const dependency = field.type.name.value; - const typ = schema.getType(dependency); - if (typ?.astNode?.kind === undefined || !targetKinds.includes(typ.astNode.kind)) - return; - - if (!dependencyGraph.hasNode(dependency)) - dependencyGraph.setNode(dependency); + // Unwrap the type + const namedTypeNode = getNamedType(field.type); + const dependency = namedTypeNode.name.value; + const namedType = schema.getType(dependency); + if ( + namedType?.astNode?.kind === undefined + || !targetKinds.includes(namedType.astNode.kind) + ) { + return; + } - dependencyGraph.setEdge(typeName, dependency); + if (!dependencyGraph.hasNode(dependency)) { + dependencyGraph.setNode(dependency); } + dependencyGraph.setEdge(typeName, dependency); }); } break; } - case 'ScalarTypeDefinition': - case 'EnumTypeDefinition': { + case Kind.SCALAR_TYPE_DEFINITION: + case Kind.ENUM_TYPE_DEFINITION: { dependencyGraph.setNode(node.name.value); break; } - case 'UnionTypeDefinition': { + case Kind.UNION_TYPE_DEFINITION: { const dependency = node.name.value; if (!dependencyGraph.hasNode(dependency)) dependencyGraph.setNode(dependency); @@ -111,7 +128,7 @@ export function topologicalSortAST(schema: GraphQLSchema, ast: DocumentNode): Do // Create a map of definitions for quick access, using the definition's name as the key. const definitionsMap: Map = new Map(); - // SCHEMA_DEFINITION does not have name. + // SCHEMA_DEFINITION does not have a name. // https://spec.graphql.org/October2021/#sec-Schema const astDefinitions = ast.definitions.filter(def => def.kind !== 'SchemaDefinition'); @@ -120,7 +137,7 @@ export function topologicalSortAST(schema: GraphQLSchema, ast: DocumentNode): Do definitionsMap.set(definition.name.value, definition); }); - // Two arrays to store sorted and not sorted definitions. + // Two arrays to store sorted and unsorted definitions. const sortedDefinitions: DefinitionNode[] = []; const notSortedDefinitions: DefinitionNode[] = []; @@ -141,7 +158,7 @@ export function topologicalSortAST(schema: GraphQLSchema, ast: DocumentNode): Do if (newDefinitions.length !== astDefinitions.length) { throw new Error( - `unexpected definition length after sorted: want ${astDefinitions.length} but got ${newDefinitions.length}`, + `Unexpected definition length after sorting: want ${astDefinitions.length} but got ${newDefinitions.length}`, ); } @@ -155,33 +172,35 @@ function hasNameField(node: ASTNode): node is DefinitionNode & { name?: NameNode return 'name' in node; } -// Re-implemented w/o CycleException version -// https://github.com/dagrejs/graphlib/blob/8d27cb89029081c72eb89dde652602805bdd0a34/lib/alg/topsort.js +/** + * [Re-implemented topsort function][topsort-ref] with cycle handling. This version iterates over + * all nodes in the graph to ensure every node is visited, even if the graph contains cycles. + * + * [topsort-ref]: https://github.com/dagrejs/graphlib/blob/8d27cb89029081c72eb89dde652602805bdd0a34/lib/alg/topsort.js + */ export function topsort(g: Graph): string[] { - const visited: Record = {}; - const stack: Record = {}; - const results: any[] = []; + const visited = new Set(); + const results: Array = []; function visit(node: string) { - if (!(node in visited)) { - stack[node] = true; - visited[node] = true; - const predecessors = g.predecessors(node); - if (Array.isArray(predecessors)) - predecessors.forEach(node => visit(node)); - - delete stack[node]; - results.push(node); - } + if (visited.has(node)) + return; + visited.add(node); + // Recursively visit all predecessors of the node. + g.predecessors(node)?.forEach(visit); + results.push(node); } - g.sinks().forEach(node => visit(node)); + // Visit every node in the graph (instead of only sinks) + g.nodes().forEach(visit); return results.reverse(); } export function isGeneratedByIntrospection(schema: GraphQLSchema): boolean { - return Object.entries(schema.getTypeMap()).filter(([name, type]) => !name.startsWith('__') && !isSpecifiedScalarType(type)).every(([, type]) => type.astNode === undefined) + return Object.entries(schema.getTypeMap()) + .filter(([name, type]) => !name.startsWith('__') && !isSpecifiedScalarType(type)) + .every(([, type]) => type.astNode === undefined) } // https://spec.graphql.org/October2021/#EscapedCharacter diff --git a/tests/graphql.spec.ts b/tests/graphql.spec.ts index 87e90b87..be62fa21 100644 --- a/tests/graphql.spec.ts +++ b/tests/graphql.spec.ts @@ -235,13 +235,13 @@ describe('topologicalSortAST', () => { const sortedSchema = getSortedSchema(schema); const expectedSortedSchema = dedent/* GraphQL */` - input A { - a: A - } - input B { b: B } + + input A { + a: A + } `; expect(sortedSchema).toBe(expectedSortedSchema); diff --git a/tests/myzod.spec.ts b/tests/myzod.spec.ts index 56b003e2..a299de81 100644 --- a/tests/myzod.spec.ts +++ b/tests/myzod.spec.ts @@ -1506,12 +1506,6 @@ describe('myzod', () => { ); expect(removedInitialEmitValue(result.content)).toMatchInlineSnapshot(` " - export const UserCreateInputSchema: myzod.Type = myzod.object({ - name: myzod.string(), - date: myzod.date(), - email: myzod.string().email() - }); - export const UserSchema: myzod.Type = myzod.object({ __typename: myzod.literal('User').optional(), id: myzod.string(), @@ -1521,6 +1515,12 @@ describe('myzod', () => { isMember: myzod.boolean().optional().nullable(), createdAt: myzod.date() }); + + export const UserCreateInputSchema: myzod.Type = myzod.object({ + name: myzod.string(), + date: myzod.date(), + email: myzod.string().email() + }); " `) for (const wantNotContain of ['Query', 'Mutation', 'Subscription']) diff --git a/tests/yup.spec.ts b/tests/yup.spec.ts index 982d7407..1df30acd 100644 --- a/tests/yup.spec.ts +++ b/tests/yup.spec.ts @@ -1486,6 +1486,7 @@ describe('yup', () => { }, {}, ); + expect(result.content).toMatchInlineSnapshot(` " @@ -1495,12 +1496,6 @@ describe('yup', () => { }).defined() } - export const UserCreateInputSchema: yup.ObjectSchema = yup.object({ - name: yup.string().defined().nonNullable(), - date: yup.date().defined().nonNullable(), - email: yup.string().email().defined().nonNullable() - }); - export const UserSchema: yup.ObjectSchema = yup.object({ __typename: yup.string<'User'>().optional(), id: yup.string().defined().nonNullable(), @@ -1510,6 +1505,12 @@ describe('yup', () => { isMember: yup.boolean().defined().nullable().optional(), createdAt: yup.date().defined().nonNullable() }); + + export const UserCreateInputSchema: yup.ObjectSchema = yup.object({ + name: yup.string().defined().nonNullable(), + date: yup.date().defined().nonNullable(), + email: yup.string().email().defined().nonNullable() + }); " `) diff --git a/tests/zod.spec.ts b/tests/zod.spec.ts index 53e451ed..b025ae04 100644 --- a/tests/zod.spec.ts +++ b/tests/zod.spec.ts @@ -1732,14 +1732,9 @@ describe('zod', () => { }, {}, ); + expect(removedInitialEmitValue(result.content)).toMatchInlineSnapshot(` " - export const UserCreateInputSchema: z.ZodObject> = z.object({ - name: z.string(), - date: z.date(), - email: z.string().email() - }); - export const UserSchema: z.ZodObject> = z.object({ __typename: z.literal('User').optional(), id: z.string(), @@ -1749,6 +1744,12 @@ describe('zod', () => { isMember: z.boolean().nullish(), createdAt: z.date() }); + + export const UserCreateInputSchema: z.ZodObject> = z.object({ + name: z.string(), + date: z.date(), + email: z.string().email() + }); " `) From 58f437781fce3387212e06fa5eed3bef00259c61 Mon Sep 17 00:00:00 2001 From: trevor-anderson <43518091+trevor-anderson@users.noreply.github.com> Date: Sat, 8 Feb 2025 10:10:32 -0500 Subject: [PATCH 2/3] test: add test for interface type defs --- tests/graphql.spec.ts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/graphql.spec.ts b/tests/graphql.spec.ts index be62fa21..d9d238ef 100644 --- a/tests/graphql.spec.ts +++ b/tests/graphql.spec.ts @@ -197,6 +197,34 @@ describe('topologicalSortAST', () => { expect(sortedSchema).toBe(expectedSortedSchema); }); + it('should place interface definitions before types that depend on them', () => { + const schema = /* GraphQL */ ` + type A { + id: ID! + node: Node + } + + interface Node { + id: ID! + } + `; + + const sortedSchema = getSortedSchema(schema); + + const expectedSortedSchema = dedent/* GraphQL */` + interface Node { + id: ID! + } + + type A { + id: ID! + node: Node + } + `; + + expect(sortedSchema).toBe(expectedSortedSchema); + }); + it('should correctly handle schema with circular dependencies', () => { const schema = /* GraphQL */ ` input A { From ce1e1aa96e803dc159bc281a09db9f166589c1ee Mon Sep 17 00:00:00 2001 From: trevor-anderson <43518091+trevor-anderson@users.noreply.github.com> Date: Sat, 8 Feb 2025 10:11:20 -0500 Subject: [PATCH 3/3] refactor: use `Kind` enum to filter ast defs --- src/graphql.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/graphql.ts b/src/graphql.ts index aff04b3b..08085b7b 100644 --- a/src/graphql.ts +++ b/src/graphql.ts @@ -130,7 +130,7 @@ export function topologicalSortAST(schema: GraphQLSchema, ast: DocumentNode): Do // SCHEMA_DEFINITION does not have a name. // https://spec.graphql.org/October2021/#sec-Schema - const astDefinitions = ast.definitions.filter(def => def.kind !== 'SchemaDefinition'); + const astDefinitions = ast.definitions.filter(def => def.kind !== Kind.SCHEMA_DEFINITION); astDefinitions.forEach((definition) => { if (hasNameField(definition) && definition.name)