feat: add eloquent cursor pagination implementation

[haddowg]

Cursor Pagination

Adds a Cursor Pagination implementation that uses eloquent's cursorPaginate under the hood but maintains the contract from the existing cursor-pagination package implementation meaning it should act as a drop-in replacement in most cases.

Changes to existing CursorPagination API:

Features

• now supports arbitrary sorting, you freely sort as required either via query params or a default sort or global scopes pagination will still work as expected, your cursors will just be a little longer. With one caveat... see notes below.
• adds a withTotal and withTotalOnFirstPage method, this will add a total to the page meta that is a count of the total results, the former will do this on any paginated query (with or without a cursor), whereas the later will only do this additional query on the first page i.e. when paginating but with no before or after cursor provided.

Breaking

• removes the withCursorColumn method, this is no longer necessary as you can sort by any arbitrary column(s).
• 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.
  If you relied specifically on the default created_at sorting applied when not providing a withCursorColumn (with a non sequential monotonic key column, such that the order would actually change) then you will need to explicitly add this as a sort either as a default or via a global scope etc.

Notes

• will support encoding/decoding the key column using the provided ID class as required but supporting encoding/decoding the values for additional sort columns that cannot reliably be encoded to json in the cursor, for example uuids represented as binary in the DB, would likely involve further thought/work and or some changes to the underlying laravel cursor implementation, this is a limitation there really.

[lindyhopchris]

Thanks for this! I'm going to need a bit of time to look through it, which I don't have tonight. But I will hopefully get to it soon.

[haddowg]

Thanks for this! I'm going to need a bit of time to look through it, which I don't have tonight. But I will hopefully get to it soon.

No rush, will investigate that failing test... 🤔

[haddowg]

Thanks for this! I'm going to need a bit of time to look through it, which I don't have tonight. But I will hopefully get to it soon.

No rush, will investigate that failing test... 🤔

fixed, didn't realise the Video models uuids were v4's (random) and therefore would not provide a deterministic sort. this is probably something to callout in the docs for this if/when merged. It does assume your model's keys are sequential/monotonic, such as an autoincrement or ordered uuid (v7).
In the case of uuid V4 key this is not the case, making them a terrible choice for a primary key but hey 🤷‍♂️ .
If this were the case you would need to use the withoutKeySort option and ensure you apply a sort(s) that is determanistic yourself.

[lindyhopchris]

@haddowg sorry for taking a bit of time to look at this, but finally been through it.

This is an excellent bit of work - thank you so much! I'm sure there's lots of people who are going to use this, and we can recommend it as the approach for cursor pagination going forward.

There's a number of tidying up things, which I was going to do myself rather than expecting you to do - but unfortunately I couldn't push to your fork. I'll do them after merging this PR into develop instead.

My only main comment is just to check we've got all the correct tests in there. We seem to be covering a lot, but I wanted to check we haven't missed anything. Did you base your tests on the test file that's in the cursor pagination package? I.e. this one: https://github.com/laravel-json-api/cursor-pagination/blob/develop/tests/lib/Acceptance/Test.php

The ones that I think are missing are as follows:

We need to test the ID encoding. We need to check that if you base64_decode the value that the Eloquent cursor implementation gives you, the ID contained within it is the JSON:API encoded value. That's because any client could decode it, and the point of the JSON:API ID encoding is not to expose the database value. Are you able to add a test for both the before and after encoding? In the test file linked above, it's testAfterWithIdEncoding() and testBeforeWithIdEncoding. You can bring in the EncodedId class from that package for the test.

The other thing that I think we need to have a test for is what happens if either the before or after IDs sent by the client do not exist. In the existing cursor pagination package, it throws an exception as it expects you to have validated the query parameters. We should have tests for this scenario. If the Eloquent cursor handles it differently, i.e. if it would work in this scenario, then we should align to what the Eloquent cursor does. But I'd still like to know exactly what happens in this scenario, which is why it would be good to have testAfterDoesNotExist() and testBeforeDoesNotExist() so that we're 100% sure what it does.

Hope that all makes sense? Do let me know if there's anything that doesn't make sense!

[haddowg]

That all makes sense, the id encoding definitely works as I am using it for binary uuids but should indeed be tested.
Will take a look as soon as I can.

Re fork/pr permission it seems GitHub treats forks in an organisation differently.. next time I will fork to my personal account.

[haddowg]

@lindyhopchris

We need to test the ID encoding. We need to check that if you base64_decode the value that the Eloquent cursor implementation gives you, the ID contained within it is the JSON:API encoded value. That's because any client could decode it, and the point of the JSON:API ID encoding is not to expose the database value. Are you able to add a test for both the before and after encoding? In the test file linked above, it's testAfterWithIdEncoding() and testBeforeWithIdEncoding. You can bring in the EncodedId class from that package for the test.

This behavior is fixed and tested, I was in-fact only decoding from the cursor and not encoding when creating, this worked in my own use case since my binary uuids were already encoded (string cast) by the model itself, so they only needed decoding back to binary for the query, so great catch 👍.

The other thing that I think we need to have a test for is what happens if either the before or after IDs sent by the client do not exist. In the existing cursor pagination package, it throws an exception as it expects you to have validated the query parameters. We should have tests for this scenario. If the Eloquent cursor handles it differently, i.e. if it would work in this scenario, then we should align to what the Eloquent cursor does. But I'd still like to know exactly what happens in this scenario, which is why it would be good to have testAfterDoesNotExist() and testBeforeDoesNotExist() so that we're 100% sure what it does.

This one I am not sure how best to approach, since there may in fact be no ids in the cursor at all depending on your sort and if you used the withoutKeySort.
The laravel cursor can essentially be arbitrary column key-value pairs dependent on your sorting, that are used to create where > or where < expressions depending on the cursor direction, if this results in a valid query you will get a result, if you gave bogus values you might get an odd result or an empty result, if you gave it something that produced a SQL error, like a non existant column, you would get an exception.
I am not sure what there is value in testing here as it is dependent on the tables columns and data.
The cursors are designed to be opaque, despite technically being decodable and craftable by a client/consumer, the package cannot meaningfully validate them, and laravel never tries, I think if you manipulate an opaque cursor it's reasonable to expect odd behavior or an error.

Let me know what you think.

[lindyhopchris]

Thanks! Have some allocated Laravel JSON:API time this evening (UK time) so will look into this and get back to you. Hoping I can merge this PR and tag but will look into the issue you've raised first.

[lindyhopchris]

Looks good! Thanks so much for the extra changes. I'm going to merge this into develop now so I can tidy up some things before releasing. If I find anything that's more than tidying up, I'll create a separate PR and ask for your review of it.

[lindyhopchris]

@haddowg put a question on #40 to resolve before I tag.