Skip to content
This repository has been archived by the owner on Sep 3, 2021. It is now read-only.

Support arrays of orderBy arguments #184

Merged
merged 2 commits into from
Jan 21, 2019
Merged

Support arrays of orderBy arguments #184

merged 2 commits into from
Jan 21, 2019

Conversation

whostolebenfrog
Copy link
Contributor

This allows us to support orderBy arguments passed as arrays. For example:

{
  Repo (orderBy : [name_desc,owner_asc]) {
    name
  }
}

This brings the functionality inline with the plugin which auto-generates this as an array.

One extra issue I have noticed though is that neo4j-graphql-js doesn't support using orderBy on anything but the top level element of the graphql query. I looked at adding this myself but as the generated cypher uses pattern comprehensions which don't seem to support order by I'm not sure how this would be possible right now. I'd love some guidance here though as this is a bit of an issue for us.

Please let me know if there is anything else I can add here or to the above issue.

Thanks for the project!
Ben at Atomist

@johnymontana
Copy link
Contributor

Awesome - thanks, Ben. Looks good to me, I'll merge it in.

Since this PR doesn't address schema generation, are you all not using makeAugmentedSchema or augmentSchema?

Regarding only supporting top-level ordering: right, unfortunately, the pattern comprehensions in Cypher don't support ordering so currently we only catch the orderBy on the top level selection. For ordering on the nested fields our plan there is to make use of APOC, specifically apoc.coll.sort I think will do the trick, but we just haven't had time to address it yet. The plugin uses a similar approach, but with a custom function instead of APOC. If you have time, we'd definitely appreciate a PR for nested ordering :-)

@johnymontana johnymontana merged commit 7f674cb into neo4j-graphql:master Jan 21, 2019
@whostolebenfrog
Copy link
Contributor Author

whostolebenfrog commented Jan 22, 2019

Thanks for merging @johnymontana !

Our schema was originally from the plugin. We then took a copy of this schema for use with apollo from which point we've been modifying and trimming it down to reflect what we want the schema to really be. Although it's still some 20,000 lines or so!

I'm going to look into making that change with apoc.coll.sort -- that's a great tip thank you. This is pretty important for us as we need to be fully moved over to apollo to unblock some other unrelated work. Hopefully I can figure it out! I'll need to learn a bit more about how this library works.

@johnymontana Is there already a strategy for dealing with plugins that may or may not be installed? Presumably this would result in "cypher" that would error on servers without the plugin. Does it need to be feature flagged or do you think it's sufficient to error if someone is using nested orderBys without the required plugin?

Thanks again!

@johnymontana
Copy link
Contributor

johnymontana commented Jan 22, 2019

Currently, the APOC procedure library is required for certain features of neo4j-graphql.js. We use it now for supporting the @cypher schema directives and UUID generation. I've tried to note this dependency in the documentation for these specific features, for example here. .

Once we add nested ordering as well I think I'll just make it a more explicit dependency in the documentation for this library. I'm OK with this dependency as a large percentage of Neo4j users are using APOC anyway. I don't plan to introduce dependencies on other Neo4j procedure libraries - with the possible exception of neo4j-graph-algorithms, but that is not currently on the roadmap.

So no, I don't think we need to bother with feature flagging - I think we can just assume they have APOC installed and risk the error if they don't.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants