Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cursor pagination does not support hashids #201

Closed
othneildrew opened this issue Jun 25, 2022 · 5 comments
Closed

Cursor pagination does not support hashids #201

othneildrew opened this issue Jun 25, 2022 · 5 comments
Labels
bug Something isn't working

Comments

@othneildrew
Copy link

othneildrew commented Jun 25, 2022

Having setup and followed the docs to enable hashids, I've discovered that when using hashids with Cursor Pagination, the meta page info uses the ids from the DB column.

5a1b926235b7a9562e1aa6a727700148

_Trying without validating page query. I get "the selected page.after is invalid with proper validation _
c47f971f459a39b725c499e1561d6ece

f7e350b4cc839ba98b98fc2ce61f883b

@lindyhopchris lindyhopchris added the bug Something isn't working label Jun 25, 2022
@lindyhopchris
Copy link
Contributor

Thanks for reporting this - it is indeed a bug. There's already a PR in the cursor pagination package relating to this, and my comment on it here explains that I need to amend that package so the ID field is injected into the cursor paginator.

I'll prioritise this for a fix, probably in the next week or two.

@lindyhopchris
Copy link
Contributor

@othneildrew I think I've solved this but it would be good if you could give it a go.

To give it a go, run the following:

composer require "laravel-json-api/cursor-pagination:dev-feature/id-decoding"

To use in your schema, you would just need to do the following:

public function pagination(): CursorPagination
{
    return CursorPagination::make($this->id());
}

@othneildrew
Copy link
Author

othneildrew commented Jul 21, 2022

@lindyhopchris My apologies that this took me soon long to review. But I'm happy to report that it works wonderfully, thanks so much for fixing this!

The only missing piece I think is the validation of page params now.

For example, something like this would no longer pass validation checks:

  'page.after' => 'filled|string|exists:lessons,id',
  'page.before' => 'filled|string|exists:lessons,id',

@lindyhopchris
Copy link
Contributor

Yeah good point, I need to add some Rule objects that cover validating JSON:API identifiers in query parameters (JSON:API identifiers in JSON:API documents are already validated automatically).

@lindyhopchris
Copy link
Contributor

Closing in favour of #237 - that documents that I need to add things to make validating resource IDs a lot easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants