Question on the default order of the cursor pagination implementation

[lindyhopchris]

@haddowg The cursor pagination implementation looks great, but I have a question around the sort order that we need to resolve before I tag - because once I tag, any changes would be breaking.

From the description to #37 -

By default the paginator will add a descending sort by the model key (or provided withKeyName column) to the query to ensure a deterministic order even if no sort is applied. If you know your provided sort/order, or default via global scope etc, is deterministic you can turn this off with withoutKeySort.
The withAscending method will now only affect this default key sort. I.e you can reverse the default applied key sort, but if its disabled with withoutKeySort this will do nothing.

Question: Should it be descending by default?

When you do this in Eloquent:

$posts = Post::query()->cursorPaginate(5);

You get posts in ascending order, not descending order. It took me be surprise when reviewing the tests as it felt like the default order was the wrong way round.

So the question is, should the order be ascending by default? So that it matches Eloquent, which feels more predictable from a DX perspective. Then use a withDescending() method if the developer wants to change the default?

[lindyhopchris]

One thing to note, is we need to make sure this works if the client has changed the sort order.

For example, if the default was ascending, if the client sent a sort parameter of -id then the results should be of descending order not ascending. I might need to add a test for that scenario, but will do that once we've confirmed the default order.

[lindyhopchris]

Hmmm... I am starting to wonder whether descending does actually make sense, considering an API client probably always wants the latest results on the first "page". I'm guessing that was your thinking?

[haddowg]

One thing to note, is we need to make sure this works if the client has changed the sort order.

For example, if the default was ascending, if the client sent a sort parameter of -id then the results should be of descending order not ascending. I might need to add a test for that scenario, but will do that once we've confirmed the default order.

If a sort for key column is already applied to the query then the paginator won't add or change it, so this should work fine, probably good to explicitly have a test for it however.

[haddowg]

Hmmm... I am starting to wonder whether descending does actually make sense, considering an API client probably always wants the latest results on the first "page". I'm guessing that was your thinking?

My thinking was to make it a drop in replacement for the existing cursor pagination, which in the absence of you specifying a column will sort desc on createdAt and then id by default. Assuming an incrementing or monotonic key this implementation will give the same sort out of the box without config.
If your key is not then year it's breaking but you would just need to add your own default sort on createdAt or similar. My assumption is if your using the existing package you have disabled client sorting as per the docs so swapping it out and configuring and verifying your default sort will just need to be part of the docs/upgraded guide.

[haddowg]

also if your in a merging mood on this package #38 😉

[lindyhopchris]

Yeah all great points - thank you. Will keep the descending order, think that makes sense.

I think there's two things I need to do before tagging:

1. Add the test for the client overriding the order via a sort parameter, as you suggest.
2. See whether I can use this new implementation to match the existing implementation. Think I can do that in the cursor pagination package by duplicating the test case and setting it up with this implementation.

For (2) I'm not overly worried if there's something that is totally breaking. In theory an API could continue to use the current package then when they move to a new version introduce the new implementation. Alternatively, they would be able to exist side-by-side using the MultiPaginator if different query parameters were used for each implementation.

Would be useful though for me to know whether they can be set up in an identical way, or if we need to provide some sort of upgrade guide.

Thanks for the nudge about #38, have added that to the project board so I stay on top of it.

[haddowg]

Yeah all great points - thank you. Will keep the descending order, think that makes sense.

I think there's two things I need to do before tagging:

1. Add the test for the client overriding the order via a sort parameter, as you suggest.
2. See whether I can use this new implementation to match the existing implementation. Think I can do that in the cursor pagination package by duplicating the test case and setting it up with this implementation.

For (2) I'm not overly worried if there's something that is totally breaking. In theory an API could continue to use the current package then when they move to a new version introduce the new implementation. Alternatively, they would be able to exist side-by-side using the MultiPaginator if different query parameters were used for each implementation.

Would be useful though for me to know whether they can be set up in an identical way, or if we need to provide some sort of upgrade guide.

Thanks for the nudge about #38, have added that to the project board so I stay on top of it.

I think a very small upgrade-guide/warning would be good just to highlight the default sort behaviour being different really, as the rest of the API should be fully compatible.
It was drop in replacement for myself other than needing to update my openapi spec to stop it expecting the cursors being uuids 😂.
I would be happy to assist updating the docs, as I think you would want to document the new withTotal stuff as well as the withoutKeySort in the case you don't want the this applied, remove the warning about sorting etc and at that point a small upgrade callout is no big deal.
Would be interested in seeing what other issues you encounter though.. If there are some test using the save video schema then I suspect they will fail as it uses a uuidv4 as key so has a random order using the default key sort

[lindyhopchris]

I think a very small upgrade-guide/warning would be good just to highlight the default sort behaviour being different really, as the rest of the API should be fully compatible. It was drop in replacement for myself other than needing to update my openapi spec to stop it expecting the cursors being uuids 😂. I would be happy to assist updating the docs, as I think you would want to document the new withTotal stuff as well as the withoutKeySort in the case you don't want the this applied, remove the warning about sorting etc and at that point a small upgrade callout is no big deal. Would be interested in seeing what other issues you encounter though.. If there are some test using the save video schema then I suspect they will fail as it uses a uuidv4 as key so has a random order using the default key sort

Help with the docs would be great! Means I'll have time to look at other JSON:API tickets including your other PR.

I've created this issue in the docs repo: laravel-json-api/laravel-json-api.github.io#38

If it's too much for you to do all of it, just say and we can split up the tasks on that ticket. Really appreciate your help but don't want to put too much on you!

[lindyhopchris]

Been going through it again tonight and we're looking really close to tagging. The only thing left for me to do is create the test case in the existing cursor pagination package to ensure it is backwards compatible. (With the caveat it's never backwards compatible if clients have been told they can just use any resource ID as the before/after cursor position.)

[haddowg]

Help with the docs would be great! Means I'll have time to look at other JSON:API tickets including your other PR.

I've created this issue in the docs repo: laravel-json-api/laravel-json-api.github.io#38

If it's too much for you to do all of it, just say and we can split up the tasks on that ticket. Really appreciate your help but don't want to put too much on you!

That all sounds sensible, I will make a start and keep you updated. I am happy to give something back since I have used this package for many years in a number or roles.

[lindyhopchris]

Unfortunately run out of time to finish this off tonight. I'll prioritise it next time I'm on JSON:API things, which will probably be at the weekend.

[lindyhopchris]

Had a play around with the tests in the cursor-pagination package to see if I can replicate with the new cursor implementation. It's a bit tricky as it's really complicated working out what the cursor would be. That's due to the Eloquent cursor including both the ID plus the created date as I'd applied a default sort order of -createdAt to get it to match the existing implementation.

Decided it's not the best use of my time trying to get the tests working. I think we can provide broad advice that it should be a drop in replacement if you add a -createdAt default sort to your schema and allowing the createdAt sort parameter in your validation (because that will start getting added to links as a sort parameter). However, it's safest to treat it as a breaking change.

[lindyhopchris]

@haddowg I've tagged the cursor pagination in the Eloquent package as 4.2.0 🎉

Great to get it out and thanks for all your hard work on it! I'll announce it once we've sorted out the docs.