Merge pull request #6573 from neo4j/6572-changes-of-6488-lead-to-populatedby-directives-within-relationship-properties-not-being-executed-anymore

Fix missing populated by callback in connect

Author: angrykoala

.changeset/large-knives-clean.md

@@ -0,0 +1,5 @@
+---
+"@neo4j/graphql": patch
+---
+
+Fix populatedBy directive used in connect operations

packages/graphql/src/translate/queryAST/factory/OperationFactory.ts

@@ -229,12 +229,19 @@ export class OperationsFactory {
         entity: ConcreteEntityAdapter | InterfaceEntityAdapter | UnionEntityAdapter,
         relationship: RelationshipAdapter,
         input: Record<string, any>[],
-        context: Neo4jGraphQLTranslationContext
+        context: Neo4jGraphQLTranslationContext,
+        callbackBucket: CallbackBucket
     ) {
         if (isConcreteEntity(entity)) {
-            return this.connectFactory.createConnectOperation(entity, relationship, input, context);
+            return this.connectFactory.createConnectOperation(entity, relationship, input, context, callbackBucket);
         } else {
-            return this.connectFactory.createCompositeConnectOperation(entity, relationship, input, context);
+            return this.connectFactory.createCompositeConnectOperation(
+                entity,
+                relationship,
+                input,
+                context,
+                callbackBucket
+            );
         }
     }
 

packages/graphql/src/translate/queryAST/factory/Operations/ConnectFactory.ts

@@ -17,6 +17,7 @@
  * limitations under the License.
  */
 
+import Cypher from "@neo4j/cypher-builder";
 import type { ConcreteEntityAdapter } from "../../../../schema-model/entity/model-adapters/ConcreteEntityAdapter";
 import type { InterfaceEntityAdapter } from "../../../../schema-model/entity/model-adapters/InterfaceEntityAdapter";
 import type { UnionEntityAdapter } from "../../../../schema-model/entity/model-adapters/UnionEntityAdapter";
@@ -30,6 +31,7 @@ import { CompositeConnectOperation } from "../../ast/operations/composite/Compos
 import { CompositeConnectPartial } from "../../ast/operations/composite/CompositeConnectPartial";
 import { ConnectOperation } from "../../ast/operations/ConnectOperation";
 import { NodeSelectionPattern } from "../../ast/selection/SelectionPattern/NodeSelectionPattern";
+import type { CallbackBucket } from "../../utils/callback-bucket";
 import { isConcreteEntity } from "../../utils/is-concrete-entity";
 import { isInterfaceEntity } from "../../utils/is-interface-entity";
 import { raiseAttributeAmbiguity } from "../../utils/raise-attribute-ambiguity";
@@ -46,7 +48,8 @@ export class ConnectFactory {
         entity: ConcreteEntityAdapter,
         relationship: RelationshipAdapter,
         input: Record<string, any>[],
-        context: Neo4jGraphQLTranslationContext
+        context: Neo4jGraphQLTranslationContext,
+        callbackBucket: CallbackBucket
     ): ConnectOperation {
         const connectOP = new ConnectOperation({
             target: entity,
@@ -62,6 +65,7 @@ export class ConnectFactory {
             input,
             connect: connectOP,
             context,
+            callbackBucket,
         });
         return connectOP;
     }
@@ -70,11 +74,18 @@ export class ConnectFactory {
         entity: InterfaceEntityAdapter | UnionEntityAdapter,
         relationship: RelationshipAdapter,
         input: Record<string, any>[],
-        context: Neo4jGraphQLTranslationContext
+        context: Neo4jGraphQLTranslationContext,
+        callbackBucket: CallbackBucket
     ): CompositeConnectOperation {
         const partials: CompositeConnectPartial[] = [];
         for (const concreteEntity of entity.concreteEntities) {
-            const partial = this.createCompositeConnectPartial(concreteEntity, relationship, input, context);
+            const partial = this.createCompositeConnectPartial(
+                concreteEntity,
+                relationship,
+                input,
+                context,
+                callbackBucket
+            );
             partials.push(partial);
         }
 
@@ -88,7 +99,8 @@ export class ConnectFactory {
         entity: ConcreteEntityAdapter,
         relationship: RelationshipAdapter,
         input: Record<string, any>[],
-        context: Neo4jGraphQLTranslationContext
+        context: Neo4jGraphQLTranslationContext,
+        callbackBucket: CallbackBucket
     ): CompositeConnectPartial {
         const connectOP = new CompositeConnectPartial({
             target: entity,
@@ -104,6 +116,7 @@ export class ConnectFactory {
             input,
             connect: connectOP,
             context,
+            callbackBucket,
         });
         return connectOP;
     }
@@ -114,12 +127,14 @@ export class ConnectFactory {
         input,
         connect,
         context,
+        callbackBucket,
     }: {
         target: ConcreteEntityAdapter;
         relationship: RelationshipAdapter;
         input: Record<string, any>[];
         connect: ConnectOperation;
         context: Neo4jGraphQLTranslationContext;
+        callbackBucket: CallbackBucket;
     }) {
         this.addEntityAuthorization({
             entity: target,
@@ -161,7 +176,8 @@ export class ConnectFactory {
                             nestedEntity,
                             nestedRelationship,
                             nestedConnectInputItem,
-                            context
+                            context,
+                            callbackBucket
                         );
 
                         const mutationOperationField = new MutationOperationField(nestedConnectOperation, key);
@@ -188,6 +204,50 @@ export class ConnectFactory {
                 }
             }
         });
+
+        this.addPopulatedByFieldToConnect({
+            connect,
+            input,
+            callbackBucket,
+            relationship,
+        });
+    }
+
+    private addPopulatedByFieldToConnect({
+        connect,
+        input,
+        callbackBucket,
+        relationship,
+    }: {
+        connect: ConnectOperation;
+        input: Record<string, any>;
+        callbackBucket: CallbackBucket;
+        relationship?: RelationshipAdapter;
+    }) {
+        // Using create here because the relationship is created on connect
+        relationship?.getPopulatedByFields("CREATE").forEach((attribute) => {
+            const attachedTo = "relationship";
+            // the param value it's irrelevant as it will be overwritten by the callback function
+            const relCallbackParam = new Cypher.Param("");
+            const relField = new ParamInputField({
+                attribute,
+                attachedTo,
+                inputValue: relCallbackParam,
+            });
+            connect.addField(relField, "relationship");
+
+            const callbackFunctionName = attribute.annotations.populatedBy?.callback;
+            if (!callbackFunctionName) {
+                throw new Error(`PopulatedBy callback not found for attribute ${attribute.name}`);
+            }
+
+            callbackBucket.addCallback({
+                functionName: callbackFunctionName,
+                param: relCallbackParam,
+                parent: input.edge,
+                type: attribute.type,
+            });
+        });
     }
 
     private addEntityAuthorization({

packages/graphql/src/translate/queryAST/factory/Operations/CreateFactory.ts

@@ -354,7 +354,8 @@ export class CreateFactory {
                                         nestedEntity,
                                         nestedRelationship,
                                         nestedConnectInputItem,
-                                        context
+                                        context,
+                                        callbackBucket
                                     );
 
                                 const mutationOperationField = new MutationOperationField(nestedConnectOperation, key);
@@ -497,7 +498,7 @@ export class CreateFactory {
             relationship?.getPopulatedByFields("CREATE").forEach((attribute) => {
                 const attachedTo = "relationship";
                 // the param value it's irrelevant as it will be overwritten by the callback function
-                const relCallbackParam = new Cypher.Param("5678");
+                const relCallbackParam = new Cypher.Param("");
                 const relField = new ParamInputField({
                     attribute,
                     attachedTo,

packages/graphql/tests/integration/issues/6488.int.test.ts

@@ -0,0 +1,137 @@
+/*
+ * 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 { UniqueType } from "../../utils/graphql-types";
+import { TestHelper } from "../../utils/tests-helper";
+
+describe("https://github.com/neo4j/graphql/issues/6572", () => {
+    const testHelper = new TestHelper();
+    let typeDefs: string;
+
+    let MediaAssetDerivative: UniqueType;
+    let MediaAssetDerivativeType: UniqueType;
+
+    beforeAll(async () => {
+        MediaAssetDerivative = testHelper.createUniqueType("MediaAssetDerivative");
+        MediaAssetDerivativeType = testHelper.createUniqueType("MediaAssetDerivativeType");
+
+        typeDefs = /* GraphQL */ `
+            type ${MediaAssetDerivative}
+                @mutation(operations: [CREATE, UPDATE, DELETE])
+                @node
+                @subscription(events: []) {
+                changedAt: DateTime @timestamp(operations: [CREATE, UPDATE])
+                createdAt: DateTime @timestamp(operations: [CREATE])
+                id: String! @settable(onCreate: true, onUpdate: false)
+                isPublic: Boolean! @default(value: false)
+                type: [${MediaAssetDerivativeType}!]!
+                    @relationship(
+                        type: "MEDIAASSETDERIVATIVE_IS_OF_MEDIAASSETDERIVATIVETYPE"
+                        properties: "MediaassetderivativeIsOfMediaassetderivativetypeProperties"
+                        direction: OUT
+                        nestedOperations: [CREATE, UPDATE, DELETE, CONNECT, DISCONNECT]
+                        queryDirection: DIRECTED
+                    )
+                    @settable(onCreate: true, onUpdate: true)
+            }
+
+            type ${MediaAssetDerivativeType}
+                @mutation(operations: [CREATE, UPDATE, DELETE])
+                @node
+                @subscription(events: []) {
+                id: Int! @settable(onCreate: true, onUpdate: false)
+                value: String! @populatedBy(
+                        callback: "AnotherCallback"
+                        operations: [UPDATE]
+                    )
+            }
+
+            type MediaassetderivativeIsOfMediaassetderivativetypeProperties @relationshipProperties {
+                _pkFrom: String!
+                    @populatedBy(
+                        callback: "MEDIAASSETDERIVATIVE_IS_OF_MEDIAASSETDERIVATIVETYPE_Callback"
+                        operations: [CREATE, UPDATE]
+                    )
+                    @settable(onCreate: false, onUpdate: false)
+            }
+        `;
+
+        await testHelper.executeCypher(`
+            CREATE(:${MediaAssetDerivativeType} { id: 1, value: "ShouldNotChange" })
+        `);
+
+        const callback1 = () => Promise.resolve("FROM_1");
+        const callback2 = () => Promise.resolve("New Value");
+
+        await testHelper.initNeo4jGraphQL({
+            typeDefs,
+            features: {
+                populatedBy: {
+                    callbacks: {
+                        MEDIAASSETDERIVATIVE_IS_OF_MEDIAASSETDERIVATIVETYPE_Callback: callback1,
+                        AnotherCallback: callback2,
+                    },
+                },
+            },
+        });
+    });
+
+    afterAll(async () => {
+        await testHelper.close();
+    });
+
+    test("calls populatedBy on create -> connect mutation", async () => {
+        const query = /* GraphQL */ `
+            mutation {
+                ${MediaAssetDerivative.operations.create}(
+                    input: [{ id: "1", type: { connect: [{ where: { node: { id: { eq: 1 } } } }] } }]
+                ) {
+                    __typename
+                }
+            }
+        `;
+
+        const queryResult = await testHelper.executeGraphQL(query);
+        expect(queryResult.errors).toBeUndefined();
+
+        await testHelper
+            .expectRelationship(
+                MediaAssetDerivative,
+                MediaAssetDerivativeType,
+                "MEDIAASSETDERIVATIVE_IS_OF_MEDIAASSETDERIVATIVETYPE"
+            )
+            .toEqual([
+                {
+                    from: {
+                        id: "1",
+                        changedAt: expect.toBeDate(),
+                        createdAt: expect.toBeDate(),
+                        isPublic: false,
+                    },
+                    to: {
+                        id: 1,
+                        value: "ShouldNotChange",
+                    },
+                    relationship: {
+                        _pkFrom: "FROM_1",
+                    },
+                },
+            ]);
+    });
+});

packages/graphql/tests/utils/neo-expect/neo-expect.ts

@@ -81,10 +81,11 @@ export abstract class NeoExpect {
             return value.toNumber();
         }
 
+        if (neo4j.isDateTime(value) || neo4j.isLocalDateTime(value) || neo4j.isDate(value)) {
+            return value.toStandardDate();
+        }
+
         if (
-            neo4j.isLocalDateTime(value) ||
-            neo4j.isDate(value) ||
-            neo4j.isDateTime(value) ||
             neo4j.isDuration(value) ||
             neo4j.isLocalTime(value) ||
             neo4j.isPoint(value) ||

packages/graphql/tests/utils/tests-helper.ts

@@ -67,13 +67,17 @@ export class TestHelper {
     }
 
     /** Expectations over a Node label */
-    public expectNode(type: UniqueType): NeoExpectNode {
+    public expectNode(type: UniqueType | string): NeoExpectNode {
         const executeFn = this.executeCypher.bind(this);
         return new NeoExpectNode(executeFn, type);
     }
 
     /** Expectations over a Relationship */
-    public expectRelationship(from: UniqueType, to: UniqueType, type?: string): NeoExpectRelationship {
+    public expectRelationship(
+        from: UniqueType | string,
+        to: UniqueType | string,
+        type?: string
+    ): NeoExpectRelationship {
         const executeFn = this.executeCypher.bind(this);
         return new NeoExpectRelationship(executeFn, from, to, type);
     }