diff options
22 files changed, 273 insertions, 42 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 1bb451b207a..8fb44fa826d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,10 +2,6 @@ documentation](doc/development/changelog.md) for instructions on adding your own entry. -## 12.6.3 - -- No changes. - ## 12.6.2 ### Security (6 changes) diff --git a/app/controllers/projects/error_tracking_controller.rb b/app/controllers/projects/error_tracking_controller.rb index dea8cd81f4f..88f739ce29e 100644 --- a/app/controllers/projects/error_tracking_controller.rb +++ b/app/controllers/projects/error_tracking_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Projects::ErrorTrackingController < Projects::ErrorTracking::BaseController + respond_to :json + before_action :authorize_read_sentry_issue! before_action :set_issue_id, only: :details @@ -24,6 +26,17 @@ class Projects::ErrorTrackingController < Projects::ErrorTracking::BaseControlle end end + def update + service = ErrorTracking::IssueUpdateService.new(project, current_user, issue_update_params) + result = service.execute + + return if handle_errors(result) + + render json: { + result: result + } + end + private def render_index_json @@ -65,6 +78,10 @@ class Projects::ErrorTrackingController < Projects::ErrorTracking::BaseControlle params.permit(:search_term, :sort, :cursor) end + def issue_update_params + params.permit(:issue_id, :status) + end + def issue_details_params params.permit(:issue_id) end diff --git a/app/helpers/projects/error_tracking_helper.rb b/app/helpers/projects/error_tracking_helper.rb index de21a78f5f0..2ec7fa91c86 100644 --- a/app/helpers/projects/error_tracking_helper.rb +++ b/app/helpers/projects/error_tracking_helper.rb @@ -20,6 +20,7 @@ module Projects::ErrorTrackingHelper { 'project-issues-path' => project_issues_path(project), 'issue-details-path' => details_project_error_tracking_index_path(*opts), + 'issue-update-path' => update_project_error_tracking_index_path(*opts), 'issue-stack-trace-path' => stack_trace_project_error_tracking_index_path(*opts) } end diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb index f229b42ade6..0f2a389f0a3 100644 --- a/app/models/concerns/awardable.rb +++ b/app/models/concerns/awardable.rb @@ -67,7 +67,9 @@ module Awardable ) ).join_sources - joins(join_clause).group(awardable_table[:id]).reorder("COUNT(award_emoji.id) #{direction}") + joins(join_clause).group(awardable_table[:id]).reorder( + Arel.sql("COUNT(award_emoji.id) #{direction}") + ) end end diff --git a/app/models/error_tracking/project_error_tracking_setting.rb b/app/models/error_tracking/project_error_tracking_setting.rb index bbafcaed46e..4aba70cb124 100644 --- a/app/models/error_tracking/project_error_tracking_setting.rb +++ b/app/models/error_tracking/project_error_tracking_setting.rb @@ -101,30 +101,27 @@ module ErrorTracking end end + def update_issue(opts = {} ) + handle_exceptions do + { updated: sentry_client.update_issue(opts) } + end + end + def calculate_reactive_cache(request, opts) - case request - when 'list_issues' - sentry_client.list_issues(**opts.symbolize_keys) - when 'issue_details' - { - issue: sentry_client.issue_details(**opts.symbolize_keys) - } - when 'issue_latest_event' - { - latest_event: sentry_client.issue_latest_event(**opts.symbolize_keys) - } + handle_exceptions do + case request + when 'list_issues' + sentry_client.list_issues(**opts.symbolize_keys) + when 'issue_details' + { + issue: sentry_client.issue_details(**opts.symbolize_keys) + } + when 'issue_latest_event' + { + latest_event: sentry_client.issue_latest_event(**opts.symbolize_keys) + } + end end - rescue Sentry::Client::Error => e - { error: e.message, error_type: SENTRY_API_ERROR_TYPE_NON_20X_RESPONSE } - rescue Sentry::Client::MissingKeysError => e - { error: e.message, error_type: SENTRY_API_ERROR_TYPE_MISSING_KEYS } - rescue Sentry::Client::ResponseInvalidSizeError => e - { error: e.message, error_type: SENTRY_API_ERROR_INVALID_SIZE } - rescue Sentry::Client::BadRequestError => e - { error: e.message, error_type: SENTRY_API_ERROR_TYPE_BAD_REQUEST } - rescue StandardError => e - Gitlab::ErrorTracking.track_exception(e) - { error: 'Unexpected Error' } end # http://HOST/api/0/projects/ORG/PROJECT @@ -143,6 +140,21 @@ module ErrorTracking private + def handle_exceptions + yield + rescue Sentry::Client::Error => e + { error: e.message, error_type: SENTRY_API_ERROR_TYPE_NON_20X_RESPONSE } + rescue Sentry::Client::MissingKeysError => e + { error: e.message, error_type: SENTRY_API_ERROR_TYPE_MISSING_KEYS } + rescue Sentry::Client::ResponseInvalidSizeError => e + { error: e.message, error_type: SENTRY_API_ERROR_INVALID_SIZE } + rescue Sentry::Client::BadRequestError => e + { error: e.message, error_type: SENTRY_API_ERROR_TYPE_BAD_REQUEST } + rescue StandardError => e + Gitlab::ErrorTracking.track_exception(e) + { error: 'Unexpected Error' } + end + def project_name_from_slug @project_name_from_slug ||= project_slug_from_api_url&.titleize end diff --git a/app/services/error_tracking/issue_update_service.rb b/app/services/error_tracking/issue_update_service.rb new file mode 100644 index 00000000000..e433b4a11f2 --- /dev/null +++ b/app/services/error_tracking/issue_update_service.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module ErrorTracking + class IssueUpdateService < ErrorTracking::BaseService + private + + def fetch + project_error_tracking_setting.update_issue( + issue_id: params[:issue_id], + params: update_params + ) + end + + def update_params + params.except(:issue_id) + end + + def parse_response(response) + { updated: response[:updated].present? } + end + end +end diff --git a/changelogs/unreleased/121670-redis-cache-read-error-prevents-cas-users-from-remaining-signed-in.yml b/changelogs/unreleased/121670-redis-cache-read-error-prevents-cas-users-from-remaining-signed-in.yml new file mode 100644 index 00000000000..3a54a6dce20 --- /dev/null +++ b/changelogs/unreleased/121670-redis-cache-read-error-prevents-cas-users-from-remaining-signed-in.yml @@ -0,0 +1,5 @@ +--- +title: Fix CAS users being signed out repeatedly +merge_request: 22704 +author: +type: fixed diff --git a/changelogs/unreleased/39825-resolve-sentry-error-backend.yml b/changelogs/unreleased/39825-resolve-sentry-error-backend.yml new file mode 100644 index 00000000000..bb879088d76 --- /dev/null +++ b/changelogs/unreleased/39825-resolve-sentry-error-backend.yml @@ -0,0 +1,5 @@ +--- +title: Add internal API to update Sentry error status +merge_request: 22454 +author: +type: added diff --git a/changelogs/unreleased/ab-projects-api-created-at-indexes.yml b/changelogs/unreleased/ab-projects-api-created-at-indexes.yml new file mode 100644 index 00000000000..8948106db08 --- /dev/null +++ b/changelogs/unreleased/ab-projects-api-created-at-indexes.yml @@ -0,0 +1,5 @@ +--- +title: Create optimal indexes for created_at order (Projects API) +merge_request: 22623 +author: +type: performance diff --git a/config/application.rb b/config/application.rb index da674b2b944..f9cc1cb543a 100644 --- a/config/application.rb +++ b/config/application.rb @@ -261,6 +261,10 @@ module Gitlab caching_config_hash[:pool_timeout] = 1 end + # Overrides RedisCacheStore's default value of 0 + # This makes the default value the same with Gitlab::Redis::Cache + caching_config_hash[:reconnect_attempts] ||= ::Redis::Client::DEFAULTS[:reconnect_attempts] + config.cache_store = :redis_cache_store, caching_config_hash config.active_job.queue_adapter = :sidekiq diff --git a/config/routes/project.rb b/config/routes/project.rb index be207f155f9..b420f07c7a1 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -275,6 +275,9 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do get ':issue_id/stack_trace', to: 'error_tracking/stack_traces#index', as: 'stack_trace' + put ':issue_id', + to: 'error_tracking#update', + as: 'update' end end diff --git a/db/migrate/20200108155731_create_indexes_for_project_api_created_at_order.rb b/db/migrate/20200108155731_create_indexes_for_project_api_created_at_order.rb new file mode 100644 index 00000000000..2356f08b681 --- /dev/null +++ b/db/migrate/20200108155731_create_indexes_for_project_api_created_at_order.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class CreateIndexesForProjectApiCreatedAtOrder < ActiveRecord::Migration[5.2] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :projects, %i(visibility_level created_at id), order: { id: :desc }, name: 'index_projects_on_visibility_level_created_at_id_desc' + add_concurrent_index :projects, %i(visibility_level created_at id), order: { created_at: :desc, id: :desc }, name: 'index_projects_on_visibility_level_created_at_desc_id_desc' + remove_concurrent_index_by_name :projects, 'index_projects_on_visibility_level_and_created_at_and_id' + end + + def down + add_concurrent_index :projects, %i(visibility_level created_at id), name: 'index_projects_on_visibility_level_and_created_at_and_id' + remove_concurrent_index_by_name :projects, 'index_projects_on_visibility_level_created_at_id_desc' + remove_concurrent_index_by_name :projects, 'index_projects_on_visibility_level_created_at_desc_id_desc' + end +end diff --git a/db/schema.rb b/db/schema.rb index 1d2b4020915..e0f419f1a74 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: 2020_01_08_100603) do +ActiveRecord::Schema.define(version: 2020_01_08_155731) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -3376,7 +3376,8 @@ ActiveRecord::Schema.define(version: 2020_01_08_100603) do t.index ["runners_token"], name: "index_projects_on_runners_token" t.index ["runners_token_encrypted"], name: "index_projects_on_runners_token_encrypted" t.index ["star_count"], name: "index_projects_on_star_count" - t.index ["visibility_level", "created_at", "id"], name: "index_projects_on_visibility_level_and_created_at_and_id" + t.index ["visibility_level", "created_at", "id"], name: "index_projects_on_visibility_level_created_at_desc_id_desc", order: { created_at: :desc, id: :desc } + t.index ["visibility_level", "created_at", "id"], name: "index_projects_on_visibility_level_created_at_id_desc", order: { id: :desc } end create_table "prometheus_alert_events", force: :cascade do |t| diff --git a/lib/sentry/client.rb b/lib/sentry/client.rb index 40821e2b233..29b00a6f8b2 100644 --- a/lib/sentry/client.rb +++ b/lib/sentry/client.rb @@ -43,7 +43,7 @@ module Sentry def http_put(url, params = {}) http_request do - Gitlab::HTTP.put(url, **request_params.merge({ body: params })) + Gitlab::HTTP.put(url, **request_params.merge(body: params)) end end diff --git a/lib/sentry/client/issue.rb b/lib/sentry/client/issue.rb index b6f2e07d233..eb5b32289ba 100644 --- a/lib/sentry/client/issue.rb +++ b/lib/sentry/client/issue.rb @@ -34,6 +34,10 @@ module Sentry map_to_detailed_error(issue) end + def update_issue(issue_id:, params:) + http_put(issue_api_url(issue_id), params)[:body] + end + private def get_issues(**keyword_args) @@ -71,10 +75,6 @@ module Sentry http_get(issue_api_url(issue_id))[:body] end - def update_issue(issue_id:, params:) - http_put(issue_api_url(issue_id), params)[:body] - end - def issues_api_url issues_url = URI("#{url}/issues/") issues_url.path.squeeze!('/') diff --git a/qa/qa/specs/features/browser_ui/3_create/web_ide/add_file_template_spec.rb b/qa/qa/specs/features/browser_ui/3_create/web_ide/add_file_template_spec.rb index 7c9db5ee496..70b571a316a 100644 --- a/qa/qa/specs/features/browser_ui/3_create/web_ide/add_file_template_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/web_ide/add_file_template_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module QA - context 'Create' do + context 'Create', quarantine: 'https://gitlab.com/gitlab-org/gitlab/issues/196034' do describe 'Web IDE file templates' do include Runtime::Fixtures diff --git a/qa/qa/specs/features/browser_ui/6_release/deploy_key/clone_using_deploy_key_spec.rb b/qa/qa/specs/features/browser_ui/6_release/deploy_key/clone_using_deploy_key_spec.rb index 2e03e4bd43c..0ca49bd080b 100644 --- a/qa/qa/specs/features/browser_ui/6_release/deploy_key/clone_using_deploy_key_spec.rb +++ b/qa/qa/specs/features/browser_ui/6_release/deploy_key/clone_using_deploy_key_spec.rb @@ -3,7 +3,7 @@ require 'digest/sha1' module QA - context 'Release', :docker do + context 'Release', :docker, quarantine: 'https://gitlab.com/gitlab-org/gitlab/issues/196047' do describe 'Git clone using a deploy key' do before do Flow::Login.sign_in diff --git a/spec/controllers/projects/error_tracking_controller_spec.rb b/spec/controllers/projects/error_tracking_controller_spec.rb index a0ecc288b15..ce96eb1b9a9 100644 --- a/spec/controllers/projects/error_tracking_controller_spec.rb +++ b/spec/controllers/projects/error_tracking_controller_spec.rb @@ -271,6 +271,58 @@ describe Projects::ErrorTrackingController do end end + describe 'PUT #update' do + let(:issue_id) { 1234 } + let(:issue_update_service) { spy(:issue_update_service) } + let(:permitted_params) do + ActionController::Parameters.new( + { issue_id: issue_id.to_s, status: 'resolved' } + ).permit! + end + + subject(:update_issue) do + put :update, params: issue_params(issue_id: issue_id, status: 'resolved', format: :json) + end + + before do + expect(ErrorTracking::IssueUpdateService) + .to receive(:new).with(project, user, permitted_params) + .and_return(issue_update_service) + end + + describe 'format json' do + context 'update result is successful' do + before do + expect(issue_update_service).to receive(:execute) + .and_return(status: :success, updated: true) + + update_issue + end + + it 'returns a success' do + expect(response).to have_gitlab_http_status(:ok) + expect(response).to match_response_schema('error_tracking/update_issue') + end + end + + context 'update result is erroneous' do + let(:error_message) { 'error message' } + + before do + expect(issue_update_service).to receive(:execute) + .and_return(status: :error, message: error_message) + + update_issue + end + + it 'returns 400 with message' do + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['message']).to eq(error_message) + end + end + end + end + private def issue_params(opts = {}) diff --git a/spec/fixtures/api/schemas/error_tracking/update_issue.json b/spec/fixtures/api/schemas/error_tracking/update_issue.json new file mode 100644 index 00000000000..72514ce647d --- /dev/null +++ b/spec/fixtures/api/schemas/error_tracking/update_issue.json @@ -0,0 +1,16 @@ +{ + "type": "object", + "required" : [ + "result" + ], + "properties" : { + "result": { + "type": "object", + "properties": { + "status": { "type": "string" }, + "updated": { "type": "boolean" } + } + } + }, + "additionalProperties": false +} diff --git a/spec/lib/sentry/client/issue_spec.rb b/spec/lib/sentry/client/issue_spec.rb index b5ee5063e86..f4796a27b4d 100644 --- a/spec/lib/sentry/client/issue_spec.rb +++ b/spec/lib/sentry/client/issue_spec.rb @@ -6,7 +6,9 @@ describe Sentry::Client::Issue do include SentryClientHelpers let(:token) { 'test-token' } + let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0' } let(:client) { Sentry::Client.new(sentry_url, token) } + let(:issue_id) { 503504 } describe '#list_issues' do shared_examples 'issues have correct return type' do |klass| @@ -225,15 +227,11 @@ describe Sentry::Client::Issue do ) end - let(:issue_id) { 503504 } - let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0' } let(:sentry_request_url) { "#{sentry_url}/issues/#{issue_id}/" } let!(:sentry_api_request) { stub_sentry_request(sentry_request_url, body: issue_sample_response) } subject { client.issue_details(issue_id: issue_id) } - it_behaves_like 'calls sentry api' - it 'escapes issue ID' do allow(CGI).to receive(:escape).and_call_original @@ -290,4 +288,41 @@ describe Sentry::Client::Issue do end end end + + describe '#update_issue' do + let(:sentry_url) { 'https://sentrytest.gitlab.com/api/0' } + let(:sentry_request_url) { "#{sentry_url}/issues/#{issue_id}/" } + + before do + stub_sentry_request(sentry_request_url, :put) + end + + let(:params) do + { + status: 'resolved' + } + end + + subject { client.update_issue(issue_id: issue_id, params: params) } + + it_behaves_like 'calls sentry api' do + let(:sentry_api_request) { stub_sentry_request(sentry_request_url, :put) } + end + + it 'returns a truthy result' do + expect(subject).to be_truthy + end + + context 'error encountered' do + let(:error) { StandardError.new('error') } + + before do + allow(client).to receive(:update_issue).and_raise(error) + end + + it 'raises the error' do + expect { subject }.to raise_error(error) + end + end + end end diff --git a/spec/models/error_tracking/project_error_tracking_setting_spec.rb b/spec/models/error_tracking/project_error_tracking_setting_spec.rb index 5b8be7914d4..03e4c04f97e 100644 --- a/spec/models/error_tracking/project_error_tracking_setting_spec.rb +++ b/spec/models/error_tracking/project_error_tracking_setting_spec.rb @@ -210,6 +210,40 @@ describe ErrorTracking::ProjectErrorTrackingSetting do end end + describe '#update_issue' do + let(:opts) do + { status: 'resolved' } + end + + let(:result) do + subject.update_issue(**opts) + end + + let(:sentry_client) { spy(:sentry_client) } + + context 'successful call to sentry' do + before do + allow(subject).to receive(:sentry_client).and_return(sentry_client) + allow(sentry_client).to receive(:update_issue).with(opts).and_return(true) + end + + it 'returns the successful response' do + expect(result).to eq(updated: true) + end + end + + context 'sentry raises an error' do + before do + allow(subject).to receive(:sentry_client).and_return(sentry_client) + allow(sentry_client).to receive(:update_issue).with(opts).and_raise(StandardError) + end + + it 'returns the successful response' do + expect(result).to eq(error: 'Unexpected Error') + end + end + end + context 'slugs' do shared_examples_for 'slug from api_url' do |method, slug| context 'when api_url is correct' do diff --git a/spec/support/helpers/sentry_client_helpers.rb b/spec/support/helpers/sentry_client_helpers.rb index 7476b5fb249..d473fe89fee 100644 --- a/spec/support/helpers/sentry_client_helpers.rb +++ b/spec/support/helpers/sentry_client_helpers.rb @@ -3,8 +3,8 @@ module SentryClientHelpers private - def stub_sentry_request(url, body: {}, status: 200, headers: {}) - stub_request(:get, url) + def stub_sentry_request(url, http_method = :get, body: {}, status: 200, headers: {}) + stub_request(http_method, url) .to_return( status: status, headers: { 'Content-Type' => 'application/json' }.merge(headers), |