Merge pull request #23624 from opf/code-maintenance/68063-journals-predecessor-cte

Code Maintenance/STC-462: Resolve only-changes predecessor once via a journals CTE

app/services/work_packages/activities_tab/paginator.rb

@@ -114,9 +114,7 @@ class WorkPackages::ActivitiesTab::Paginator
   # package's journals plus the journals written for its changesets — changesets
   # are journalized, so each carries a journal timestamped with its committed_on.
   def activities_scope(filter: self.filter)
-    scope = filtered_journals(filter)
-    scope = scope.or(changeset_journals) if include_changesets?(filter)
-    scope.reorder(created_at: :desc, id: :desc)
+    filtered_journals(filter).reorder(created_at: :desc, id: :desc)
   end
 
   def pagy_options
@@ -160,8 +158,8 @@ class WorkPackages::ActivitiesTab::Paginator
   def filtered_journals(filter)
     case filter
     when Filters::ONLY_COMMENTS then apply_comments_only_filter(visible_journals)
-    when Filters::ONLY_CHANGES then apply_changes_only_filter(visible_journals)
-    else visible_journals
+    when Filters::ONLY_CHANGES then apply_changes_only_filter(with_changesets(visible_journals))
+    else with_changesets(visible_journals)
     end
   end
 
@@ -170,11 +168,12 @@ class WorkPackages::ActivitiesTab::Paginator
                   journable_id: work_package.changesets.except(:order).select(:id))
   end
 
-  # Most work packages have no changesets (revisions are a legacy feature), so
-  # the changeset leg is merged in only when one exists — keeping the common
-  # query scoped to the work package's own journals.
-  def include_changesets?(filter)
-    filter != Filters::ONLY_COMMENTS && work_package.changesets.exists?
+  # Most work packages have no changesets (revisions are a legacy feature). Merging
+  # the changeset leg in only when one exists keeps the common query scoped to the
+  # work package's own journals; always merging the empty leg pushes the planner
+  # onto a slower global scan.
+  def with_changesets(scope)
+    work_package.changesets.exists? ? scope.or(changeset_journals) : scope
   end
 
   def page_journals(page_relation)

app/services/work_packages/activities_tab/paginator/journal_changes_filter.rb

@@ -38,32 +38,64 @@
 #   * Cause metadata (system-triggered changes)
 #   * Attribute/data changes (compares work_package_journals columns with immediate predecessor)
 #
-# This heuristic compares association records with the predecessor journal to detect actual changes,
-# not just the presence of snapshot records.
+# Each journal's immediate predecessor is resolved once, in a CTE named `journals` that
+# shadows the table and exposes `predecessor_id` / `predecessor_data_id` columns. Every
+# change-detection branch then reads those columns instead of re-seeking the predecessor.
+# Changeset journals carry no detectable attribute history and are always included.
 class WorkPackages::ActivitiesTab::Paginator::JournalChangesFilter
   class << self
-    def apply(scope)
-      sql = <<~SQL.squish
-        version = 1
-        OR (cause IS NOT NULL AND cause != '{}')
+    # @param membership [ActiveRecord::Relation] the activity feed's journals
+    #   (visible work package journals, optionally unioned with changeset journals)
+    def apply(membership)
+      Journal
+        .with(journals: Arel.sql(enriched_journals_sql(membership)))
+        .where(OpenProject::SqlSanitization.sanitize(changes_condition_sql))
+    end
+
+    private
+
+    # Enrich each journal row with its immediate predecessor's id and data_id. The
+    # predecessor is the highest version below the current one, located through the
+    # (journable_type, journable_id, version) index; versions are incremental but may
+    # have gaps, so the seek matches on `< version` rather than `version - 1`. A LEFT
+    # JOIN keeps initial (version = 1) journals, whose predecessor columns stay NULL.
+    def enriched_journals_sql(membership)
+      <<~SQL.squish
+        SELECT curr.*,
+               predecessor.id AS predecessor_id,
+               predecessor.data_id AS predecessor_data_id
+        FROM (#{membership.to_sql}) curr
+        LEFT JOIN LATERAL (
+          SELECT p.id, p.data_id
+          FROM journals p
+          WHERE p.journable_id = curr.journable_id
+            AND p.journable_type = curr.journable_type
+            AND p.version < curr.version
+          ORDER BY p.version DESC
+          LIMIT 1
+        ) predecessor ON TRUE
+      SQL
+    end
+
+    def changes_condition_sql
+      <<~SQL.squish
+        journals.journable_type = '#{Changeset.name}'
+        OR journals.version = 1
+        OR (journals.cause IS NOT NULL AND journals.cause != '{}')
         OR EXISTS (#{attribute_data_changes_condition_sql})
         OR EXISTS (#{attachment_changes_condition_sql})
         OR EXISTS (#{custom_field_changes_condition_sql})
         OR EXISTS (#{file_link_changes_condition_sql})
       SQL
-
-      scope.where(OpenProject::SqlSanitization.sanitize(sql))
     end
 
-    private
-
     def attribute_data_changes_condition_sql
       <<~SQL.squish
         SELECT 1
-          FROM #{predecessor_lateral_sql} predecessor
-          INNER JOIN work_package_journals pred_data ON predecessor.data_id = pred_data.id
-          INNER JOIN work_package_journals curr_data ON journals.data_id = curr_data.id
-          WHERE (#{data_changes_condition_sql})
+          FROM work_package_journals pred_data
+          INNER JOIN work_package_journals curr_data ON curr_data.id = journals.data_id
+          WHERE pred_data.id = journals.predecessor_data_id
+            AND (#{data_changes_condition_sql})
       SQL
     end
 
@@ -95,25 +127,6 @@ class WorkPackages::ActivitiesTab::Paginator::JournalChangesFilter
       )
     end
 
-    # The immediate predecessor journal, for comparison against the current one.
-    # Callers alias this as `predecessor`. Seeking the highest version below the
-    # current one through the (journable_type, journable_id, version) index keeps
-    # this a single-row lookup per journal; versions are incremental but may have
-    # gaps, so the seek matches on `< version` rather than `version - 1`.
-    def predecessor_lateral_sql
-      <<~SQL.squish
-        LATERAL (
-          SELECT p.id, p.data_id
-          FROM journals p
-          WHERE p.journable_id = journals.journable_id
-            AND p.journable_type = journals.journable_type
-            AND p.version < journals.version
-          ORDER BY p.version DESC
-          LIMIT 1
-        )
-      SQL
-    end
-
     def data_changes_condition_sql
       data_change_columns = Journal::WorkPackageJournal.column_names - ["id"]
 
@@ -158,9 +171,8 @@ class WorkPackages::ActivitiesTab::Paginator::JournalChangesFilter
       <<~SQL.squish
         SELECT 1
           FROM #{table} curr
-          LEFT JOIN #{predecessor_lateral_sql} predecessor ON TRUE
           LEFT JOIN #{table} pred
-            ON pred.journal_id = predecessor.id
+            ON pred.journal_id = journals.predecessor_id
             AND #{join_conditions}
           WHERE curr.journal_id = journals.id
             AND (#{where_clause})
@@ -176,13 +188,12 @@ class WorkPackages::ActivitiesTab::Paginator::JournalChangesFilter
 
       <<~SQL.squish
         SELECT 1
-          FROM #{predecessor_lateral_sql} predecessor
-          INNER JOIN #{table} pred
-            ON pred.journal_id = predecessor.id
+          FROM #{table} pred
           LEFT JOIN #{table} curr
             ON curr.journal_id = journals.id
             AND #{join_conditions}
-          WHERE curr.id IS NULL
+          WHERE pred.journal_id = journals.predecessor_id
+            AND curr.id IS NULL
       SQL
     end
   end

spec/services/work_packages/activities_tab/paginator_spec.rb

@@ -529,6 +529,26 @@ RSpec.describe WorkPackages::ActivitiesTab::Paginator, with_settings: { journal_
         expect(records.map(&:id)).to include(initial_journal.id)
       end
 
+      context "with changesets" do
+        let(:repository) { create(:repository_subversion, project:) }
+        let!(:changeset) do
+          create(:changeset,
+                 repository:,
+                 committed_on: 1.day.ago,
+                 revision: "rev1")
+        end
+
+        before do
+          work_package.changesets << changeset
+        end
+
+        it "includes changesets (commits are activity, not attribute-diffable journals)" do
+          _pagy, records = paginator.call
+
+          expect(records).to include(changeset)
+        end
+      end
+
       context "with attribute changes" do
         let!(:journal_with_attribute_change) do
           work_package.subject = "Updated subject"