Skip to content

Commit 7c517da

Browse files
committed
Paginate work package activities tab at the database level
The activity tab paginator previously loaded every journal and changeset for a work package on each request, merged and sorted them in Ruby, then sliced for the requested page. The eager-loading wrapper ran four queries sized to the full pre-pagination set (capped at limit × MAX_PAGES = 2000 rows by a Ruby-side ceiling). Anchor jumps materialised the same array to call `find_index`. Pagination now happens in Postgres. A `UNION ALL` of journals and changesets, ordered by `(activity_at, id) DESC`, is paginated by pagy at the database level. The eager-loading wrapper runs against the page slice only — typically 20 rows instead of up to 2000. Anchor jumps resolve to a `(created_at, id)` tuple and count rows ahead in one query, so the path is independent of history size and the MAX_PAGES cap goes away. The `internal_visible` predicate is also exposed as a `Journal` class scope (`internal_visible_for(project:, user:)`) so the activities feed and anchor lookups share one visibility definition. Existing fluent callers (`work_package.journals.internal_visible`) keep working via a delegating shim on the has_many extension. Anchor lookups go through the same scope so unviewable internal journals fall back to page 1 rather than leaking through URL behaviour. A concurrent index on `journals (journable_type, journable_id, created_at DESC, id DESC)` serves the new `WHERE … ORDER BY` plan and supports the row-constructor predicate `(created_at, id) > (?, ?)` used by anchor counting. The `activity-N` anchor format (resolves a journal's `sequence_version` via a LATERAL `ROW_NUMBER`) is preserved for old bookmarks.
1 parent 2b36f61 commit 7c517da

7 files changed

Lines changed: 352 additions & 109 deletions

File tree

app/models/journal.rb

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,8 @@ class Journal < ApplicationRecord
122122

123123
include ::Scopes::Scoped
124124

125-
scopes :with_sequence_version
125+
scopes :with_sequence_version,
126+
:internal_visible_for
126127

127128
# Scopes to all journals excluding the initial journal - useful for change
128129
# logs like the history on issue#show
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
# frozen_string_literal: true
2+
3+
# -- copyright
4+
# OpenProject is an open source project management software.
5+
# Copyright (C) the OpenProject GmbH
6+
#
7+
# This program is free software; you can redistribute it and/or
8+
# modify it under the terms of the GNU General Public License version 3.
9+
#
10+
# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
11+
# Copyright (C) 2006-2013 Jean-Philippe Lang
12+
# Copyright (C) 2010-2013 the ChiliProject Team
13+
#
14+
# This program is free software; you can redistribute it and/or
15+
# modify it under the terms of the GNU General Public License
16+
# as published by the Free Software Foundation; either version 2
17+
# of the License, or (at your option) any later version.
18+
#
19+
# This program is distributed in the hope that it will be useful,
20+
# but WITHOUT ANY WARRANTY; without even the implied warranty of
21+
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
22+
# GNU General Public License for more details.
23+
#
24+
# You should have received a copy of the GNU General Public License
25+
# along with this program; if not, write to the Free Software
26+
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
27+
#
28+
# See COPYRIGHT and LICENSE files for more details.
29+
# ++
30+
31+
module Journals::Scopes
32+
# Restricts journals to those the given user is allowed to see in the given
33+
# project. Internal journals are hidden unless the project has internal
34+
# comments enabled, the Enterprise add-on is active, and the user holds
35+
# `view_internal_comments`.
36+
#
37+
# This is a class scope (rather than a `has_many` extension) so it composes
38+
# inside a subquery — for example, the journals leg of a UNION.
39+
#
40+
# The `user:` default of `User.current` assumes request context. Pass `user:`
41+
# explicitly from background jobs where `User.current` is not set.
42+
module InternalVisibleFor
43+
extend ActiveSupport::Concern
44+
45+
class_methods do
46+
def internal_visible_for(project:, user: User.current)
47+
if EnterpriseToken.allows_to?(:internal_comments) &&
48+
project.enabled_internal_comments &&
49+
user.allowed_in_project?(:view_internal_comments, project)
50+
all
51+
else
52+
where(internal: false)
53+
end
54+
end
55+
end
56+
end
57+
end

app/models/work_package/journalized.rb

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -34,13 +34,7 @@ module WorkPackage::Journalized
3434
included do
3535
acts_as_journalized journals_association_extension: proc {
3636
def internal_visible
37-
if EnterpriseToken.allows_to?(:internal_comments) &&
38-
proxy_association.owner.project.enabled_internal_comments &&
39-
User.current.allowed_in_project?(:view_internal_comments, proxy_association.owner.project)
40-
all
41-
else
42-
where(internal: false)
43-
end
37+
merge(Journal.internal_visible_for(project: proxy_association.owner.project))
4438
end
4539
}
4640

app/services/work_packages/activities_tab/paginator.rb

Lines changed: 99 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@
2828
# See COPYRIGHT and LICENSE files for more details.
2929
#++
3030

31-
# Paginates work package activities (journals and changesets) with support for filtering and anchor navigation.
31+
# Paginates work package activities (journals and changesets) with support for
32+
# filtering and anchor navigation.
3233
#
3334
# Filter modes:
3435
# - :all - Shows all activities (default)
@@ -39,6 +40,11 @@
3940
# - "comment-{journal_id}" - Navigate to specific journal by ID
4041
# - "activity-{sequence_version}" - Navigate to journal by sequence version
4142
#
43+
# Internally, the activities feed is materialised as a single UNION ALL
44+
# relation of journals and changesets (see {ActivitiesQuery}) and paginated
45+
# by pagy at the database level. Only the page slice is hydrated and wrapped
46+
# for eager-loading, keeping per-request cost independent of total history size.
47+
#
4248
# @param work_package [WorkPackage] The work package to paginate activities for
4349
# @param params [Hash] Pagination and filtering parameters
4450
#
@@ -52,8 +58,6 @@ class WorkPackages::ActivitiesTab::Paginator
5258
include Pagy::Method
5359
include WorkPackages::ActivitiesTab::JournalSortingInquirable
5460

55-
MAX_PAGES = 100
56-
5761
def self.paginate(work_package, params = {})
5862
new(work_package, params).call
5963
end
@@ -67,124 +71,138 @@ def initialize(work_package, params = {})
6771
end
6872

6973
def call
70-
anchor_type, target_record_id = extract_target_record_id
74+
anchor_type, target_record_id = parse_anchor
7175

72-
pagy, records =
76+
pagy_obj, page_relation =
7377
if anchor_type && target_record_id
74-
@filter = :all # Ignore filter when jumping to specific journal
75-
pagy_array_for_target_journal(anchor_type, target_record_id)
78+
@filter = :all
79+
pagy_at_anchor(anchor_type, target_record_id)
7680
else
77-
pagy(:offset, capped_journals, **pagy_options)
81+
pagy(:offset, activities_scope, **pagy_options)
7882
end
7983

80-
# For UI display: if user wants "oldest first" UI, reverse the array
81-
records = records.reverse if journal_sorting.asc?
84+
activities = load_activities(page_relation)
85+
activities = activities.reverse if journal_sorting.asc?
8286

83-
[pagy, records]
87+
[pagy_obj, activities]
8488
end
8589

8690
private
8791

92+
def activities_query
93+
@activities_query ||= ActivitiesQuery.new(work_package, filter:)
94+
end
95+
96+
def activities_scope
97+
@activities_scope ||= activities_query.call
98+
end
99+
100+
def limit
101+
params[:limit] || Pagy::DEFAULT[:limit]
102+
end
103+
88104
def pagy_options
89105
{
90106
page: params[:page] || 1,
91-
limit: params[:limit] || Pagy::DEFAULT[:limit],
107+
limit:,
92108
request: { params: }
93109
}.compact
94110
end
95111

96-
def extract_target_record_id
112+
def parse_anchor
97113
anchor = params[:anchor] # e.g., "comment-78758" (without #)
98-
return nil unless anchor
114+
return unless anchor
99115

100116
match = anchor.match(/^(comment|activity)-(\d+)$/)
101-
match && match.length == 3 ? [match[1].inquiry, match[2].to_i] : []
102-
end
103-
104-
def pagy_array_for_target_journal(anchor_type, target_record_id)
105-
journals = base_journals
106-
107-
target_index = journals.find_index do |record|
108-
if anchor_type.comment?
109-
record.id == target_record_id
110-
elsif anchor_type.activity?
111-
record.sequence_version == target_record_id
112-
else
113-
false
114-
end
115-
end
117+
return unless match
116118

117-
if target_index
118-
limit = pagy_options[:limit]
119-
target_page = (target_index / limit) + 1
120-
pagy(:offset, journals, **pagy_options, page: target_page)
121-
else
122-
# Journal might be filtered out or deleted - fallback to page 1
123-
pagy(:offset, journals, **pagy_options, page: 1)
124-
end
119+
[match[1].inquiry, match[2].to_i]
125120
end
126121

127-
def base_journals
128-
combine_and_sort_records(fetch_journals, fetch_revisions)
122+
# An unresolvable anchor (deleted, never existed, not visible to the user)
123+
# falls back to the default page from `pagy_options`.
124+
def pagy_at_anchor(anchor_type, target_record_id)
125+
options = pagy_options
126+
options[:page] = page_for_anchor(anchor_type, target_record_id) || options[:page]
127+
pagy(:offset, activities_scope, **options)
129128
end
130129

131-
def capped_journals
132-
max_records = (params[:limit] || Pagy::DEFAULT[:limit]) * MAX_PAGES
133-
base_journals.first(max_records)
130+
# Resolves an anchor to its target page by counting records ahead of it.
131+
# Returns nil when the anchor is unresolvable (e.g. deleted) so the caller
132+
# falls back to page 1.
133+
def page_for_anchor(anchor_type, target_record_id)
134+
activity_at, anchor_id = locate_anchor(anchor_type, target_record_id)
135+
return nil unless activity_at && anchor_id
136+
137+
rows_ahead = activities_scope
138+
.where("(activities.activity_at, activities.id) > (?, ?)", activity_at, anchor_id)
139+
.count(:all)
140+
141+
(rows_ahead / limit) + 1
134142
end
135143

136-
def fetch_journals
137-
API::V3::Activities::ActivityEagerLoadingWrapper.wrap(fetch_ar_journals)
144+
# Anchors must observe the same visibility rules as the activities feed.
145+
# Otherwise the count-ahead would route an unviewable journal to a page
146+
# number and leak the existence of internal journals through the URL.
147+
def locate_anchor(anchor_type, target_record_id)
148+
if anchor_type.comment?
149+
activities_query.visible_journals
150+
.where(id: target_record_id)
151+
.pick(:created_at, :id)
152+
elsif anchor_type.activity?
153+
locate_anchor_by_sequence_version(target_record_id)
154+
end
138155
end
139156

140-
def fetch_ar_journals
141-
journals = work_package
142-
.journals
143-
.internal_visible
144-
.includes(
145-
:user,
146-
:customizable_journals,
147-
:attachable_journals,
148-
:storable_journals,
149-
:notifications
150-
)
151-
.reorder(version: :desc) # Always fetch newest first for pagination
157+
def locate_anchor_by_sequence_version(sequence_version)
158+
activities_query.visible_journals
152159
.with_sequence_version
153-
154-
case filter
155-
when :only_comments then apply_comments_only_filter(journals)
156-
when :only_changes then apply_changes_only_filter(journals)
157-
else
158-
journals
159-
end
160+
.where(ranked: { sequence_version: sequence_version })
161+
.pick(:created_at, :id)
160162
end
161163

162-
def fetch_revisions
163-
return Changeset.none if filter == :only_comments
164+
def load_activities(page_relation)
165+
activity_refs = page_relation.pluck(Arel.sql("activities.kind"), Arel.sql("activities.id"))
166+
activities_by_kind = load_page_activities_by_kind(activity_refs)
164167

165-
work_package.changesets.includes(:user, :repository)
168+
ordered_activities = activity_refs.filter_map { |kind, id| activities_by_kind[kind][id] }
169+
eager_load_journals(ordered_activities)
166170
end
167171

168-
def combine_and_sort_records(journals, revisions)
169-
(journals + revisions).sort_by do |record|
170-
timestamp = record_timestamp(record)
171-
[-timestamp, -record.id] # Always sort DESC (newest first)
172-
end
172+
def load_page_activities_by_kind(activity_refs)
173+
ids_by_kind = activity_refs.group_by(&:first).transform_values { it.map(&:last) }
174+
{
175+
ActivitiesQuery::KIND_JOURNAL => load_page_journals(ids_by_kind[ActivitiesQuery::KIND_JOURNAL] || []),
176+
ActivitiesQuery::KIND_CHANGESET => load_page_changesets(ids_by_kind[ActivitiesQuery::KIND_CHANGESET] || [])
177+
}
173178
end
174179

175-
def record_timestamp(record)
176-
if record.is_a?(API::V3::Activities::ActivityEagerLoadingWrapper)
177-
record.created_at&.to_i
178-
elsif record.is_a?(Changeset)
179-
record.committed_on.to_i
180-
end
180+
def load_page_journals(ids)
181+
return {} if ids.empty?
182+
183+
Journal
184+
.where(id: ids)
185+
.with_sequence_version
186+
.includes(:user, :customizable_journals, :attachable_journals, :storable_journals, :notifications)
187+
.index_by(&:id)
181188
end
182189

183-
def apply_comments_only_filter(scope)
184-
scope.where.not(notes: [nil, ""])
190+
def load_page_changesets(ids)
191+
return {} if ids.empty?
192+
193+
Changeset
194+
.where(id: ids)
195+
.includes(:user, :repository)
196+
.index_by(&:id)
185197
end
186198

187-
def apply_changes_only_filter(scope)
188-
JournalChangesFilter.apply(scope)
199+
# Substitutes journals with their eager-loading wrappers so the wrapper's
200+
# batch queries (journable, predecessor, data, notifications) run against
201+
# the page slice only. Order from the input is preserved.
202+
def eager_load_journals(activities)
203+
journals = activities.grep(Journal)
204+
wrapped_by_id = API::V3::Activities::ActivityEagerLoadingWrapper.wrap(journals).index_by(&:id)
205+
206+
activities.map { it.is_a?(Journal) ? wrapped_by_id[it.id] : it }
189207
end
190208
end

0 commit comments

Comments
 (0)