Skip to content

fix: update visitor logic in topologicalSortAST to fix Issue #528 #958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 9, 2025
Merged

fix: update visitor logic in topologicalSortAST to fix Issue #528 #958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
22cd8c3
fix: update visitor logic in `topologicalSortAST` to fix Issue #528
trevor-anderson Feb 7, 2025
58f4377
test: add test for interface type defs
trevor-anderson Feb 8, 2025
ce1e1aa
refactor: use `Kind` enum to filter ast defs
trevor-anderson Feb 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 62 additions & 43 deletions src/graphql.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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<DocumentNode>(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: {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new case, so please add a test to confirm that the interface is also sorted

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);
Expand All @@ -111,16 +128,16 @@ 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<string, DefinitionNode> = 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');
const astDefinitions = ast.definitions.filter(def => def.kind !== Kind.SCHEMA_DEFINITION);

astDefinitions.forEach((definition) => {
if (hasNameField(definition) && definition.name)
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[] = [];

Expand All @@ -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}`,
);
}

Expand All @@ -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<string, boolean> = {};
const stack: Record<string, boolean> = {};
const results: any[] = [];
const visited = new Set<string>();
const results: Array<string> = [];

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
Expand Down
36 changes: 32 additions & 4 deletions tests/graphql.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -235,13 +263,13 @@ describe('topologicalSortAST', () => {
const sortedSchema = getSortedSchema(schema);

const expectedSortedSchema = dedent/* GraphQL */`
input A {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do I need to change this test case? I expect this test case to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relevant code change is in the topsort algo fn, where g.sinks().forEach(visit) is replaced with g.nodes().forEach(visit) — that change makes this necessary.

The reason for the change is that, in order to ensure that every node is visited, we need to iterate over all nodes in the graph. In the presence of cycles, some nodes may not be considered sinks (i.e., they have no outgoing edges), and therefore would be skipped in a traditional topological sort. This is precisely what was happening in this test case with the old implementation — it's why input A and input B were not re-ordered like all the other test cases.

The new g.nodes() approach guarantees that every definition is included. The difference is in the starting point for the DFS that produces the order:

  • Using g.sinks().forEach(visit):
    This approach starts the DFS only from nodes that are sinks — that is, nodes with no outgoing edges that do not depend on any other nodes. In a graph without cycles, sinks are safe starting points because they can be placed first in the order. However, if a node is part of a cycle — or self-references, as in the relevant test-case — it will have an outgoing edge (even if that edge is to itself), so it will not be considered a sink. That means the DFS might never start from those nodes, and they could be entirely skipped in the ordering. This is why input A and input B were not re-ordered in the previous implementation.

  • Using g.nodes().forEach(visit):
    This approach goes over every node in the graph and ensures that each one is visited, regardless of whether it’s a sink or not. This is especially important for self-circular or mutually dependent nodes, because even if they are in a cycle (and therefore not sinks), they’ll still be processed and appear in the final list.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your explanation!
Let's Go!!

a: A
}

input B {
b: B
}

input A {
a: A
}
`;

expect(sortedSchema).toBe(expectedSortedSchema);
Expand Down
12 changes: 6 additions & 6 deletions tests/myzod.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1506,12 +1506,6 @@ describe('myzod', () => {
);
expect(removedInitialEmitValue(result.content)).toMatchInlineSnapshot(`
"
export const UserCreateInputSchema: myzod.Type<UserCreateInput> = myzod.object({
name: myzod.string(),
date: myzod.date(),
email: myzod.string().email()
});

export const UserSchema: myzod.Type<User> = myzod.object({
__typename: myzod.literal('User').optional(),
id: myzod.string(),
Expand All @@ -1521,6 +1515,12 @@ describe('myzod', () => {
isMember: myzod.boolean().optional().nullable(),
createdAt: myzod.date()
});

export const UserCreateInputSchema: myzod.Type<UserCreateInput> = myzod.object({
name: myzod.string(),
date: myzod.date(),
email: myzod.string().email()
});
"
`)
for (const wantNotContain of ['Query', 'Mutation', 'Subscription'])
Expand Down
13 changes: 7 additions & 6 deletions tests/yup.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1486,6 +1486,7 @@ describe('yup', () => {
},
{},
);

expect(result.content).toMatchInlineSnapshot(`
"

Expand All @@ -1495,12 +1496,6 @@ describe('yup', () => {
}).defined()
}

export const UserCreateInputSchema: yup.ObjectSchema<UserCreateInput> = yup.object({
name: yup.string().defined().nonNullable(),
date: yup.date().defined().nonNullable(),
email: yup.string().email().defined().nonNullable()
});

export const UserSchema: yup.ObjectSchema<User> = yup.object({
__typename: yup.string<'User'>().optional(),
id: yup.string().defined().nonNullable(),
Expand All @@ -1510,6 +1505,12 @@ describe('yup', () => {
isMember: yup.boolean().defined().nullable().optional(),
createdAt: yup.date().defined().nonNullable()
});

export const UserCreateInputSchema: yup.ObjectSchema<UserCreateInput> = yup.object({
name: yup.string().defined().nonNullable(),
date: yup.date().defined().nonNullable(),
email: yup.string().email().defined().nonNullable()
});
"
`)

Expand Down
13 changes: 7 additions & 6 deletions tests/zod.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1732,14 +1732,9 @@ describe('zod', () => {
},
{},
);

expect(removedInitialEmitValue(result.content)).toMatchInlineSnapshot(`
"
export const UserCreateInputSchema: z.ZodObject<Properties<UserCreateInput>> = z.object({
name: z.string(),
date: z.date(),
email: z.string().email()
});
export const UserSchema: z.ZodObject<Properties<User>> = z.object({
__typename: z.literal('User').optional(),
id: z.string(),
Expand All @@ -1749,6 +1744,12 @@ describe('zod', () => {
isMember: z.boolean().nullish(),
createdAt: z.date()
});
export const UserCreateInputSchema: z.ZodObject<Properties<UserCreateInput>> = z.object({
name: z.string(),
date: z.date(),
email: z.string().email()
});
"
`)

Expand Down