Commit 49a1dd59 authored by Vitali Tatarintev's avatar Vitali Tatarintev Committed by Peter Leitzen

Return ServiceResponse from Alert::NotifyService

Unify return type for `Projects::Alerting::NotifyService`
and `Projects::Prometheus::Alerts::NotifyService`
parent 513d8a6b
...@@ -14,7 +14,7 @@ module Projects ...@@ -14,7 +14,7 @@ module Projects
token = extract_alert_manager_token(request) token = extract_alert_manager_token(request)
result = notify_service.execute(token) result = notify_service.execute(token)
head(response_status(result)) head result.http_status
end end
private private
...@@ -33,12 +33,6 @@ module Projects ...@@ -33,12 +33,6 @@ module Projects
.new(project, current_user, notification_payload) .new(project, current_user, notification_payload)
end end
def response_status(result)
return :ok if result.success?
result.http_status
end
def notification_payload def notification_payload
params.permit![:notification] params.permit![:notification]
end end
......
...@@ -26,12 +26,9 @@ module Projects ...@@ -26,12 +26,9 @@ module Projects
def notify def notify
token = extract_alert_manager_token(request) token = extract_alert_manager_token(request)
result = notify_service.execute(token)
if notify_service.execute(token) head result.http_status
head :ok
else
head :unprocessable_entity
end
end end
def create def create
......
...@@ -46,15 +46,15 @@ module Projects ...@@ -46,15 +46,15 @@ module Projects
end end
def bad_request def bad_request
ServiceResponse.error(message: 'Bad Request', http_status: 400) ServiceResponse.error(message: 'Bad Request', http_status: :bad_request)
end end
def unauthorized def unauthorized
ServiceResponse.error(message: 'Unauthorized', http_status: 401) ServiceResponse.error(message: 'Unauthorized', http_status: :unauthorized)
end end
def forbidden def forbidden
ServiceResponse.error(message: 'Forbidden', http_status: 403) ServiceResponse.error(message: 'Forbidden', http_status: :forbidden)
end end
end end
end end
......
...@@ -8,15 +8,15 @@ module Projects ...@@ -8,15 +8,15 @@ module Projects
include IncidentManagement::Settings include IncidentManagement::Settings
def execute(token) def execute(token)
return false unless valid_payload_size? return bad_request unless valid_payload_size?
return false unless valid_version? return unprocessable_entity unless valid_version?
return false unless valid_alert_manager_token?(token) return unauthorized unless valid_alert_manager_token?(token)
persist_events persist_events
send_alert_email if send_email? send_alert_email if send_email?
process_incident_issues if process_issues? process_incident_issues if process_issues?
true ServiceResponse.success
end end
private private
...@@ -118,6 +118,18 @@ module Projects ...@@ -118,6 +118,18 @@ module Projects
def persist_events def persist_events
CreateEventsService.new(project, nil, params).execute CreateEventsService.new(project, nil, params).execute
end end
def bad_request
ServiceResponse.error(message: 'Bad Request', http_status: :bad_request)
end
def unauthorized
ServiceResponse.error(message: 'Unauthorized', http_status: :unauthorized)
end
def unprocessable_entity
ServiceResponse.error(message: 'Unprocessable Entity', http_status: :unprocessable_entity)
end
end end
end end
end end
......
...@@ -87,7 +87,7 @@ describe 'Rack Attack EE throttles' do ...@@ -87,7 +87,7 @@ describe 'Rack Attack EE throttles' do
describe 'requests to prometheus alert notify endpoint with oauth token' do describe 'requests to prometheus alert notify endpoint with oauth token' do
before do before do
allow_next_instance_of(Projects::Prometheus::Alerts::NotifyService) do |instance| allow_next_instance_of(Projects::Prometheus::Alerts::NotifyService) do |instance|
allow(instance).to receive(:execute).and_return true allow(instance).to receive(:execute).and_return(ServiceResponse.success)
end end
end end
...@@ -99,7 +99,7 @@ describe 'Rack Attack EE throttles' do ...@@ -99,7 +99,7 @@ describe 'Rack Attack EE throttles' do
describe 'requests to generic alert notify endpoint with oauth token' do describe 'requests to generic alert notify endpoint with oauth token' do
before do before do
allow_next_instance_of(Projects::Alerting::NotifyService) do |instance| allow_next_instance_of(Projects::Alerting::NotifyService) do |instance|
allow(instance).to receive(:execute).and_return double(success?: true) allow(instance).to receive(:execute).and_return(ServiceResponse.success)
end end
end end
......
...@@ -30,7 +30,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -30,7 +30,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
expect(notification_service) expect(notification_service)
.to receive_message_chain(:async, :prometheus_alerts_fired) .to receive_message_chain(:async, :prometheus_alerts_fired)
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
...@@ -44,7 +44,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -44,7 +44,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
.exactly(amount).times .exactly(amount).times
Sidekiq::Testing.inline! do Sidekiq::Testing.inline! do
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
end end
...@@ -54,7 +54,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -54,7 +54,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
expect(IncidentManagement::ProcessPrometheusAlertWorker) expect(IncidentManagement::ProcessPrometheusAlertWorker)
.not_to receive(:perform_async) .not_to receive(:perform_async)
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
...@@ -69,7 +69,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -69,7 +69,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
expect(create_events_service) expect(create_events_service)
.to receive(:execute) .to receive(:execute)
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
...@@ -78,7 +78,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -78,7 +78,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
it_behaves_like 'persists events' it_behaves_like 'persists events'
end end
shared_examples 'no notifications' do shared_examples 'no notifications' do |http_status:|
let(:notification_service) { spy } let(:notification_service) { spy }
let(:create_events_service) { spy } let(:create_events_service) { spy }
...@@ -86,7 +86,8 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -86,7 +86,8 @@ describe Projects::Prometheus::Alerts::NotifyService do
expect(notification_service).not_to receive(:async) expect(notification_service).not_to receive(:async)
expect(create_events_service).not_to receive(:execute) expect(create_events_service).not_to receive(:execute)
expect(subject).to eq(false) expect(subject).to be_error
expect(subject.http_status).to eq(http_status)
end end
end end
...@@ -130,7 +131,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -130,7 +131,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
end end
context 'with token' do context 'with token' do
it_behaves_like 'no notifications' it_behaves_like 'no notifications', http_status: :unauthorized
end end
end end
end end
......
...@@ -48,7 +48,7 @@ describe Projects::Alerting::NotificationsController do ...@@ -48,7 +48,7 @@ describe Projects::Alerting::NotificationsController do
end end
context 'when notification service fails' do context 'when notification service fails' do
let(:service_response) { ServiceResponse.error(message: 'Unauthorized', http_status: 401) } let(:service_response) { ServiceResponse.error(message: 'Unauthorized', http_status: :unauthorized) }
it 'responds with the service response' do it 'responds with the service response' do
make_request make_request
......
...@@ -158,7 +158,8 @@ describe Projects::Prometheus::AlertsController do ...@@ -158,7 +158,8 @@ describe Projects::Prometheus::AlertsController do
end end
describe 'POST #notify' do describe 'POST #notify' do
let(:notify_service) { spy } let(:service_response) { ServiceResponse.success }
let(:notify_service) { instance_double(Projects::Prometheus::Alerts::NotifyService, execute: service_response) }
before do before do
sign_out(user) sign_out(user)
...@@ -170,7 +171,7 @@ describe Projects::Prometheus::AlertsController do ...@@ -170,7 +171,7 @@ describe Projects::Prometheus::AlertsController do
end end
it 'returns ok if notification succeeds' do it 'returns ok if notification succeeds' do
expect(notify_service).to receive(:execute).and_return(true) expect(notify_service).to receive(:execute).and_return(ServiceResponse.success)
post :notify, params: project_params, session: { as: :json } post :notify, params: project_params, session: { as: :json }
...@@ -178,7 +179,9 @@ describe Projects::Prometheus::AlertsController do ...@@ -178,7 +179,9 @@ describe Projects::Prometheus::AlertsController do
end end
it 'returns unprocessable entity if notification fails' do it 'returns unprocessable entity if notification fails' do
expect(notify_service).to receive(:execute).and_return(false) expect(notify_service).to receive(:execute).and_return(
ServiceResponse.error(message: 'Unprocessable Entity', http_status: :unprocessable_entity)
)
post :notify, params: project_params, session: { as: :json } post :notify, params: project_params, session: { as: :json }
......
...@@ -20,7 +20,7 @@ describe Projects::Alerting::NotifyService do ...@@ -20,7 +20,7 @@ describe Projects::Alerting::NotifyService do
.exactly(amount).times .exactly(amount).times
Sidekiq::Testing.inline! do Sidekiq::Testing.inline! do
expect(subject.status).to eq(:success) expect(subject).to be_success
end end
end end
end end
...@@ -36,7 +36,7 @@ describe Projects::Alerting::NotifyService do ...@@ -36,7 +36,7 @@ describe Projects::Alerting::NotifyService do
expect(notification_service) expect(notification_service)
.to receive_message_chain(:async, :prometheus_alerts_fired) .to receive_message_chain(:async, :prometheus_alerts_fired)
expect(subject.status).to eq(:success) expect(subject).to be_success
end end
end end
...@@ -45,7 +45,7 @@ describe Projects::Alerting::NotifyService do ...@@ -45,7 +45,7 @@ describe Projects::Alerting::NotifyService do
expect(IncidentManagement::ProcessAlertWorker) expect(IncidentManagement::ProcessAlertWorker)
.not_to receive(:perform_async) .not_to receive(:perform_async)
expect(subject.status).to eq(:success) expect(subject).to be_success
end end
end end
...@@ -54,7 +54,7 @@ describe Projects::Alerting::NotifyService do ...@@ -54,7 +54,7 @@ describe Projects::Alerting::NotifyService do
expect(IncidentManagement::ProcessAlertWorker) expect(IncidentManagement::ProcessAlertWorker)
.not_to receive(:perform_async) .not_to receive(:perform_async)
expect(subject.status).to eq(:error) expect(subject).to be_error
expect(subject.http_status).to eq(http_status) expect(subject.http_status).to eq(http_status)
end end
end end
...@@ -102,7 +102,7 @@ describe Projects::Alerting::NotifyService do ...@@ -102,7 +102,7 @@ describe Projects::Alerting::NotifyService do
.and_raise(Gitlab::Alerting::NotificationPayloadParser::BadPayloadError) .and_raise(Gitlab::Alerting::NotificationPayloadParser::BadPayloadError)
end end
it_behaves_like 'does not process incident issues due to error', http_status: 400 it_behaves_like 'does not process incident issues due to error', http_status: :bad_request
end end
end end
...@@ -114,13 +114,13 @@ describe Projects::Alerting::NotifyService do ...@@ -114,13 +114,13 @@ describe Projects::Alerting::NotifyService do
end end
context 'with invalid token' do context 'with invalid token' do
it_behaves_like 'does not process incident issues due to error', http_status: 401 it_behaves_like 'does not process incident issues due to error', http_status: :unauthorized
end end
context 'with deactivated Alerts Service' do context 'with deactivated Alerts Service' do
let!(:alerts_service) { create(:alerts_service, :inactive, project: project) } let!(:alerts_service) { create(:alerts_service, :inactive, project: project) }
it_behaves_like 'does not process incident issues due to error', http_status: 403 it_behaves_like 'does not process incident issues due to error', http_status: :forbidden
end end
end end
end end
......
...@@ -30,7 +30,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -30,7 +30,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
expect(notification_service) expect(notification_service)
.to receive_message_chain(:async, :prometheus_alerts_fired) .to receive_message_chain(:async, :prometheus_alerts_fired)
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
...@@ -44,7 +44,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -44,7 +44,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
.exactly(amount).times .exactly(amount).times
Sidekiq::Testing.inline! do Sidekiq::Testing.inline! do
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
end end
...@@ -54,7 +54,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -54,7 +54,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
expect(IncidentManagement::ProcessPrometheusAlertWorker) expect(IncidentManagement::ProcessPrometheusAlertWorker)
.not_to receive(:perform_async) .not_to receive(:perform_async)
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
...@@ -69,7 +69,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -69,7 +69,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
expect(create_events_service) expect(create_events_service)
.to receive(:execute) .to receive(:execute)
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
...@@ -78,7 +78,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -78,7 +78,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
it_behaves_like 'persists events' it_behaves_like 'persists events'
end end
shared_examples 'no notifications' do shared_examples 'no notifications' do |http_status:|
let(:notification_service) { spy } let(:notification_service) { spy }
let(:create_events_service) { spy } let(:create_events_service) { spy }
...@@ -86,7 +86,8 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -86,7 +86,8 @@ describe Projects::Prometheus::Alerts::NotifyService do
expect(notification_service).not_to receive(:async) expect(notification_service).not_to receive(:async)
expect(create_events_service).not_to receive(:execute) expect(create_events_service).not_to receive(:execute)
expect(subject).to eq(false) expect(subject).to be_error
expect(subject.http_status).to eq(http_status)
end end
end end
...@@ -130,7 +131,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -130,7 +131,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
when :success when :success
it_behaves_like 'notifies alerts' it_behaves_like 'notifies alerts'
when :failure when :failure
it_behaves_like 'no notifications' it_behaves_like 'no notifications', http_status: :unauthorized
else else
raise "invalid result: #{result.inspect}" raise "invalid result: #{result.inspect}"
end end
...@@ -140,7 +141,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -140,7 +141,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
context 'without project specific cluster' do context 'without project specific cluster' do
let!(:cluster) { create(:cluster, enabled: true) } let!(:cluster) { create(:cluster, enabled: true) }
it_behaves_like 'no notifications' it_behaves_like 'no notifications', http_status: :unauthorized
end end
context 'with manual prometheus installation' do context 'with manual prometheus installation' do
...@@ -171,7 +172,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -171,7 +172,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
when :success when :success
it_behaves_like 'notifies alerts' it_behaves_like 'notifies alerts'
when :failure when :failure
it_behaves_like 'no notifications' it_behaves_like 'no notifications', http_status: :unauthorized
else else
raise "invalid result: #{result.inspect}" raise "invalid result: #{result.inspect}"
end end
...@@ -193,7 +194,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -193,7 +194,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
expect_any_instance_of(NotificationService) expect_any_instance_of(NotificationService)
.not_to receive(:async) .not_to receive(:async)
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
...@@ -211,7 +212,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -211,7 +212,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
it 'does not send notification' do it 'does not send notification' do
expect(NotificationService).not_to receive(:new) expect(NotificationService).not_to receive(:new)
expect(subject).to eq(true) expect(subject).to be_success
end end
end end
end end
...@@ -260,19 +261,19 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -260,19 +261,19 @@ describe Projects::Prometheus::Alerts::NotifyService do
context 'without version' do context 'without version' do
let(:payload) { {} } let(:payload) { {} }
it_behaves_like 'no notifications' it_behaves_like 'no notifications', http_status: :unprocessable_entity
end end
context 'when version is not "4"' do context 'when version is not "4"' do
let(:payload) { { 'version' => '5' } } let(:payload) { { 'version' => '5' } }
it_behaves_like 'no notifications' it_behaves_like 'no notifications', http_status: :unprocessable_entity
end end
context 'with missing alerts' do context 'with missing alerts' do
let(:payload) { { 'version' => '4' } } let(:payload) { { 'version' => '4' } }
it_behaves_like 'no notifications' it_behaves_like 'no notifications', http_status: :unauthorized
end end
context 'when the payload is too big' do context 'when the payload is too big' do
...@@ -283,7 +284,7 @@ describe Projects::Prometheus::Alerts::NotifyService do ...@@ -283,7 +284,7 @@ describe Projects::Prometheus::Alerts::NotifyService do
allow(Gitlab::Utils::DeepSize).to receive(:new).and_return(deep_size_object) allow(Gitlab::Utils::DeepSize).to receive(:new).and_return(deep_size_object)
end end
it_behaves_like 'no notifications' it_behaves_like 'no notifications', http_status: :bad_request
it 'does not process issues' do it 'does not process issues' do
expect(IncidentManagement::ProcessPrometheusAlertWorker) expect(IncidentManagement::ProcessPrometheusAlertWorker)
......
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