From f8e26b31a29c80e0d98288906b55e384d92a7de3 Mon Sep 17 00:00:00 2001 From: Andreas Berger Date: Fri, 7 Aug 2020 16:04:34 +0200 Subject: [PATCH] Fix for relationship mutations are not using the correct direction (resolves #98) (#107) This bugfix solves the problem that the mapping was wrong for incoming relations --- .../org/neo4j/graphql/GraphQLExtensions.kt | 27 +- .../handler/projection/ProjectionBase.kt | 5 +- .../handler/relation/CreateRelationHandler.kt | 2 +- .../handler/relation/DeleteRelationHandler.kt | 2 +- .../kotlin/org/neo4j/graphql/CypherTests.kt | 3 + src/test/resources/movie-tests.adoc | 4 +- src/test/resources/relationship-tests.adoc | 323 ++++++++++++++++++ 7 files changed, 344 insertions(+), 22 deletions(-) create mode 100644 src/test/resources/relationship-tests.adoc diff --git a/src/main/kotlin/org/neo4j/graphql/GraphQLExtensions.kt b/src/main/kotlin/org/neo4j/graphql/GraphQLExtensions.kt index 708a698e..f95c6a26 100644 --- 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(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(RELATION_DIRECTION, null)}") } return RelationshipInfo(type, relType, outgoing, - directiveResolver(RELATION_FROM, null), - directiveResolver(RELATION_TO, null)) + relDirective.getArgument(RELATION_FROM, null), + relDirective.getArgument(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 index 8db81a29..40e33a10 100644 --- 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, private val skip: Translator.CypherArgument? = convertArgument(variable, arguments, OFFSET), @@ -395,4 +392,4 @@ open class ProjectionBase { } } } -} \ No newline at end of file +} diff --git a/src/main/kotlin/org/neo4j/graphql/handler/relation/CreateRelationHandler.kt b/src/main/kotlin/org/neo4j/graphql/handler/relation/CreateRelationHandler.kt index 384f1637..a4acd6fe 100644 --- 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 index fa5be2df..b2d78044 100644 --- 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 index 6b9bfabe..e7bdd810 100644 --- 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 index 708668a2..6f505045 100644 --- 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 ----- \ No newline at end of file +---- diff --git a/src/test/resources/relationship-tests.adoc b/src/test/resources/relationship-tests.adoc new file mode 100644 index 00000000..38a013bb --- /dev/null +++ b/src/test/resources/relationship-tests.adoc @@ -0,0 +1,323 @@ +:toc: + += Movie Test TCK + +== Schema + +[source,graphql,schema=true] +---- +type Team { + id: ID! + name: String! + players: [Player!]! @relation(from: "team", to: "player", name: "MEMBER_OF", direction: IN) + memberships: [Membership!]! +} + +type Player { + id: ID! + name: String! + teams: [Team!] @relation(from: "player", to: "team", name: "MEMBER_OF") + memberships: [Membership!]! +} + +type Membership @relation(from: "player", to: "team", name: "MEMBER_OF") { + player: Player! + team: Team! + prop: String +} +---- + +== Mutations + +=== add incoming relationship + +.GraphQL-Query +[source,graphql] +---- +mutation{ + addTeamPlayers(id:1, players: [2]){ + id + } +} +---- + +.Cypher Params +[source,json] +---- +{ + "teamId":1, + "playerPlayers":[2] +} +---- + +.Cypher +[source,cypher] +---- +MATCH (team: Team { id: $teamId }) +MATCH (player: Player) +WHERE player.id IN $playerPlayers +MERGE (team)<-[: MEMBER_OF]-(player) +WITH DISTINCT team AS addTeamPlayers +RETURN addTeamPlayers { .id } AS addTeamPlayers +---- + +=== delete incoming relationship + +.GraphQL-Query +[source,graphql] +---- +mutation{ + deleteTeamPlayers(id:1, players: [2]){ + id + } +} +---- + +.Cypher Params +[source,json] +---- +{ + "teamId":1, + "playerPlayers":[2] +} +---- + +.Cypher +[source,cypher] +---- +MATCH (team: Team { id: $teamId }) +MATCH (player: Player) WHERE player.id IN $playerPlayers +MATCH (team)<-[r: MEMBER_OF]-(player) +DELETE r +WITH DISTINCT team AS deleteTeamPlayers +RETURN deleteTeamPlayers { .id } AS deleteTeamPlayers +---- + +=== add outgoing relationship + +.GraphQL-Query +[source,graphql] +---- +mutation{ + addPlayerTeams(id:1, teams: [2]){ + id + } +} +---- + +.Cypher Params +[source,json] +---- +{ + "playerId":1, + "teamTeams":[2] +} +---- + +.Cypher +[source,cypher] +---- +MATCH (player: Player { id: $playerId }) +MATCH (team: Team) WHERE team.id IN $teamTeams +MERGE (player)-[: MEMBER_OF]->(team) +WITH DISTINCT player AS addPlayerTeams +RETURN addPlayerTeams { .id } AS addPlayerTeams +---- + +=== delete outgoing relationship + +.GraphQL-Query +[source,graphql] +---- +mutation{ + deletePlayerTeams(id:1, teams: [2]){ + id + } +} +---- + +.Cypher Params +[source,json] +---- +{ + "playerId":1, + "teamTeams":[2] +} +---- + +.Cypher +[source,cypher] +---- +MATCH (player: Player { id: $playerId }) +MATCH (team: Team) WHERE team.id IN $teamTeams +MATCH (player)-[r: MEMBER_OF]->(team) +DELETE r +WITH DISTINCT player AS deletePlayerTeams +RETURN deletePlayerTeams { .id } AS deletePlayerTeams +---- + +=== create relationship + +.GraphQL-Query +[source,graphql] +---- +mutation{ + createMembership(player_id: 1, team_id: 2, prop: "foo"){ + prop + } +} +---- + +.Cypher Params +[source,json] +---- +{ + "playerPlayer_id":1, + "teamTeam_id":2, + "createMembershipProp":"foo" +} +---- + +.Cypher +[source,cypher] +---- +MATCH (player: Player { id: $playerPlayer_id }) +MATCH (team: Team { id: $teamTeam_id }) +CREATE (player)-[createMembership: MEMBER_OF { prop: $createMembershipProp }]->(team) +WITH createMembership +RETURN createMembership { .prop } AS createMembership +---- + +== Queries + +=== query incoming node + +.GraphQL-Query +[source,graphql] +---- +{ + team{ + id + players { id } + } +} +---- + +.Cypher Params +[source,json] +---- +{} +---- + +.Cypher +[source,cypher] +---- +MATCH (team: Team) +RETURN team { + .id, + players: [(team)<-[: MEMBER_OF]-(teamPlayers: Player) | teamPlayers { .id }] +} AS team +---- + +=== query outgoing node + +.GraphQL-Query +[source,graphql] +---- +{ + player{ + id + teams { id } + } +} +---- + +.Cypher Params +[source,json] +---- +{} +---- + +.Cypher +[source,cypher] +---- +MATCH (player: Player) +RETURN player { + .id, + teams: [(player)-[: MEMBER_OF]->(playerTeams: Team) | playerTeams { .id }] +} AS player +---- + +=== query incoming relation + +.GraphQL-Query +[source,graphql] +---- +{ + team{ + id + memberships { + player { + id + } + prop + } + } +} +---- + +.Cypher Params +[source,json] +---- +{} +---- + +.Cypher +[source,cypher] +---- +MATCH (team: Team) +RETURN team { + .id, + memberships: [(team)<-[teamMemberships: MEMBER_OF]-(teamMembershipsPlayer: Player) | teamMemberships { + player: teamMembershipsPlayer { .id }, + .prop + }] +} AS team +---- + +=== query outgoing relation + +.GraphQL-Query +[source,graphql] +---- +{ + player{ + id + memberships { + team { + id + } + prop + } + } +} +---- + +.Cypher Params +[source,json] +---- +{} +---- + +.Cypher +[source,cypher] +---- +MATCH (player: Player) +RETURN player { + .id, + memberships: [(player)<-[playerMemberships: MEMBER_OF]-(playerMembershipsPlayer: Player) | playerMemberships { + team: playerMembershipsPlayer { .id }, + .prop + }] +} AS player +----