Cursor Reconfigure For Order #165

brad-decker · 2016-02-22T22:58:45Z

Updates

Removed the base64 encoding of order attribute from the cursor. Replaced with the index of the item itself.
Updated all the logic based on this to provide true pagination regardless of the order by statement.
Added a test case in integration/connections for custom queries and made sure all tests pass there.

Caveats

Need more test cases to make sure nothing can break the pagination.
A unit test for $parent failed on simplifyAST but i haven't touched that.
Signed-off-by: brad-decker bhdecker84@gmail.com

aweary · 2016-02-22T23:21:11Z

👏

mickhansen · 2016-02-23T06:34:23Z

Travis isn't currently running the integration tests since a unit test fails (looks like it might be related to a graphql update, not sure).

mickhansen · 2016-02-23T06:35:10Z

lib/argsToFindOptions.js

+  return result;
+}
+
+module.exports = exports['default'];


lib/ should be in gitignore, either way please remove from PR

I added the lib folder to this so i could use it straight from github for our application. I can remove it directly before its ready for merge. If you have an alternative suggestion to make this able to be installed from github so we can install versions that aren't ready for NPM for testing etc i'd love to implement that feature.

We use automated build processes on heroku so we are limited to what can be automatically installed.

Hm alright, please do remove before we merge.

mickhansen · 2016-02-23T08:49:57Z

I've fixed the unit test, please rebase.

mickhansen · 2016-02-23T16:12:21Z

I generally like the approach of encoding the index in the cursor and then using that index.
The only issue i see is that it does not take into account the connection edges changing (i.e. more being added)

brad-decker · 2016-02-23T16:19:25Z

I'm not familiar with that part of it. Could I alter the mutation test to account for this and add some tests in their for resulting order? Though I do believe that some of that would need to be handled on the client side in the relay mutation configuration using the fields config? Give me some guidance and i can work towards a test case that fails and then work on a resolution.

mickhansen · 2016-02-23T17:13:14Z

I was thinking more of changes to rows from outside graphql-relay.
But i'm not sure that the existing solution solved that any better actually.

mickhansen · 2016-02-23T17:18:40Z

test/integration/relay/connection.test.js

+                  ELSE "updatedAt" End ASC`);
+            }
+            return options;
+          },


You can simply add the literal to the value of the ENUM i believe.

Didn't work for me because literal returns an object with {val : }

Hmm, i believe i have that in one of my projects. Having an object shouldn't matter, the value in ENUM can be anything AFAIR.

let orderBy = args.orderBy; let orderAttribute = orderByAttribute(orderBy); let orderDirection = args.orderBy[0][1];``` specifically this part of the connections configuration prevents that case because its looking directly for an array value. I tried setting it up directly in the order configuration first and it failed.

Ah alright, was sure i tried that out, but maybe i used a hook too.

mickhansen · 2016-03-08T07:52:55Z

@brad-decker Any update?

mickhansen · 2016-03-10T16:47:24Z

Do we even need to care about id in the cursor with this update? We could possible just move to the cursor generators provided by graphql-relay

chrisbolin · 2016-03-21T19:21:35Z

hey guys, what's the current status of this?

mickhansen · 2016-03-21T19:32:45Z

@brad-decker has been unavailable for a bit, waiting for him to be back. Do you have a usecase not solved by the current method @chrisbolin?

chrisbolin · 2016-03-21T19:43:29Z

I hit #170, which led me here :)

mickhansen · 2016-03-21T19:49:13Z

Ah, i didn't actually envision seeing a lot of MSSQL users i must admit :)

chrisbolin · 2016-03-21T19:51:39Z

Oh, I'm on Postgres. pageInfo.hasPrevious is always false though, due to the way the where query is created

chrisbolin · 2016-03-21T19:53:19Z

e.g. the where query looks like this

        "id": {
          "$gt": "71"
        }

if you have an after that evaluates to 71. Therefore full_count will not include anything less than 71.

chrisbolin · 2016-03-21T19:55:58Z

full_count as: https://github.com/mickhansen/graphql-sequelize/blob/master/src/relay.js#L277

chrisbolin · 2016-03-21T19:59:11Z

actually, looking at the code closely, it looks like hasPreviousPage will always be false no matter what if you are running a first query

brad-decker · 2016-03-21T20:01:12Z

@chrisbolin @mickhansen sorry for the delay... No promises but i should be able to work on this this week.

chrisbolin · 2016-03-21T20:04:04Z

no worries, @brad-decker! Thanks for taking a stab at this.

chrisbolin · 2016-03-21T20:08:55Z

I think this is probably related to the full_count issue: The last arg seems to generate funky queries. e.g.

"where": {
    "$or": [
      {
        "id": {
          "$lt": "103"
        }
      },
      {
        "id": {
          "$gt": "103"
        }
      }
    ]
  },

For example (last:3, before: "YXJyYXljb25uZWN0aW9uJDEwMyQxMDM=") (where that cursor is decoded to arrayconnection$103$103)

mickhansen · 2016-03-21T20:45:16Z

@chrisbolin Hmm yeah, full count is affected by where, odd i haven't encountered this since we do filter by id/order value.

mickhansen · 2016-03-21T20:45:43Z

All in all @brad-decker's update should work better. Not sure why i ruled out index in cursor as a proper fix in the beginning.

brad-decker · 2016-04-17T21:12:06Z

@mickhansen can you do a final review of this and then i'll put our project that is using our fork on pause and remove the lib folder from the repository so you can merge. sorry for such a long delay on this.

mickhansen · 2016-04-18T11:24:52Z

@brad-decker IIRC i was quite happy with most of it, however the diff is near impossible at the moment.
If you could rebase and remove lib to clean it up i'll review asap.

brad-decker · 2016-04-18T14:53:25Z

@mickhansen rebased and removed lib. I've got a failing test that was added in the relay test file regarding model connections that i have to figure out but i'll get on it shortly.

mickhansen · 2016-04-18T15:13:12Z

Still like 30-40 commits in the PR including merge commits.

brad-decker · 2016-04-18T15:25:34Z

Hmm

brad-decker · 2016-04-18T15:53:04Z

Ok successfully rebased, need to do some cleanup tho and fix that test still. Sorry about the false alarm there

brad-decker · 2016-04-18T17:24:18Z

Cleanup done, test passing.

mickhansen · 2016-04-18T18:19:57Z

src/relay.js

+  /**
+   * Creates a cursor given a item returned from the Database
+   * @param  {Object}   item   sequelize model instance
+   * @param  {Integer}  index  the index of this item within the results, 1 indexed


brad-decker · 2016-04-18T19:15:00Z

@mickhansen most of your points have been addressed now. Thanks for the feedback

mickhansen · 2016-04-19T14:36:14Z

test/integration/relay/connection.test.js


-      it('should support nested aliased fields', async function () {
-        let result = await graphql(this.schema, `
+      it('should support in-query slicing with user provided args/where', async function () {


Looks like this test was changed although i don't think it was intentional.

Signed-off-by: brad-decker <bhdecker84@gmail.com> removed defaultOrderBy for lint Signed-off-by: brad-decker <bhdecker84@gmail.com> make graphql-sequelize useable from github Signed-off-by: brad-decker <bhdecker84@gmail.com> build should use internal version of babel Signed-off-by: brad-decker <bhdecker84@gmail.com> use internal node_modules version take 2 Signed-off-by: brad-decker <bhdecker84@gmail.com> Remove postinstall, add lib folder temporarily Signed-off-by: brad-decker <bhdecker84@gmail.com> Cursor Reconfigure For Order Signed-off-by: brad-decker <bhdecker84@gmail.com> make graphql-sequelize useable from github Signed-off-by: brad-decker <bhdecker84@gmail.com> build should use internal version of babel Signed-off-by: brad-decker <bhdecker84@gmail.com> use internal node_modules version take 2 Signed-off-by: brad-decker <bhdecker84@gmail.com> Remove postinstall, add lib folder temporarily Signed-off-by: brad-decker <bhdecker84@gmail.com> Cursor Reconfigure For Order Signed-off-by: brad-decker <bhdecker84@gmail.com> make graphql-sequelize useable from github Signed-off-by: brad-decker <bhdecker84@gmail.com> build should use internal version of babel Signed-off-by: brad-decker <bhdecker84@gmail.com> use internal node_modules version take 2 Signed-off-by: brad-decker <bhdecker84@gmail.com> Remove postinstall, add lib folder temporarily Signed-off-by: brad-decker <bhdecker84@gmail.com> Cursor Reconfigure For Order Signed-off-by: brad-decker <bhdecker84@gmail.com> make graphql-sequelize useable from github Signed-off-by: brad-decker <bhdecker84@gmail.com> build should use internal version of babel Signed-off-by: brad-decker <bhdecker84@gmail.com> use internal node_modules version take 2 Signed-off-by: brad-decker <bhdecker84@gmail.com> Remove postinstall, add lib folder temporarily Signed-off-by: brad-decker <bhdecker84@gmail.com> Weird conflict issue, fixing. Signed-off-by: brad-decker <bhdecker84@gmail.com> add one to startIndex if its not 0 to account for 0 indexed cursor Signed-off-by: brad-decker <bhdecker84@gmail.com> update tests to match results Signed-off-by: brad-decker <bhdecker84@gmail.com> fixing test Signed-off-by: brad-decker <bhdecker84@gmail.com> Cursor Reconfigure For Order Signed-off-by: brad-decker <bhdecker84@gmail.com> removed defaultOrderBy for lint Signed-off-by: brad-decker <bhdecker84@gmail.com> make graphql-sequelize useable from github Signed-off-by: brad-decker <bhdecker84@gmail.com> build should use internal version of babel Signed-off-by: brad-decker <bhdecker84@gmail.com> use internal node_modules version take 2 Signed-off-by: brad-decker <bhdecker84@gmail.com> Remove postinstall, add lib folder temporarily Signed-off-by: brad-decker <bhdecker84@gmail.com> Cursor Reconfigure For Order Signed-off-by: brad-decker <bhdecker84@gmail.com> make graphql-sequelize useable from github Signed-off-by: brad-decker <bhdecker84@gmail.com> build should use internal version of babel Signed-off-by: brad-decker <bhdecker84@gmail.com> use internal node_modules version take 2 Signed-off-by: brad-decker <bhdecker84@gmail.com> Remove postinstall, add lib folder temporarily Signed-off-by: brad-decker <bhdecker84@gmail.com> Cursor Reconfigure For Order Signed-off-by: brad-decker <bhdecker84@gmail.com> removed defaultOrderBy for lint Signed-off-by: brad-decker <bhdecker84@gmail.com> make graphql-sequelize useable from github Signed-off-by: brad-decker <bhdecker84@gmail.com> build should use internal version of babel Signed-off-by: brad-decker <bhdecker84@gmail.com> use internal node_modules version take 2 Signed-off-by: brad-decker <bhdecker84@gmail.com> Remove postinstall, add lib folder temporarily Signed-off-by: brad-decker <bhdecker84@gmail.com> Cursor Reconfigure For Order Signed-off-by: brad-decker <bhdecker84@gmail.com> removed defaultOrderBy for lint Signed-off-by: brad-decker <bhdecker84@gmail.com> make graphql-sequelize useable from github Signed-off-by: brad-decker <bhdecker84@gmail.com> build should use internal version of babel Signed-off-by: brad-decker <bhdecker84@gmail.com> use internal node_modules version take 2 Signed-off-by: brad-decker <bhdecker84@gmail.com> Remove postinstall, add lib folder temporarily Signed-off-by: brad-decker <bhdecker84@gmail.com> Weird conflict issue, fixing. Signed-off-by: brad-decker <bhdecker84@gmail.com> add one to startIndex if its not 0 to account for 0 indexed cursor Signed-off-by: brad-decker <bhdecker84@gmail.com> update tests to match results Signed-off-by: brad-decker <bhdecker84@gmail.com> removed lib Signed-off-by: brad-decker <bhdecker84@gmail.com> Cleanup, fixed tests Signed-off-by: brad-decker <bhdecker84@gmail.com> Making requested Changes Signed-off-by: brad-decker <bhdecker84@gmail.com> renaming cursor to queriedCursor Signed-off-by: brad-decker <bhdecker84@gmail.com> rebase messed up some tests. replacing them Signed-off-by: brad-decker <bhdecker84@gmail.com> fixing one more test. Signed-off-by: brad-decker <bhdecker84@gmail.com>

brad-decker · 2016-04-19T15:13:12Z

Thanks @KATT @chrisbolin @mickhansen and everyone else for your patience here. 👏

KATT · 2016-04-19T15:40:22Z

Amazing!! Thank you so much for all the the time & effort you both put into this @mickhansen + @brad-decker! Can't wait to try it out.

mickhansen reviewed Feb 23, 2016
View reviewed changes

mickhansen mentioned this pull request Feb 28, 2016

Use OFFSET syntax to make pageInfo.hasPrevious page work #170

Closed

mickhansen mentioned this pull request Mar 10, 2016

Make pageInfo.hasNextPage work for MSSQL #169

Merged

mickhansen reviewed Apr 18, 2016
View reviewed changes

mickhansen reviewed Apr 19, 2016
View reviewed changes

mickhansen merged commit 4641eb6 into mickhansen:master Apr 19, 2016

Cursor Reconfigure For Order #165

Cursor Reconfigure For Order #165

Uh oh!

Conversation

brad-decker commented Feb 22, 2016

Updates

Caveats

Uh oh!

aweary commented Feb 22, 2016

Uh oh!

mickhansen commented Feb 23, 2016

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!

mickhansen commented Feb 23, 2016

Uh oh!

mickhansen commented Feb 23, 2016

Uh oh!

brad-decker commented Feb 23, 2016

Uh oh!

mickhansen commented Feb 23, 2016

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!

mickhansen commented Mar 8, 2016

Uh oh!

mickhansen commented Mar 10, 2016

Uh oh!

chrisbolin commented Mar 21, 2016

Uh oh!

mickhansen commented Mar 21, 2016

Uh oh!

chrisbolin commented Mar 21, 2016

Uh oh!

mickhansen commented Mar 21, 2016

Uh oh!

chrisbolin commented Mar 21, 2016

Uh oh!

chrisbolin commented Mar 21, 2016

Uh oh!

chrisbolin commented Mar 21, 2016

Uh oh!

chrisbolin commented Mar 21, 2016

Uh oh!

brad-decker commented Mar 21, 2016

Uh oh!

chrisbolin commented Mar 21, 2016

Uh oh!

chrisbolin commented Mar 21, 2016

Uh oh!

mickhansen commented Mar 21, 2016

Uh oh!

mickhansen commented Mar 21, 2016

Uh oh!

brad-decker commented Apr 17, 2016

Uh oh!

mickhansen commented Apr 18, 2016

Uh oh!

brad-decker commented Apr 18, 2016

Uh oh!

mickhansen commented Apr 18, 2016

Uh oh!

brad-decker commented Apr 18, 2016

Uh oh!

brad-decker commented Apr 18, 2016

Uh oh!