Merge branch 'refactor-sentry-response' into 'master'

Provide pagination data in the Sentry's response

See merge request gitlab-org/gitlab!20305

app/controllers/projects/error_tracking_controller.rb

@@ -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

app/models/error_tracking/project_error_tracking_setting.rb

@@ -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)

app/services/error_tracking/list_issues_service.rb

@@ -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

lib/sentry/client.rb

@@ -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)

lib/sentry/pagination_parser.rb

+# 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

spec/controllers/projects/error_tracking_controller_spec.rb

@@ -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
 

spec/fixtures/api/schemas/error_tracking/index.json

@@ -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

spec/lib/sentry/client_spec.rb

@@ -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
 

spec/lib/sentry/pagination_parser_spec.rb

+# 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

spec/models/error_tracking/project_error_tracking_setting_spec.rb

@@ -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
 

spec/services/error_tracking/list_issues_service_spec.rb

@@ -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