From 9a78094556ca2171b30a2aff28e84e402d550f1a Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Tue, 14 Mar 2023 11:23:50 +0100 Subject: [PATCH 01/13] refactor: move memoization of schemaModel to setSchemaModel method --- packages/graphql/src/classes/Neo4jGraphQL.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/graphql/src/classes/Neo4jGraphQL.ts b/packages/graphql/src/classes/Neo4jGraphQL.ts index 9fea6bf47d..21f4b1c159 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -286,6 +286,14 @@ class Neo4jGraphQL { return composeResolvers(mergedResolvers, resolversComposition); } + private setSchemaModel(document: DocumentNode) { + // This can be run several times but it will always be the same result, + // so we memoize the schemaModel. + if (!this.schemaModel) { + this.schemaModel = generateModel(document); + } + } + private generateExecutableSchema(): Promise { return new Promise((resolve) => { const document = this.getDocument(this.typeDefs); @@ -294,9 +302,7 @@ class Neo4jGraphQL { validateDocument(document, validateTypeDefs); - if (!this.schemaModel) { - this.schemaModel = generateModel(document); - } + this.setSchemaModel(document); const { nodes, relationships, typeDefs, resolvers } = makeAugmentedSchema(document, { features: this.features, @@ -334,9 +340,7 @@ class Neo4jGraphQL { validateDocument(document, validateTypeDefs, directives, types); - if (!this.schemaModel) { - this.schemaModel = generateModel(document); - } + this.setSchemaModel(document); const { nodes, relationships, typeDefs, resolvers } = makeAugmentedSchema(document, { features: this.features, From 7fad2cd3ca67f8ae2f0df4f98c5022b3337463de Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Tue, 14 Mar 2023 16:19:18 +0100 Subject: [PATCH 02/13] feat: add validation for duplicate relationship fields --- .../validate-custom-resolver-requires.ts | 57 +++++++------ .../schema/validation/validate-document.ts | 4 +- ...date-duplicate-relationship-fields.test.ts | 85 +++++++++++++++++++ .../validate-duplicate-relationship-fields.ts | 65 ++++++++++++++ .../validate-schema-customizations.ts | 39 +++++++++ 5 files changed, 220 insertions(+), 30 deletions(-) create mode 100644 packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts create mode 100644 packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.ts create mode 100644 packages/graphql/src/schema/validation/validate-schema-customizations.ts diff --git a/packages/graphql/src/schema/validation/validate-custom-resolver-requires.ts b/packages/graphql/src/schema/validation/validate-custom-resolver-requires.ts index d16e6a4742..ed814b3ddc 100644 --- a/packages/graphql/src/schema/validation/validate-custom-resolver-requires.ts +++ b/packages/graphql/src/schema/validation/validate-custom-resolver-requires.ts @@ -18,35 +18,36 @@ */ import { mergeSchemas } from "@graphql-tools/schema"; -import type { - DocumentNode, - GraphQLSchema, - InterfaceTypeDefinitionNode, - ObjectTypeDefinitionNode} from "graphql"; -import { - Kind, - parse, - validate, -} from "graphql"; -import { getDefinitionNodes } from "../get-definition-nodes"; +import type { DocumentNode, GraphQLSchema, InterfaceTypeDefinitionNode, ObjectTypeDefinitionNode } from "graphql"; +import { Kind, parse, validate } from "graphql"; -export function validateCustomResolverRequires(document: DocumentNode, schema: GraphQLSchema) { - const definitionNodes = getDefinitionNodes(document); - definitionNodes.objectTypes.forEach((objType) => { - objType.fields?.forEach((field) => { - const customResolverDirective = field.directives?.find( - (directive) => directive.name.value === "customResolver" - ); - const requiresArg = customResolverDirective?.arguments?.find((arg) => arg.name.value === "requires"); - if (requiresArg) { - if (requiresArg?.value.kind !== Kind.STRING) { - throw new Error("@customResolver requires expects a string"); - } - const selectionSetDocument = parse(`{ ${requiresArg.value.value} }`); - validateSelectionSet(schema, objType, selectionSetDocument); - } - }); - }); +export function validateCustomResolverRequires(objType: ObjectTypeDefinitionNode, schema: GraphQLSchema) { + if (!objType.fields) { + return; + } + + for (const field of objType.fields) { + if (!field.directives) { + continue; + } + + const customResolverDirective = field.directives.find((directive) => directive.name.value === "customResolver"); + if (!customResolverDirective || !customResolverDirective.arguments) { + continue; + } + + const requiresArg = customResolverDirective.arguments.find((arg) => arg.name.value === "requires"); + if (!requiresArg) { + continue; + } + + if (requiresArg.value.kind !== Kind.STRING) { + throw new Error("@customResolver requires expects a string"); + } + + const selectionSetDocument = parse(`{ ${requiresArg.value.value} }`); + validateSelectionSet(schema, objType, selectionSetDocument); + } } function validateSelectionSet( diff --git a/packages/graphql/src/schema/validation/validate-document.ts b/packages/graphql/src/schema/validation/validate-document.ts index 929c5a38bb..ad323c60ca 100644 --- a/packages/graphql/src/schema/validation/validate-document.ts +++ b/packages/graphql/src/schema/validation/validate-document.ts @@ -40,7 +40,7 @@ import { PointDistance } from "../../graphql/input-objects/PointDistance"; import { CartesianPointDistance } from "../../graphql/input-objects/CartesianPointDistance"; import { RESERVED_TYPE_NAMES } from "../../constants"; import { isRootType } from "../../utils/is-root-type"; -import { validateCustomResolverRequires } from "./validate-custom-resolver-requires"; +import { validateSchemaAugments } from "./validate-schema-augments"; function filterDocument(document: DocumentNode): DocumentNode { const nodeNames = document.definitions @@ -199,7 +199,7 @@ function validateDocument( throw new Error(filteredErrors.join("\n")); } } - validateCustomResolverRequires(document, schema); + validateSchemaAugments(document, schema); } export default validateDocument; diff --git a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts new file mode 100644 index 0000000000..5ae2e2a674 --- /dev/null +++ b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts @@ -0,0 +1,85 @@ +/* + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { gql } from "apollo-server"; +import validateDocument from "./validate-document"; + +describe("validateDuplicateRelationshipFields", () => { + test("should throw an error if multiple relationship fields in the same type have the same relationship type.", () => { + const doc = gql` + type Team { + name: String! + player1: Person! @relationship(type: "PLAYS_IN", direction: IN) + player2: Person! @relationship(type: "PLAYS_IN", direction: IN) + backupPlayers: [Person!]! @relationship(type: "PLAYS_IN", direction: IN) + } + + type Person { + name: String! + teams: [Team!]! @relationship(type: "PLAYS_IN", direction: OUT) + } + `; + + expect(() => validateDocument(doc)).toThrow( + "Multiple relationship fields with the same type and direction may not have the same relationship type" + ); + }); + + test("should not throw an error if multiple relationship fields of different types have the same relationship type.", () => { + const doc = gql` + type Team { + name: String! + player: Person! @relationship(type: "PLAYS_IN", direction: IN) + venue: Venue! @relationship(type: "PLAYS_IN", direction: IN) + } + + type Person { + name: String! + teams: [Team!]! @relationship(type: "PLAYS_IN", direction: OUT) + } + + type Venue { + location: String! + } + `; + + expect(() => validateDocument(doc)).not.toThrow(); + }); + + test("should not throw an error if multiple relationship fields in the same type have the same relationship type but have different directions.", () => { + const doc = gql` + type Team { + name: String! + player: Person! @relationship(type: "PLAYS_IN", direction: IN) + venue: Venue! @relationship(type: "PLAYS_IN", direction: IN) + } + + type Person { + name: String! + teams: [Team!]! @relationship(type: "PLAYS_IN", direction: OUT) + } + + type Venue { + location: String! + } + `; + + expect(() => validateDocument(doc)).not.toThrow(); + }); +}); diff --git a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.ts b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.ts new file mode 100644 index 0000000000..67de97835b --- /dev/null +++ b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.ts @@ -0,0 +1,65 @@ +/* + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import type { ObjectTypeDefinitionNode } from "graphql"; +import getFieldTypeMeta from "../get-field-type-meta"; +import { Kind } from "graphql"; + +export function validateDuplicateRelationshipFields(objType: ObjectTypeDefinitionNode) { + if (!objType.fields) { + return; + } + + const relationshipUsages = new Set(); + + for (const field of objType.fields) { + if (!field.directives) { + continue; + } + + const relationshipDirective = field.directives.find((directive) => directive.name.value === "relationship"); + if (!relationshipDirective || !relationshipDirective.arguments) { + continue; + } + + const typeArg = relationshipDirective.arguments.find((arg) => arg.name.value === "type"); + const directionArg = relationshipDirective.arguments.find((arg) => arg.name.value === "direction"); + if (!typeArg || !directionArg) { + continue; + } + + if (typeArg.value.kind !== Kind.STRING) { + throw new Error("@relationship type expects a string"); + } + + if (directionArg.value.kind !== Kind.ENUM) { + throw new Error("@relationship direction expects an enum"); + } + + const typeMeta = getFieldTypeMeta(field.type); + + if (relationshipUsages.has(`${typeMeta.name}__${typeArg.value.value}__${directionArg.value.value}`)) { + throw new Error( + "Multiple relationship fields with the same type and direction may not have the same relationship type" + ); + } + + relationshipUsages.add(`${typeMeta.name}__${typeArg.value.value}__${directionArg.value.value}`); + } +} diff --git a/packages/graphql/src/schema/validation/validate-schema-customizations.ts b/packages/graphql/src/schema/validation/validate-schema-customizations.ts new file mode 100644 index 0000000000..4d2bc89c9c --- /dev/null +++ b/packages/graphql/src/schema/validation/validate-schema-customizations.ts @@ -0,0 +1,39 @@ +/* + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import type { DocumentNode, GraphQLSchema } from "graphql"; +import type { ValidationConfig } from "../../classes/Neo4jGraphQL"; +import { getDefinitionNodes } from "../get-definition-nodes"; +import { validateCustomResolverRequires } from "./validate-custom-resolver-requires"; +import { validateDuplicateRelationshipFields } from "./validate-duplicate-relationship-fields"; + +export function validateSchemaCustomizations( + document: DocumentNode, + schema: GraphQLSchema, + validationConfig: ValidationConfig +) { + const definitionNodes = getDefinitionNodes(document); + + for (const objectType of definitionNodes.objectTypes) { + validateCustomResolverRequires(objectType, schema); + if (validationConfig.validateDuplicateRelationshipFields) { + validateDuplicateRelationshipFields(objectType); + } + } +} From a128b13e863bff9b2b621e20797be9c90b41686a Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Tue, 14 Mar 2023 17:00:56 +0100 Subject: [PATCH 03/13] test: update failing test This test failed after validation was introduced. We do want this test so we update the types. --- packages/graphql/tests/schema/rfcs/rfc-003.test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/graphql/tests/schema/rfcs/rfc-003.test.ts b/packages/graphql/tests/schema/rfcs/rfc-003.test.ts index c87038ba9a..7dfab683ed 100644 --- a/packages/graphql/tests/schema/rfcs/rfc-003.test.ts +++ b/packages/graphql/tests/schema/rfcs/rfc-003.test.ts @@ -29,13 +29,21 @@ describe("schema/rfc/003", () => { const typeDefs = gql` type Source { targets: [Target!]! @relationship(type: "HAS_TARGET", direction: OUT) - target1: Target! @relationship(type: "HAS_TARGET", direction: OUT) - target2: Target @relationship(type: "HAS_TARGET", direction: OUT) + target1: SecondTarget! @relationship(type: "HAS_TARGET", direction: OUT) + target2: ThirdTarget @relationship(type: "HAS_TARGET", direction: OUT) } type Target { id: ID @id } + + type SecondTarget { + id: ID @id + } + + type ThirdTarget { + id: ID @id + } `; const neoSchema = new Neo4jGraphQL({ typeDefs }); From b0f65f87fe3935aca8d6f01ae32e80fa6aa371d5 Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Wed, 15 Mar 2023 15:03:46 +0100 Subject: [PATCH 04/13] feat: add validation config for noDuplicateRelationshipFields --- packages/graphql/src/classes/Neo4jGraphQL.ts | 46 +++++++++++-------- .../schema/validation/validate-document.ts | 12 +++-- ...date-duplicate-relationship-fields.test.ts | 2 +- .../validate-duplicate-relationship-fields.ts | 2 +- packages/graphql/src/types.ts | 1 + 5 files changed, 37 insertions(+), 26 deletions(-) diff --git a/packages/graphql/src/classes/Neo4jGraphQL.ts b/packages/graphql/src/classes/Neo4jGraphQL.ts index 21f4b1c159..04c025cdd1 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -67,6 +67,12 @@ export interface Neo4jGraphQLConfig { callbacks?: Neo4jGraphQLCallbacks; } +export type ValidationConfig = { + validateTypeDefs: boolean; + validateResolvers: boolean; + validateDuplicateRelationshipFields: boolean; +}; + export interface Neo4jGraphQLConstructor { typeDefs: TypeSource; resolvers?: IExecutableSchemaDefinition["resolvers"]; @@ -76,6 +82,12 @@ export interface Neo4jGraphQLConstructor { plugins?: Neo4jGraphQLPlugins; } +export const defaultValidationConfig: ValidationConfig = { + validateTypeDefs: true, + validateResolvers: true, + validateDuplicateRelationshipFields: true, +}; + class Neo4jGraphQL { typeDefs: TypeSource; resolvers?: IExecutableSchemaDefinition["resolvers"]; @@ -298,16 +310,16 @@ class Neo4jGraphQL { return new Promise((resolve) => { const document = this.getDocument(this.typeDefs); - const { validateTypeDefs, validateResolvers } = this.parseStartupValidationConfig(); + const validationConfig = this.parseStartupValidationConfig(); - validateDocument(document, validateTypeDefs); + validateDocument(document, validationConfig); this.setSchemaModel(document); const { nodes, relationships, typeDefs, resolvers } = makeAugmentedSchema(document, { features: this.features, enableRegex: this.config?.enableRegex, - validateResolvers, + validateResolvers: validationConfig.validateResolvers, generateSubscriptions: Boolean(this.plugins?.subscriptions), callbacks: this.config.callbacks, userCustomResolvers: this.resolvers, @@ -336,16 +348,16 @@ class Neo4jGraphQL { const { directives, types } = subgraph.getValidationDefinitions(); - const { validateTypeDefs, validateResolvers } = this.parseStartupValidationConfig(); + const validationConfig = this.parseStartupValidationConfig(); - validateDocument(document, validateTypeDefs, directives, types); + validateDocument(document, validationConfig, directives, types); this.setSchemaModel(document); const { nodes, relationships, typeDefs, resolvers } = makeAugmentedSchema(document, { features: this.features, enableRegex: this.config?.enableRegex, - validateResolvers, + validateResolvers: validationConfig.validateResolvers, generateSubscriptions: Boolean(this.plugins?.subscriptions), callbacks: this.config.callbacks, userCustomResolvers: this.resolvers, @@ -377,32 +389,28 @@ class Neo4jGraphQL { return this.addDefaultFieldResolvers(schema); } - private parseStartupValidationConfig(): { - validateTypeDefs: boolean; - validateResolvers: boolean; - } { - let validateTypeDefs = true; - let validateResolvers = true; + private parseStartupValidationConfig(): ValidationConfig { + const validationConfig = defaultValidationConfig; if (this.config?.startupValidation === false) { return { validateTypeDefs: false, validateResolvers: false, + validateDuplicateRelationshipFields: false, }; } // TODO - remove in 4.0.0 when skipValidateTypeDefs is removed - if (this.config?.skipValidateTypeDefs === true) validateTypeDefs = false; + if (this.config?.skipValidateTypeDefs === true) validationConfig.validateTypeDefs = false; if (typeof this.config?.startupValidation === "object") { - if (this.config?.startupValidation.typeDefs === false) validateTypeDefs = false; - if (this.config?.startupValidation.resolvers === false) validateResolvers = false; + if (this.config?.startupValidation.typeDefs === false) validationConfig.validateTypeDefs = false; + if (this.config?.startupValidation.resolvers === false) validationConfig.validateResolvers = false; + if (this.config?.startupValidation.noDuplicateRelationshipFields === false) + validationConfig.validateDuplicateRelationshipFields = false; } - return { - validateTypeDefs, - validateResolvers, - }; + return validationConfig; } private pluginsSetup(): Promise { diff --git a/packages/graphql/src/schema/validation/validate-document.ts b/packages/graphql/src/schema/validation/validate-document.ts index ad323c60ca..0e45e2af2b 100644 --- a/packages/graphql/src/schema/validation/validate-document.ts +++ b/packages/graphql/src/schema/validation/validate-document.ts @@ -40,7 +40,9 @@ import { PointDistance } from "../../graphql/input-objects/PointDistance"; import { CartesianPointDistance } from "../../graphql/input-objects/CartesianPointDistance"; import { RESERVED_TYPE_NAMES } from "../../constants"; import { isRootType } from "../../utils/is-root-type"; -import { validateSchemaAugments } from "./validate-schema-augments"; +import { validateSchemaCustomizations } from "./validate-schema-customizations"; +import type { ValidationConfig } from "../../classes/Neo4jGraphQL"; +import { defaultValidationConfig } from "../../classes/Neo4jGraphQL"; function filterDocument(document: DocumentNode): DocumentNode { const nodeNames = document.definitions @@ -187,19 +189,19 @@ function getBaseSchema( function validateDocument( document: DocumentNode, - validateTypeDefs = true, + validationConfig: ValidationConfig = defaultValidationConfig, additionalDirectives: Array = [], additionalTypes: Array = [] ): void { - const schema = getBaseSchema(document, validateTypeDefs, additionalDirectives, additionalTypes); - if (validateTypeDefs) { + const schema = getBaseSchema(document, validationConfig.validateTypeDefs, additionalDirectives, additionalTypes); + if (validationConfig.validateTypeDefs) { const errors = validateSchema(schema); const filteredErrors = errors.filter((e) => e.message !== "Query root type must be provided."); if (filteredErrors.length) { throw new Error(filteredErrors.join("\n")); } } - validateSchemaAugments(document, schema); + validateSchemaCustomizations(document, schema, validationConfig); } export default validateDocument; diff --git a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts index 5ae2e2a674..eccbbadf7d 100644 --- a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts +++ b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts @@ -37,7 +37,7 @@ describe("validateDuplicateRelationshipFields", () => { `; expect(() => validateDocument(doc)).toThrow( - "Multiple relationship fields with the same type and direction may not have the same relationship type" + "Multiple relationship fields with the same type and direction may not have the same relationship type." ); }); diff --git a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.ts b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.ts index 67de97835b..8e22f6c037 100644 --- a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.ts +++ b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.ts @@ -56,7 +56,7 @@ export function validateDuplicateRelationshipFields(objType: ObjectTypeDefinitio if (relationshipUsages.has(`${typeMeta.name}__${typeArg.value.value}__${directionArg.value.value}`)) { throw new Error( - "Multiple relationship fields with the same type and direction may not have the same relationship type" + "Multiple relationship fields with the same type and direction may not have the same relationship type." ); } diff --git a/packages/graphql/src/types.ts b/packages/graphql/src/types.ts index 5a805b15de..2d4ac5f892 100644 --- a/packages/graphql/src/types.ts +++ b/packages/graphql/src/types.ts @@ -366,6 +366,7 @@ export interface CypherQueryOptions { export interface StartupValidationOptions { typeDefs?: boolean; resolvers?: boolean; + noDuplicateRelationshipFields?: boolean; } /** From ceb30d2c70aca6ac230589360789162c219e7221 Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Wed, 15 Mar 2023 09:44:04 +0100 Subject: [PATCH 05/13] docs: add changeset --- .changeset/khaki-experts-decide.md | 34 ++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 .changeset/khaki-experts-decide.md diff --git a/.changeset/khaki-experts-decide.md b/.changeset/khaki-experts-decide.md new file mode 100644 index 0000000000..101a3c7d33 --- /dev/null +++ b/.changeset/khaki-experts-decide.md @@ -0,0 +1,34 @@ +--- +"@neo4j/graphql": major +--- + +It was possible to define schemas with types that have multiple relationship fields connected by the same type of relationships. Instances of this scenario are now detected during schema generation and an error is thrown so developers are informed to remedy the type definitions. + +An example of what is now considered invalid with these checks: + +```graphql +type Team { + player1: Person! @relationship(type: "PLAYS_IN", direction: IN) + player2: Person! @relationship(type: "PLAYS_IN", direction: IN) + backupPlayers: [Person!]! @relationship(type: "PLAYS_IN", direction: IN) +} + +type Person { + teams: [Team!]! @relationship(type: "PLAYS_IN", direction: OUT) +} +``` + +In this example, there are multiple fields in the `Team` type which have the same `Person` type, the same `@relationship` type and ("PLAYS_IN") direction (IN). This is an issue when returning data from the database, as there would be no difference between `player1`, `player2` and `backupPlayers`. Selecting these fields would then return the same data. + +To disable checks for duplicate relationship fields, the `noDuplicateRelationshipFields` config option should be used: + +```ts +const neoSchema = new Neo4jGraphQL({ + typeDefs, + config: { + startupValidation: { + noDuplicateRelationshipFields: false, + }, + }, +}); +``` From 21178e4dbbeeec5df95f7c482b62bc3a81db097a Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Wed, 15 Mar 2023 16:30:28 +0100 Subject: [PATCH 06/13] docs: update migration guide --- .../ROOT/pages/guides/v4-migration/index.adoc | 53 ++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/guides/v4-migration/index.adoc b/docs/modules/ROOT/pages/guides/v4-migration/index.adoc index 00fe6fa045..945c8d0775 100644 --- a/docs/modules/ROOT/pages/guides/v4-migration/index.adoc +++ b/docs/modules/ROOT/pages/guides/v4-migration/index.adoc @@ -526,12 +526,48 @@ type User { } ---- +=== Duplicate relationship fields are now checked for + +It was possible to define schemas with types that have multiple relationship fields connected by the same type of relationships. Instances of this scenario are now detected during schema generation and an error is thrown so developers are informed to remedy the type definitions. + +An example of what is now considered invalid with these checks: + +[source, graphql, indent=0] +---- +type Team { + player1: Person! @relationship(type: "PLAYS_IN", direction: IN) + player2: Person! @relationship(type: "PLAYS_IN", direction: IN) + backupPlayers: [Person!]! @relationship(type: "PLAYS_IN", direction: IN) +} + +type Person { + teams: [Team!]! @relationship(type: "PLAYS_IN", direction: OUT) +} +---- + +In this example, there are multiple fields in the `Team` type which have the same `Person` type, the same `@relationship` type and ("PLAYS_IN") direction (IN). This is an issue when returning data from the database, as there would be no difference between `player1`, `player2` and `backupPlayers`. Selecting these fields would then return the same data. + +To disable checks for duplicate relationship fields, the `noDuplicateRelationshipFields` config option should be used: + +[source, javascript, indent=0] +---- +const neoSchema = new Neo4jGraphQL({ + typeDefs, + config: { + startupValidation: { + noDuplicateRelationshipFields: false, + }, + }, +}); +---- + + == Miscellaneous changes [[startup-validation]] === Startup validation -In version 4.0.0, startup xref::guides/v4-migration/index.adoc#customResolver-checks[checks for custom resolvers] have been added. As a result, a new configuration option has been added that can disable these checks. +In version 4.0.0, startup xref::guides/v4-migration/index.adoc#customResolver-checks[checks for custom resolvers], and checks for duplicate relationship fields have been added. As a result, a new configuration option has been added that can disable these checks. This new option has been combined with the option to `skipValidateTypeDefs`. As a result, `skipValidateTypeDefs` will be removed and replaced by `startupValidation`. To only disable strict type definition validation, the following config option should be used: @@ -562,6 +598,21 @@ const neoSchema = new Neo4jGraphQL({ }) ---- +To only disable checks for duplicate relationship fields, the following config option should be used: + +[source, javascript, indent=0] +---- +const neoSchema = new Neo4jGraphQL({ + typeDefs, + config: { + startupValidation: { + noDuplicateRelationshipFields: false + }, + }, +}) +---- + + To disable all startup checks, the following config option should be used: [source, javascript, indent=0] From eb9b96dfb8c0a30042488dc75cb96624ab0573b4 Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Wed, 15 Mar 2023 16:39:42 +0100 Subject: [PATCH 07/13] refactor: use objects as function parameters --- packages/graphql/src/classes/Neo4jGraphQL.ts | 4 +- .../validation/validate-document.test.ts | 62 +++++++++---------- .../schema/validation/validate-document.ts | 43 ++++++++----- ...date-duplicate-relationship-fields.test.ts | 6 +- .../validate-schema-customizations.ts | 14 +++-- 5 files changed, 74 insertions(+), 55 deletions(-) diff --git a/packages/graphql/src/classes/Neo4jGraphQL.ts b/packages/graphql/src/classes/Neo4jGraphQL.ts index 04c025cdd1..31d6264b4c 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -312,7 +312,7 @@ class Neo4jGraphQL { const validationConfig = this.parseStartupValidationConfig(); - validateDocument(document, validationConfig); + validateDocument({ document, validationConfig }); this.setSchemaModel(document); @@ -350,7 +350,7 @@ class Neo4jGraphQL { const validationConfig = this.parseStartupValidationConfig(); - validateDocument(document, validationConfig, directives, types); + validateDocument({ document, validationConfig, additionalDirectives: directives, additionalTypes: types }); this.setSchemaModel(document); diff --git a/packages/graphql/src/schema/validation/validate-document.test.ts b/packages/graphql/src/schema/validation/validate-document.test.ts index 19f0bc7dcf..3ffe593a6f 100644 --- a/packages/graphql/src/schema/validation/validate-document.test.ts +++ b/packages/graphql/src/schema/validation/validate-document.test.ts @@ -29,7 +29,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow('Directive "@coalesce" may not be used on OBJECT.'); + expect(() => validateDocument({ document: doc })).toThrow('Directive "@coalesce" may not be used on OBJECT.'); }); test("should throw an error if a directive is missing an argument", () => { @@ -39,7 +39,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( 'Directive "@coalesce" argument "value" of type "ScalarOrEnum!" is required, but it was not provided.' ); }); @@ -51,7 +51,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow('Unknown type "Unknown".'); + expect(() => validateDocument({ document: doc })).toThrow('Unknown type "Unknown".'); }); test("should throw an error if a user tries to pass in their own Point definition", () => { @@ -66,7 +66,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( 'Type "Point" already exists in the schema. It cannot also be defined in this type definition.' ); }); @@ -80,7 +80,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( 'Type "DateTime" already exists in the schema. It cannot also be defined in this type definition.' ); }); @@ -94,7 +94,7 @@ describe("validateDocument", () => { extend type User @fulltext `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( 'Directive "@fulltext" argument "indexes" of type "[FullTextInput]!" is required, but it was not provided.' ); }); @@ -111,7 +111,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( 'Type "PointInput" already exists in the schema. It cannot also be defined in this type definition.' ); }); @@ -127,7 +127,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( "Interface field UserInterface.age expected but User does not provide it." ); }); @@ -141,7 +141,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( 'Directive "@relationship" already exists in the schema. It cannot be redefined.' ); }); @@ -153,7 +153,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); test("should not throw error on use of internal node input types", () => { @@ -181,7 +181,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); describe("relationshipProperties directive", () => { @@ -202,7 +202,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); test("should throw if used on an object type", () => { @@ -212,7 +212,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( 'Directive "@relationshipProperties" may not be used on OBJECT.' ); }); @@ -224,7 +224,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( 'Directive "@relationshipProperties" may not be used on FIELD_DEFINITION.' ); }); @@ -307,7 +307,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); describe("Github Issue 158", () => { @@ -322,7 +322,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); }); @@ -339,7 +339,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); test("should not throw error on validation of schema if SortDirection used", () => { @@ -354,7 +354,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); }); @@ -487,7 +487,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); }); @@ -509,7 +509,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); }); @@ -520,7 +520,7 @@ describe("validateDocument", () => { name: String @alias } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( 'Directive "@alias" argument "property" of type "String!" is required, but it was not provided.' ); }); @@ -530,7 +530,7 @@ describe("validateDocument", () => { name: String } `; - expect(() => validateDocument(doc)).toThrow('Directive "@alias" may not be used on OBJECT.'); + expect(() => validateDocument({ document: doc })).toThrow('Directive "@alias" may not be used on OBJECT.'); }); test("should not throw when used correctly", () => { const doc = gql` @@ -538,7 +538,7 @@ describe("validateDocument", () => { name: String @alias(property: "dbName") } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); }); @@ -551,7 +551,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( RESERVED_TYPE_NAMES.find((x) => x.regex.test("PageInfo"))?.error ); }); @@ -563,7 +563,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( RESERVED_TYPE_NAMES.find((x) => x.regex.test("NodeConnection"))?.error ); }); @@ -575,7 +575,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( RESERVED_TYPE_NAMES.find((x) => x.regex.test("Node"))?.error ); }); @@ -598,7 +598,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( RESERVED_TYPE_NAMES.find((x) => x.regex.test("PageInfo"))?.error ); }); @@ -619,7 +619,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( RESERVED_TYPE_NAMES.find((x) => x.regex.test("NodeConnection"))?.error ); }); @@ -640,7 +640,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( RESERVED_TYPE_NAMES.find((x) => x.regex.test("Node"))?.error ); }); @@ -655,7 +655,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); }); @@ -675,7 +675,7 @@ describe("validateDocument", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); }); }); diff --git a/packages/graphql/src/schema/validation/validate-document.ts b/packages/graphql/src/schema/validation/validate-document.ts index 0e45e2af2b..e249273b3d 100644 --- a/packages/graphql/src/schema/validation/validate-document.ts +++ b/packages/graphql/src/schema/validation/validate-document.ts @@ -17,6 +17,7 @@ * limitations under the License. */ +import { GraphQLSchema, extendSchema, validateSchema, specifiedDirectives, Kind } from "graphql"; import type { DefinitionNode, DocumentNode, @@ -27,7 +28,6 @@ import type { GraphQLDirective, GraphQLNamedType, } from "graphql"; -import { GraphQLSchema, extendSchema, validateSchema, specifiedDirectives, Kind } from "graphql"; import pluralize from "pluralize"; import * as scalars from "../../graphql/scalars"; import * as directives from "../../graphql/directives"; @@ -161,12 +161,17 @@ function filterDocument(document: DocumentNode): DocumentNode { }; } -function getBaseSchema( - document: DocumentNode, +function getBaseSchema({ + document, validateTypeDefs = true, - additionalDirectives: Array = [], - additionalTypes: Array = [] -): GraphQLSchema { + additionalDirectives = [], + additionalTypes = [], +}: { + document: DocumentNode; + validateTypeDefs: boolean; + additionalDirectives: Array; + additionalTypes: Array; +}): GraphQLSchema { const doc = filterDocument(document); const schemaToExtend = new GraphQLSchema({ @@ -187,13 +192,23 @@ function getBaseSchema( return extendSchema(schemaToExtend, doc, { assumeValid: !validateTypeDefs }); } -function validateDocument( - document: DocumentNode, - validationConfig: ValidationConfig = defaultValidationConfig, - additionalDirectives: Array = [], - additionalTypes: Array = [] -): void { - const schema = getBaseSchema(document, validationConfig.validateTypeDefs, additionalDirectives, additionalTypes); +function validateDocument({ + document, + validationConfig = defaultValidationConfig, + additionalDirectives = [], + additionalTypes = [], +}: { + document: DocumentNode; + validationConfig?: ValidationConfig; + additionalDirectives?: Array; + additionalTypes?: Array; +}): void { + const schema = getBaseSchema({ + document, + validateTypeDefs: validationConfig.validateTypeDefs, + additionalDirectives, + additionalTypes, + }); if (validationConfig.validateTypeDefs) { const errors = validateSchema(schema); const filteredErrors = errors.filter((e) => e.message !== "Query root type must be provided."); @@ -201,7 +216,7 @@ function validateDocument( throw new Error(filteredErrors.join("\n")); } } - validateSchemaCustomizations(document, schema, validationConfig); + validateSchemaCustomizations({ document, schema, validationConfig }); } export default validateDocument; diff --git a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts index eccbbadf7d..ac09adbd0a 100644 --- a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts +++ b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts @@ -36,7 +36,7 @@ describe("validateDuplicateRelationshipFields", () => { } `; - expect(() => validateDocument(doc)).toThrow( + expect(() => validateDocument({ document: doc })).toThrow( "Multiple relationship fields with the same type and direction may not have the same relationship type." ); }); @@ -59,7 +59,7 @@ describe("validateDuplicateRelationshipFields", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); test("should not throw an error if multiple relationship fields in the same type have the same relationship type but have different directions.", () => { @@ -80,6 +80,6 @@ describe("validateDuplicateRelationshipFields", () => { } `; - expect(() => validateDocument(doc)).not.toThrow(); + expect(() => validateDocument({ document: doc })).not.toThrow(); }); }); diff --git a/packages/graphql/src/schema/validation/validate-schema-customizations.ts b/packages/graphql/src/schema/validation/validate-schema-customizations.ts index 4d2bc89c9c..1b9bc99eaa 100644 --- a/packages/graphql/src/schema/validation/validate-schema-customizations.ts +++ b/packages/graphql/src/schema/validation/validate-schema-customizations.ts @@ -23,11 +23,15 @@ import { getDefinitionNodes } from "../get-definition-nodes"; import { validateCustomResolverRequires } from "./validate-custom-resolver-requires"; import { validateDuplicateRelationshipFields } from "./validate-duplicate-relationship-fields"; -export function validateSchemaCustomizations( - document: DocumentNode, - schema: GraphQLSchema, - validationConfig: ValidationConfig -) { +export function validateSchemaCustomizations({ + document, + schema, + validationConfig, +}: { + document: DocumentNode; + schema: GraphQLSchema; + validationConfig: ValidationConfig; +}) { const definitionNodes = getDefinitionNodes(document); for (const objectType of definitionNodes.objectTypes) { From 27ad3817d441e75b81796021a46a3555d206ec45 Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Wed, 15 Mar 2023 17:10:58 +0100 Subject: [PATCH 08/13] fix: shallow copy defaultValidationConfig in parseStartupValidationConfig --- 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 31d6264b4c..83bd0e4db8 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -390,7 +390,7 @@ class Neo4jGraphQL { } private parseStartupValidationConfig(): ValidationConfig { - const validationConfig = defaultValidationConfig; + const validationConfig: ValidationConfig = { ...defaultValidationConfig }; if (this.config?.startupValidation === false) { return { From 43987f418b1598b3ab4f56ab0626299357a9c61a Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Wed, 15 Mar 2023 17:21:51 +0100 Subject: [PATCH 09/13] test: add startup-validation integration tests --- .../startup-validation.int.test.ts | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/packages/graphql/tests/integration/config-options/startup-validation.int.test.ts b/packages/graphql/tests/integration/config-options/startup-validation.int.test.ts index 3eeb4e9fef..80729fe199 100644 --- a/packages/graphql/tests/integration/config-options/startup-validation.int.test.ts +++ b/packages/graphql/tests/integration/config-options/startup-validation.int.test.ts @@ -27,6 +27,8 @@ describe("Startup Validation", () => { const invalidTypeDefsError = 'Type "Point" already exists in the schema.'; const missingCustomResolverError = "Custom resolver for fullName has not been provided"; + const duplicateRelationshipFieldsError = + "Multiple relationship fields with the same type and direction may not have the same relationship type."; const customResolverTypeDefs = ` type User { @@ -66,6 +68,32 @@ describe("Startup Validation", () => { } `; + const invalidDuplicateRelationship = ` + type User { + id: ID! + firstName: String! + lastName: String! + friend1: User! @relationship(type: "FRIENDS_WITH", direction: IN) + friend2: User! @relationship(type: "FRIENDS_WITH", direction: IN) + } + `; + + const invalidAll = ` + type User { + id: ID! + firstName: String! + lastName: String! + fullName: String @customResolver(requires: "firstName lastName") + friend1: User! @relationship(type: "FRIENDS_WITH", direction: IN) + friend2: User! @relationship(type: "FRIENDS_WITH", direction: IN) + } + + type Point { + latitude: Float! + longitude: Float! + } + `; + beforeAll(async () => { neo4j = new Neo4j(); driver = await neo4j.getDriver(); @@ -318,4 +346,52 @@ describe("Startup Validation", () => { await expect(neoSchema.getSchema()).resolves.not.toThrow(); }); + + test("should throw an error for duplicate relationship fields when startupValidation.noDuplicateRelationshipFields is true", async () => { + const neoSchema = new Neo4jGraphQL({ + typeDefs: invalidDuplicateRelationship, + driver, + config: { + startupValidation: { + typeDefs: true, + resolvers: true, + noDuplicateRelationshipFields: true, + }, + }, + }); + + await expect(neoSchema.getSchema()).rejects.toThrow(duplicateRelationshipFieldsError); + }); + + test("should not throw an error for duplicate relationship fields when startupValidation.noDuplicateRelationshipFields is false", async () => { + const neoSchema = new Neo4jGraphQL({ + typeDefs: invalidDuplicateRelationship, + driver, + config: { + startupValidation: { + typeDefs: true, + resolvers: true, + noDuplicateRelationshipFields: false, + }, + }, + }); + + await expect(neoSchema.getSchema()).resolves.not.toThrow(); + }); + + test("should throw no errors when startupValidation.customResolver, startupValidation.typeDefs, and startupValidation.noDuplicateRelationshipFields are false", async () => { + const neoSchema = new Neo4jGraphQL({ + typeDefs: invalidAll, + driver, + config: { + startupValidation: { + resolvers: false, + typeDefs: false, + noDuplicateRelationshipFields: false, + }, + }, + }); + + await expect(neoSchema.getSchema()).resolves.not.toThrow(); + }); }); From 90750e8412ebbeee90ccc1fe781d8578d8427ef2 Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Thu, 16 Mar 2023 15:06:10 +0100 Subject: [PATCH 10/13] test: check for no errors thrown when startupValidation is set to false --- .../config-options/startup-validation.int.test.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/graphql/tests/integration/config-options/startup-validation.int.test.ts b/packages/graphql/tests/integration/config-options/startup-validation.int.test.ts index 80729fe199..350b231f01 100644 --- a/packages/graphql/tests/integration/config-options/startup-validation.int.test.ts +++ b/packages/graphql/tests/integration/config-options/startup-validation.int.test.ts @@ -379,16 +379,12 @@ describe("Startup Validation", () => { await expect(neoSchema.getSchema()).resolves.not.toThrow(); }); - test("should throw no errors when startupValidation.customResolver, startupValidation.typeDefs, and startupValidation.noDuplicateRelationshipFields are false", async () => { + test("should throw no errors when startupValidation false", async () => { const neoSchema = new Neo4jGraphQL({ typeDefs: invalidAll, driver, config: { - startupValidation: { - resolvers: false, - typeDefs: false, - noDuplicateRelationshipFields: false, - }, + startupValidation: false, }, }); From 48568b32f75cd95dd8191ce55d22b33870c2c3d3 Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Thu, 16 Mar 2023 15:21:14 +0100 Subject: [PATCH 11/13] docs: update changeset to point towards migration guide for more info --- .changeset/khaki-experts-decide.md | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/.changeset/khaki-experts-decide.md b/.changeset/khaki-experts-decide.md index 101a3c7d33..2f13dfe2df 100644 --- a/.changeset/khaki-experts-decide.md +++ b/.changeset/khaki-experts-decide.md @@ -18,17 +18,4 @@ type Person { } ``` -In this example, there are multiple fields in the `Team` type which have the same `Person` type, the same `@relationship` type and ("PLAYS_IN") direction (IN). This is an issue when returning data from the database, as there would be no difference between `player1`, `player2` and `backupPlayers`. Selecting these fields would then return the same data. - -To disable checks for duplicate relationship fields, the `noDuplicateRelationshipFields` config option should be used: - -```ts -const neoSchema = new Neo4jGraphQL({ - typeDefs, - config: { - startupValidation: { - noDuplicateRelationshipFields: false, - }, - }, -}); -``` +For more information about this change and how to disable this validation please see the [4.0.0 migration guide](https://neo4j.com/docs/graphql-manual/current/guides/v4-migration/) From 8bf6b0e26e50ca2215cba6ee3766ec85cc21cdfe Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Fri, 17 Mar 2023 08:42:39 +0100 Subject: [PATCH 12/13] test: update test case to match test description --- .../validate-duplicate-relationship-fields.test.ts | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts index ac09adbd0a..380ce1cdb7 100644 --- a/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts +++ b/packages/graphql/src/schema/validation/validate-duplicate-relationship-fields.test.ts @@ -64,19 +64,10 @@ describe("validateDuplicateRelationshipFields", () => { test("should not throw an error if multiple relationship fields in the same type have the same relationship type but have different directions.", () => { const doc = gql` - type Team { - name: String! - player: Person! @relationship(type: "PLAYS_IN", direction: IN) - venue: Venue! @relationship(type: "PLAYS_IN", direction: IN) - } - type Person { name: String! - teams: [Team!]! @relationship(type: "PLAYS_IN", direction: OUT) - } - - type Venue { - location: String! + knows: [Person!]! @relationship(type: "KNOWS", direction: OUT) + knowedBy: [Person!]! @relationship(type: "KNOWS", direction: IN) } `; From d6a1cb689085025fe350f86f9fcc1e61a4fd0c62 Mon Sep 17 00:00:00 2001 From: Michael Webb Date: Fri, 17 Mar 2023 08:52:01 +0100 Subject: [PATCH 13/13] refactor: change to generateSchemaModel and use the return of the method This allows us to remove two possible undefined cases, as we will only pass the defined schemaModel to wrapResolvers and wrapFederationResolvers. --- packages/graphql/src/classes/Neo4jGraphQL.ts | 39 ++++++++++---------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/packages/graphql/src/classes/Neo4jGraphQL.ts b/packages/graphql/src/classes/Neo4jGraphQL.ts index 83bd0e4db8..60ec3a72af 100644 --- a/packages/graphql/src/classes/Neo4jGraphQL.ts +++ b/packages/graphql/src/classes/Neo4jGraphQL.ts @@ -250,17 +250,16 @@ class Neo4jGraphQL { return getNeo4jDatabaseInfo(new Executor(executorConstructorParam)); } - private wrapResolvers(resolvers: NonNullable) { - if (!this.schemaModel) { - throw new Error("Schema Model is not defined"); - } - + private wrapResolvers( + resolvers: NonNullable, + schemaModel: Neo4jGraphQLSchemaModel + ) { const wrapResolverArgs = { driver: this.driver, config: this.config, nodes: this.nodes, relationships: this.relationships, - schemaModel: this.schemaModel, + schemaModel: schemaModel, plugins: this.plugins, }; @@ -275,17 +274,16 @@ class Neo4jGraphQL { return composeResolvers(mergedResolvers, resolversComposition); } - private wrapFederationResolvers(resolvers: NonNullable) { - if (!this.schemaModel) { - throw new Error("Schema Model is not defined"); - } - + private wrapFederationResolvers( + resolvers: NonNullable, + schemaModel: Neo4jGraphQLSchemaModel + ) { const wrapResolverArgs = { driver: this.driver, config: this.config, nodes: this.nodes, relationships: this.relationships, - schemaModel: this.schemaModel, + schemaModel: schemaModel, plugins: this.plugins, }; @@ -298,12 +296,13 @@ class Neo4jGraphQL { return composeResolvers(mergedResolvers, resolversComposition); } - private setSchemaModel(document: DocumentNode) { + private generateSchemaModel(document: DocumentNode): Neo4jGraphQLSchemaModel { // This can be run several times but it will always be the same result, // so we memoize the schemaModel. if (!this.schemaModel) { this.schemaModel = generateModel(document); } + return this.schemaModel; } private generateExecutableSchema(): Promise { @@ -314,8 +313,6 @@ class Neo4jGraphQL { validateDocument({ document, validationConfig }); - this.setSchemaModel(document); - const { nodes, relationships, typeDefs, resolvers } = makeAugmentedSchema(document, { features: this.features, enableRegex: this.config?.enableRegex, @@ -328,8 +325,10 @@ class Neo4jGraphQL { this._nodes = nodes; this._relationships = relationships; + const schemaModel = this.generateSchemaModel(document); + // Wrap the generated and custom resolvers, which adds a context including the schema to every request - const wrappedResolvers = this.wrapResolvers(resolvers); + const wrappedResolvers = this.wrapResolvers(resolvers, schemaModel); const schema = makeExecutableSchema({ typeDefs, @@ -352,8 +351,6 @@ class Neo4jGraphQL { validateDocument({ document, validationConfig, additionalDirectives: directives, additionalTypes: types }); - this.setSchemaModel(document); - const { nodes, relationships, typeDefs, resolvers } = makeAugmentedSchema(document, { features: this.features, enableRegex: this.config?.enableRegex, @@ -370,7 +367,9 @@ class Neo4jGraphQL { // TODO: Move into makeAugmentedSchema, add resolvers alongside other resolvers const referenceResolvers = subgraph.getReferenceResolvers(this._nodes); - const wrappedResolvers = this.wrapResolvers([resolvers, referenceResolvers]); + const schemaModel = this.generateSchemaModel(document); + + const wrappedResolvers = this.wrapResolvers([resolvers, referenceResolvers], schemaModel); const schema = subgraph.buildSchema({ typeDefs, @@ -381,7 +380,7 @@ class Neo4jGraphQL { const subgraphResolvers = getResolversFromSchema(schema); // Wrap the _entities and _service Query resolvers - const wrappedSubgraphResolvers = this.wrapFederationResolvers(subgraphResolvers); + const wrappedSubgraphResolvers = this.wrapFederationResolvers(subgraphResolvers, schemaModel); // Add the wrapped resolvers back to the schema, context will now be populated addResolversToSchema({ schema, resolvers: wrappedSubgraphResolvers, updateResolversInPlace: true });