diff --git a/app/helpers/web_hooks/web_hooks_helper.rb b/app/helpers/web_hooks/web_hooks_helper.rb index bda9bf58fb7917667c00b30b8cbcdd4a2ce06a63..514db6ba8a2ff2ff19bc643be9cba9e66aa510d2 100644 --- a/app/helpers/web_hooks/web_hooks_helper.rb +++ b/app/helpers/web_hooks/web_hooks_helper.rb @@ -2,8 +2,6 @@ module WebHooks module WebHooksHelper - EXPIRY_TTL = 1.hour - def show_project_hook_failed_callout?(project:) return false if project_hook_page? return false unless current_user @@ -12,17 +10,11 @@ def show_project_hook_failed_callout?(project:) # Assumes include of Users::CalloutsHelper return false if web_hook_disabled_dismissed?(project) - any_project_hook_failed?(project) # Most expensive query last + project.fetch_web_hook_failure end private - def any_project_hook_failed?(project) - Rails.cache.fetch("any_web_hook_failed:#{project.id}", expires_in: EXPIRY_TTL) do - ProjectHook.for_projects(project).disabled.exists? - end - end - def project_hook_page? current_controller?('projects/hooks') || current_controller?('projects/hook_logs') end diff --git a/app/models/concerns/web_hooks/has_web_hooks.rb b/app/models/concerns/web_hooks/has_web_hooks.rb new file mode 100644 index 0000000000000000000000000000000000000000..161ce106b9b7ce3c4f5b1e811dd9f9c4782a8874 --- /dev/null +++ b/app/models/concerns/web_hooks/has_web_hooks.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module WebHooks + module HasWebHooks + extend ActiveSupport::Concern + + WEB_HOOK_CACHE_EXPIRY = 1.hour + + def any_hook_failed? + hooks.disabled.exists? + end + + def web_hook_failure_redis_key + "any_web_hook_failed:#{id}" + end + + def last_failure_redis_key + "web_hooks:last_failure:project-#{id}" + end + + def get_web_hook_failure + Gitlab::Redis::SharedState.with do |redis| + current = redis.get(web_hook_failure_redis_key) + + Gitlab::Utils.to_boolean(current) if current + end + end + + def fetch_web_hook_failure + Gitlab::Redis::SharedState.with do |_redis| + current = get_web_hook_failure + next current unless current.nil? + + cache_web_hook_failure + end + end + + def cache_web_hook_failure(state = any_hook_failed?) + Gitlab::Redis::SharedState.with do |redis| + redis.set(web_hook_failure_redis_key, state.to_s, ex: WEB_HOOK_CACHE_EXPIRY) + + state + end + end + end +end diff --git a/app/models/hooks/project_hook.rb b/app/models/hooks/project_hook.rb index 81122c3ea10a443595091f43719ca5f81146ef6e..8e9a74a68d031b2faf131c50d90b578a4f261a8d 100644 --- a/app/models/hooks/project_hook.rb +++ b/app/models/hooks/project_hook.rb @@ -46,14 +46,18 @@ def parent override :update_last_failure def update_last_failure - return if executable? + if executable? + project.cache_web_hook_failure if project.get_web_hook_failure # may need update + else + project.cache_web_hook_failure(true) # definitely failing, no need to check - key = "web_hooks:last_failure:project-#{project_id}" - time = Time.current.utc.iso8601 + Gitlab::Redis::SharedState.with do |redis| + last_failure_key = project.last_failure_redis_key + time = Time.current.utc.iso8601 + prev = redis.get(last_failure_key) - Gitlab::Redis::SharedState.with do |redis| - prev = redis.get(key) - redis.set(key, time) if !prev || prev < time + redis.set(last_failure_key, time) if !prev || prev < time + end end end end diff --git a/app/models/project.rb b/app/models/project.rb index 27be84d787ee68b569c4febf65879785eeb27df3..63ea19202ceaee6c87fb26374240e8fcfca6092b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -41,6 +41,7 @@ class Project < ApplicationRecord include BlocksUnsafeSerialization include Subquery include IssueParent + include WebHooks::HasWebHooks extend Gitlab::Cache::RequestCache extend Gitlab::Utils::Override diff --git a/spec/helpers/web_hooks/web_hooks_helper_spec.rb b/spec/helpers/web_hooks/web_hooks_helper_spec.rb index bcd9d2df1dc26638a70e10da69eee9db3c6f7a36..fdd0be8095b2e8b7c9e2f1c595fcc24692f7dec6 100644 --- a/spec/helpers/web_hooks/web_hooks_helper_spec.rb +++ b/spec/helpers/web_hooks/web_hooks_helper_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe WebHooks::WebHooksHelper do +RSpec.describe WebHooks::WebHooksHelper, :clean_gitlab_redis_shared_state, feature_category: :integrations do let_it_be_with_reload(:project) { create(:project) } let(:current_user) { nil } @@ -43,20 +43,12 @@ expect(helper).to be_show_project_hook_failed_callout(project: project) end - it 'caches the DB calls until the TTL', :use_clean_rails_memory_store_caching, :request_store do - helper.show_project_hook_failed_callout?(project: project) - - travel_to((described_class::EXPIRY_TTL - 1.second).from_now) do - expect do - helper.show_project_hook_failed_callout?(project: project) - end.not_to exceed_query_limit(0) + it 'stores a value' do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).to receive(:set).with(anything, 'true', ex: 1.hour) end - travel_to((described_class::EXPIRY_TTL + 1.second).from_now) do - expect do - helper.show_project_hook_failed_callout?(project: project) - end.to exceed_query_limit(0) - end + helper.show_project_hook_failed_callout?(project: project) end end diff --git a/spec/models/hooks/project_hook_spec.rb b/spec/models/hooks/project_hook_spec.rb index 9a78d7b1719135a72921800fce79d52cee9669cc..c3484c4a42c97ce5a4f8cdfb34b5b76cf0af8bb4 100644 --- a/spec/models/hooks/project_hook_spec.rb +++ b/spec/models/hooks/project_hook_spec.rb @@ -79,17 +79,91 @@ def find_hooks end describe '#update_last_failure', :clean_gitlab_redis_shared_state do - let(:hook) { build(:project_hook) } + let_it_be(:hook) { create(:project_hook) } + + def last_failure + Gitlab::Redis::SharedState.with do |redis| + redis.get(hook.project.last_failure_redis_key) + end + end + + def any_failed? + Gitlab::Redis::SharedState.with do |redis| + Gitlab::Utils.to_boolean(redis.get(hook.project.web_hook_failure_redis_key)) + end + end it 'is a method of this class' do expect { hook.update_last_failure }.not_to raise_error end context 'when the hook is executable' do - it 'does not update the state' do - expect(Gitlab::Redis::SharedState).not_to receive(:with) + let(:redis_key) { hook.project.web_hook_failure_redis_key } + + def redis_value + any_failed? + end + + context 'when the state was previously failing' do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set(redis_key, true) + end + end - hook.update_last_failure + it 'does update the state' do + expect { hook.update_last_failure }.to change { redis_value }.to(false) + end + + context 'when there is another failing sibling hook' do + before do + create(:project_hook, :permanently_disabled, project: hook.project) + end + + it 'does not update the state' do + expect { hook.update_last_failure }.not_to change { redis_value }.from(true) + end + + it 'caches the current value' do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).to receive(:set).with(redis_key, 'true', ex: 1.hour).and_call_original + end + + hook.update_last_failure + end + end + end + + context 'when the state was previously unknown' do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.del(redis_key) + end + end + + it 'does not update the state' do + expect { hook.update_last_failure }.not_to change { redis_value }.from(nil) + end + end + + context 'when the state was previously not failing' do + before do + Gitlab::Redis::SharedState.with do |redis| + redis.set(redis_key, false) + end + end + + it 'does not update the state' do + expect { hook.update_last_failure }.not_to change { redis_value }.from(false) + end + + it 'does not cache the current value' do + Gitlab::Redis::SharedState.with do |redis| + expect(redis).not_to receive(:set) + end + + hook.update_last_failure + end end end @@ -98,28 +172,34 @@ def find_hooks allow(hook).to receive(:executable?).and_return(false) end - def last_failure - Gitlab::Redis::SharedState.with do |redis| - redis.get("web_hooks:last_failure:project-#{hook.project.id}") - end - end - context 'there is no prior value', :freeze_time do - it 'updates the state' do + it 'updates last_failure' do expect { hook.update_last_failure }.to change { last_failure }.to(Time.current) end + + it 'updates any_failed?' do + expect { hook.update_last_failure }.to change { any_failed? }.to(true) + end end - context 'there is a prior value, from before now' do + context 'when there is a prior last_failure, from before now' do it 'updates the state' do the_future = 1.minute.from_now - hook.update_last_failure travel_to(the_future) do expect { hook.update_last_failure }.to change { last_failure }.to(the_future.iso8601) end end + + it 'does not change the failing state' do + the_future = 1.minute.from_now + hook.update_last_failure + + travel_to(the_future) do + expect { hook.update_last_failure }.not_to change { any_failed? }.from(true) + end + end end context 'there is a prior value, from after now' do diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb index 4bfa33280378dd9eddc643f78a7cc58d82aa61f8..72958a54e108efd2b656abd16df253ad3efafa47 100644 --- a/spec/models/hooks/web_hook_spec.rb +++ b/spec/models/hooks/web_hook_spec.rb @@ -443,7 +443,7 @@ describe '#update_last_failure' do it 'is a method of this class' do - expect { described_class.new.update_last_failure }.not_to raise_error + expect { described_class.new(project: project).update_last_failure }.not_to raise_error end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index c0af71a7e2705bcf869e3f77d730a834be63188d..23dae7d03745347367e6020da6a9adce2469a756 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -8826,6 +8826,14 @@ def has_external_wiki end end + it_behaves_like 'something that has web-hooks' do + let_it_be_with_reload(:object) { create(:project) } + + def create_hook + create(:project_hook, project: object) + end + end + private def finish_job(export_job) diff --git a/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb b/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..cd6eb8c77faf1c25c0a9cd6ef199274bfcfa583f --- /dev/null +++ b/spec/support/shared_examples/models/concerns/web_hooks/has_web_hooks_shared_examples.rb @@ -0,0 +1,107 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'something that has web-hooks' do + describe '#any_hook_failed?', :clean_gitlab_redis_shared_state do + subject { object.any_hook_failed? } + + context 'when there are no hooks' do + it { is_expected.to eq(false) } + end + + context 'when there are hooks' do + before do + create_hook + create_hook + end + + it { is_expected.to eq(false) } + + context 'when there is a failed hook' do + before do + hook = create_hook + hook.update!(recent_failures: WebHook::FAILURE_THRESHOLD + 1) + end + + it { is_expected.to eq(true) } + end + end + end + + describe '#cache_web_hook_failure', :clean_gitlab_redis_shared_state do + context 'when no value is passed' do + it 'stores the return value of #any_hook_failed? and passes it back' do + allow(object).to receive(:any_hook_failed?).and_return(true) + + Gitlab::Redis::SharedState.with do |r| + expect(r).to receive(:set) + .with(object.web_hook_failure_redis_key, 'true', ex: 1.hour) + .and_call_original + end + + expect(object.cache_web_hook_failure).to eq(true) + end + end + + context 'when a value is passed' do + it 'stores the value and passes it back' do + expect(object).not_to receive(:any_hook_failed?) + + Gitlab::Redis::SharedState.with do |r| + expect(r).to receive(:set) + .with(object.web_hook_failure_redis_key, 'foo', ex: 1.hour) + .and_call_original + end + + expect(object.cache_web_hook_failure(:foo)).to eq(:foo) + end + end + end + + describe '#get_web_hook_failure', :clean_gitlab_redis_shared_state do + subject { object.get_web_hook_failure } + + context 'when no value is stored' do + it { is_expected.to be_nil } + end + + context 'when stored as true' do + before do + object.cache_web_hook_failure(true) + end + + it { is_expected.to eq(true) } + end + + context 'when stored as false' do + before do + object.cache_web_hook_failure(false) + end + + it { is_expected.to eq(false) } + end + end + + describe '#fetch_web_hook_failure', :clean_gitlab_redis_shared_state do + context 'when a value has not been stored' do + it 'does not call #any_hook_failed?' do + expect(object.get_web_hook_failure).to be_nil + expect(object).to receive(:any_hook_failed?).and_return(true) + + expect(object.fetch_web_hook_failure).to eq(true) + expect(object.get_web_hook_failure).to eq(true) + end + end + + context 'when a value has been stored' do + before do + object.cache_web_hook_failure(true) + end + + it 'does not call #any_hook_failed?' do + expect(object).not_to receive(:any_hook_failed?) + + expect(object.fetch_web_hook_failure).to eq(true) + end + end + end +end