From b87a1229c74aebec02cd08091f25613f6dff4428 Mon Sep 17 00:00:00 2001 From: tateisu Date: Mon, 28 May 2018 18:04:06 +0900 Subject: [PATCH] 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 --- .../api/v1/timelines/direct_controller.rb | 15 ++++--- app/models/status.rb | 43 ++++++++++++++++--- spec/models/status_spec.rb | 23 +++++----- 3 files changed, 59 insertions(+), 22 deletions(-) diff --git a/app/controllers/api/v1/timelines/direct_controller.rb b/app/controllers/api/v1/timelines/direct_controller.rb index d455227eb5..ef64078be8 100644 --- 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 853e75b43f..54f3f68f54 100644 --- 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 - apply_timeline_filters(query, account, false) + # _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 + + 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 c6701018e5..aee4f49b47 100644 --- 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