diff options
author | Stan Hu <stanhu@gmail.com> | 2019-09-10 03:05:55 +0300 |
---|---|---|
committer | Stan Hu <stanhu@gmail.com> | 2019-09-10 07:51:57 +0300 |
commit | 08c3e59aeed34ad71e74afb674ddda7327fdc3a7 (patch) | |
tree | 544d9fa244519ac8b18b29ff8abf47bfdd40b30a | |
parent | ffa5328c39f195d3253e586569fc2474d3aa6860 (diff) |
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
-rw-r--r-- | app/controllers/admin/applications_controller.rb | 4 | ||||
-rw-r--r-- | app/models/oauth_access_token.rb | 2 | ||||
-rw-r--r-- | app/views/admin/applications/index.html.haml | 3 | ||||
-rw-r--r-- | changelogs/unreleased/sh-fix-oauth-application-page.yml | 5 | ||||
-rw-r--r-- | db/post_migrate/20190910000130_add_index_on_application_id_on_oauth_access_tokens.rb | 17 | ||||
-rw-r--r-- | db/schema.rb | 3 | ||||
-rw-r--r-- | spec/controllers/admin/applications_controller_spec.rb | 10 | ||||
-rw-r--r-- | spec/models/oauth_access_token_spec.rb | 28 |
8 files changed, 69 insertions, 3 deletions
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 |