Skip to content

Commit

Permalink
Support min_id-based pagination in REST API (mastodon#8736)
Browse files Browse the repository at this point in the history
* Allow min_id pagination in Feed#get

* Add min_id pagination to home and list timeline APIs

* Add min_id pagination to account statuses, public and tag APIs

* Remove unused stub in reports API

* Use min_id pagination in notifications, favourites, and fix order

* Fix HomeFeed#from_database not using paginate_by_id
  • Loading branch information
Gargron authored Sep 28, 2018
1 parent 3d7f68c commit f0fff3e
Show file tree
Hide file tree
Showing 15 changed files with 49 additions and 51 deletions.
4 changes: 4 additions & 0 deletions app/controllers/api/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ def limit_param(default_limit)
[params[:limit].to_i.abs, default_limit * 2].min
end

def params_slice(*keys)
params.slice(*keys).permit(*keys)
end

def current_resource_owner
@current_user ||= User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token
end
Expand Down
7 changes: 3 additions & 4 deletions app/controllers/api/v1/accounts/statuses_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ def cached_account_statuses

def account_statuses
statuses = truthy_param?(:pinned) ? pinned_scope : permitted_account_statuses
statuses = statuses.paginate_by_max_id(
statuses = statuses.paginate_by_id(
limit_param(DEFAULT_STATUSES_LIMIT),
params[:max_id],
params[:since_id]
params_slice(:max_id, :since_id, :min_id)
)

statuses.merge!(only_media_scope) if truthy_param?(:only_media)
Expand Down Expand Up @@ -82,7 +81,7 @@ def next_path

def prev_path
unless @statuses.empty?
api_v1_account_statuses_url pagination_params(since_id: pagination_since_id)
api_v1_account_statuses_url pagination_params(min_id: pagination_since_id)
end
end

Expand Down
7 changes: 3 additions & 4 deletions app/controllers/api/v1/favourites_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,9 @@ def cached_favourites
end

def results
@_results ||= account_favourites.paginate_by_max_id(
@_results ||= account_favourites.paginate_by_id(
limit_param(DEFAULT_STATUSES_LIMIT),
params[:max_id],
params[:since_id]
params_slice(:max_id, :since_id, :min_id)
)
end

Expand All @@ -49,7 +48,7 @@ def next_path

def prev_path
unless results.empty?
api_v1_favourites_url pagination_params(since_id: pagination_since_id)
api_v1_favourites_url pagination_params(min_id: pagination_since_id)
end
end

Expand Down
7 changes: 3 additions & 4 deletions app/controllers/api/v1/notifications_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,9 @@ def load_notifications
end

def paginated_notifications
browserable_account_notifications.paginate_by_max_id(
browserable_account_notifications.paginate_by_id(
limit_param(DEFAULT_NOTIFICATIONS_LIMIT),
params[:max_id],
params[:since_id]
params_slice(:max_id, :since_id, :min_id)
)
end

Expand All @@ -64,7 +63,7 @@ def next_path

def prev_path
unless @notifications.empty?
api_v1_notifications_url pagination_params(since_id: pagination_since_id)
api_v1_notifications_url pagination_params(min_id: pagination_since_id)
end
end

Expand Down
5 changes: 0 additions & 5 deletions app/controllers/api/v1/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@ class Api::V1::ReportsController < Api::BaseController

respond_to :json

def index
@reports = current_account.reports
render json: @reports, each_serializer: REST::ReportSerializer
end

def create
@report = ReportService.new.call(
current_account,
Expand Down
5 changes: 3 additions & 2 deletions app/controllers/api/v1/timelines/home_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ def home_statuses
account_home_feed.get(
limit_param(DEFAULT_STATUSES_LIMIT),
params[:max_id],
params[:since_id]
params[:since_id],
params[:min_id]
)
end

Expand All @@ -51,7 +52,7 @@ def next_path
end

def prev_path
api_v1_timelines_home_url pagination_params(since_id: pagination_since_id)
api_v1_timelines_home_url pagination_params(min_id: pagination_since_id)
end

def pagination_max_id
Expand Down
5 changes: 3 additions & 2 deletions app/controllers/api/v1/timelines/list_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ def list_statuses
list_feed.get(
limit_param(DEFAULT_STATUSES_LIMIT),
params[:max_id],
params[:since_id]
params[:since_id],
params[:min_id]
)
end

Expand All @@ -53,7 +54,7 @@ def next_path
end

def prev_path
api_v1_timelines_list_url params[:id], pagination_params(since_id: pagination_since_id)
api_v1_timelines_list_url params[:id], pagination_params(min_id: pagination_since_id)
end

def pagination_max_id
Expand Down
7 changes: 3 additions & 4 deletions app/controllers/api/v1/timelines/public_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,9 @@ def cached_public_statuses
end

def public_statuses
statuses = public_timeline_statuses.paginate_by_max_id(
statuses = public_timeline_statuses.paginate_by_id(
limit_param(DEFAULT_STATUSES_LIMIT),
params[:max_id],
params[:since_id]
params_slice(:max_id, :since_id, :min_id)
)

if truthy_param?(:only_media)
Expand Down Expand Up @@ -53,7 +52,7 @@ def next_path
end

def prev_path
api_v1_timelines_public_url pagination_params(since_id: pagination_since_id)
api_v1_timelines_public_url pagination_params(min_id: pagination_since_id)
end

def pagination_max_id
Expand Down
7 changes: 3 additions & 4 deletions app/controllers/api/v1/timelines/tag_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,9 @@ def tagged_statuses
if @tag.nil?
[]
else
statuses = tag_timeline_statuses.paginate_by_max_id(
statuses = tag_timeline_statuses.paginate_by_id(
limit_param(DEFAULT_STATUSES_LIMIT),
params[:max_id],
params[:since_id]
params_slice(:max_id, :since_id, :min_id)
)

if truthy_param?(:only_media)
Expand Down Expand Up @@ -62,7 +61,7 @@ def next_path
end

def prev_path
api_v1_timelines_tag_url params[:id], pagination_params(since_id: pagination_since_id)
api_v1_timelines_tag_url params[:id], pagination_params(min_id: pagination_since_id)
end

def pagination_max_id
Expand Down
8 changes: 8 additions & 0 deletions app/models/concerns/paginable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,13 @@ module Paginable
query = query.where(arel_table[:id].gt(min_id)) if min_id.present?
query
}

scope :paginate_by_id, ->(limit, **options) {
if options[:min_id].present?
paginate_by_min_id(limit, options[:min_id]).reverse
else
paginate_by_max_id(limit, options[:max_id], options[:since_id])
end
}
end
end
16 changes: 10 additions & 6 deletions app/models/feed.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,20 @@ def initialize(type, id)
@id = id
end

def get(limit, max_id = nil, since_id = nil)
from_redis(limit, max_id, since_id)
def get(limit, max_id = nil, since_id = nil, min_id = nil)
from_redis(limit, max_id, since_id, min_id)
end

protected

def from_redis(limit, max_id, since_id)
max_id = '+inf' if max_id.blank?
since_id = '-inf' if since_id.blank?
unhydrated = redis.zrevrangebyscore(key, "(#{max_id}", "(#{since_id}", limit: [0, limit], with_scores: true).map(&:first).map(&:to_i)
def from_redis(limit, max_id, since_id, min_id)
if min_id.blank?
max_id = '+inf' if max_id.blank?
since_id = '-inf' if since_id.blank?
unhydrated = redis.zrevrangebyscore(key, "(#{max_id}", "(#{since_id}", limit: [0, limit], with_scores: true).map(&:first).map(&:to_i)
else
unhydrated = redis.zrangebyscore(key, "(#{min_id}", '+inf', limit: [0, limit], with_scores: true).map(&:first).map(&:to_i)
end

Status.where(id: unhydrated).cache_ids
end
Expand Down
8 changes: 4 additions & 4 deletions app/models/home_feed.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,19 @@ def initialize(account)
@account = account
end

def get(limit, max_id = nil, since_id = nil)
def get(limit, max_id = nil, since_id = nil, min_id = nil)
if redis.exists("account:#{@account.id}:regeneration")
from_database(limit, max_id, since_id)
from_database(limit, max_id, since_id, min_id)
else
super
end
end

private

def from_database(limit, max_id, since_id)
def from_database(limit, max_id, since_id, min_id)
Status.as_home_timeline(@account)
.paginate_by_max_id(limit, max_id, since_id)
.paginate_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id)
.reject { |status| FeedManager.instance.filter?(:home, status, @account.id) }
end
end
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@
resources :blocks, only: [:index]
resources :mutes, only: [:index]
resources :favourites, only: [:index]
resources :reports, only: [:index, :create]
resources :reports, only: [:create]
resources :filters, only: [:index, :create, :show, :update, :destroy]
resources :endorsements, only: [:index]

Expand Down
2 changes: 1 addition & 1 deletion spec/controllers/api/v1/favourites_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
get :index, params: { limit: 1 }

expect(response.headers['Link'].find_link(['rel', 'next']).href).to eq "http://test.host/api/v1/favourites?limit=1&max_id=#{favourite.id}"
expect(response.headers['Link'].find_link(['rel', 'prev']).href).to eq "http://test.host/api/v1/favourites?limit=1&since_id=#{favourite.id}"
expect(response.headers['Link'].find_link(['rel', 'prev']).href).to eq "http://test.host/api/v1/favourites?limit=1&min_id=#{favourite.id}"
end

it 'does not add pagination headers if not necessary' do
Expand Down
10 changes: 0 additions & 10 deletions spec/controllers/api/v1/reports_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,6 @@
allow(controller).to receive(:doorkeeper_token) { token }
end

describe 'GET #index' do
let(:scopes) { 'read:reports' }

it 'returns http success' do
get :index

expect(response).to have_http_status(200)
end
end

describe 'POST #create' do
let(:scopes) { 'write:reports' }
let!(:status) { Fabricate(:status) }
Expand Down

0 comments on commit f0fff3e

Please sign in to comment.