Restrict multiple relationship fields with same type

.changeset/khaki-experts-decide.md

@@ -0,0 +1,21 @@
+---
+"@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)
+}
+```
+
+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/)

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]

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"];
@@ -238,17 +250,16 @@ class Neo4jGraphQL {
         return getNeo4jDatabaseInfo(new Executor(executorConstructorParam));
     }
 
-    private wrapResolvers(resolvers: NonNullable<IExecutableSchemaDefinition["resolvers"]>) {
-        if (!this.schemaModel) {
-            throw new Error("Schema Model is not defined");
-        }
-
+    private wrapResolvers(
+        resolvers: NonNullable<IExecutableSchemaDefinition["resolvers"]>,
+        schemaModel: Neo4jGraphQLSchemaModel
+    ) {
         const wrapResolverArgs = {
             driver: this.driver,
             config: this.config,
             nodes: this.nodes,
             relationships: this.relationships,
-            schemaModel: this.schemaModel,
+            schemaModel: schemaModel,
             plugins: this.plugins,
         };
 
@@ -263,17 +274,16 @@ class Neo4jGraphQL {
         return composeResolvers(mergedResolvers, resolversComposition);
     }
 
-    private wrapFederationResolvers(resolvers: NonNullable<IExecutableSchemaDefinition["resolvers"]>) {
-        if (!this.schemaModel) {
-            throw new Error("Schema Model is not defined");
-        }
-
+    private wrapFederationResolvers(
+        resolvers: NonNullable<IExecutableSchemaDefinition["resolvers"]>,
+        schemaModel: Neo4jGraphQLSchemaModel
+    ) {
         const wrapResolverArgs = {
             driver: this.driver,
             config: this.config,
             nodes: this.nodes,
             relationships: this.relationships,
-            schemaModel: this.schemaModel,
+            schemaModel: schemaModel,
             plugins: this.plugins,
         };
 
@@ -286,22 +296,27 @@ class Neo4jGraphQL {
         return composeResolvers(mergedResolvers, resolversComposition);
     }
 
+    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<GraphQLSchema> {
         return new Promise((resolve) => {
             const document = this.getDocument(this.typeDefs);
 
-            const { validateTypeDefs, validateResolvers } = this.parseStartupValidationConfig();
-
-            validateDocument(document, validateTypeDefs);
+            const validationConfig = this.parseStartupValidationConfig();
 
-            if (!this.schemaModel) {
-                this.schemaModel = generateModel(document);
-            }
+            validateDocument({ document, validationConfig });
 
             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,
@@ -310,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,
@@ -330,18 +347,14 @@ class Neo4jGraphQL {
 
         const { directives, types } = subgraph.getValidationDefinitions();
 
-        const { validateTypeDefs, validateResolvers } = this.parseStartupValidationConfig();
+        const validationConfig = this.parseStartupValidationConfig();
 
-        validateDocument(document, validateTypeDefs, directives, types);
-
-        if (!this.schemaModel) {
-            this.schemaModel = generateModel(document);
-        }
+        validateDocument({ document, validationConfig, additionalDirectives: directives, additionalTypes: types });
 
         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,
@@ -354,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,
@@ -365,40 +380,36 @@ 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 });
 
         return this.addDefaultFieldResolvers(schema);
     }
 
-    private parseStartupValidationConfig(): {
-        validateTypeDefs: boolean;
-        validateResolvers: boolean;
-    } {
-        let validateTypeDefs = true;
-        let validateResolvers = true;
+    private parseStartupValidationConfig(): ValidationConfig {
+        const validationConfig: 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<void> {

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(