Commit 6c71d8a3 authored by Catalin Irimie's avatar Catalin Irimie

Rescue Geo after_{create,destroy} hooks on regular models

This prevents possible errors in Geo logic from saving or destroying
the models as expected, and can cause silent failures if that happens.
parent 91fd974f
...@@ -4,11 +4,20 @@ module Geo ...@@ -4,11 +4,20 @@ module Geo
module ReplicableModel module ReplicableModel
extend ActiveSupport::Concern extend ActiveSupport::Concern
include Checksummable include Checksummable
include ::Gitlab::Geo::LogHelpers
included do included do
# If this hook turns out not to apply to all Models, perhaps we should extract a `ReplicableBlobModel` # If this hook turns out not to apply to all Models, perhaps we should extract a `ReplicableBlobModel`
after_create_commit -> { replicator.handle_after_create_commit if replicator.respond_to?(:handle_after_create_commit) } after_create_commit -> do
after_destroy -> { replicator.handle_after_destroy if replicator.respond_to?(:handle_after_destroy) } replicator.handle_after_create_commit if replicator.respond_to?(:handle_after_create_commit)
rescue StandardError => err
log_error("Geo replicator after_create_commit failed", err)
end
after_destroy -> do
replicator.handle_after_destroy if replicator.respond_to?(:handle_after_destroy)
rescue StandardError => err
log_error("Geo replicator after_destroy failed", err)
end
# Temporarily defining `verification_succeeded` and # Temporarily defining `verification_succeeded` and
# `verification_failed` for unverified models while verification is # `verification_failed` for unverified models while verification is
......
...@@ -34,6 +34,40 @@ RSpec.describe Geo::ReplicableModel do ...@@ -34,6 +34,40 @@ RSpec.describe Geo::ReplicableModel do
let(:replicator_class) { Geo::DummyReplicator } let(:replicator_class) { Geo::DummyReplicator }
end end
describe 'after_create_commit hook' do
context 'when the replicator raises an error' do
let(:error) { StandardError.new("testing error") }
before do
expect_next_instance_of(Geo::DummyReplicator) do |instance|
expect(instance).to receive(:handle_after_create_commit).and_raise(error)
end
end
it 'saves the model' do
expect { subject.save! }.to change { DummyModel.count }.by(1)
end
end
end
describe 'after_destroy hook' do
context 'when the replicator raises an error' do
let(:error) { StandardError.new("testing error") }
before do
expect_next_instance_of(Geo::DummyReplicator) do |instance|
expect(instance).to receive(:handle_after_destroy).and_raise(error)
end
end
it 'destroys the model' do
subject.save!
expect { subject.destroy! }.to change { DummyModel.count }.by(-1)
end
end
end
describe '.verifiables' do describe '.verifiables' do
context 'when the model can be filtered by locally stored files' do context 'when the model can be filtered by locally stored files' do
it 'filters by locally stored files' do it 'filters by locally stored files' do
......
...@@ -67,6 +67,10 @@ module EE ...@@ -67,6 +67,10 @@ module EE
true true
end end
def handle_after_destroy
true
end
protected protected
def consume_event_test(user:, other:) def consume_event_test(user:, other:)
......
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