From b4175247285fd20fb8be71bc13e5d6fa0c619887 Mon Sep 17 00:00:00 2001 From: MacondoExpress Date: Mon, 12 Aug 2024 14:53:01 +0100 Subject: [PATCH 1/3] add conflicting properties error on the CreateFactory --- .../queryIRFactory/CreateOperationFactory.ts | 7 + .../utils/raise-on-conflicting-input.ts | 45 +++++ .../alias/conflicting-properties.int.test.ts | 163 ++++++++++++++++++ ...t.ts => cypher-default-values.int.test.ts} | 2 +- .../directives/alias/nodes.int.test.ts | 86 +-------- .../tests/integration/info.int.test.ts | 56 +----- .../unwind-create/unwind-create.int.test.ts | 55 ------ 7 files changed, 218 insertions(+), 196 deletions(-) create mode 100644 packages/graphql/src/api-v6/queryIRFactory/utils/raise-on-conflicting-input.ts create mode 100644 packages/graphql/tests/api-v6/integration/directives/alias/conflicting-properties.int.test.ts rename packages/graphql/tests/integration/{default-values.int.test.ts => cypher-default-values.int.test.ts} (98%) diff --git a/packages/graphql/src/api-v6/queryIRFactory/CreateOperationFactory.ts b/packages/graphql/src/api-v6/queryIRFactory/CreateOperationFactory.ts index f4016a2668..7bd8b24c11 100644 --- a/packages/graphql/src/api-v6/queryIRFactory/CreateOperationFactory.ts +++ b/packages/graphql/src/api-v6/queryIRFactory/CreateOperationFactory.ts @@ -32,6 +32,7 @@ import { V6CreateOperation } from "../queryIR/CreateOperation"; import { ReadOperationFactory } from "./ReadOperationFactory"; import { FactoryParseError } from "./factory-parse-error"; import type { GraphQLTreeCreate, GraphQLTreeCreateInput } from "./resolve-tree-parser/graphql-tree/graphql-tree"; +import { raiseOnConflictingInput } from "./utils/raise-on-conflicting-input"; export class CreateOperationFactory { public schemaModel: Neo4jGraphQLSchemaModel; @@ -72,6 +73,7 @@ export class CreateOperationFactory { entity, }); } + const inputFields = this.getInputFields({ target: targetAdapter, createInput: topLevelCreateInput, @@ -93,10 +95,15 @@ export class CreateOperationFactory { target: ConcreteEntityAdapter; createInput: GraphQLTreeCreateInput[]; }): InputField[] { + // inputFieldsExistence is used to keep track of the fields that have been added to the inputFields array + // as with the unwind clause we define a single tree for multiple inputs + // this is to avoid adding the same field multiple times const inputFieldsExistence = new Set(); const inputFields: InputField[] = []; + inputFields.push(...this.addAutogeneratedFields(target, inputFieldsExistence)); for (const inputItem of createInput) { + raiseOnConflictingInput(inputItem, target.entity); for (const key of Object.keys(inputItem)) { const attribute = getAttribute(target, key); diff --git a/packages/graphql/src/api-v6/queryIRFactory/utils/raise-on-conflicting-input.ts b/packages/graphql/src/api-v6/queryIRFactory/utils/raise-on-conflicting-input.ts new file mode 100644 index 0000000000..59e3f96071 --- /dev/null +++ b/packages/graphql/src/api-v6/queryIRFactory/utils/raise-on-conflicting-input.ts @@ -0,0 +1,45 @@ +/* + * 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 { ConcreteEntity } from "../../../schema-model/entity/ConcreteEntity"; +import type { Relationship } from "../../../schema-model/relationship/Relationship"; +import { FactoryParseError } from "../factory-parse-error"; +import type { GraphQLTreeCreateInput } from "../resolve-tree-parser/graphql-tree/graphql-tree"; + +export function raiseOnConflictingInput( + input: GraphQLTreeCreateInput, // TODO: add Update types as well + entityOrRel: ConcreteEntity | Relationship +): void { + const hash = {}; + const properties = Object.keys(input); + properties.forEach((property) => { + const dbName = entityOrRel.findAttribute(property)?.databaseName; + if (dbName === undefined) { + throw new FactoryParseError(`Impossible to translate property ${property} on entity ${entityOrRel.name}`); + } + if (hash[dbName]) { + throw new FactoryParseError( + `Conflicting modification of ${[hash[dbName], property].map((n) => `[[${n}]]`).join(", ")} on type ${ + entityOrRel.name + }` + ); + } + hash[dbName] = property; + }); +} diff --git a/packages/graphql/tests/api-v6/integration/directives/alias/conflicting-properties.int.test.ts b/packages/graphql/tests/api-v6/integration/directives/alias/conflicting-properties.int.test.ts new file mode 100644 index 0000000000..e8d30c08d9 --- /dev/null +++ b/packages/graphql/tests/api-v6/integration/directives/alias/conflicting-properties.int.test.ts @@ -0,0 +1,163 @@ +/* + * 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 { GraphQLError } from "graphql"; +import type { UniqueType } from "../../../../utils/graphql-types"; +import { TestHelper } from "../../../../utils/tests-helper"; + +describe("conflicting properties", () => { + const testHelper = new TestHelper({ v6Api: true }); + + let typeMovie: UniqueType; + let typeDirector: UniqueType; + + beforeEach(async () => { + typeMovie = testHelper.createUniqueType("Movie"); + typeDirector = testHelper.createUniqueType("Director"); + + const typeDefs = /* GraphQL */ ` + type ${typeDirector} @node { + name: String + nameAgain: String @alias(property: "name") + movies: [${typeMovie}!]! @relationship(direction: OUT, type: "DIRECTED", properties: "Directed") + } + + type Directed @relationshipProperties { + year: Int! + movieYear: Int @alias(property: "year") + } + + type ${typeMovie} @node { + title: String + titleAgain: String @alias(property: "title") + directors: [${typeDirector}!]! @relationship(direction: IN, type: "DIRECTED", properties: "Directed") + } + `; + + await testHelper.initNeo4jGraphQL({ + typeDefs, + }); + }); + + afterEach(async () => { + await testHelper.close(); + }); + + test("Create mutation with alias referring to existing field, include both fields as inputs", async () => { + const userMutation = /* GraphQL */ ` + mutation { + ${typeDirector.operations.create}(input: [{ node: { name: "Tim Burton", nameAgain: "Timmy Burton" }}]) { + ${typeDirector.plural} { + name + nameAgain + } + } + } + `; + + const gqlResult = await testHelper.executeGraphQL(userMutation); + + expect(gqlResult.errors).toBeDefined(); + expect(gqlResult.errors).toHaveLength(1); + expect(gqlResult.errors).toEqual([ + new GraphQLError(`Conflicting modification of [[name]], [[nameAgain]] on type ${typeDirector.name}`), + ]); + expect(gqlResult?.data).toEqual({ + [typeDirector.operations.create]: null, + }); + }); + + test("Create mutation with alias referring to existing field, include only field as inputs", async () => { + const userMutation = /* GraphQL */ ` + mutation { + ${typeDirector.operations.create}(input: [{ node: {name: "Tim Burton"} }]) { + ${typeDirector.plural} { + name + nameAgain + } + } + } + `; + + const gqlResult = await testHelper.executeGraphQL(userMutation); + + expect(gqlResult.errors).toBeUndefined(); + expect(gqlResult?.data).toEqual({ + [typeDirector.operations.create]: { + [typeDirector.plural]: [ + { + name: "Tim Burton", + nameAgain: "Tim Burton", + }, + ], + }, + }); + }); + + test("Create mutation with alias referring to existing field, include only alias field as inputs", async () => { + const userMutation = /* GraphQL */ ` + mutation { + ${typeDirector.operations.create}(input: [{ node: { nameAgain: "Tim Burton" } }]) { + ${typeDirector.plural} { + name + nameAgain + } + } + } + `; + + const gqlResult = await testHelper.executeGraphQL(userMutation); + + expect(gqlResult.errors).toBeUndefined(); + expect(gqlResult?.data).toEqual({ + [typeDirector.operations.create]: { + [typeDirector.plural]: [ + { + name: "Tim Burton", + nameAgain: "Tim Burton", + }, + ], + }, + }); + }); + + test("Create mutation with alias referring to existing field, include both bad and good inputs", async () => { + const userMutation = /* GraphQL */ ` + mutation { + ${typeDirector.operations.create}(input: [{ node: {name: "Tim Burton", nameAgain: "Timmy Burton"} }, { node: { name: "Someone" }}]) { + ${typeDirector.plural} { + name + nameAgain + } + } + } + `; + + const gqlResult = await testHelper.executeGraphQL(userMutation); + + expect(gqlResult.errors).toBeDefined(); + expect(gqlResult.errors).toHaveLength(1); + expect(gqlResult.errors).toEqual([ + new GraphQLError(`Conflicting modification of [[name]], [[nameAgain]] on type ${typeDirector.name}`), + ]); + expect(gqlResult?.data).toEqual({ + [typeDirector.operations.create]: null, + }); + }); +}); diff --git a/packages/graphql/tests/integration/default-values.int.test.ts b/packages/graphql/tests/integration/cypher-default-values.int.test.ts similarity index 98% rename from packages/graphql/tests/integration/default-values.int.test.ts rename to packages/graphql/tests/integration/cypher-default-values.int.test.ts index 79eb832a1d..4c4505255f 100644 --- a/packages/graphql/tests/integration/default-values.int.test.ts +++ b/packages/graphql/tests/integration/cypher-default-values.int.test.ts @@ -21,7 +21,7 @@ import { generate } from "randomstring"; import type { UniqueType } from "../utils/graphql-types"; import { TestHelper } from "../utils/tests-helper"; -describe("Default values", () => { +describe("@cypher default values", () => { const testHelper = new TestHelper(); let Movie: UniqueType; diff --git a/packages/graphql/tests/integration/directives/alias/nodes.int.test.ts b/packages/graphql/tests/integration/directives/alias/nodes.int.test.ts index b912955f8c..f6efa1e5d4 100644 --- a/packages/graphql/tests/integration/directives/alias/nodes.int.test.ts +++ b/packages/graphql/tests/integration/directives/alias/nodes.int.test.ts @@ -58,92 +58,8 @@ describe("@alias directive", () => { await testHelper.close(); }); - test("Create mutation with alias referring to existing field, include both fields as inputs", async () => { - const userMutation = ` - mutation { - ${typeDirector.operations.create}(input: [{ name: "Tim Burton", nameAgain: "Timmy Burton" }]) { - ${typeDirector.plural} { - name - nameAgain - } - } - } - `; - - const gqlResult = await testHelper.executeGraphQL(userMutation); - - expect(gqlResult.errors).toBeDefined(); - expect(gqlResult.errors).toHaveLength(1); - expect(gqlResult.errors?.[0]?.message).toBe( - `Conflicting modification of [[name]], [[nameAgain]] on type ${typeDirector.name}` - ); - expect(gqlResult?.data?.[typeDirector.operations.create]?.[typeDirector.plural]).toBeUndefined(); - }); - test("Create mutation with alias referring to existing field, include only field as inputs", async () => { - const userMutation = ` - mutation { - ${typeDirector.operations.create}(input: [{ name: "Tim Burton" }]) { - ${typeDirector.plural} { - name - nameAgain - } - } - } - `; - - const gqlResult = await testHelper.executeGraphQL(userMutation); - - expect(gqlResult.errors).toBeUndefined(); - expect(gqlResult?.data?.[typeDirector.operations.create]?.[typeDirector.plural]).toEqual([ - { - name: "Tim Burton", - nameAgain: "Tim Burton", - }, - ]); - }); - test("Create mutation with alias referring to existing field, include only alias field as inputs", async () => { - const userMutation = ` - mutation { - ${typeDirector.operations.create}(input: [{ nameAgain: "Timmy Burton" }]) { - ${typeDirector.plural} { - name - nameAgain - } - } - } - `; - const gqlResult = await testHelper.executeGraphQL(userMutation); - - expect(gqlResult.errors).toBeUndefined(); - expect(gqlResult?.data?.[typeDirector.operations.create]?.[typeDirector.plural]).toEqual([ - { - name: "Timmy Burton", - nameAgain: "Timmy Burton", - }, - ]); - }); - test("Create mutation with alias referring to existing field, include both bad and good inputs", async () => { - const userMutation = ` - mutation { - ${typeDirector.operations.create}(input: [{ name: "Tim Burton", nameAgain: "Timmy Burton" }, { name: "Someone" }]) { - ${typeDirector.plural} { - name - nameAgain - } - } - } - `; - - const gqlResult = await testHelper.executeGraphQL(userMutation); - - expect(gqlResult.errors).toBeDefined(); - expect(gqlResult.errors).toHaveLength(1); - expect(gqlResult.errors?.[0]?.message).toBe( - `Conflicting modification of [[name]], [[nameAgain]] on type ${typeDirector.name}` - ); - expect(gqlResult?.data?.[typeDirector.operations.create]?.[typeDirector.plural]).toBeUndefined(); - }); + test("Create mutation with alias on connection referring to existing field, include only field as inputs", async () => { const userMutation = ` mutation { diff --git a/packages/graphql/tests/integration/info.int.test.ts b/packages/graphql/tests/integration/info.int.test.ts index e808c238a5..d69e6040da 100644 --- a/packages/graphql/tests/integration/info.int.test.ts +++ b/packages/graphql/tests/integration/info.int.test.ts @@ -17,6 +17,7 @@ * limitations under the License. */ +import { afterEach } from "node:test"; import { generate } from "randomstring"; import type { UniqueType } from "../utils/graphql-types"; import { TestHelper } from "../utils/tests-helper"; @@ -24,70 +25,15 @@ import { TestHelper } from "../utils/tests-helper"; describe("info", () => { const testHelper = new TestHelper(); let Movie: UniqueType; - let Actor: UniqueType; beforeEach(() => { Movie = testHelper.createUniqueType("Movie"); - Actor = testHelper.createUniqueType("Actor"); }); afterEach(async () => { await testHelper.close(); }); - test("should return info from a create mutation", async () => { - const typeDefs = ` - type ${Actor} { - name: String! - } - - type ${Movie} { - title: String! - actors: [${Actor}!]! @relationship(type: "ACTED_IN", direction: IN) - } - `; - - await testHelper.initNeo4jGraphQL({ typeDefs }); - - const title = generate({ - charset: "alphabetic", - }); - const name = generate({ - charset: "alphabetic", - }); - - const query = ` - mutation($title: String!, $name: String!) { - ${Movie.operations.create}(input: [{ title: $title, actors: { create: [{ node: { name: $name } }] } }]) { - info { - bookmark - nodesCreated - relationshipsCreated - } - ${Movie.plural} { - title - actors { - name - } - } - } - } - `; - - const gqlResult = await testHelper.executeGraphQL(query, { - variableValues: { title, name }, - }); - - expect(gqlResult.errors).toBeFalsy(); - - expect(typeof (gqlResult?.data as any)?.[Movie.operations.create].info.bookmark).toBe("string"); - expect((gqlResult?.data as any)?.[Movie.operations.create].info.nodesCreated).toBe(2); - expect((gqlResult?.data as any)?.[Movie.operations.create].info.relationshipsCreated).toBe(1); - expect((gqlResult?.data as any)?.[Movie.operations.create][Movie.plural]).toEqual([ - { title, actors: [{ name }] }, - ]); - }); - test("should return info from a delete mutation", async () => { const typeDefs = ` type ${Movie} { diff --git a/packages/graphql/tests/integration/unwind-create/unwind-create.int.test.ts b/packages/graphql/tests/integration/unwind-create/unwind-create.int.test.ts index 7b760480b9..4f7daa8ae8 100644 --- a/packages/graphql/tests/integration/unwind-create/unwind-create.int.test.ts +++ b/packages/graphql/tests/integration/unwind-create/unwind-create.int.test.ts @@ -31,61 +31,6 @@ describe("unwind-create", () => { await testHelper.close(); }); - test("should create a batch of movies", async () => { - const Movie = new UniqueType("Movie"); - - const typeDefs = ` - type ${Movie} { - id: ID! - } - `; - - await testHelper.initNeo4jGraphQL({ typeDefs }); - - const id = generate({ - charset: "alphabetic", - }); - - const id2 = generate({ - charset: "alphabetic", - }); - - const query = ` - mutation($id: ID!, $id2: ID!) { - ${Movie.operations.create}(input: [{ id: $id }, {id: $id2 }]) { - ${Movie.plural} { - id - } - } - } - `; - - const gqlResult = await testHelper.executeGraphQL(query, { - variableValues: { id, id2 }, - }); - - expect(gqlResult.errors).toBeFalsy(); - - expect(gqlResult?.data?.[Movie.operations.create]?.[Movie.plural]).toEqual( - expect.arrayContaining([{ id }, { id: id2 }]) - ); - - const reFind = await testHelper.executeCypher( - ` - MATCH (m:${Movie}) - RETURN m - `, - {} - ); - const records = reFind.records.map((record) => record.toObject()); - expect(records).toEqual( - expect.arrayContaining([ - { m: expect.objectContaining({ properties: { id } }) }, - { m: expect.objectContaining({ properties: { id: id2 } }) }, - ]) - ); - }); - test("should create a batch of movies with nested actors", async () => { const Movie = new UniqueType("Movie"); const Actor = new UniqueType("Actor"); From 28f339ad877787fd6f1c9ad5f88673acfcf53de3 Mon Sep 17 00:00:00 2001 From: MacondoExpress Date: Tue, 13 Aug 2024 16:13:37 +0100 Subject: [PATCH 2/3] initialiaze inputFields with autogeneratedFields --- .../src/api-v6/queryIRFactory/CreateOperationFactory.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/graphql/src/api-v6/queryIRFactory/CreateOperationFactory.ts b/packages/graphql/src/api-v6/queryIRFactory/CreateOperationFactory.ts index 7bd8b24c11..fcfe922fd3 100644 --- a/packages/graphql/src/api-v6/queryIRFactory/CreateOperationFactory.ts +++ b/packages/graphql/src/api-v6/queryIRFactory/CreateOperationFactory.ts @@ -99,9 +99,8 @@ export class CreateOperationFactory { // as with the unwind clause we define a single tree for multiple inputs // this is to avoid adding the same field multiple times const inputFieldsExistence = new Set(); - const inputFields: InputField[] = []; + const inputFields: InputField[] = this.addAutogeneratedFields(target, inputFieldsExistence); - inputFields.push(...this.addAutogeneratedFields(target, inputFieldsExistence)); for (const inputItem of createInput) { raiseOnConflictingInput(inputItem, target.entity); for (const key of Object.keys(inputItem)) { From dd49184e07142a15a1dcc6b353a82ec06a7f1f7b Mon Sep 17 00:00:00 2001 From: MacondoExpress Date: Tue, 13 Aug 2024 16:16:30 +0100 Subject: [PATCH 3/3] update info.int.test --- packages/graphql/tests/integration/info.int.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/graphql/tests/integration/info.int.test.ts b/packages/graphql/tests/integration/info.int.test.ts index d69e6040da..45b88a1176 100644 --- a/packages/graphql/tests/integration/info.int.test.ts +++ b/packages/graphql/tests/integration/info.int.test.ts @@ -17,7 +17,6 @@ * limitations under the License. */ -import { afterEach } from "node:test"; import { generate } from "randomstring"; import type { UniqueType } from "../utils/graphql-types"; import { TestHelper } from "../utils/tests-helper";