Commit 45d9380b authored by Robert Speicher's avatar Robert Speicher

Merge branch 'optimise-avatar-requests-part-two' into 'master'

Remove unnecessary SQL queries from avatar requests

See merge request gitlab-org/gitlab!19822
parents 317d0ac6 7f1d9323
......@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base
before_action :authenticate_user!, except: [:route_not_found]
before_action :enforce_terms!, if: :should_enforce_terms?
before_action :validate_user_service_ticket!
before_action :check_password_expiration
before_action :check_password_expiration, if: :html_request?
before_action :ldap_security_check
before_action :sentry_context
before_action :default_headers
before_action :add_gon_variables, unless: [:peek_request?, :json_request?]
before_action :add_gon_variables, if: :html_request?
before_action :configure_permitted_parameters, if: :devise_controller?
before_action :require_email, unless: :devise_controller?
before_action :active_user_check, unless: :devise_controller?
......@@ -455,8 +455,8 @@ class ApplicationController < ActionController::Base
response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
end
def peek_request?
request.path.start_with?('/-/peek')
def html_request?
request.format.html?
end
def json_request?
......@@ -466,7 +466,7 @@ class ApplicationController < ActionController::Base
def should_enforce_terms?
return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms
!(peek_request? || devise_controller?)
html_request? && !devise_controller?
end
def set_usage_stats_consent_flag
......
......@@ -4,15 +4,18 @@ module ConfirmEmailWarning
extend ActiveSupport::Concern
included do
before_action :set_confirm_warning, if: -> { Feature.enabled?(:soft_email_confirmation) }
before_action :set_confirm_warning, if: :show_confirm_warning?
end
protected
def show_confirm_warning?
html_request? && request.get? && Feature.enabled?(:soft_email_confirmation)
end
def set_confirm_warning
return unless current_user
return if current_user.confirmed?
return if peek_request? || json_request? || !request.get?
email = current_user.unconfirmed_email || current_user.email
......
# frozen_string_literal: true
module UploadsActions
extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize
include SendFileUpload
UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze
included do
prepend_before_action :set_request_format_from_path_extension
end
def create
uploader = UploadService.new(model, params[:file], uploader_class).execute
......@@ -64,6 +69,18 @@ module UploadsActions
private
# From ActionDispatch::Http::MimeNegotiation. We have an initializer that
# monkey-patches this method out (so that repository paths don't guess a
# format based on extension), but we do want this behaviour when serving
# uploads.
def set_request_format_from_path_extension
path = request.headers['action_dispatch.original_path'] || request.headers['PATH_INFO']
if match = path&.match(/\.(\w+)\z/)
request.format = match.captures.first
end
end
def uploader_class
raise NotImplementedError
end
......
......@@ -20,7 +20,7 @@ class UploadsController < ApplicationController
skip_before_action :authenticate_user!
before_action :upload_mount_satisfied?
before_action :find_model
before_action :model
before_action :authorize_access!, only: [:show]
before_action :authorize_create_access!, only: [:create, :authorize]
before_action :verify_workhorse_api!, only: [:authorize]
......
......@@ -90,14 +90,6 @@ describe ApplicationController do
let(:format) { :html }
it_behaves_like 'setting gon variables'
context 'for peek requests' do
before do
request.path = '/-/peek'
end
it_behaves_like 'not setting gon variables'
end
end
context 'with json format' do
......@@ -105,6 +97,12 @@ describe ApplicationController do
it_behaves_like 'not setting gon variables'
end
context 'with atom format' do
let(:format) { :atom }
it_behaves_like 'not setting gon variables'
end
end
describe 'session expiration' do
......
......@@ -228,10 +228,10 @@ describe UploadsController do
user.block
end
it "redirects to the sign in page" do
it "responds with status 401" do
get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" }
expect(response).to redirect_to(new_user_session_path)
expect(response).to have_gitlab_http_status(401)
end
end
......@@ -320,10 +320,10 @@ describe UploadsController do
end
context "when not signed in" do
it "redirects to the sign in page" do
it "responds with status 401" do
get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
expect(response).to redirect_to(new_user_session_path)
expect(response).to have_gitlab_http_status(401)
end
end
......@@ -343,10 +343,10 @@ describe UploadsController do
project.add_maintainer(user)
end
it "redirects to the sign in page" do
it "responds with status 401" do
get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
expect(response).to redirect_to(new_user_session_path)
expect(response).to have_gitlab_http_status(401)
end
end
......@@ -439,10 +439,10 @@ describe UploadsController do
user.block
end
it "redirects to the sign in page" do
it "responds with status 401" do
get :show, params: { model: "group", mounted_as: "avatar", id: group.id, filename: "dk.png" }
expect(response).to redirect_to(new_user_session_path)
expect(response).to have_gitlab_http_status(401)
end
end
......@@ -526,10 +526,10 @@ describe UploadsController do
end
context "when not signed in" do
it "redirects to the sign in page" do
it "responds with status 401" do
get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
expect(response).to redirect_to(new_user_session_path)
expect(response).to have_gitlab_http_status(401)
end
end
......@@ -549,10 +549,10 @@ describe UploadsController do
project.add_maintainer(user)
end
it "redirects to the sign in page" do
it "responds with status 401" do
get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
expect(response).to redirect_to(new_user_session_path)
expect(response).to have_gitlab_http_status(401)
end
end
......
# frozen_string_literal: true
require 'spec_helper'
describe 'Loading a user avatar' do
let(:user) { create(:user, :with_avatar) }
context 'when logged in' do
# The exact query count will vary depending on the 2FA settings of the
# instance, group, and user. Removing those extra 2FA queries in this case
# may not be a good idea, so we just set up the ideal case.
before do
stub_application_setting(require_two_factor_authentication: true)
login_as(create(:user, :two_factor))
end
# One each for: current user, avatar user, and upload record
it 'only performs three SQL queries' do
get user.avatar_url # Skip queries on first application load
expect(response).to have_gitlab_http_status(200)
expect { get user.avatar_url }.not_to exceed_query_limit(3)
end
end
context 'when logged out' do
# One each for avatar user and upload record
it 'only performs two SQL queries' do
get user.avatar_url # Skip queries on first application load
expect(response).to have_gitlab_http_status(200)
expect { get user.avatar_url }.not_to exceed_query_limit(2)
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment