diff options
author | Andreas Brandl <abrandl@gitlab.com> | 2018-02-26 16:44:35 +0100 |
---|---|---|
committer | Andreas Brandl <abrandl@gitlab.com> | 2018-03-06 12:53:13 +0100 |
commit | 375a5c9f1e69a5fbf49a98cecc7f0c0cb61df989 (patch) | |
tree | f748f286a26b82535ad9b261806ee84bee6d66e1 | |
parent | 8dde03012f0f3c66333916740483643b193664ad (diff) | |
download | gitlab-ce-375a5c9f1e69a5fbf49a98cecc7f0c0cb61df989.tar.gz |
Only track contributions if table is available.
This is due to the problem that the callback can be called while running
an earlier database schema version (for example during earlier
migrations). We work around this by checking the current schema version
and only track contributions if the table is available.
-rw-r--r-- | app/models/event.rb | 5 | ||||
-rw-r--r-- | app/models/user_contributed_projects.rb | 8 | ||||
-rw-r--r-- | spec/models/event_spec.rb | 10 | ||||
-rw-r--r-- | spec/models/user_contributed_projects_spec.rb | 15 |
4 files changed, 36 insertions, 2 deletions
diff --git a/app/models/event.rb b/app/models/event.rb index f0cc99a9242..e855ed02274 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -392,6 +392,9 @@ class Event < ActiveRecord::Base end def track_user_contributed_projects - UserContributedProjects.track(self) + # Note the call to .available? is due to earlier migrations + # that would otherwise conflict with the call to .track + # (because the table does not exist yet). + UserContributedProjects.track(self) if UserContributedProjects.available? end end diff --git a/app/models/user_contributed_projects.rb b/app/models/user_contributed_projects.rb index 1f8e9a3f480..2965b956f3f 100644 --- a/app/models/user_contributed_projects.rb +++ b/app/models/user_contributed_projects.rb @@ -7,6 +7,9 @@ class UserContributedProjects < ActiveRecord::Base CACHE_EXPIRY_TIME = 1.day + # Schema version required for this model + REQUIRED_SCHEMA_VERSION = 20180223120443 + class << self def track(event) # For events without a project, we simply don't care. @@ -35,6 +38,11 @@ class UserContributedProjects < ActiveRecord::Base end end + # Check if we can safely call .track (table exists) + def available? + @available_flag ||= ActiveRecord::Migrator.current_version >= REQUIRED_SCHEMA_VERSION # rubocop:disable Gitlab/PredicateMemoization + end + private def cached_exists?(project_id:, user_id:, &block) diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 11f6ae74712..445de37d2b1 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -51,11 +51,19 @@ describe Event do end describe 'after_create :track_user_contributed_projects' do + let(:event) { build(:push_event, project: project, author: project.owner) } + it 'passes event to UserContributedProjects.track' do - event = build(:push_event, project: project, author: project.owner) + expect(UserContributedProjects).to receive(:available?).and_return(true) expect(UserContributedProjects).to receive(:track).with(event) event.save end + + it 'does not call UserContributedProjects.track if its not yet available' do + expect(UserContributedProjects).to receive(:available?).and_return(false) + expect(UserContributedProjects).not_to receive(:track) + event.save + end end end diff --git a/spec/models/user_contributed_projects_spec.rb b/spec/models/user_contributed_projects_spec.rb index f92a9d91f8b..092821c8aea 100644 --- a/spec/models/user_contributed_projects_spec.rb +++ b/spec/models/user_contributed_projects_spec.rb @@ -40,6 +40,21 @@ describe UserContributedProjects do end end + describe '.available?' do + before do + described_class.instance_variable_set('@available_flag', nil) + end + + it 'checks schema version and properly caches positive result' do + expect(ActiveRecord::Migrator).to receive(:current_version).and_return(described_class::REQUIRED_SCHEMA_VERSION - 1 - rand(1000)) + expect(described_class.available?).to be_falsey + expect(ActiveRecord::Migrator).to receive(:current_version).and_return(described_class::REQUIRED_SCHEMA_VERSION + rand(1000)) + expect(described_class.available?).to be_truthy + expect(ActiveRecord::Migrator).not_to receive(:current_version) + expect(described_class.available?).to be_truthy # cached response + end + end + it { is_expected.to validate_presence_of(:project) } it { is_expected.to validate_presence_of(:user) } end |