diff --git a/.changeset/thin-guests-fail.md b/.changeset/thin-guests-fail.md new file mode 100644 index 0000000000..ed27b2e6d8 --- /dev/null +++ b/.changeset/thin-guests-fail.md @@ -0,0 +1,18 @@ +--- +"@neo4j/graphql": major +--- + +Throws an error when the same field is updated multiple times on same update operation. + +For example: + +```graphql +mutation { + updateMovies(update: { tags_POP: 1, tags_PUSH: "d" }) { + movies { + title + tags + } + } +} +``` diff --git a/packages/graphql/src/translate/create-update-and-params.ts b/packages/graphql/src/translate/create-update-and-params.ts index 83180b8872..1f4b203b80 100644 --- a/packages/graphql/src/translate/create-update-and-params.ts +++ b/packages/graphql/src/translate/create-update-and-params.ts @@ -22,7 +22,6 @@ import pluralize from "pluralize"; import type { Node, Relationship } from "../classes"; import { Neo4jGraphQLError } from "../classes"; import type { CallbackBucket } from "../classes/CallbackBucket"; -import { META_CYPHER_VARIABLE, META_OLD_PROPS_CYPHER_VARIABLE } from "../constants"; import type { BaseField } from "../types"; import type { Neo4jGraphQLTranslationContext } from "../types/neo4j-graphql-translation-context"; import { caseWhere } from "../utils/case-where"; @@ -45,8 +44,6 @@ import createDeleteAndParams from "./create-delete-and-params"; import createDisconnectAndParams from "./create-disconnect-and-params"; import { createRelationshipValidationString } from "./create-relationship-validation-string"; import createSetRelationshipProperties from "./create-set-relationship-properties"; -import { createEventMeta } from "./subscriptions/create-event-meta"; -import { filterMetaVariable } from "./subscriptions/filter-meta-variable"; import { addCallbackAndSetParam } from "./utils/callback-utils"; import { getAuthorizationStatements } from "./utils/get-authorization-statements"; import { indentBlock } from "./utils/indent-block"; @@ -770,26 +767,3 @@ export default function createUpdateAndParams({ function validateNonNullProperty(res: Res, varName: string, field: BaseField) { res.meta.preArrayMethodValidationStrs.push([`${varName}.${field.dbPropertyName}`, `${field.dbPropertyName}`]); } - -function wrapInSubscriptionsMetaCall({ - statements, - nodeVariable, - typename, - withVars, -}: { - statements: string[]; - nodeVariable: string; - typename: string; - withVars: string[]; -}): string[] { - const updateMetaVariable = "update_meta"; - const preCallWith = `WITH ${nodeVariable} { .* } AS ${META_OLD_PROPS_CYPHER_VARIABLE}, ${withVars.join(", ")}`; - - const callBlock = ["WITH *", ...statements, `RETURN ${META_CYPHER_VARIABLE} as ${updateMetaVariable}`]; - const postCallWith = `WITH *, ${updateMetaVariable} as ${META_CYPHER_VARIABLE}`; - - const eventMeta = createEventMeta({ event: "update", nodeVariable, typename }); - const eventMetaWith = `WITH ${filterMetaVariable(withVars).join(", ")}, ${eventMeta}`; - - return [preCallWith, "CALL {", ...indentBlock(callBlock), "}", postCallWith, eventMetaWith]; -} diff --git a/packages/graphql/src/translate/queryAST/factory/parsers/parse-mutation-field.ts b/packages/graphql/src/translate/queryAST/factory/parsers/parse-mutation-field.ts new file mode 100644 index 0000000000..5c6e4d3086 --- /dev/null +++ b/packages/graphql/src/translate/queryAST/factory/parsers/parse-mutation-field.ts @@ -0,0 +1,34 @@ +/* + * 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. + */ + +type MutationOperation = "PUSH" | "POP" | "ADD" | "SUBTRACT" | "MULTIPLY" | "DIVIDE" | "INCREMENT" | "DECREMENT"; + +export type MutationRegexGroups = { + fieldName: string; + operator: MutationOperation | undefined; +}; + +const mutationRegEx = + /(?[_A-Za-z]\w*?)(?:_(?PUSH|POP|ADD|SUBTRACT|MULTIPLY|DIVIDE|INCREMENT|DECREMENT))?$/; + +export function parseMutationField(field: string): MutationRegexGroups { + const match = mutationRegEx.exec(field); + + return match?.groups as MutationRegexGroups; +} diff --git a/packages/graphql/src/utils/find-conflicting-properties.ts b/packages/graphql/src/utils/find-conflicting-properties.ts index 6af697c361..eee6c3b5e9 100644 --- a/packages/graphql/src/utils/find-conflicting-properties.ts +++ b/packages/graphql/src/utils/find-conflicting-properties.ts @@ -18,6 +18,7 @@ */ import type { Node } from "../classes"; +import { parseMutationField } from "../translate/queryAST/factory/parsers/parse-mutation-field"; import mapToDbProperty from "./map-to-db-property"; /* returns conflicting mutation input properties */ @@ -31,16 +32,18 @@ export function findConflictingProperties({ if (!input) { return []; } - const dbPropertiesToInputFieldNames: Record = Object.keys(input).reduce((acc, fieldName) => { + const dbPropertiesToInputFieldNames: Record = Object.keys(input).reduce((acc, rawField) => { + const { fieldName } = parseMutationField(rawField); + const dbName = mapToDbProperty(node, fieldName); // some input fields (eg relation fields) have no corresponding db name in the map if (!dbName) { return acc; } if (acc[dbName]) { - acc[dbName].push(fieldName); + acc[dbName].push(rawField); } else { - acc[dbName] = [fieldName]; + acc[dbName] = [rawField]; } return acc; }, {}); diff --git a/packages/graphql/tests/integration/array-methods/array-pop-errors.int.test.ts b/packages/graphql/tests/integration/array-methods/array-pop-errors.int.test.ts index a9069cd588..3dd50e6714 100644 --- a/packages/graphql/tests/integration/array-methods/array-pop-errors.int.test.ts +++ b/packages/graphql/tests/integration/array-methods/array-pop-errors.int.test.ts @@ -17,7 +17,7 @@ * limitations under the License. */ -import type { GraphQLError } from "graphql"; +import { GraphQLError } from "graphql"; import { gql } from "graphql-tag"; import { IncomingMessage } from "http"; import { Socket } from "net"; @@ -253,12 +253,9 @@ describe("array-pop-errors", () => { const gqlResult = await testHelper.executeGraphQL(update); - expect(gqlResult.errors).toBeDefined(); - expect( - (gqlResult.errors as GraphQLError[]).some((el) => - el.message.includes("Cannot mutate the same field multiple times in one Mutation") - ) - ).toBeTruthy(); + expect(gqlResult.errors).toEqual([ + new GraphQLError(`Conflicting modification of [[tags]], [[tags_POP]] on type ${typeMovie}`), + ]); expect(gqlResult.data).toBeNull(); }); diff --git a/packages/graphql/tests/integration/array-methods/array-push-errors.int.test.ts b/packages/graphql/tests/integration/array-methods/array-push-errors.int.test.ts index 34bda0f76f..03af43247d 100644 --- a/packages/graphql/tests/integration/array-methods/array-push-errors.int.test.ts +++ b/packages/graphql/tests/integration/array-methods/array-push-errors.int.test.ts @@ -17,7 +17,7 @@ * limitations under the License. */ -import type { GraphQLError } from "graphql"; +import { GraphQLError } from "graphql"; import { gql } from "graphql-tag"; import { IncomingMessage } from "http"; import { Socket } from "net"; @@ -206,12 +206,9 @@ describe("array-push", () => { const gqlResult = await testHelper.executeGraphQL(update); - expect(gqlResult.errors).toBeDefined(); - expect( - (gqlResult.errors as GraphQLError[]).some((el) => - el.message.includes("Cannot mutate the same field multiple times in one Mutation") - ) - ).toBeTruthy(); + expect(gqlResult.errors).toEqual([ + new GraphQLError(`Conflicting modification of [[tags]], [[tags_PUSH]] on type ${typeMovie}`), + ]); expect(gqlResult.data).toBeNull(); }); diff --git a/packages/graphql/tests/integration/issues/5671.int.test.ts b/packages/graphql/tests/integration/issues/5671.int.test.ts new file mode 100644 index 0000000000..fd7d064d08 --- /dev/null +++ b/packages/graphql/tests/integration/issues/5671.int.test.ts @@ -0,0 +1,82 @@ +/* + * 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("https://github.com/neo4j/graphql/issues/5671", () => { + let Movie: UniqueType; + + const testHelper = new TestHelper(); + + beforeEach(async () => { + Movie = testHelper.createUniqueType("Movie"); + + const typeDefs = /* GraphQL */ ` + type ${Movie} @node { + title: String! + tags: [String!]! + rating: Int + } + `; + await testHelper.initNeo4jGraphQL({ + typeDefs, + }); + }); + + afterEach(async () => { + await testHelper.close(); + }); + + test("should fail to update an array in the same update", async () => { + const mutation = /* GraphQL */ ` + mutation { + ${Movie.operations.update}(update: { tags_POP: 1, tags_PUSH: "d" }) { + ${Movie.plural} { + title + tags + } + } + } + `; + + const result = await testHelper.executeGraphQL(mutation); + expect(result.errors).toEqual([ + new GraphQLError(`Conflicting modification of [[tags_POP]], [[tags_PUSH]] on type ${Movie}`), + ]); + }); + + test("should fail to update an int in the same update", async () => { + const mutation = /* GraphQL */ ` + mutation UpdateMovies { + ${Movie.operations.update}(update: { rating_INCREMENT: 5, rating_DECREMENT: 2 }) { + ${Movie.plural} { + title + } + } + } + `; + + const result = await testHelper.executeGraphQL(mutation); + expect(result.errors).toEqual([ + new GraphQLError(`Conflicting modification of [[rating_INCREMENT]], [[rating_DECREMENT]] on type ${Movie}`), + ]); + }); +}); diff --git a/packages/graphql/tests/integration/math.int.test.ts b/packages/graphql/tests/integration/math.int.test.ts index edcfda9fee..447f3138a5 100644 --- a/packages/graphql/tests/integration/math.int.test.ts +++ b/packages/graphql/tests/integration/math.int.test.ts @@ -17,7 +17,7 @@ * limitations under the License. */ -import type { GraphQLError } from "graphql"; +import { GraphQLError } from "graphql"; import { int } from "neo4j-driver"; import { generate } from "randomstring"; import { TestHelper } from "../utils/tests-helper"; @@ -215,12 +215,9 @@ describe("Mathematical operations tests", () => { const gqlResult = await testHelper.executeGraphQL(query, { variableValues: { id, value: 10 }, }); - expect(gqlResult.errors).toBeDefined(); - expect( - (gqlResult.errors as GraphQLError[]).some((el) => - el.message.includes("Cannot mutate the same field multiple times in one Mutation") - ) - ).toBeTruthy(); + expect(gqlResult.errors).toEqual([ + new GraphQLError(`Conflicting modification of [[viewers]], [[viewers_INCREMENT]] on type ${movie}`), + ]); const storedValue = await testHelper.executeCypher( ` MATCH (n:${movie.name} {id: $id}) RETURN n.viewers AS viewers