Commit d3e623ee authored by Vitali Tatarintev's avatar Vitali Tatarintev Committed by James Lopez

Add extra parameters to Sentry response

Instead of returning a list of errors, the response which fetches
a list of Sentry issues now can carry additional data
such as pagination.
parent 0fb95bbe
......@@ -55,6 +55,7 @@ class Projects::ErrorTrackingController < Projects::ApplicationController
render json: {
errors: serialize_errors(result[:issues]),
pagination: result[:pagination],
external_url: service.external_url
}
end
......@@ -111,7 +112,7 @@ class Projects::ErrorTrackingController < Projects::ApplicationController
end
def list_issues_params
params.permit([:search_term, :sort])
params.permit(:search_term, :sort, :cursor)
end
def list_projects_params
......
......@@ -104,7 +104,7 @@ module ErrorTracking
def calculate_reactive_cache(request, opts)
case request
when 'list_issues'
{ issues: sentry_client.list_issues(**opts.symbolize_keys) }
sentry_client.list_issues(**opts.symbolize_keys)
when 'issue_details'
{
issue: sentry_client.issue_details(**opts.symbolize_keys)
......
......@@ -6,37 +6,24 @@ module ErrorTracking
DEFAULT_LIMIT = 20
DEFAULT_SORT = 'last_seen'
def execute
return error('Error Tracking is not enabled') unless enabled?
return error('Access denied', :unauthorized) unless can_read?
result = project_error_tracking_setting.list_sentry_issues(
issue_status: issue_status,
limit: limit,
search_term: search_term,
sort: sort
)
# our results are not yet ready
unless result
return error('Not ready. Try again later', :no_content)
end
if result[:error].present?
return error(result[:error], http_status_for(result[:error_type]))
end
success(issues: result[:issues])
end
def external_url
project_error_tracking_setting&.sentry_external_url
end
private
def fetch
project_error_tracking_setting.list_sentry_issues(
issue_status: issue_status,
limit: limit,
search_term: params[:search_term].presence,
sort: sort,
cursor: params[:cursor].presence
)
end
def parse_response(response)
{ issues: response[:issues] }
response.slice(:issues, :pagination)
end
def issue_status
......@@ -47,18 +34,6 @@ module ErrorTracking
params[:limit] || DEFAULT_LIMIT
end
def search_term
params[:search_term].presence
end
def enabled?
project_error_tracking_setting&.enabled?
end
def can_read?
can?(current_user, :read_sentry_issue, project)
end
def sort
params[:sort] || DEFAULT_SORT
end
......
......@@ -34,12 +34,18 @@ module Sentry
end
def list_issues(**keyword_args)
issues = get_issues(keyword_args)
response = get_issues(keyword_args)
issues = response[:issues]
pagination = response[:pagination]
validate_size(issues)
handle_mapping_exceptions do
map_to_errors(issues)
{
issues: map_to_errors(issues),
pagination: pagination
}
end
end
......@@ -83,36 +89,40 @@ module Sentry
end
def get_issues(**keyword_args)
http_get(
response = http_get(
issues_api_url,
query: list_issue_sentry_query(keyword_args)
)
{
issues: response[:body],
pagination: Sentry::PaginationParser.parse(response[:headers])
}
end
def list_issue_sentry_query(issue_status:, limit:, sort: nil, search_term: '')
def list_issue_sentry_query(issue_status:, limit:, sort: nil, search_term: '', cursor: nil)
unless SENTRY_API_SORT_VALUE_MAP.key?(sort)
raise BadRequestError, 'Invalid value for sort param'
end
query_params = {
{
query: "is:#{issue_status} #{search_term}".strip,
limit: limit,
sort: SENTRY_API_SORT_VALUE_MAP[sort]
}
query_params.compact
sort: SENTRY_API_SORT_VALUE_MAP[sort],
cursor: cursor
}.compact
end
def get_issue(issue_id:)
http_get(issue_api_url(issue_id))
http_get(issue_api_url(issue_id))[:body]
end
def get_issue_latest_event(issue_id:)
http_get(issue_latest_event_api_url(issue_id))
http_get(issue_latest_event_api_url(issue_id))[:body]
end
def get_projects
http_get(projects_api_url)
http_get(projects_api_url)[:body]
end
def handle_request_exceptions
......@@ -138,7 +148,7 @@ module Sentry
raise_error "Sentry response status code: #{response.code}"
end
response.parsed_response
{ body: response.parsed_response, headers: response.headers }
end
def raise_error(message)
......
# frozen_string_literal: true
module Sentry
module PaginationParser
PATTERN = /rel=\"(?<direction>\w+)\";\sresults=\"(?<results>\w+)\";\scursor=\"(?<cursor>.+)\"/.freeze
def self.parse(headers)
links = headers['link'].to_s.split(',')
links.map { |link| parse_link(link) }.compact.to_h
end
def self.parse_link(link)
match = link.match(PATTERN)
return unless match
return if match['results'] != "true"
[match['direction'], { 'cursor' => match['cursor'] }]
end
private_class_method :parse_link
end
end
......@@ -50,8 +50,6 @@ describe Projects::ErrorTrackingController do
let(:external_url) { 'http://example.com' }
context 'no data' do
let(:params) { project_params(format: :json) }
let(:permitted_params) do
ActionController::Parameters.new({}).permit!
end
......@@ -72,11 +70,13 @@ describe Projects::ErrorTrackingController do
end
end
context 'with a search_term and sort params' do
let(:params) { project_params(format: :json, search_term: 'something', sort: 'last_seen') }
context 'with extra params' do
let(:cursor) { '1572959139000:0:0' }
let(:search_term) { 'something' }
let(:sort) { 'last_seen' }
let(:params) { project_params(format: :json, search_term: search_term, sort: sort, cursor: cursor) }
let(:permitted_params) do
ActionController::Parameters.new(search_term: 'something', sort: 'last_seen').permit!
ActionController::Parameters.new(search_term: search_term, sort: sort, cursor: cursor).permit!
end
before do
......@@ -88,7 +88,7 @@ describe Projects::ErrorTrackingController do
context 'service result is successful' do
before do
expect(list_issues_service).to receive(:execute)
.and_return(status: :success, issues: [error])
.and_return(status: :success, issues: [error], pagination: {})
expect(list_issues_service).to receive(:external_url)
.and_return(external_url)
end
......@@ -100,13 +100,16 @@ describe Projects::ErrorTrackingController do
expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('error_tracking/index')
expect(json_response['external_url']).to eq(external_url)
expect(json_response['errors']).to eq([error].as_json)
expect(json_response).to eq(
'errors' => [error].as_json,
'pagination' => {},
'external_url' => external_url
)
end
end
end
context 'without params' do
context 'without extra params' do
before do
expect(ErrorTracking::ListIssuesService)
.to receive(:new).with(project, user, {})
......@@ -116,7 +119,7 @@ describe Projects::ErrorTrackingController do
context 'service result is successful' do
before do
expect(list_issues_service).to receive(:execute)
.and_return(status: :success, issues: [error])
.and_return(status: :success, issues: [error], pagination: {})
expect(list_issues_service).to receive(:external_url)
.and_return(external_url)
end
......@@ -128,8 +131,11 @@ describe Projects::ErrorTrackingController do
expect(response).to have_gitlab_http_status(:ok)
expect(response).to match_response_schema('error_tracking/index')
expect(json_response['external_url']).to eq(external_url)
expect(json_response['errors']).to eq([error].as_json)
expect(json_response).to eq(
'errors' => [error].as_json,
'pagination' => {},
'external_url' => external_url
)
end
end
......
......@@ -2,6 +2,7 @@
"type": "object",
"required": [
"external_url",
"pagination",
"errors"
],
"properties": {
......@@ -9,6 +10,9 @@
"errors": {
"type": "array",
"items": { "$ref": "error.json" }
},
"pagination": {
"type": "object"
}
},
"additionalProperties": false
......
......@@ -54,10 +54,20 @@ describe Sentry::Client do
end
end
shared_examples 'issues has correct return type' do |klass|
it "returns objects of type #{klass}" do
expect(subject[:issues]).to all( be_a(klass) )
end
end
shared_examples 'has correct length' do |length|
it { expect(subject.length).to eq(length) }
end
shared_examples 'issues has correct length' do |length|
it { expect(subject[:issues].length).to eq(length) }
end
# Requires sentry_api_request and subject to be defined
shared_examples 'calls sentry api' do
it 'calls sentry api' do
......@@ -95,26 +105,44 @@ describe Sentry::Client do
let(:issue_status) { 'unresolved' }
let(:limit) { 20 }
let(:search_term) { '' }
let(:cursor) { nil }
let(:sort) { 'last_seen' }
let(:sentry_api_response) { issues_sample_response }
let(:sentry_request_url) { sentry_url + '/issues/?limit=20&query=is:unresolved' }
let!(:sentry_api_request) { stub_sentry_request(sentry_request_url, body: sentry_api_response) }
subject { client.list_issues(issue_status: issue_status, limit: limit, search_term: search_term, sort: 'last_seen') }
subject { client.list_issues(issue_status: issue_status, limit: limit, search_term: search_term, sort: sort, cursor: cursor) }
it_behaves_like 'calls sentry api'
it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Error
it_behaves_like 'has correct length', 1
it_behaves_like 'issues has correct return type', Gitlab::ErrorTracking::Error
it_behaves_like 'issues has correct length', 1
shared_examples 'has correct external_url' do
context 'external_url' do
it 'is constructed correctly' do
expect(subject[0].external_url).to eq('https://sentrytest.gitlab.com/sentry-org/sentry-project/issues/11')
expect(subject[:issues][0].external_url).to eq('https://sentrytest.gitlab.com/sentry-org/sentry-project/issues/11')
end
end
end
context 'when response has a pagination info' do
let(:headers) do
{
link: '<https://sentrytest.gitlab.com>; rel="previous"; results="true"; cursor="1573556671000:0:1", <https://sentrytest.gitlab.com>; rel="next"; results="true"; cursor="1572959139000:0:0"'
}
end
let!(:sentry_api_request) { stub_sentry_request(sentry_request_url, body: sentry_api_response, headers: headers) }
it 'parses the pagination' do
expect(subject[:pagination]).to eq(
'previous' => { 'cursor' => '1573556671000:0:1' },
'next' => { 'cursor' => '1572959139000:0:0' }
)
end
end
context 'error object created from sentry response' do
using RSpec::Parameterized::TableSyntax
......@@ -137,7 +165,7 @@ describe Sentry::Client do
end
with_them do
it { expect(subject[0].public_send(error_object)).to eq(sentry_api_response[0].dig(*sentry_response)) }
it { expect(subject[:issues][0].public_send(error_object)).to eq(sentry_api_response[0].dig(*sentry_response)) }
end
it_behaves_like 'has correct external_url'
......@@ -210,8 +238,8 @@ describe Sentry::Client do
it_behaves_like 'calls sentry api'
it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Error
it_behaves_like 'has correct length', 1
it_behaves_like 'issues has correct return type', Gitlab::ErrorTracking::Error
it_behaves_like 'issues has correct length', 1
it_behaves_like 'has correct external_url'
end
......@@ -240,13 +268,23 @@ describe Sentry::Client do
it_behaves_like 'maps exceptions'
context 'when search term is present' do
let(:search_term) { 'NoMethodError'}
let(:search_term) { 'NoMethodError' }
let(:sentry_request_url) { "#{sentry_url}/issues/?limit=20&query=is:unresolved NoMethodError" }
it_behaves_like 'calls sentry api'
it_behaves_like 'has correct return type', Gitlab::ErrorTracking::Error
it_behaves_like 'has correct length', 1
it_behaves_like 'issues has correct return type', Gitlab::ErrorTracking::Error
it_behaves_like 'issues has correct length', 1
end
context 'when cursor is present' do
let(:cursor) { '1572959139000:0:0' }
let(:sentry_request_url) { "#{sentry_url}/issues/?limit=20&cursor=#{cursor}&query=is:unresolved" }
it_behaves_like 'calls sentry api'
it_behaves_like 'issues has correct return type', Gitlab::ErrorTracking::Error
it_behaves_like 'issues has correct length', 1
end
end
......
# frozen_string_literal: true
require 'fast_spec_helper'
require 'support/helpers/fixture_helpers'
describe Sentry::PaginationParser do
include FixtureHelpers
describe '.parse' do
subject { described_class.parse(headers) }
context 'when headers do not have "link" param' do
let(:headers) { {} }
it 'returns empty hash' do
is_expected.to eq({})
end
end
context 'when headers.link has previous and next pages' do
let(:headers) do
{
'link' => '<https://sentrytest.gitlab.com>; rel="previous"; results="true"; cursor="1573556671000:0:1", <https://sentrytest.gitlab.com>; rel="next"; results="true"; cursor="1572959139000:0:0"'
}
end
it 'returns info about both pages' do
is_expected.to eq(
'previous' => { 'cursor' => '1573556671000:0:1' },
'next' => { 'cursor' => '1572959139000:0:0' }
)
end
end
context 'when headers.link has only next page' do
let(:headers) do
{
'link' => '<https://sentrytest.gitlab.com>; rel="previous"; results="false"; cursor="1573556671000:0:1", <https://sentrytest.gitlab.com>; rel="next"; results="true"; cursor="1572959139000:0:0"'
}
end
it 'returns only info about the next page' do
is_expected.to eq(
'next' => { 'cursor' => '1572959139000:0:0' }
)
end
end
context 'when headers.link has only previous page' do
let(:headers) do
{
'link' => '<https://sentrytest.gitlab.com>; rel="previous"; results="true"; cursor="1573556671000:0:1", <https://sentrytest.gitlab.com>; rel="next"; results="false"; cursor="1572959139000:0:0"'
}
end
it 'returns only info about the previous page' do
is_expected.to eq(
'previous' => { 'cursor' => '1573556671000:0:1' }
)
end
end
end
end
......@@ -153,9 +153,9 @@ describe ErrorTracking::ProjectErrorTrackingSetting do
it 'returns cached issues' do
expect(sentry_client).to receive(:list_issues).with(opts)
.and_return(issues)
.and_return(issues: issues, pagination: {})
expect(result).to eq(issues: issues)
expect(result).to eq(issues: issues, pagination: {})
end
end
......
......@@ -5,13 +5,14 @@ require 'spec_helper'
describe ErrorTracking::ListIssuesService do
set(:user) { create(:user) }
set(:project) { create(:project) }
let(:params) { { search_term: 'something', sort: 'last_seen' } }
let(:params) { { search_term: 'something', sort: 'last_seen', cursor: 'some-cursor' } }
let(:list_sentry_issues_args) do
{
issue_status: 'unresolved',
limit: 20,
search_term: params[:search_term],
sort: params[:sort]
search_term: 'something',
sort: 'last_seen',
cursor: 'some-cursor'
}
end
......@@ -40,11 +41,11 @@ describe ErrorTracking::ListIssuesService do
expect(error_tracking_setting)
.to receive(:list_sentry_issues)
.with(list_sentry_issues_args)
.and_return(issues: issues)
.and_return(issues: issues, pagination: {})
end
it 'returns the issues' do
expect(result).to eq(status: :success, issues: issues)
expect(result).to eq(status: :success, pagination: {}, issues: issues)
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