Commit 6a4c856c authored by Peter Leitzen's avatar Peter Leitzen

Status Page: Track incident issue without subtransaction

`StatusPage::PublishedIncident#track` used `safe_find_or_create_by` to
circumvent a potential race condition in case of a duplicate published
issue.  This, however, uses subtransactions under the hood which is
problematic in nested transactions.

See https://gitlab.com/groups/gitlab-org/-/epics/6540 for more context.

In the rare event of `ActiveRecord::RecordNotUnique` users end up seeing
a meaningful error message. This behaviour is acceptable and that's why
switched to unsafe method `find_or_create_by`.
parent 3d77b272
......@@ -8,8 +8,22 @@ module StatusPage
belongs_to :issue, inverse_of: :status_page_published_incident
validates :issue, presence: true
# NOTE: This method is not atomic and might raise
# +ActiveRecord::RecordNotUnique+ in case of a duplicate published issue.
#
# Previously, we've used +safe_find_or_create_by+ to circumvent this fact
# but it uses subtransactions under the hood which is problematic in nested
# transactions.
#
# See https://gitlab.com/groups/gitlab-org/-/epics/6540 for more context.
#
# In the rare event of +ActiveRecord::RecordNotUnique+ users end up seeing
# a meaningful error message. This behaviour is acceptable and that's why
# switched to unsafe method +find_or_create_by+.
#
# @raise ActiveRecord::RecordNotUnique
def self.track(issue)
safe_find_or_create_by(issue: issue)
find_or_create_by!(issue: issue)
end
def self.untrack(issue)
......
......@@ -3,6 +3,13 @@
require 'spec_helper'
RSpec.describe StatusPage::PublishedIncident do
let_it_be_with_reload(:issue) { create(:issue) }
before do
# prefill association cache
issue&.status_page_published_incident
end
describe 'associations' do
it { is_expected.to belong_to(:issue).inverse_of(:status_page_published_incident) }
end
......@@ -12,11 +19,10 @@ RSpec.describe StatusPage::PublishedIncident do
end
describe '.track' do
let_it_be(:issue) { create(:issue) }
subject { described_class.track(issue) }
it { is_expected.to be_a(described_class) }
it { is_expected.to eq(issue.status_page_published_incident) }
specify { expect(subject.issue).to eq issue }
specify { expect { subject }.to change { described_class.count }.by(1) }
......@@ -26,14 +32,31 @@ RSpec.describe StatusPage::PublishedIncident do
end
it { is_expected.to be_a(described_class) }
it { is_expected.to eq(issue.status_page_published_incident) }
specify { expect(subject.issue).to eq issue }
specify { expect { subject }.not_to change { described_class.count } }
end
context 'when issue is new record' do
let(:issue) { build(:issue) }
it { is_expected.to be_a(described_class) }
it { is_expected.to eq(issue.status_page_published_incident) }
specify { expect(subject.issue).to eq issue }
specify { expect { subject }.to change { described_class.count }.by(1) }
end
context 'when issue is nil' do
let(:issue) { nil }
specify do
expect { subject }
.to raise_error(ActiveRecord::RecordInvalid, /Issue can't be blank/)
end
end
end
describe '.untrack' do
let_it_be(:issue) { create(:issue) }
subject { described_class.untrack(issue) }
context 'when the incident is not yet tracked' do
......
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