From 85d89b472dff2c3d06801dbd42f91c325d21a434 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 8 Sep 2016 20:36:01 +0200 Subject: [PATCH] Optimized n+1 queries in accounts Atom and HTML views Added stack trace for SQL queries in development Removed badly thought out accounts/lookup API --- Gemfile | 1 + Gemfile.lock | 2 ++ app/controllers/accounts_controller.rb | 17 +++++++++---- .../api/accounts/lookup_controller.rb | 14 ----------- app/helpers/api/accounts/lookup_helper.rb | 2 -- app/helpers/application_helper.rb | 19 ++++++++------- app/helpers/stream_entries_helper.rb | 4 ++-- app/models/status.rb | 2 +- app/models/stream_entry.rb | 12 +++++++++- config/environments/development.rb | 6 +++-- config/routes.rb | 4 ---- .../api/accounts/lookup_controller_spec.rb | 24 ------------------- .../api/accounts/lookup_helper_spec.rb | 5 ---- 13 files changed, 44 insertions(+), 68 deletions(-) delete mode 100644 app/controllers/api/accounts/lookup_controller.rb delete mode 100644 app/helpers/api/accounts/lookup_helper.rb delete mode 100644 spec/controllers/api/accounts/lookup_controller_spec.rb delete mode 100644 spec/helpers/api/accounts/lookup_helper_spec.rb diff --git a/Gemfile b/Gemfile index d03a8ac4a4..eff16be54c 100644 --- a/Gemfile +++ b/Gemfile @@ -60,6 +60,7 @@ group :development do gem 'binding_of_caller' gem 'letter_opener' gem 'bullet' + gem 'active_record_query_trace' end group :production do diff --git a/Gemfile.lock b/Gemfile.lock index aab4ee335b..7c504d9d1e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -36,6 +36,7 @@ GEM erubis (~> 2.7.0) rails-dom-testing (~> 2.0) rails-html-sanitizer (~> 1.0, >= 1.0.2) + active_record_query_trace (1.5.3) activejob (5.0.0.1) activesupport (= 5.0.0.1) globalid (>= 0.3.6) @@ -360,6 +361,7 @@ PLATFORMS ruby DEPENDENCIES + active_record_query_trace addressable better_errors binding_of_caller diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 3c02e0becd..c10a2c680c 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -6,14 +6,21 @@ class AccountsController < ApplicationController def show respond_to do |format| - format.html { @statuses = @account.statuses.order('id desc').with_includes.with_counters.paginate(page: params[:page], per_page: 10)} + format.html do + @statuses = @account.statuses.order('id desc').with_includes.with_counters.paginate(page: params[:page], per_page: 10) + + if user_signed_in? + status_ids = @statuses.collect { |s| [s.id, s.reblog_of_id] }.flatten.uniq + @favourited = Favourite.where(status_id: status_ids).where(account_id: current_user.account_id).map { |f| [f.status_id, true] }.to_h + @reblogged = Status.where(reblog_of_id: status_ids).where(account_id: current_user.account_id).map { |s| [s.reblog_of_id, true] }.to_h + else + @favourited = {} + @reblogged = {} + end + end format.atom do @entries = @account.stream_entries.order('id desc').with_includes.paginate_by_max_id(20, params[:max_id] || nil) - - ActiveRecord::Associations::Preloader.new.preload(@entries.select { |a| a.activity_type == 'Status' }, activity: [:mentions, :media_attachments, reblog: :account, thread: :account]) - ActiveRecord::Associations::Preloader.new.preload(@entries.select { |a| a.activity_type == 'Favourite' }, activity: [:account, :status]) - ActiveRecord::Associations::Preloader.new.preload(@entries.select { |a| a.activity_type == 'Follow' }, activity: :target_account) end end end diff --git a/app/controllers/api/accounts/lookup_controller.rb b/app/controllers/api/accounts/lookup_controller.rb deleted file mode 100644 index 319401a2e2..0000000000 --- a/app/controllers/api/accounts/lookup_controller.rb +++ /dev/null @@ -1,14 +0,0 @@ -class Api::Accounts::LookupController < ApiController - before_action :doorkeeper_authorize! - respond_to :json - - def index - @accounts = Account.where(domain: nil).where(username: lookup_params) - end - - private - - def lookup_params - (params[:usernames] || '').split(',').map(&:strip) - end -end diff --git a/app/helpers/api/accounts/lookup_helper.rb b/app/helpers/api/accounts/lookup_helper.rb deleted file mode 100644 index 5caf0e28cc..0000000000 --- a/app/helpers/api/accounts/lookup_helper.rb +++ /dev/null @@ -1,2 +0,0 @@ -module Api::Accounts::LookupHelper -end diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index e0105ee54f..e43875544a 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -22,23 +22,26 @@ module ApplicationHelper def account_from_mentions(search_string, mentions) mentions.each { |x| return x.account if x.account.acct.eql?(search_string) } + nil # If that was unsuccessful, try fetching user from db separately # But this shouldn't ever happen if the mentions were created correctly! - username, domain = search_string.split('@') + # username, domain = search_string.split('@') - if domain == Rails.configuration.x.local_domain - account = Account.find_local(username) - else - account = Account.find_remote(username, domain) - end + # if domain == Rails.configuration.x.local_domain + # account = Account.find_local(username) + # else + # account = Account.find_remote(username, domain) + # end - account + # account end def linkify(status) auto_link(HTMLEntities.new.encode(status.text), link: :urls, html: { rel: 'nofollow noopener' }).gsub(Account::MENTION_RE) do |m| - if account = account_from_mentions(Account::MENTION_RE.match(m)[1], status.mentions) + account = account_from_mentions(Account::MENTION_RE.match(m)[1], status.mentions) + + unless account.nil? "#{m.split('@').first}@#{account.acct}" else m diff --git a/app/helpers/stream_entries_helper.rb b/app/helpers/stream_entries_helper.rb index a29014d1b0..ce77206ea0 100644 --- a/app/helpers/stream_entries_helper.rb +++ b/app/helpers/stream_entries_helper.rb @@ -21,11 +21,11 @@ module StreamEntriesHelper end def reblogged_by_me_class(status) - user_signed_in? && current_user.account.reblogged?(status) ? 'reblogged' : '' + user_signed_in? && @reblogged.has_key?(status.id) ? 'reblogged' : '' end def favourited_by_me_class(status) - user_signed_in? && current_user.account.favourited?(status) ? 'favourited' : '' + user_signed_in? && @favourited.has_key?(status.id) ? 'favourited' : '' end def proper_status(status) diff --git a/app/models/status.rb b/app/models/status.rb index df9eceaff9..12c58733c0 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -18,7 +18,7 @@ class Status < ApplicationRecord validates :text, presence: true, if: Proc.new { |s| s.local? && !s.reblog? } scope :with_counters, -> { select('statuses.*, (select count(r.id) from statuses as r where r.reblog_of_id = statuses.id) as reblogs_count, (select count(f.id) from favourites as f where f.status_id = statuses.id) as favourites_count') } - scope :with_includes, -> { includes(:account, :mentions, :media_attachments, :stream_entry, reblog: [:account, :mentions], thread: :account) } + scope :with_includes, -> { includes(:account, :media_attachments, :stream_entry, mentions: :account, reblog: [:account, mentions: :account], thread: :account) } def local? self.uri.nil? diff --git a/app/models/stream_entry.rb b/app/models/stream_entry.rb index 165f62f200..f332957964 100644 --- a/app/models/stream_entry.rb +++ b/app/models/stream_entry.rb @@ -4,9 +4,15 @@ class StreamEntry < ApplicationRecord belongs_to :account, inverse_of: :stream_entries belongs_to :activity, polymorphic: true + belongs_to :status, foreign_type: 'Status', foreign_key: 'activity_id' + belongs_to :follow, foreign_type: 'Follow', foreign_key: 'activity_id' + belongs_to :favourite, foreign_type: 'Favourite', foreign_key: 'activity_id' + validates :account, :activity, presence: true - scope :with_includes, -> { includes(:activity) } + STATUS_INCLUDES = [:account, :stream_entry, :media_attachments, mentions: :account, reblog: [:stream_entry, :account, mentions: :account], thread: [:stream_entry, :account]] + + scope :with_includes, -> { includes(:account, status: STATUS_INCLUDES, favourite: [:account, :stream_entry, status: STATUS_INCLUDES], follow: [:target_account, :stream_entry]) } def object_type orphaned? ? :activity : (targeted? ? :activity : self.activity.object_type) @@ -44,6 +50,10 @@ class StreamEntry < ApplicationRecord self.activity.respond_to?(:mentions) ? self.activity.mentions.map { |x| x.account } : [] end + def activity + self.send(self.activity_type.downcase.to_sym) + end + private def orphaned? diff --git a/config/environments/development.rb b/config/environments/development.rb index d0ff03754b..3a2ab2a0ea 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -59,9 +59,9 @@ Rails.application.configure do config.action_mailer.delivery_method = :letter_opener config.after_initialize do - Bullet.enable = true + Bullet.enable = true Bullet.bullet_logger = true - Bullet.rails_logger = true + Bullet.rails_logger = false Bullet.add_whitelist type: :n_plus_one_query, class_name: 'User', association: :account end @@ -71,3 +71,5 @@ end require 'sidekiq/testing' Sidekiq::Testing.inline! + +ActiveRecordQueryTrace.enabled = true diff --git a/config/routes.rb b/config/routes.rb index 6201a4ee72..297426f6a2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -56,10 +56,6 @@ Rails.application.routes.draw do resources :media, only: [:create] resources :accounts, only: [:show] do - collection do - get :lookup, to: 'accounts/lookup#index', as: :lookup - end - member do get :statuses get :followers diff --git a/spec/controllers/api/accounts/lookup_controller_spec.rb b/spec/controllers/api/accounts/lookup_controller_spec.rb deleted file mode 100644 index 3f590b82f0..0000000000 --- a/spec/controllers/api/accounts/lookup_controller_spec.rb +++ /dev/null @@ -1,24 +0,0 @@ -require 'rails_helper' - -RSpec.describe Api::Accounts::LookupController, type: :controller do - render_views - - let(:user) { Fabricate(:user, account: Fabricate(:account, username: 'alice')) } - let(:token) { double acceptable?: true, resource_owner_id: user.id } - - before do - allow(controller).to receive(:doorkeeper_token) { token } - end - - describe 'GET #index' do - before do - Fabricate(:account, username: 'bob') - Fabricate(:account, username: 'mcbeth') - get :index, params: { usernames: 'alice,bob,mcbeth' } - end - - it 'returns http success' do - expect(response).to have_http_status(:success) - end - end -end diff --git a/spec/helpers/api/accounts/lookup_helper_spec.rb b/spec/helpers/api/accounts/lookup_helper_spec.rb deleted file mode 100644 index 8ae1c6f9d1..0000000000 --- a/spec/helpers/api/accounts/lookup_helper_spec.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'rails_helper' - -RSpec.describe Api::Accounts::LookupHelper, type: :helper do - -end