From 08c3e59aeed34ad71e74afb674ddda7327fdc3a7 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 9 Sep 2019 17:05:55 -0700 Subject: Optimize /admin/applications so that it does not timeout On our dev instance, /admin/applications as not loading because: 1. There was an unindexed query by `application_id`. 2. There was an expensive query that attempted to load 1 million unique entries via ActiveRecord just to find the unique count. We fix the first issue by adding an index for that column. We fix the second issue with a simple SELECT COUNT(DISTINCT resource_owner_id) SQL query. In addition, we add pagination to avoid loading more than 20 applications at once. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/67228 --- app/controllers/admin/applications_controller.rb | 4 +++- app/models/oauth_access_token.rb | 2 ++ app/views/admin/applications/index.html.haml | 3 ++- .../unreleased/sh-fix-oauth-application-page.yml | 5 ++++ ...dex_on_application_id_on_oauth_access_tokens.rb | 17 +++++++++++++ db/schema.rb | 3 ++- .../admin/applications_controller_spec.rb | 10 ++++++++ spec/models/oauth_access_token_spec.rb | 28 ++++++++++++++++++++++ 8 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-oauth-application-page.yml create mode 100644 db/post_migrate/20190910000130_add_index_on_application_id_on_oauth_access_tokens.rb create mode 100644 spec/models/oauth_access_token_spec.rb diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb index 3648c8be426..22e629ccf59 100644 --- a/app/controllers/admin/applications_controller.rb +++ b/app/controllers/admin/applications_controller.rb @@ -7,7 +7,9 @@ class Admin::ApplicationsController < Admin::ApplicationController before_action :load_scopes, only: [:new, :create, :edit, :update] def index - @applications = ApplicationsFinder.new.execute + applications = ApplicationsFinder.new.execute + @applications = Kaminari.paginate_array(applications).page(params[:page]) + @application_counts = OauthAccessToken.distinct_resource_owner_counts(@applications) end def show diff --git a/app/models/oauth_access_token.rb b/app/models/oauth_access_token.rb index 0aa920fa828..9789d8ed62b 100644 --- a/app/models/oauth_access_token.rb +++ b/app/models/oauth_access_token.rb @@ -6,6 +6,8 @@ class OauthAccessToken < Doorkeeper::AccessToken alias_attribute :user, :resource_owner + scope :distinct_resource_owner_counts, ->(applications) { where(application: applications).distinct.group(:application_id).count(:resource_owner_id) } + def scopes=(value) if value.is_a?(Array) super(Doorkeeper::OAuth::Scopes.from_array(value).to_s) diff --git a/app/views/admin/applications/index.html.haml b/app/views/admin/applications/index.html.haml index 2cdf98075d1..758d722cc63 100644 --- a/app/views/admin/applications/index.html.haml +++ b/app/views/admin/applications/index.html.haml @@ -19,7 +19,8 @@ %tr{ :id => "application_#{application.id}" } %td= link_to application.name, admin_application_path(application) %td= application.redirect_uri - %td= application.access_tokens.map(&:resource_owner_id).uniq.count + %td= @application_counts[application.id].to_i %td= application.trusted? ? 'Y': 'N' %td= link_to 'Edit', edit_admin_application_path(application), class: 'btn btn-link' %td= render 'delete_form', application: application += paginate @applications, theme: 'gitlab' diff --git a/changelogs/unreleased/sh-fix-oauth-application-page.yml b/changelogs/unreleased/sh-fix-oauth-application-page.yml new file mode 100644 index 00000000000..9ac78db2c79 --- /dev/null +++ b/changelogs/unreleased/sh-fix-oauth-application-page.yml @@ -0,0 +1,5 @@ +--- +title: Optimize /admin/applications so that it does not timeout +merge_request: 32852 +author: +type: performance diff --git a/db/post_migrate/20190910000130_add_index_on_application_id_on_oauth_access_tokens.rb b/db/post_migrate/20190910000130_add_index_on_application_id_on_oauth_access_tokens.rb new file mode 100644 index 00000000000..78f7c0ecf0f --- /dev/null +++ b/db/post_migrate/20190910000130_add_index_on_application_id_on_oauth_access_tokens.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIndexOnApplicationIdOnOauthAccessTokens < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :oauth_access_tokens, :application_id + end + + def down + remove_concurrent_index :oauth_access_tokens, :application_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 6ddfb8bcb39..342e3a8d623 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_09_05_223900) do +ActiveRecord::Schema.define(version: 2019_09_10_000130) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -2390,6 +2390,7 @@ ActiveRecord::Schema.define(version: 2019_09_05_223900) do t.datetime "revoked_at" t.datetime "created_at", null: false t.string "scopes" + t.index ["application_id"], name: "index_oauth_access_tokens_on_application_id" t.index ["refresh_token"], name: "index_oauth_access_tokens_on_refresh_token", unique: true t.index ["resource_owner_id"], name: "index_oauth_access_tokens_on_resource_owner_id" t.index ["token"], name: "index_oauth_access_tokens_on_token", unique: true diff --git a/spec/controllers/admin/applications_controller_spec.rb b/spec/controllers/admin/applications_controller_spec.rb index 9c9f658a0bd..2f3c7da484b 100644 --- a/spec/controllers/admin/applications_controller_spec.rb +++ b/spec/controllers/admin/applications_controller_spec.rb @@ -10,6 +10,16 @@ describe Admin::ApplicationsController do sign_in(admin) end + describe 'GET #index' do + render_views + + it 'renders the application form' do + get :index + + expect(response).to have_http_status(200) + end + end + describe 'GET #new' do it 'renders the application form' do get :new diff --git a/spec/models/oauth_access_token_spec.rb b/spec/models/oauth_access_token_spec.rb new file mode 100644 index 00000000000..0a1c576a5e7 --- /dev/null +++ b/spec/models/oauth_access_token_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe OauthAccessToken do + let(:user) { create(:user) } + let(:app_one) { create(:oauth_application) } + let(:app_two) { create(:oauth_application) } + let(:app_three) { create(:oauth_application) } + let(:tokens) { described_class.all } + + before do + create(:oauth_access_token, application_id: app_one.id) + create_list(:oauth_access_token, 2, resource_owner: user, application_id: app_two.id) + end + + it 'returns unique owners' do + expect(tokens.count).to eq(3) + expect(tokens.distinct_resource_owner_counts([app_one])).to eq({ app_one.id => 1 }) + expect(tokens.distinct_resource_owner_counts([app_two])).to eq({ app_two.id => 1 }) + expect(tokens.distinct_resource_owner_counts([app_three])).to eq({}) + expect(tokens.distinct_resource_owner_counts([app_one, app_two])) + .to eq({ + app_one.id => 1, + app_two.id => 1 + }) + end +end -- cgit v1.2.1