Skip to content

Fix for relationship mutations are not using the correct direction #107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 7, 2020
Merged

Fix for relationship mutations are not using the correct direction #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions src/main/kotlin/org/neo4j/graphql/GraphQLExtensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we can simplify this code a bit so it gets easier to understand.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to be careful, this doesn't always work. if you have a rich relationship which has the same type in to/from then we need to use the explicit information from that type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need the information the rel-field then I mean that indicates the correct direction otherwise it's "random" if from/to is matching the type name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see: #45

}
?: throw IllegalStateException("Type ${this.name} needs an @relation directive")
} else {
(fieldObjectType as? GraphQLDirectiveContainer)
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this bit handled now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was the fix for issue #45

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it to https://github.com/Andy2003/neo4j-graphql-java/blob/bugfix/%2398-fix-relationship-mapping/src/main/kotlin/org/neo4j/graphql/GraphQLExtensions.kt#L57 since it is only relevant for rich relations

return if (inverse) relInfo.copy(out = relInfo.out?.let { !it }, startField = relInfo.endField, endField = relInfo.startField) else relInfo
}

Expand Down Expand Up @@ -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(
Expand Down
5 changes: 1 addition & 4 deletions src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -395,4 +392,4 @@ open class ProjectionBase {
}
}
}
}
}
2 changes: 1 addition & 1 deletion src/main/kotlin/org/neo4j/graphql/handler/relation/CreateRelationHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/main/kotlin/org/neo4j/graphql/handler/relation/DeleteRelationHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
3 changes: 3 additions & 0 deletions src/test/kotlin/org/neo4j/graphql/CypherTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
4 changes: 2 additions & 2 deletions src/test/resources/movie-tests.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
----
Expand Down Expand Up @@ -1239,4 +1239,4 @@ CREATE (actor:Actor:Person {
})
WITH actor
RETURN actor { .name,born: { year: actor.born.year, month: actor.born.month } } AS actor
----
----
Loading