Nested sorting on fields #103

Andy2003 · 2020-08-05T12:49:25Z

This commit add the following features:

sorting on multiple properties
sorting on fields
augmentation-tests provides a diff on failure that can be viewed in IntelliJ

Bugfix:

The formatted field for temporal is now a string rather than an object

resolves #3

src/main/kotlin/org/neo4j/graphql/SchemaBuilder.kt

jexp · 2020-08-07T14:08:36Z

src/main/kotlin/org/neo4j/graphql/handler/QueryHandler.kt

@@ -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)))


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

Yes but we should define the schema correctly

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

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?

This commit add the following features: * sorting on multiple properties * sorting on fields * augmentation-tests provides a diff on failure that can be viewed in IntelliJ Bugfix: * The formatted field for temporal is now a string rather than an object

Andy2003 · 2020-08-28T09:49:53Z

fixes #115

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt

jexp · 2020-08-30T22:00:18Z

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt

@@ -206,7 +214,11 @@ open class ProjectionBase {
        } ?: when {
            isObjectField -> {
                val patternComprehensions = if (fieldDefinition.isNeo4jType()) {
-                    projectNeo4jObjectType(variable, field)
+                    if (neo4jFieldsToPass.contains(fieldDefinition.innerName())) {


could we give more of a semantic meaning to this expression?

jexp · 2020-08-30T22:00:41Z

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt

@@ -230,7 +242,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})"


why did we have to do this?

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.

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt

jexp · 2020-08-30T22:03:26Z

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt

+                val deferredProjection = projectSelection("sortedElement", neo4jFiledSelection, nodeType, env, variableSuffix)
+                    .map { cypher -> cypher.query }
+                    .joinNonEmpty(", ")
+                comprehension = "[sortedElement IN $comprehension | sortedElement { .*, $deferredProjection }]"


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

Are those sort fields that are not in the projection?

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

jexp · 2020-08-30T22:08:19Z

src/test/resources/movie-tests.adoc

+    title
+    actors(orderBy:name_desc) {
+      name
+      movies(orderBy:[title_asc, title_desc]) {


should that be allowed to sort by the same property twice in two ways?

This test is taken over form the js-library. No it does not make any sense. I adjust it.

src/test/resources/movie-tests.adoc

jexp · 2020-08-30T22:09:49Z

src/test/resources/movie-tests.adoc

+    knows: [sortedElement IN apoc.coll.sortMulti([(userKnows)-[:KNOWS]->(userKnowsKnows:User) | userKnowsKnows {
+      .name,
+      born: userKnowsKnows.born
+    }], ['born','born']) | sortedElement {


should one of these be ascending?

do we need to differentiate between nested levels?

Do you mean to have different names?

src/test/resources/movie-tests.adoc

jexp · 2020-08-30T22:12:20Z

src/test/resources/movie-tests.adoc

+WITH createUser
+RETURN createUser {
+  .userId,
+  __typename: head( [ label IN labels(createUser) WHERE label IN $createUserValidTypes ] )


what was this again?

This is to have the name of the type when using interfaces, so that you know what concrete type the current entry is of.

src/test/resources/translator-tests1.adoc

jexp

Looks good to me, can you quickly check my comments, then we can merge it.

# Conflicts: # src/test/kotlin/org/neo4j/graphql/utils/GraphQLSchemaTestSuite.kt

jexp · 2020-08-31T07:51:23Z

Also needs to be rebased.

jexp · 2020-09-02T19:28:34Z

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt

+        return arg?.value
+            ?.let { it ->
+                when (it) {
+                    is ArrayValue -> it.values.map { it.toJavaValue().toString() }


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

it.toJavaValue() does the check

jexp · 2020-09-02T19:29:53Z

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt

+        if (values.isEmpty()) {
+            return ""
+        }
+        return " ORDER BY " + values.joinToString(", ", transform = { (property, direction) -> "$variable.$property $direction" })


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

jexp

you can check my comments for a later fix.

Andy2003 force-pushed the feature/GH3-nested-sorting-on-fields branch from 18a5595 to 9899853 Compare August 6, 2020 12:00

jexp reviewed Aug 7, 2020

View reviewed changes

src/main/kotlin/org/neo4j/graphql/SchemaBuilder.kt Outdated Show resolved Hide resolved

jexp reviewed Aug 7, 2020

View reviewed changes

Andy2003 force-pushed the feature/GH3-nested-sorting-on-fields branch from 9899853 to 891365c Compare August 7, 2020 14:19

Add test for neo4j-graphql#115

33ec43b

Andy2003 mentioned this pull request Aug 28, 2020

orderBy a field with underscores in the name generates bad cypher #115

Closed

jexp reviewed Aug 30, 2020

View reviewed changes

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt Show resolved Hide resolved

jexp reviewed Aug 30, 2020

View reviewed changes

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt Show resolved Hide resolved

jexp reviewed Aug 30, 2020

View reviewed changes

src/main/kotlin/org/neo4j/graphql/handler/projection/ProjectionBase.kt Outdated Show resolved Hide resolved

jexp reviewed Aug 30, 2020

View reviewed changes

src/test/resources/movie-tests.adoc Show resolved Hide resolved

jexp reviewed Aug 30, 2020

View reviewed changes

src/test/resources/movie-tests.adoc Outdated Show resolved Hide resolved

jexp reviewed Aug 30, 2020

View reviewed changes

src/test/resources/translator-tests1.adoc Outdated Show resolved Hide resolved

jexp approved these changes Aug 30, 2020

View reviewed changes

Merge branch 'master' into feature/GH3-nested-sorting-on-fields

c179305

# Conflicts: # src/test/kotlin/org/neo4j/graphql/utils/GraphQLSchemaTestSuite.kt

adjustments after review

aa5809f

Andy2003 requested a review from jexp August 31, 2020 09:17

jexp reviewed Sep 2, 2020

View reviewed changes

jexp approved these changes Sep 2, 2020

View reviewed changes

jexp merged commit ba08a65 into neo4j-graphql:master Sep 2, 2020

Andy2003 deleted the feature/GH3-nested-sorting-on-fields branch September 3, 2020 05:58

Nested sorting on fields #103

Nested sorting on fields #103

Uh oh!

Conversation

Andy2003 commented Aug 5, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andy2003 commented Aug 28, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andy2003 Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jexp left a comment

Choose a reason for hiding this comment

Uh oh!

jexp commented Aug 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jexp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Andy2003 Aug 31, 2020 •

edited

Loading