Fix for relationship mutations are not using the correct direction (r…

…esolves neo4j-graphql#98) This bugfix solves the problem that the mapping was wrong for incoming relations
Andy2003 · Aug 7, 2020 · d7350d3 · d7350d3
1 parent 9602a55
commit d7350d3
Show file tree

Hide file tree

Showing 7 changed files with 344 additions and 22 deletions.
diff --git a/src/main/kotlin/org/neo4j/graphql/GraphQLExtensions.kt b/src/main/kotlin/org/neo4j/graphql/GraphQLExtensions.kt
@@ -49,9 +49,13 @@ fun GraphQLFieldsContainer.relationshipFor(name: String): RelationshipInfo? {
             ?: throw IllegalArgumentException("$name is not defined on ${this.name}")
     val fieldObjectType = field.type.inner() as? GraphQLFieldsContainer ?: return null
 
-    val (relDirective, isRelFromType) = if (isRelationType()) {
+    val (relDirective, inverse) = if (isRelationType()) {
+        val typeName = this.name
         (this as? GraphQLDirectiveContainer)
-            ?.getDirective(DirectiveConstants.RELATION)?.let { it to false }
+            ?.getDirective(DirectiveConstants.RELATION)?.let {
+                // do inverse mapping, if the current type  is the `to` mapping of the relation
+                it to (fieldObjectType.getFieldDefinition(it.getArgument(RELATION_TO, null))?.name == typeName)
+            }
                 ?: throw IllegalStateException("Type ${this.name} needs an @relation directive")
     } else {
         (fieldObjectType as? GraphQLDirectiveContainer)
@@ -60,11 +64,8 @@ fun GraphQLFieldsContainer.relationshipFor(name: String): RelationshipInfo? {
                 ?: throw IllegalStateException("Field $field needs an @relation directive")
     }
 
-    // TODO direction is depending on source/target type
-
-    val relInfo = relDetails(fieldObjectType) { argName, defaultValue -> relDirective.getArgument(argName, defaultValue) }
+    val relInfo = relDetails(fieldObjectType, relDirective)
 
-    val inverse = isRelFromType && fieldObjectType.getFieldDefinition(relInfo.startField)?.name != this.name
     return if (inverse) relInfo.copy(out = relInfo.out?.let { !it }, startField = relInfo.endField, endField = relInfo.startField) else relInfo
 }
 
@@ -121,21 +122,19 @@ fun GraphQLType.ref(): GraphQLType = when (this) {
     else -> GraphQLTypeReference(name)
 }
 
-fun relDetails(type: GraphQLFieldsContainer,
-        // TODO simplify uasage (no more callback)
-        directiveResolver: (name: String, defaultValue: String?) -> String?): RelationshipInfo {
-    val relType = directiveResolver(RELATION_NAME, "")!!
-    val outgoing = when (directiveResolver(RELATION_DIRECTION, null)) {
+fun relDetails(type: GraphQLFieldsContainer, relDirective: GraphQLDirective): RelationshipInfo {
+    val relType = relDirective.getArgument(RELATION_NAME, "")!!
+    val outgoing = when (relDirective.getArgument<String>(RELATION_DIRECTION, null)) {
         RELATION_DIRECTION_IN -> false
         RELATION_DIRECTION_BOTH -> null
         RELATION_DIRECTION_OUT -> true
-        else -> throw IllegalStateException("Unknown direction ${directiveResolver(RELATION_DIRECTION, null)}")
+        else -> throw IllegalStateException("Unknown direction ${relDirective.getArgument<String>(RELATION_DIRECTION, null)}")
     }
     return RelationshipInfo(type,
             relType,
             outgoing,
-            directiveResolver(RELATION_FROM, null),
-            directiveResolver(RELATION_TO, null))
+            relDirective.getArgument<String>(RELATION_FROM, null),
+            relDirective.getArgument<String>(RELATION_TO, null))
 }
 
 data class RelationshipInfo(

diff --git a/src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt b/src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt
@@ -344,9 +344,6 @@ open class ProjectionBase {
         return Cypher(comprehension + slice.query, (where.params + fieldProjection.params + slice.params))
     }
 
-    private fun relDetails(type: GraphQLFieldsContainer, relDirective: GraphQLDirective) =
-            relDetails(type) { name, defaultValue -> relDirective.getArgument(name, defaultValue) }
-
     class SkipLimit(variable: String,
             arguments: List<Argument>,
             private val skip: Translator.CypherArgument? = convertArgument(variable, arguments, OFFSET),
@@ -395,4 +392,4 @@ open class ProjectionBase {
             }
         }
     }
-}
+}
diff --git a/src/main/kotlin/org/neo4j/graphql/handler/relation/CreateRelationHandler.kt b/src/main/kotlin/org/neo4j/graphql/handler/relation/CreateRelationHandler.kt
@@ -68,7 +68,7 @@ class CreateRelationHandler private constructor(
 
         return Cypher("MATCH ${startSelect.query}" +
                 " MATCH ${endSelect.query}" +
-                " MERGE (${relation.startField})-[:${relation.relType.quote()}${properties.query}]->(${relation.endField})" +
+                " MERGE (${relation.startField})${relation.arrows.first}-[:${relation.relType.quote()}${properties.query}]-${relation.arrows.second}(${relation.endField})" +
                 " WITH DISTINCT ${relation.startField} AS $variable" +
                 " RETURN ${mapProjection.query} AS $variable",
                 startSelect.params + endSelect.params + properties.params)

diff --git a/src/main/kotlin/org/neo4j/graphql/handler/relation/DeleteRelationHandler.kt b/src/main/kotlin/org/neo4j/graphql/handler/relation/DeleteRelationHandler.kt
@@ -51,7 +51,7 @@ class DeleteRelationHandler private constructor(
 
         return Cypher("MATCH ${startSelect.query}" +
                 " MATCH ${endSelect.query}" +
-                " MATCH (${relation.startField})-[r:${relation.relType.quote()}]->(${relation.endField})" +
+                " MATCH (${relation.startField})${relation.arrows.first}-[r:${relation.relType.quote()}]-${relation.arrows.second}(${relation.endField})" +
                 " DELETE r" +
                 " WITH DISTINCT ${relation.startField} AS $variable" +
                 " RETURN ${mapProjection.query} AS $variable",

diff --git a/src/test/kotlin/org/neo4j/graphql/CypherTests.kt b/src/test/kotlin/org/neo4j/graphql/CypherTests.kt
@@ -14,6 +14,9 @@ class CypherTests {
     @TestFactory
     fun `filter-tests`() = CypherTestSuite("filter-tests.adoc").run()
 
+    @TestFactory
+    fun `relationship-tests`() = CypherTestSuite("relationship-tests.adoc").run()
+
     @TestFactory
     fun `movie-tests`() = CypherTestSuite("movie-tests.adoc").run()
 

diff --git a/src/test/resources/movie-tests.adoc b/src/test/resources/movie-tests.adoc
@@ -1008,7 +1008,7 @@ mutation {
 ----
 MATCH (from:Genre { name: $fromName })
 MATCH (to:Movie) WHERE to.movieId IN $toMovies
-MERGE (from)-[:IN_GENRE]->(to)
+MERGE (from)<-[:IN_GENRE]-(to)
 WITH DISTINCT from AS addGenreMovies
 RETURN addGenreMovies { .name } AS addGenreMovies
 ----
@@ -1239,4 +1239,4 @@ CREATE (actor:Actor:Person {
 })
 WITH actor
 RETURN actor { .name,born: { year: actor.born.year, month: actor.born.month } } AS actor
-----
+----