Skip to content

Nested sorting on fields #103

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 4 commits into from
Sep 2, 2020
Merged

Nested sorting on fields #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
891365c
Nested sorting on fields (GH #3)
Andy2003 Aug 5, 2020
33ec43b
Add test for #115
Andy2003 Aug 28, 2020
c179305
Merge branch 'master' into feature/GH3-nested-sorting-on-fields
Andy2003 Aug 31, 2020
aa5809f
adjustments after review
Andy2003 Aug 31, 2020
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
38 changes: 22 additions & 16 deletions src/main/kotlin/org/neo4j/graphql/SchemaBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ object SchemaBuilder {

val handler = getHandler(config)

var targetSchema = augmentSchema(sourceSchema, handler)
var targetSchema = augmentSchema(sourceSchema, handler, config)
targetSchema = addDataFetcher(targetSchema, dataFetchingInterceptor, handler)
return targetSchema
}
Expand All @@ -99,7 +99,7 @@ object SchemaBuilder {
return handler
}

private fun augmentSchema(sourceSchema: GraphQLSchema, handler: List<AugmentationHandler>): GraphQLSchema {
private fun augmentSchema(sourceSchema: GraphQLSchema, handler: List<AugmentationHandler>, config: SchemaConfig): GraphQLSchema {
val types = sourceSchema.typeMap.toMutableMap()
val env = BuildingEnv(types)

Expand All @@ -116,22 +116,26 @@ object SchemaBuilder {
handler.forEach { h -> h.augmentType(type, env) }
}

types.replaceAll { _, sourceType ->
// since new types my be added to `types` we copy the map, to safely modify the entries and later add these
// modified entries back to the `types`
val adjustedTypes = types.toMutableMap()
adjustedTypes.replaceAll { _, sourceType ->
when {
sourceType.name.startsWith("__") -> sourceType
sourceType is GraphQLObjectType -> sourceType.transform { builder ->
builder.clearFields().clearInterfaces()
// to prevent duplicated types in schema
sourceType.interfaces.forEach { builder.withInterface(GraphQLTypeReference(it.name)) }
sourceType.fieldDefinitions.forEach { f -> builder.field(enhanceRelations(f)) }
sourceType.fieldDefinitions.forEach { f -> builder.field(enhanceRelations(f, env)) }
}
sourceType is GraphQLInterfaceType -> sourceType.transform { builder ->
builder.clearFields()
sourceType.fieldDefinitions.forEach { f -> builder.field(enhanceRelations(f)) }
sourceType.fieldDefinitions.forEach { f -> builder.field(enhanceRelations(f, env)) }
}
else -> sourceType
}
}
types.putAll(adjustedTypes)

return GraphQLSchema
.newSchema(sourceSchema)
Expand All @@ -142,27 +146,29 @@ object SchemaBuilder {
.build()
}

private fun enhanceRelations(fd: GraphQLFieldDefinition): GraphQLFieldDefinition {
return fd.transform {
private fun enhanceRelations(fd: GraphQLFieldDefinition, env: BuildingEnv): GraphQLFieldDefinition {
return fd.transform { fieldBuilder ->
// to prevent duplicated types in schema
it.type(fd.type.ref() as GraphQLOutputType)
fieldBuilder.type(fd.type.ref() as GraphQLOutputType)

if (!fd.isRelationship() || !fd.type.isList()) {
return@transform
}

if (fd.getArgument(ProjectionBase.FIRST) == null) {
it.argument { a -> a.name(ProjectionBase.FIRST).type(Scalars.GraphQLInt) }
fieldBuilder.argument { a -> a.name(ProjectionBase.FIRST).type(Scalars.GraphQLInt) }
}
if (fd.getArgument(ProjectionBase.OFFSET) == null) {
it.argument { a -> a.name(ProjectionBase.OFFSET).type(Scalars.GraphQLInt) }
fieldBuilder.argument { a -> a.name(ProjectionBase.OFFSET).type(Scalars.GraphQLInt) }
}
if (fd.getArgument(ProjectionBase.ORDER_BY) == null && fd.type.isList()) {
(fd.type.inner() as? GraphQLFieldsContainer)?.let { fieldType ->
env.addOrdering(fieldType)?.let { orderingTypeName ->
val orderType = GraphQLList(GraphQLNonNull(GraphQLTypeReference(orderingTypeName)))
fieldBuilder.argument { a -> a.name(ProjectionBase.ORDER_BY).type(orderType) }
}
}
}
// TODO implement ordering
// if (fd.getArgument(ProjectionBase.ORDER_BY) == null) {
// val typeName = fd.type.name()!!
// val orderingType = addOrdering(typeName, metaProvider.getNodeType(typeName)!!.fieldDefinitions().filter { it.type.isScalar() })
// it.argument { a -> a.name(ProjectionBase.ORDER_BY).type(orderingType) }
// }
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/main/kotlin/org/neo4j/graphql/handler/QueryHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ class QueryHandler private constructor(
.argument(input(OFFSET, Scalars.GraphQLInt))
.type(GraphQLNonNull(GraphQLList(GraphQLNonNull(GraphQLTypeReference(type.name)))))
if (orderingTypeName != null) {
builder.argument(input(ORDER_BY, GraphQLTypeReference(orderingTypeName)))
val orderType = GraphQLList(GraphQLNonNull(GraphQLTypeReference(orderingTypeName)))
Copy link
Contributor

Choose a reason for hiding this comment

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

graphql automatically supports lists instead of single elements here for enums.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes but we should define the schema correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

won't that break single element sorts then? that already exist?

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 tested it with GraphiQL and got no errors. And the tests are also green.
Can you point me to a doc describing the behavior you explained?

builder.argument(input(ORDER_BY, orderType))
}
val def = builder.build()
buildingEnv.addOperation(QUERY, def)
Expand Down
88 changes: 63 additions & 25 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 @@ -16,21 +16,30 @@ open class ProjectionBase {
}

fun orderBy(variable: String, args: MutableList<Argument>): String {
val values = getOrderByArgs(args)
if (values.isEmpty()) {
return ""
}
return " ORDER BY " + values.joinToString(", ", transform = { (property, direction) -> "$variable.$property $direction" })
Copy link
Contributor

Choose a reason for hiding this comment

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

do we might have to quote the property? if it has special characters/spaces? e.g. from an alias

}

private fun getOrderByArgs(args: MutableList<Argument>): List<Pair<String, Sort>> {
val arg = args.find { it.name == ORDER_BY }
val values = arg?.value?.let { it ->
when (it) {
is ArrayValue -> it.values.map { it.toJavaValue().toString() }
is EnumValue -> listOf(it.name)
is StringValue -> listOf(it.value)
else -> null
return arg?.value
?.let { it ->
when (it) {
is ArrayValue -> it.values.map { it.toJavaValue().toString() }
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need to check here as well Enum/StringValue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it.toJavaValue() does the check

is EnumValue -> listOf(it.name)
is StringValue -> listOf(it.value)
else -> null
}
}
}
@Suppress("SimplifiableCallChain")
return if (values == null) ""
else " ORDER BY " + values
.map { it.split("_") }
.map { "$variable.${it[0]} ${it[1].toUpperCase()}" }
.joinToString(", ")
?.map {
val index = it.lastIndexOf('_')
val property = it.substring(0, index)
val direction = Sort.valueOf(it.substring(index + 1).toUpperCase())
property to direction
} ?: emptyList()
}

fun where(variable: String, fieldDefinition: GraphQLFieldDefinition, type: GraphQLFieldsContainer, arguments: List<Argument>, field: Field): Cypher {
Expand Down Expand Up @@ -139,9 +148,8 @@ open class ProjectionBase {
return predicates.values + defaults
}

fun projectFields(variable: String, field: Field, nodeType: GraphQLFieldsContainer, env: DataFetchingEnvironment, variableSuffix: String?): Cypher {
val queries = projectSelectionSet(variable, field.selectionSet, nodeType, env, variableSuffix)

fun projectFields(variable: String, field: Field, nodeType: GraphQLFieldsContainer, env: DataFetchingEnvironment, variableSuffix: String?, propertiesToSkipDeepProjection: Set<String> = emptySet()): Cypher {
val queries = projectSelection(variable, field.selectionSet.selections, nodeType, env, variableSuffix, propertiesToSkipDeepProjection)
@Suppress("SimplifiableCallChain")
val projection = queries
.map { it.query }
Expand All @@ -152,18 +160,18 @@ open class ProjectionBase {
return Cypher("$variable $projection", params)
}

private fun projectSelectionSet(variable: String, selectionSet: SelectionSet, nodeType: GraphQLFieldsContainer, env: DataFetchingEnvironment, variableSuffix: String?): List<Cypher> {
private fun projectSelection(variable: String, selection: List<Selection<*>>, nodeType: GraphQLFieldsContainer, env: DataFetchingEnvironment, variableSuffix: String?, propertiesToSkipDeepProjection: Set<String> = emptySet()): List<Cypher> {
// TODO just render fragments on valid types (Labels) by using cypher like this:
// apoc.map.mergeList([
// a{.name},
// CASE WHEN a:Location THEN a { .foo } ELSE {} END
// ])
var hasTypeName = false
val projections = selectionSet.selections.flatMapTo(mutableListOf<Cypher>()) {
val projections = selection.flatMapTo(mutableListOf<Cypher>()) {
when (it) {
is Field -> {
hasTypeName = hasTypeName || (it.name == TYPE_NAME)
listOf(projectField(variable, it, nodeType, env, variableSuffix))
listOf(projectField(variable, it, nodeType, env, variableSuffix, propertiesToSkipDeepProjection))
}
is InlineFragment -> projectInlineFragment(variable, it, env, variableSuffix)
is FragmentSpread -> projectNamedFragments(variable, it, env, variableSuffix)
Expand All @@ -180,7 +188,7 @@ open class ProjectionBase {
return projections
}

private fun projectField(variable: String, field: Field, type: GraphQLFieldsContainer, env: DataFetchingEnvironment, variableSuffix: String?): Cypher {
private fun projectField(variable: String, field: Field, type: GraphQLFieldsContainer, env: DataFetchingEnvironment, variableSuffix: String?, propertiesToSkipDeepProjection: Set<String> = emptySet()): Cypher {
if (field.name == TYPE_NAME) {
return if (type.isRelationType()) {
Cypher("${field.aliasOrName()}: '${type.name}'")
Expand All @@ -206,7 +214,14 @@ open class ProjectionBase {
} ?: when {
isObjectField -> {
val patternComprehensions = if (fieldDefinition.isNeo4jType()) {
projectNeo4jObjectType(variable, field)
if (propertiesToSkipDeepProjection.contains(fieldDefinition.innerName())) {
// if the property has an internal type like Date or DateTime and we want to compute on this
// type (e.g sorting), we need to pass out the whole property and do the concrete projection
// after the outer computation is done
Cypher(variable + "." + fieldDefinition.propertyName().quote())
} else {
projectNeo4jObjectType(variable, field)
}
} else {
projectRelationship(variable, field, fieldDefinition, type, env, variableSuffix)
}
Expand All @@ -230,7 +245,7 @@ open class ProjectionBase {
.filterIsInstance<Field>()
.map {
val value = when (it.name) {
NEO4j_FORMATTED_PROPERTY_KEY -> "$variable.${field.name}"
NEO4j_FORMATTED_PROPERTY_KEY -> "toString($variable.${field.name})"
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we have to do this?

Copy link
Collaborator Author

@Andy2003 Andy2003 Aug 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This was a Bug I discovered by comparing to the js-library. In case we should return the formatted Date/DateTime/* property, this ensures, that a string will be returned instead the Date/DateTime/*-Object.

else -> "$variable.${field.name}.${it.name}"
}
"${it.name}: $value"
Expand Down Expand Up @@ -266,7 +281,7 @@ open class ProjectionBase {
val fragmentType = env.graphQLSchema.getType(fragmentTypeName) as? GraphQLFieldsContainer ?: return emptyList()
// these are the nested fields of the fragment
// it could be that we have to adapt the variable name too, and perhaps add some kind of rename
return projectSelectionSet(variable, selectionSet, fragmentType, env, variableSuffix)
return projectSelection(variable, selectionSet.selections, fragmentType, env, variableSuffix)
}


Expand Down Expand Up @@ -336,9 +351,27 @@ open class ProjectionBase {
val relPattern = if (isRelFromType) "$childVariable:${relInfo.relType}" else ":${relInfo.relType}"

val where = where(childVariable, fieldDefinition, nodeType, propertyArguments(field), field)
val fieldProjection = projectFields(childVariable, field, nodeType, env, variableSuffix)

val comprehension = "[($variable)$inArrow-[$relPattern]-$outArrow($endNodePattern)${where.query} | ${fieldProjection.query}]"
val orderBy = getOrderByArgs(field.arguments)
val sortByNeo4jTypeFields = orderBy
.filter { (property, _) -> nodeType.getFieldDefinition(property)?.isNeo4jType() == true }
.map { (property, _) -> property }
.toSet()

val fieldProjection = projectFields(childVariable, field, nodeType, env, variableSuffix, sortByNeo4jTypeFields)
var comprehension = "[($variable)$inArrow-[$relPattern]-$outArrow($endNodePattern)${where.query} | ${fieldProjection.query}]"
if (orderBy.isNotEmpty()) {
val sortArgs = orderBy.joinToString(", ", transform = { (property, direction) -> if (direction == Sort.ASC) "'^$property'" else "'$property'" })
comprehension = "apoc.coll.sortMulti($comprehension, [$sortArgs])"
if (sortByNeo4jTypeFields.isNotEmpty()) {
val neo4jFieldSelection = field.selectionSet.selections
.filter { selection -> sortByNeo4jTypeFields.contains((selection as? Field)?.name) }
val deferredProjection = projectSelection("sortedElement", neo4jFieldSelection, nodeType, env, variableSuffix)
.map { cypher -> cypher.query }
.joinNonEmpty(", ")
comprehension = "[sortedElement IN $comprehension | sortedElement { .*, $deferredProjection }]"
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really want .* ? this can pull way too many properties that are not selected / asked for?
What do we need the deferredProjection for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are those sort fields that are not in the projection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This logic I copied over form the js-library as well. The selection of the *. properties is done in the pattern comprehension so the sortedElement will already be projected

}
}
val skipLimit = SkipLimit(childVariable, field.arguments)
val slice = skipLimit.slice(fieldType.isList())
return Cypher(comprehension + slice.query, (where.params + fieldProjection.params + slice.params))
Expand Down Expand Up @@ -392,4 +425,9 @@ open class ProjectionBase {
}
}
}

enum class Sort {
ASC,
DESC
}
}
Loading