Closed haddowg closed 2 months ago
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.
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... 🤔
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.
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.
@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.
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.
@haddowg put a question on #40 to resolve before I tag.
Cursor Pagination
Adds a Cursor Pagination implementation that uses eloquent's
cursorPaginate
under the hood but maintains the contract from the existingcursor-pagination
package implementation meaning it should act as a drop-in replacement in most cases.Changes to existing CursorPagination API:
Features
withTotal
andwithTotalOnFirstPage
method, this will add atotal
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 thefirst page
i.e. when paginating but with nobefore
orafter
cursor provided.Breaking
withCursorColumn
method, this is no longer necessary as you can sort by any arbitrary column(s).withoutKeySort
. ThewithAscending
method will now only affect this default key sort. I.e you can reverse the default applied key sort, but if its disabled withwithoutKeySort
this will do nothing. If you relied specifically on the default created_at sorting applied when not providing awithCursorColumn
(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