Skip to content

Commit

Permalink
reduce count calls in collection representer
Browse files Browse the repository at this point in the history
will_paginated, when actually fetching the records will do an SQL count on top of the fetching. We don`t need that
in our offset paginated represters since those implement their own counts.

And if the per_page is 0, we don`t even have to try to fetch any objects so we can use `none` to avoid the database
roundtrip.
  • Loading branch information
ulferts authored and oliverguenther committed Jun 21, 2022
1 parent 7a8a515 commit 71e6f28
Show file tree
Hide file tree
Showing 8 changed files with 35 additions and 25 deletions.
17 changes: 14 additions & 3 deletions lib/api/decorators/offset_paginated_collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,20 @@ def query_params(page = @page, page_size = @per_page)
end

def paged_models(models)
# FIXME: calling :to_a is a hack to circumvent a counting error in will_paginate
# see https://github.com/mislav/will_paginate/issues/449
models.page(@page).per_page(@per_page).to_a
if @per_page == 0
# Optimization. If we are not interested in the model, we can
# save the round trip to the database.
models.none
else
# Using WillPaginate as we have used it before but avoid the builtin
# page(@page).per_page(@per_page) way of fetching
# as it will, on top of fetching the values, also do a count of all elements matching the query
# which we do not need at this place.
page_number = ::WillPaginate::PageNumber(@page.nil? ? 1 : @page)
models
.offset(page_number.to_offset(@per_page).to_i)
.limit(@per_page)
end
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,11 @@ def schema_pairs
def all_cfs_of_project
@all_cfs_of_project ||= represented
.group_by(&:project_id)
.map { |id, wps| [id, wps.map(&:available_custom_fields).flatten.uniq] }
.to_h
.transform_values { |wps| wps.map(&:available_custom_fields).flatten.uniq }
end

def paged_models(models)
models.page(@page).per_page(@per_page).pluck(:id)
super.pluck(:id)
end

def _type
Expand Down
8 changes: 4 additions & 4 deletions spec/lib/api/v3/groups/group_collection_representer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@
let(:groups) do
build_stubbed_list(:group, 3).tap do |groups|
allow(groups)
.to receive(:per_page)
.with(page_size)
.to receive(:offset)
.with(page - 1)
.and_return(groups)

allow(groups)
.to receive(:page)
.with(page)
.to receive(:limit)
.with(page_size)
.and_return(groups)

allow(groups)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,13 @@
let(:members) do
build_stubbed_list(:member, 3).tap do |members|
allow(members)
.to receive(:per_page)
.to receive(:limit)
.with(page_size)
.and_return(members)

allow(members)
.to receive(:page)
.with(page)
.to receive(:offset)
.with(page - 1)
.and_return(members)

allow(members)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@
build_stubbed_list(:notification,
3).tap do |items|
allow(items)
.to receive(:per_page)
.to receive(:limit)
.with(page_size)
.and_return(items)

allow(items)
.to receive(:page)
.with(page)
.to receive(:offset)
.with(page - 1)
.and_return(items)

allow(items)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,13 @@
placeholders = build_stubbed_list(:placeholder_user,
actual_count)
allow(placeholders)
.to receive(:per_page)
.to receive(:limit)
.with(page_size)
.and_return(placeholders)

allow(placeholders)
.to receive(:page)
.with(page)
.to receive(:offset)
.with(page - 1)
.and_return(placeholders)

allow(placeholders)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@
let(:relations) do
build_stubbed_list(:relation, total).tap do |relations|
allow(relations)
.to receive(:per_page)
.to receive(:limit)
.with(page_size)
.and_return(relations)

allow(relations)
.to receive(:page)
.with(page)
.to receive(:offset)
.with(page - 1)
.and_return(relations)

allow(relations)
Expand Down
6 changes: 3 additions & 3 deletions spec/lib/api/v3/users/user_collection_representer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,13 @@
users = build_stubbed_list(:user,
actual_count)
allow(users)
.to receive(:per_page)
.to receive(:limit)
.with(page_size)
.and_return(users)

allow(users)
.to receive(:page)
.with(page)
.to receive(:offset)
.with(page - 1)
.and_return(users)

allow(users)
Expand Down

0 comments on commit 71e6f28

Please sign in to comment.