Skip to content

Commit

Permalink
Fix #5671
Browse files Browse the repository at this point in the history
  • Loading branch information
angrykoala committed Oct 23, 2024
1 parent 8b5d51d commit 8322ec3
Show file tree
Hide file tree
Showing 8 changed files with 152 additions and 50 deletions.
18 changes: 18 additions & 0 deletions .changeset/thin-guests-fail.md
Original file line number Diff line number Diff line change
@@ -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
}
}
}
```
26 changes: 0 additions & 26 deletions packages/graphql/src/translate/create-update-and-params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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";
Expand Down Expand Up @@ -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];
}
Original file line number Diff line number Diff line change
@@ -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 =
/(?<fieldName>[_A-Za-z]\w*?)(?:_(?<operator>PUSH|POP|ADD|SUBTRACT|MULTIPLY|DIVIDE|INCREMENT|DECREMENT))?$/;

export function parseMutationField(field: string): MutationRegexGroups {
const match = mutationRegEx.exec(field);

return match?.groups as MutationRegexGroups;
}
9 changes: 6 additions & 3 deletions packages/graphql/src/utils/find-conflicting-properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -31,16 +32,18 @@ export function findConflictingProperties({
if (!input) {
return [];
}
const dbPropertiesToInputFieldNames: Record<string, string[]> = Object.keys(input).reduce((acc, fieldName) => {
const dbPropertiesToInputFieldNames: Record<string, string[]> = 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;
}, {});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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();
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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();
});

Expand Down
82 changes: 82 additions & 0 deletions packages/graphql/tests/integration/issues/5671.int.test.ts
Original file line number Diff line number Diff line change
@@ -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}`),
]);
});
});
11 changes: 4 additions & 7 deletions packages/graphql/tests/integration/math.int.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 8322ec3

Please sign in to comment.