optimize direct timeline (#7614)

* optimize direct timeline

* fix typo in class name

* change filter condition for direct timeline

* fix codestyle issue

* revoke index_accounts_not_silenced because direct timeline does not use it.

* revoke index_accounts_not_silenced because direct timeline does not use it.

* fix rspec test condition.

* fix rspec test condition.

* fix rspec test condition.

* revoke adding column and partial index

* (direct timeline) move merging logic to model

* fix pagination parameter

* add method arguments that switches return array of status or cache_ids

* fix order by

* returns ActiveRecord.Relation in default behavor

* fix codestyle issue

diff --git a/app/controllers/api/v1/timelines/direct_controller.rb b/app/controllers/api/v1/timelines/direct_controller.rb
index d455227eb56a53b79b9a7f8fbd1b394a6d0fa0aa..ef64078be871d87ec1ebfc1223f5fecfa143c5d5 100644 (file)
--- a/app/controllers/api/v1/timelines/direct_controller.rb
+++ b/app/controllers/api/v1/timelines/direct_controller.rb
@@ -23,15 +23,18 @@ class Api::V1::Timelines::DirectController < Api::BaseController
   end
 
   def direct_statuses
-    direct_timeline_statuses.paginate_by_max_id(
-      limit_param(DEFAULT_STATUSES_LIMIT),
-      params[:max_id],
-      params[:since_id]
-    )
+    direct_timeline_statuses
   end
 
   def direct_timeline_statuses
-    Status.as_direct_timeline(current_account)
+    # this query requires built in pagination.
+    Status.as_direct_timeline(
+      current_account,
+      limit_param(DEFAULT_STATUSES_LIMIT),
+      params[:max_id],
+      params[:since_id],
+      true # returns array of cache_ids object
+    )
   end
 
   def insert_pagination_headers

diff --git a/app/models/status.rb b/app/models/status.rb
index 853e75b43fa5214d247fb56497aa56a2df64c1ba..54f3f68f54215196179919a266f5c36e2e1d3d14 100644 (file)
--- a/app/models/status.rb
+++ b/app/models/status.rb
@@ -188,12 +188,45 @@ class Status < ApplicationRecord
       where(account: [account] + account.following).where(visibility: [:public, :unlisted, :private])
     end
 
-    def as_direct_timeline(account)
-      query = joins("LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id AND mentions.account_id = #{account.id}")
-              .where("mentions.account_id = #{account.id} OR statuses.account_id = #{account.id}")
-              .where(visibility: [:direct])
+    def as_direct_timeline(account, limit = 20, max_id = nil, since_id = nil, cache_ids = false)
+      # direct timeline is mix of direct message from_me and to_me.
+      # 2 querys are executed with pagination.
+      # constant expression using arel_table is required for partial index
+
+      # _from_me part does not require any timeline filters
+      query_from_me = where(account_id: account.id)
+                      .where(Status.arel_table[:visibility].eq(3))
+                      .limit(limit)
+                      .order('statuses.id DESC')
+
+      # _to_me part requires mute and block filter.
+      # FIXME: may we check mutes.hide_notifications?
+      query_to_me = Status
+                    .joins(:mentions)
+                    .merge(Mention.where(account_id: account.id))
+                    .where(Status.arel_table[:visibility].eq(3))
+                    .limit(limit)
+                    .order('mentions.status_id DESC')
+                    .not_excluded_by_account(account)
+
+      if max_id.present?
+        query_from_me = query_from_me.where('statuses.id < ?', max_id)
+        query_to_me = query_to_me.where('mentions.status_id < ?', max_id)
+      end
+
+      if since_id.present?
+        query_from_me = query_from_me.where('statuses.id > ?', since_id)
+        query_to_me = query_to_me.where('mentions.status_id > ?', since_id)
+      end
 
-      apply_timeline_filters(query, account, false)
+      if cache_ids
+        # returns array of cache_ids object that have id and updated_at
+        (query_from_me.cache_ids.to_a + query_to_me.cache_ids.to_a).uniq(&:id).sort_by(&:id).reverse.take(limit)
+      else
+        # returns ActiveRecord.Relation
+        items = (query_from_me.select(:id).to_a + query_to_me.select(:id).to_a).uniq(&:id).sort_by(&:id).reverse.take(limit)
+        Status.where(id: items.map(&:id))
+      end
     end
 
     def as_public_timeline(account = nil, local_only = false)

diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb
index c6701018e52de520a74aa2d8bb0a3e51338d58a7..aee4f49b4768717d87ffab964aca3535b369bd2d 100644 (file)
--- a/spec/models/status_spec.rb
+++ b/spec/models/status_spec.rb
@@ -154,7 +154,7 @@ RSpec.describe Status, type: :model do
 
   describe '#target' do
     it 'returns nil if the status is self-contained' do
-      expect(subject.target).to be_nil
+     expect(subject.target).to be_nil
     end
 
     it 'returns nil if the status is a reply' do
@@ -333,24 +333,25 @@ RSpec.describe Status, type: :model do
       expect(@results).to_not include(@followed_public_status)
     end
 
-    it 'includes direct statuses mentioning recipient from followed' do
-      Fabricate(:mention, account: account, status: @followed_direct_status)
-      expect(@results).to include(@followed_direct_status)
-    end
-
     it 'does not include direct statuses not mentioning recipient from followed' do
       expect(@results).to_not include(@followed_direct_status)
     end
 
-    it 'includes direct statuses mentioning recipient from non-followed' do
-      Fabricate(:mention, account: account, status: @not_followed_direct_status)
-      expect(@results).to include(@not_followed_direct_status)
-    end
-
     it 'does not include direct statuses not mentioning recipient from non-followed' do
       expect(@results).to_not include(@not_followed_direct_status)
     end
 
+    it 'includes direct statuses mentioning recipient from followed' do
+      Fabricate(:mention, account: account, status: @followed_direct_status)
+      results2 = Status.as_direct_timeline(account)
+      expect(results2).to include(@followed_direct_status)
+    end
+
+    it 'includes direct statuses mentioning recipient from non-followed' do
+      Fabricate(:mention, account: account, status: @not_followed_direct_status)
+      results2 = Status.as_direct_timeline(account)
+      expect(results2).to include(@not_followed_direct_status)
+    end
   end
 
   describe '.as_public_timeline' do