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

Switch to cursor-based pagination with geared_pagination #363

Closed
wants to merge 2 commits into from

Conversation

adrianna-chang-shopify
Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify commented Mar 5, 2021

Closes: #362

As mentioned in the issue, Pagy uses offset-based pagination, which decreases in performance as the collection of elements being paginated grows. It doesn't scale very well, and queries that use OFFSET are outright banned in Core for performance reasons. A cursor based pagination strategy queries based on a specific point in the dataset (the cursor), and works well for
sequential datasets / datasets with ordered attributes (maintenance_tasks_runs table is guaranteed to be sortable by created_at ).

Basecamp has actually written a gem that supports cursor-based pagination, which I think is a really good candidate for us to use, replacing Pagy.

Implementation

  • As the gem's README notes, we use set_page_and_extract_portion_from with the ordered_by option to perform cursor-based pagination
  • Using this form of pagination requires the original relation to be unordered, so I've dropped the ordering on #previous_runs, since the pagination will handle this for us now
  • This provides us with an instance var @page, which contains the recordset of paginated elements which we can render in the view (similar to what was happening with Pagy)
  • What is config/initializers/geared_pagination.rb all about?
    • The gem works by including GearedPagination::Controller in ActionController::Base when the gem is initialized
      - GearedPagination::Controller sets an after_action filter, set_paginated_headers
      - This currently breaks our test suite, because set_paginated_headers checks an ivar that doesn't exist (@page) which warns and thus raises in our test suite. I also don't really like that this filter runs before every single controller action in the engine and the host app.
      - So, I've added an initializer that removes this filter and puts the onus on us to call set_paginated_headers when we need to (for now, this is only in TasksController#show)
      - Arguably this is a little brittle. We could write a wrapper to isolate the call to set_paginated_headers in case we want to reuse pagination elsewhere, but I thought maybe that was premature

To 🎩

Screen Shot 2021-03-05 at 10 33 13 AM

Tradeoffs

  • We lose the ability to show individual "pages" and to go back to a previous page. However, I think it will be pretty rare that a ton of pages will be needed
  • We also gain the ability to provide a URL to a given page based on a cursor now which is kind of nice

# frozen_string_literal: true
GearedPagination::Engine.config.after_initialize do
ActiveSupport.on_load(:action_controller) do
ActionController::Base.skip_after_action(:set_paginated_headers)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this kind of implementation detail, I wonder if we're not better off implementing it ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll see if I can mock something up!

@adrianna-chang-shopify
Copy link
Contributor Author

Opted for #371 instead

@adrianna-chang-shopify adrianna-chang-shopify deleted the geared-pagination branch March 19, 2021 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependency on Pagy
2 participants