From 0c0cdad7c1dd5ae2052a6574162732492c6592e4 Mon Sep 17 00:00:00 2001 From: Darrell Warde Date: Wed, 5 Jul 2023 15:34:26 +0100 Subject: [PATCH 1/7] Refined normalization of user type definitions --- .changeset/khaki-mayflies-float.md | 6 + .../graphql/src/classes/Neo4jGraphQL.test.ts | 188 +++++++++++++++++- packages/graphql/src/classes/Neo4jGraphQL.ts | 46 ++++- 3 files changed, 229 insertions(+), 11 deletions(-) create mode 100644 .changeset/khaki-mayflies-float.md diff --git a/.changeset/khaki-mayflies-float.md b/.changeset/khaki-mayflies-float.md new file mode 100644 index 0000000000..30767d67fa --- /dev/null +++ b/.changeset/khaki-mayflies-float.md @@ -0,0 +1,6 @@ +--- +"@neo4j/graphql": major +"@neo4j/graphql-ogm": major +--- + +The Neo4j GraphQL Library now only accepts a `string`, `DocumentNode` or an array containing these types. A callback function returning these is also accepted. This is a reduction from `TypeSource` which also included types such as `GraphQLSchema` and `DefinitionNode`, which would have resulted in unexpected behaviour if passed in. diff --git a/packages/graphql/src/classes/Neo4jGraphQL.test.ts b/packages/graphql/src/classes/Neo4jGraphQL.test.ts index 275596f641..e35f6b8fb6 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.test.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.test.ts @@ -17,9 +17,10 @@ * limitations under the License. */ -import type { GraphQLSchema } from "graphql"; +import { parse, type GraphQLSchema, print } from "graphql"; import { getErrorAsync, NoErrorThrownError } from "../../tests/utils/get-error"; import Neo4jGraphQL from "./Neo4jGraphQL"; +import gql from "graphql-tag"; describe("Neo4jGraphQL", () => { test("should construct", () => { @@ -81,4 +82,189 @@ describe("Neo4jGraphQL", () => { }); }); }); + + describe("normalizeTypeDefinitions", () => { + test("string", () => { + const typeDefs = ` + type Movie { + title: String! + } + `; + + // @ts-ignore: testing a private method + expect(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)).toEqual(parse(typeDefs)); + }); + + test("DocumentNode", () => { + const typeDefs = gql` + type Movie { + title: String! + } + `; + + // @ts-ignore: testing a private method + expect(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)).toEqual(typeDefs); + }); + + test("string[]", () => { + const typeDefs = [ + ` + type Movie { + title: String! + } + `, + ` + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `, + ]; + + // @ts-ignore: testing a private method + const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); + + const expected = gql` + type Movie { + title: String! + } + + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `; + + expect(normalizedString).toEqual(print(expected)); + }); + + test("DocumentNode[]", () => { + const typeDefs = [ + gql` + type Movie { + title: String! + } + `, + gql` + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `, + ]; + + // @ts-ignore: testing a private method + const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); + + const expected = gql` + type Movie { + title: String! + } + + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `; + + expect(normalizedString).toEqual(print(expected)); + }); + + test("(string | DocumentNode)[]", () => { + const typeDefs = [ + ` + type Movie { + title: String! + } + `, + gql` + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `, + ]; + + // @ts-ignore: testing a private method + const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); + + const expected = gql` + type Movie { + title: String! + } + + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `; + + expect(normalizedString).toEqual(print(expected)); + }); + + test("() => TypeDefinitions", () => { + const typeDefs = () => [ + ` + type Movie { + title: String! + } + `, + gql` + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `, + ]; + + // @ts-ignore: testing a private method + const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); + + const expected = gql` + type Movie { + title: String! + } + + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `; + + expect(normalizedString).toEqual(print(expected)); + }); + + test("() => (() => TypeDefinitions)", () => { + const typeDefs = () => () => + [ + ` + type Movie { + title: String! + } + `, + gql` + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `, + ]; + + // @ts-ignore: testing a private method + const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); + + const expected = gql` + type Movie { + title: String! + } + + type Actor { + name: String! + movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) + } + `; + + expect(normalizedString).toEqual(print(expected)); + }); + }); }); diff --git a/packages/graphql/src/classes/Neo4jGraphQL.ts b/packages/graphql/src/classes/Neo4jGraphQL.ts index edfa46f313..1914bae6d8 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -50,7 +50,7 @@ import type { IExecutableSchemaDefinition } from "@graphql-tools/schema"; import { addResolversToSchema, makeExecutableSchema } from "@graphql-tools/schema"; import type { TypeSource } from "@graphql-tools/utils"; import { forEachField, getResolversFromSchema } from "@graphql-tools/utils"; -import type { DocumentNode, GraphQLSchema } from "graphql"; +import { parse, type DocumentNode, type GraphQLSchema } from "graphql"; import type { Driver } from "neo4j-driver"; import { validateDocument } from "../schema/validation"; import { validateUserDefinition } from "../schema/validation/schema-validation"; @@ -71,8 +71,10 @@ export type ValidationConfig = { validateDuplicateRelationshipFields: boolean; }; +type TypeDefinitions = string | DocumentNode | TypeDefinitions[] | (() => TypeDefinitions); + export interface Neo4jGraphQLConstructor { - typeDefs: TypeSource; + typeDefs: TypeDefinitions; resolvers?: IExecutableSchemaDefinition["resolvers"]; features?: Neo4jFeaturesSettings; config?: Neo4jGraphQLConfig; @@ -87,7 +89,7 @@ export const defaultValidationConfig: ValidationConfig = { }; class Neo4jGraphQL { - private typeDefs: TypeSource; + private typeDefs: TypeDefinitions; private resolvers?: IExecutableSchemaDefinition["resolvers"]; private config: Neo4jGraphQLConfig; @@ -221,7 +223,7 @@ class Neo4jGraphQL { public neo4jValidateGraphQLDocument(): { isValid: boolean; validationErrors: string[] } { try { - const initialDocument = this.getDocument(this.typeDefs); + const initialDocument = this.normalizeTypeDefinitions(this.typeDefs); validateDocument({ document: initialDocument, features: this.features }); @@ -252,6 +254,34 @@ class Neo4jGraphQL { return { isValid: true, validationErrors: [] }; } + /** + * Normalizes the user's type definitions using the method with the lowest risk of side effects: + * - Type definitions of type `string` are parsed using the `parse` function from the reference GraphQL implementation. + * - Type definitions of type `DocumentNode` are returned as they are. + * - Type definitions in arrays are merged using `mergeTypeDefs` from `@graphql-tools/merge`. + * - Callbacks are resolved to a type which can be parsed into a document. + * + * This method maps to the Type Definition Normalization stage of the Schema Generation lifecycle. + * + * @param {TypeDefinitions} typeDefinitions - The unnormalized type definitions. + * @returns {DocumentNode} The normalized type definitons as a document. + */ + private normalizeTypeDefinitions(typeDefinitions: TypeDefinitions): DocumentNode { + if (typeof typeDefinitions === "function") { + return this.normalizeTypeDefinitions(typeDefinitions()); + } + + if (typeof typeDefinitions === "string") { + return parse(typeDefinitions); + } + + if (Array.isArray(typeDefinitions)) { + return mergeTypeDefs(typeDefinitions); + } + + return typeDefinitions; + } + private addDefaultFieldResolvers(schema: GraphQLSchema): GraphQLSchema { forEachField(schema, (field) => { if (!field.resolve) { @@ -272,10 +302,6 @@ class Neo4jGraphQL { } } - private getDocument(typeDefs: TypeSource): DocumentNode { - return mergeTypeDefs(typeDefs); - } - private async getNeo4jDatabaseInfo(driver: Driver, driverConfig?: DriverConfig): Promise { const executorConstructorParam: ExecutorConstructorParam = { executionContext: driver, @@ -368,7 +394,7 @@ class Neo4jGraphQL { private generateExecutableSchema(): Promise { return new Promise((resolve) => { - const initialDocument = this.getDocument(this.typeDefs); + const initialDocument = this.normalizeTypeDefinitions(this.typeDefs); const validationConfig = this.parseStartupValidationConfig(); @@ -411,7 +437,7 @@ class Neo4jGraphQL { // Import only when needed to avoid issues if GraphQL 15 being used const { Subgraph } = await import("./Subgraph"); - const initialDocument = this.getDocument(this.typeDefs); + const initialDocument = this.normalizeTypeDefinitions(this.typeDefs); const subgraph = new Subgraph(this.typeDefs); const { directives, types } = subgraph.getValidationDefinitions(); From 5c831a9cfb7fee12a351b28bfc204e0148bbc5cf Mon Sep 17 00:00:00 2001 From: Darrell Warde <8117355+darrellwarde@users.noreply.github.com> Date: Wed, 5 Jul 2023 15:38:32 +0100 Subject: [PATCH 2/7] Remove unused type --- packages/graphql/src/classes/Neo4jGraphQL.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/graphql/src/classes/Neo4jGraphQL.ts b/packages/graphql/src/classes/Neo4jGraphQL.ts index 1914bae6d8..a9332b942e 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -48,7 +48,6 @@ import type { Neo4jGraphQLSchemaModel } from "../schema-model/Neo4jGraphQLSchema import { composeResolvers } from "@graphql-tools/resolvers-composition"; import type { IExecutableSchemaDefinition } from "@graphql-tools/schema"; import { addResolversToSchema, makeExecutableSchema } from "@graphql-tools/schema"; -import type { TypeSource } from "@graphql-tools/utils"; import { forEachField, getResolversFromSchema } from "@graphql-tools/utils"; import { parse, type DocumentNode, type GraphQLSchema } from "graphql"; import type { Driver } from "neo4j-driver"; From b49b069aa995fb427d2a5e0dc77c3e4e9fcb213d Mon Sep 17 00:00:00 2001 From: Darrell Warde Date: Wed, 19 Jul 2023 16:26:27 +0100 Subject: [PATCH 3/7] Wrap current behaviour with new lifecycle naming --- .../graphql/src/classes/Neo4jGraphQL.test.ts | 5 +++- packages/graphql/src/classes/Neo4jGraphQL.ts | 23 +++++++++++-------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/packages/graphql/src/classes/Neo4jGraphQL.test.ts b/packages/graphql/src/classes/Neo4jGraphQL.test.ts index 47189d209c..ba029fb879 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.test.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.test.ts @@ -54,7 +54,10 @@ describe("Neo4jGraphQL", () => { }); }); - describe("normalizeTypeDefinitions", () => { + /** + * The output that we would expect from `normalizeTypeDefinitions` with an optimal implementation. + */ + describe.skip("normalizeTypeDefinitions", () => { test("string", () => { const typeDefs = ` type Movie { diff --git a/packages/graphql/src/classes/Neo4jGraphQL.ts b/packages/graphql/src/classes/Neo4jGraphQL.ts index a4e36ae501..6e05cdeb08 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -222,19 +222,22 @@ class Neo4jGraphQL { * @returns {DocumentNode} The normalized type definitons as a document. */ private normalizeTypeDefinitions(typeDefinitions: TypeDefinitions): DocumentNode { - if (typeof typeDefinitions === "function") { - return this.normalizeTypeDefinitions(typeDefinitions()); - } + // TODO: The dream: minimal modification of the type definitions. However, this does not merge extensions, which we can't currently deal with in translation. + // if (typeof typeDefinitions === "function") { + // return this.normalizeTypeDefinitions(typeDefinitions()); + // } - if (typeof typeDefinitions === "string") { - return parse(typeDefinitions); - } + // if (typeof typeDefinitions === "string") { + // return parse(typeDefinitions); + // } - if (Array.isArray(typeDefinitions)) { - return mergeTypeDefs(typeDefinitions); - } + // if (Array.isArray(typeDefinitions)) { + // return mergeTypeDefs(typeDefinitions); + // } + + // return typeDefinitions; - return typeDefinitions; + return mergeTypeDefs(typeDefinitions); } private addDefaultFieldResolvers(schema: GraphQLSchema): GraphQLSchema { From fa99c8b612c1b53607a0b9edeb8c0ce554a9e79b Mon Sep 17 00:00:00 2001 From: Darrell Warde Date: Wed, 19 Jul 2023 16:31:38 +0100 Subject: [PATCH 4/7] Fix linting error --- packages/graphql/src/classes/Neo4jGraphQL.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/graphql/src/classes/Neo4jGraphQL.ts b/packages/graphql/src/classes/Neo4jGraphQL.ts index 6e05cdeb08..49776fd5e7 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -41,7 +41,7 @@ import { composeResolvers } from "@graphql-tools/resolvers-composition"; import type { IExecutableSchemaDefinition } from "@graphql-tools/schema"; import { addResolversToSchema, makeExecutableSchema } from "@graphql-tools/schema"; import { forEachField, getResolversFromSchema } from "@graphql-tools/utils"; -import { parse, type DocumentNode, type GraphQLSchema } from "graphql"; +import type { DocumentNode, GraphQLSchema } from "graphql"; import type { Driver, SessionConfig } from "neo4j-driver"; import { validateDocument } from "../schema/validation"; import { validateUserDefinition } from "../schema/validation/schema-validation"; From 4b760fd1903d5de94f2f9b229ecf9a785b5a1dd0 Mon Sep 17 00:00:00 2001 From: Darrell Warde Date: Wed, 19 Jul 2023 16:42:15 +0100 Subject: [PATCH 5/7] Delete unused tests --- .../graphql/src/classes/Neo4jGraphQL.test.ts | 191 +----------------- 1 file changed, 1 insertion(+), 190 deletions(-) diff --git a/packages/graphql/src/classes/Neo4jGraphQL.test.ts b/packages/graphql/src/classes/Neo4jGraphQL.test.ts index ba029fb879..9eb51e2f89 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.test.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.test.ts @@ -17,10 +17,9 @@ * limitations under the License. */ -import { parse, type GraphQLSchema, print } from "graphql"; +import type { GraphQLSchema } from "graphql"; import { getErrorAsync, NoErrorThrownError } from "../../tests/utils/get-error"; import Neo4jGraphQL from "./Neo4jGraphQL"; -import gql from "graphql-tag"; describe("Neo4jGraphQL", () => { test("should construct", () => { @@ -53,192 +52,4 @@ describe("Neo4jGraphQL", () => { }); }); }); - - /** - * The output that we would expect from `normalizeTypeDefinitions` with an optimal implementation. - */ - describe.skip("normalizeTypeDefinitions", () => { - test("string", () => { - const typeDefs = ` - type Movie { - title: String! - } - `; - - // @ts-ignore: testing a private method - expect(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)).toEqual(parse(typeDefs)); - }); - - test("DocumentNode", () => { - const typeDefs = gql` - type Movie { - title: String! - } - `; - - // @ts-ignore: testing a private method - expect(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)).toEqual(typeDefs); - }); - - test("string[]", () => { - const typeDefs = [ - ` - type Movie { - title: String! - } - `, - ` - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `, - ]; - - // @ts-ignore: testing a private method - const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); - - const expected = gql` - type Movie { - title: String! - } - - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `; - - expect(normalizedString).toEqual(print(expected)); - }); - - test("DocumentNode[]", () => { - const typeDefs = [ - gql` - type Movie { - title: String! - } - `, - gql` - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `, - ]; - - // @ts-ignore: testing a private method - const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); - - const expected = gql` - type Movie { - title: String! - } - - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `; - - expect(normalizedString).toEqual(print(expected)); - }); - - test("(string | DocumentNode)[]", () => { - const typeDefs = [ - ` - type Movie { - title: String! - } - `, - gql` - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `, - ]; - - // @ts-ignore: testing a private method - const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); - - const expected = gql` - type Movie { - title: String! - } - - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `; - - expect(normalizedString).toEqual(print(expected)); - }); - - test("() => TypeDefinitions", () => { - const typeDefs = () => [ - ` - type Movie { - title: String! - } - `, - gql` - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `, - ]; - - // @ts-ignore: testing a private method - const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); - - const expected = gql` - type Movie { - title: String! - } - - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `; - - expect(normalizedString).toEqual(print(expected)); - }); - - test("() => (() => TypeDefinitions)", () => { - const typeDefs = () => () => - [ - ` - type Movie { - title: String! - } - `, - gql` - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `, - ]; - - // @ts-ignore: testing a private method - const normalizedString = print(Neo4jGraphQL.prototype.normalizeTypeDefinitions(typeDefs)); - - const expected = gql` - type Movie { - title: String! - } - - type Actor { - name: String! - movies: [Movie!]! @relationship(type: "ACTED_IN", direction: OUT) - } - `; - - expect(normalizedString).toEqual(print(expected)); - }); - }); }); From 258a41bd26158d53eab41655fe5bece783694c25 Mon Sep 17 00:00:00 2001 From: Darrell Warde Date: Wed, 19 Jul 2023 16:53:37 +0100 Subject: [PATCH 6/7] Fix test typings --- .../graphql/tests/e2e/federation/setup/subgraph.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/graphql/tests/e2e/federation/setup/subgraph.ts b/packages/graphql/tests/e2e/federation/setup/subgraph.ts index 9a5141a27f..3c6563163a 100644 --- a/packages/graphql/tests/e2e/federation/setup/subgraph.ts +++ b/packages/graphql/tests/e2e/federation/setup/subgraph.ts @@ -17,15 +17,22 @@ * limitations under the License. */ -import type { TypeSource } from "@graphql-tools/utils"; -import type { GraphQLSchema } from "graphql"; +import type { DocumentNode, GraphQLSchema } from "graphql"; import type * as neo4j from "neo4j-driver"; import { Neo4jGraphQL } from "../../../../src"; export class TestSubgraph { library: Neo4jGraphQL; - constructor({ typeDefs, resolvers, driver }: { typeDefs: TypeSource; resolvers?: any; driver: neo4j.Driver }) { + constructor({ + typeDefs, + resolvers, + driver, + }: { + typeDefs: string | DocumentNode; + resolvers?: any; + driver: neo4j.Driver; + }) { this.library = new Neo4jGraphQL({ typeDefs, resolvers, From d49fb13ad34d662649026f56760d5a9e95fddaf5 Mon Sep 17 00:00:00 2001 From: Darrell Warde <8117355+darrellwarde@users.noreply.github.com> Date: Fri, 4 Aug 2023 11:00:56 +0100 Subject: [PATCH 7/7] Update Neo4jGraphQL.ts --- packages/graphql/src/classes/Neo4jGraphQL.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/graphql/src/classes/Neo4jGraphQL.ts b/packages/graphql/src/classes/Neo4jGraphQL.ts index 49776fd5e7..5bc58aee1d 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -210,6 +210,8 @@ class Neo4jGraphQL { } /** + * Currently just merges all type definitions into a document. Eventual intention described below: + * * Normalizes the user's type definitions using the method with the lowest risk of side effects: * - Type definitions of type `string` are parsed using the `parse` function from the reference GraphQL implementation. * - Type definitions of type `DocumentNode` are returned as they are.