diff --git a/app/assets/javascripts/glql/components/common/facade.vue b/app/assets/javascripts/glql/components/common/facade.vue index 1d6b0a0ca8624d99ce855fba1c420a20f5478e1b..964cd963a70986d00ad31c68aa141f113177bdd4 100644 --- a/app/assets/javascripts/glql/components/common/facade.vue +++ b/app/assets/javascripts/glql/components/common/facade.vue @@ -35,6 +35,7 @@ export default { presenterPreview: null, presenterComponent: null, error: { + variant: 'warning', title: null, message: null, action: null, @@ -44,6 +45,11 @@ export default { preClasses: `code highlight ${gon.user_color_scheme}`, }; }, + computed: { + hasError() { + return this.error.title || this.error.message; + }, + }, async mounted() { this.loadOnClick = this.glFeatures.glqlLoadOnClick; }, @@ -71,7 +77,16 @@ export default { this.presenterComponent = await executeAndPresentQuery(this.query); this.trackRender(); } catch (error) { - this.handleQueryError(error.message); + switch (error.networkError?.statusCode) { + case 503: + this.handleTimeoutError(); + break; + case 403: + this.handleForbiddenError(); + break; + default: + this.handleQueryError(error.message); + } } }, @@ -101,8 +116,14 @@ export default { handleLimitError() { this.error = this.$options.i18n.glqlLimitError; }, + handleTimeoutError() { + this.error = this.$options.i18n.glqlTimeoutError; + }, + handleForbiddenError() { + this.error = this.$options.i18n.glqlForbiddenError; + }, dismissAlert() { - this.error = {}; + this.error = { variant: 'warning' }; }, renderMarkdown, async trackRender() { @@ -116,9 +137,11 @@ export default { safeHtmlConfig: { ALLOWED_TAGS: ['code'] }, i18n: { glqlDisplayError: { + variant: 'warning', title: __('An error occurred when trying to display this GLQL view:'), }, glqlLimitError: { + variant: 'warning', title: sprintf( __( 'Only %{n} GLQL views can be automatically displayed on a page. Click the button below to manually display this block.', @@ -127,6 +150,17 @@ export default { ), action: __('Display block'), }, + glqlTimeoutError: { + variant: 'warning', + title: sprintf(__('GLQL view timed out. Add more filters to reduce the number of results.'), { + n: MAX_GLQL_BLOCKS, + }), + action: __('Retry'), + }, + glqlForbiddenError: { + variant: 'danger', + title: __('GLQL view timed out. Try again later.'), + }, loadGlqlView: __('Load GLQL view'), }, numGlqlBlocks: new Counter(MAX_GLQL_BLOCKS), @@ -134,9 +168,9 @@ export default { </script> <template> <div data-testid="glql-facade"> - <template v-if="error.title || error.message"> + <template v-if="hasError"> <gl-alert - variant="warning" + :variant="error.variant" class="gl-mb-3" :primary-button-text="error.action" @dismiss="dismissAlert" @@ -159,8 +193,8 @@ export default { </div> <gl-intersection-observer v-else @appear="loadGlqlBlock"> <component :is="presenterComponent" v-if="presenterComponent" /> - <component :is="presenterPreview" v-else-if="presenterPreview" /> - <div v-else class="markdown-code-block gl-relative"> + <component :is="presenterPreview" v-else-if="presenterPreview && !hasError" /> + <div v-else-if="hasError" class="markdown-code-block gl-relative"> <pre :class="preClasses"><code>{{ query.trim() }}</code></pre> </div> </gl-intersection-observer> diff --git a/app/assets/javascripts/glql/core/executor.js b/app/assets/javascripts/glql/core/executor.js index ce414b9c20db1a45f1d45bc97ad16c718f1d526b..f663161cd66da5c331a47f81bba5d1bd7c454247 100644 --- a/app/assets/javascripts/glql/core/executor.js +++ b/app/assets/javascripts/glql/core/executor.js @@ -8,7 +8,7 @@ export default class Executor { #client; static taskQueue; - init(client = createDefaultClient()) { + init(client = createDefaultClient({}, { path: '/api/glql' })) { Executor.taskQueue = Executor.taskQueue || new TaskQueue(CONCURRENCY_LIMIT); this.#client = client; return this; diff --git a/app/controllers/glql/base_controller.rb b/app/controllers/glql/base_controller.rb new file mode 100644 index 0000000000000000000000000000000000000000..3f031860220a14f1f1338e20613f4b9513561dff --- /dev/null +++ b/app/controllers/glql/base_controller.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Glql + class BaseController < GraphqlController + before_action :check_rate_limit, only: [:execute] + + GlqlQueryLockedError = Class.new(StandardError) + + # Overrides GraphqlController#execute to add rate limiting for GLQL queries. + # When a query times out (raising ActiveRecord::QueryAborted), we increment the + # Gitlab::ApplicationRateLimiter counter. If a second failure occurs within a + # 15-minute window (configured in lib/gitlab/application_rate_limiter.rb), + # the request is throttled. One failure is allowed, but consecutive + # failures within the time window trigger throttling. + def execute + super + rescue ActiveRecord::QueryAborted => error + increment_rate_limit_counter + + # After incrementing the fail count with Gitlab::ApplicationRateLimiter + # we want to re-raise ActiveRecord::QueryAborted + # so that the existing error-handling flow continues on the frontend. + raise error + end + + rescue_from GlqlQueryLockedError do |exception| + log_exception(exception) + + render_error(exception.message, status: :forbidden) + end + + private + + def check_rate_limit + return unless Gitlab::ApplicationRateLimiter.peek(:glql, scope: query_sha) + + raise GlqlQueryLockedError, 'Query execution is locked due to repeated failures.' + end + + def increment_rate_limit_counter + Gitlab::ApplicationRateLimiter.throttled?(:glql, scope: query_sha) + end + + def query_sha + @query_sha ||= Digest::SHA256.hexdigest(permitted_params[:query].to_s) + end + end +end diff --git a/config/routes/api.rb b/config/routes/api.rb index 3d863b692cbcb67e896a1de28ca40100711f8c20..7a3b39e755264dff139a111508e45fa2806fa5a5 100644 --- a/config/routes/api.rb +++ b/config/routes/api.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true match '/api/graphql', via: [:get, :post], to: 'graphql#execute' +match '/api/glql', via: [:get, :post], to: 'glql/base#execute' get '/-/graphql-explorer', to: API::Graphql::GraphqlExplorerController.action(:show) ::API::API.logger Rails.logger diff --git a/lib/gitlab/application_rate_limiter.rb b/lib/gitlab/application_rate_limiter.rb index 24521414d8e86a9afaa3d56ac6a164f54ce524c1..99d684f2827846d9b58fb3d863130fb0abcff80a 100644 --- a/lib/gitlab/application_rate_limiter.rb +++ b/lib/gitlab/application_rate_limiter.rb @@ -96,7 +96,8 @@ def rate_limits # rubocop:disable Metrics/AbcSize downstream_pipeline_trigger: { threshold: -> { application_settings.downstream_pipeline_trigger_limit_per_project_user_sha }, interval: 1.minute }, - expanded_diff_files: { threshold: 6, interval: 1.minute } + expanded_diff_files: { threshold: 6, interval: 1.minute }, + glql: { threshold: 1, interval: 15.minutes } }.freeze end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1638db590f0775edcf98788dd9e0cabc0070eb13..865c59be2098ac49ead7f5da8af94fdb2b046e5d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -25769,6 +25769,12 @@ msgstr "" msgid "GCP region configured" msgstr "" +msgid "GLQL view timed out. Add more filters to reduce the number of results." +msgstr "" + +msgid "GLQL view timed out. Try again later." +msgstr "" + msgid "GPG Key ID:" msgstr "" diff --git a/spec/controllers/glql/base_controller_spec.rb b/spec/controllers/glql/base_controller_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..236520066695007b770fbd3f2fa97a4315313ade --- /dev/null +++ b/spec/controllers/glql/base_controller_spec.rb @@ -0,0 +1,139 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Glql::BaseController, feature_category: :integrations do + describe 'POST #execute' do + let(:user) { create(:user, last_activity_on: 2.days.ago.to_date) } + let(:query) { 'query GLQL { __typename }' } + let(:operation_name) { 'GLQL' } + let(:query_sha) { Digest::SHA256.hexdigest(query) } + + before do + sign_in(user) + + # Gitlab::ApplicationRateLimiter stores failed attempts in Redis to track the state + # The format is the following 'application_rate_limiter:glql:<SHA>:<TIMESTAMP>' + # Let's clean up Redis to ensure a clean state + Gitlab::Redis::RateLimiting.with(&:flushdb) + end + + context 'when a GLQL query executes successfully' do + it 'returns successful response and trigger rate limiter' do + execute_request + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response).to eq({ 'data' => { '__typename' => 'Query' } }) + + expect(Gitlab::ApplicationRateLimiter.peek(:glql, scope: query_sha)).to be_falsey + expect(current_rate_limit_value(query_sha)).to be_nil + end + end + + context 'when a single ActiveRecord::QueryAborted error occurs' do + it 're-raises ActiveRecord::QueryAborted but does not yet trigger rate limiter' do + expect(controller) + .to receive(:execute_query) + .once + .and_raise(ActiveRecord::QueryCanceled) + + execute_request + + expect(response).to have_gitlab_http_status(:service_unavailable) + expect(Gitlab::ApplicationRateLimiter.peek(:glql, scope: query_sha)).to be_falsey + expect(current_rate_limit_value(query_sha)).to eq "1" + end + end + + context 'when 2 consecutive ActiveRecord::QueryAborted errors occur' do + it 're-raises ActiveRecord::QueryAborted and triggers rate limiter' do + expect(controller) + .to receive(:execute_query) + .twice + .and_raise(ActiveRecord::QueryCanceled) + + # 1st ActiveRecord::QueryAborted raised, error counter 1 + execute_request + + expect(response).to have_gitlab_http_status(:service_unavailable) + expect(Gitlab::ApplicationRateLimiter.peek(:glql, scope: query_sha)).to be_falsey + expect(current_rate_limit_value(query_sha)).to eq "1" + + # 2nd ActiveRecord::QueryAborted raised, error counter 2 + execute_request + + expect(response).to have_gitlab_http_status(:service_unavailable) + expect(Gitlab::ApplicationRateLimiter.peek(:glql, scope: query_sha)).to be_truthy + expect(current_rate_limit_value(query_sha)).to eq "2" + + # 3rd request returns GlqlQueryLockedError right away, error counter remains the same + execute_request + + expect(response).to have_gitlab_http_status(:forbidden) + expect(current_rate_limit_value(query_sha)).to eq "2" + end + end + + context 'when an error other than ActiveRecord::QueryAborted is raised' do + it 'handles the error but does not trigger rate limiter' do + expect(controller).to receive(:execute_query).and_raise(StandardError) + + execute_request + + expect(Gitlab::ApplicationRateLimiter.peek(:glql, scope: query_sha)).to be_falsey + expect(current_rate_limit_value(query_sha)).to be_nil + end + end + + context 'when 2 consecutive errors other than ActiveRecord::QueryAborted are raised' do + it 'handles the error but does not trigger rate limiter' do + expect(controller).to receive(:execute_query).twice.and_raise(StandardError) + + execute_request + + expect(Gitlab::ApplicationRateLimiter.peek(:glql, scope: query_sha)).to be_falsey + expect(current_rate_limit_value(query_sha)).to be_nil + + execute_request + + expect(Gitlab::ApplicationRateLimiter.peek(:glql, scope: query_sha)).to be_falsey + expect(current_rate_limit_value(query_sha)).to be_nil + end + end + end + + describe 'rescue_from' do + let(:error_message) do + 'Query execution is locked due to repeated failures.' + end + + it 'handles GlqlQueryLockedError' do + allow(controller).to receive(:execute) do + raise Glql::BaseController::GlqlQueryLockedError, error_message + end + + post :execute + + expect(json_response).to include( + 'errors' => include(a_hash_including('message' => error_message)) + ) + end + end + + def execute_request + post :execute, params: { query: query, operationName: operation_name } + end + + def current_rate_limit_value(sha) + # The value is nil if the key does not yet exist in Redis + value = nil + + Gitlab::Redis::RateLimiting.with do |redis| + redis.scan_each(match: "application_rate_limiter:glql:#{sha}*") do |key| + value = redis.get(key) + end + end + + value + end +end diff --git a/spec/frontend/glql/components/common/facade_spec.js b/spec/frontend/glql/components/common/facade_spec.js index f0996bf9c99434cc4b78a6030bee2fb63d5ec197..30df8a75290945f9068c025029710565a463b107 100644 --- a/spec/frontend/glql/components/common/facade_spec.js +++ b/spec/frontend/glql/components/common/facade_spec.js @@ -87,7 +87,56 @@ describe('GlqlFacade', () => { }); }); - describe('when the query results in an error', () => { + describe('when the query results in a timeout (503) error', () => { + beforeEach(async () => { + presentPreview.mockResolvedValue({ render: (h) => h(GlSkeletonLoader) }); + executeAndPresentQuery.mockRejectedValue({ networkError: { statusCode: 503 } }); + + await createComponent(); + await triggerIntersectionObserver(); + }); + + it('displays timeout error alert', () => { + const alert = wrapper.findComponent(GlAlert); + expect(alert.exists()).toBe(true); + expect(alert.props('variant')).toBe('warning'); + expect(alert.text()).toContain( + 'GLQL view timed out. Add more filters to reduce the number of results.', + ); + expect(alert.props('primaryButtonText')).toBe('Retry'); + }); + + it('retries query execution when primary action of timeout error alert is triggered', async () => { + presentPreview.mockClear(); + executeAndPresentQuery.mockClear(); + executeAndPresentQuery.mockResolvedValue({ render: (h) => h('div') }); + + const alert = wrapper.findComponent(GlAlert); + alert.vm.$emit('primaryAction'); + await nextTick(); + + expect(executeAndPresentQuery).toHaveBeenCalledWith('assignee = "foo"'); + }); + }); + + describe('when the query results in a forbidden (403) error', () => { + beforeEach(async () => { + presentPreview.mockResolvedValue({ render: (h) => h(GlSkeletonLoader) }); + executeAndPresentQuery.mockRejectedValue({ networkError: { statusCode: 403 } }); + + await createComponent(); + await triggerIntersectionObserver(); + }); + + it('displays forbidden error alert', () => { + const alert = wrapper.findComponent(GlAlert); + expect(alert.exists()).toBe(true); + expect(alert.props('variant')).toBe('danger'); + expect(alert.text()).toContain('GLQL view timed out. Try again later.'); + }); + }); + + describe('when the query results in a syntax error', () => { beforeEach(async () => { presentPreview.mockRejectedValue(new Error('Syntax error: Unexpected `=`')); executeAndPresentQuery.mockRejectedValue(new Error('Syntax error: Unexpected `=`')); @@ -99,6 +148,7 @@ describe('GlqlFacade', () => { it('displays error alert on query failure, formatted by marked', () => { const alert = wrapper.findComponent(GlAlert); expect(alert.exists()).toBe(true); + expect(alert.props('variant')).toBe('warning'); expect(alert.find('ul li').html()).toMatchInlineSnapshot(` <li> Syntax error: Unexpected @@ -138,6 +188,7 @@ describe('GlqlFacade', () => { it('displays limit error alert after exceeding GLQL block limit', () => { const alert = wrapper.findComponent(GlAlert); expect(alert.exists()).toBe(true); + expect(alert.props('variant')).toBe('warning'); expect(alert.text()).toContain( 'Only 20 GLQL views can be automatically displayed on a page. Click the button below to manually display this block.', );