diff --git a/.rubocop_todo/layout/line_length.yml b/.rubocop_todo/layout/line_length.yml
index ef2e380ea950a1e43ad71f7d5ecb7457b1c3693f..d81f3c5004e5741b7d1977a347b046ebc13e993b 100644
--- a/.rubocop_todo/layout/line_length.yml
+++ b/.rubocop_todo/layout/line_length.yml
@@ -920,7 +920,6 @@ Layout/LineLength:
- 'ee/app/models/ee/upload.rb'
- 'ee/app/models/ee/user.rb'
- 'ee/app/models/ee/vulnerability.rb'
- - 'ee/app/models/elastic/migration_record.rb'
- 'ee/app/models/elastic/reindexing_slice.rb'
- 'ee/app/models/epic_issue.rb'
- 'ee/app/models/geo/project_registry.rb'
diff --git a/.rubocop_todo/style/missing_respond_to_missing.yml b/.rubocop_todo/style/missing_respond_to_missing.yml
index 9387ae72d0011e1273a3731c55139f7acb5b3a3f..4c61c7fc1a662ecf09c1f02af832cfea6400327b 100644
--- a/.rubocop_todo/style/missing_respond_to_missing.yml
+++ b/.rubocop_todo/style/missing_respond_to_missing.yml
@@ -5,7 +5,6 @@ Style/MissingRespondToMissing:
- 'app/models/network/commit.rb'
- 'app/services/notification_service.rb'
- 'ee/app/controllers/ee/groups/application_controller.rb'
- - 'ee/app/models/elastic/migration_record.rb'
- 'ee/app/services/ee/audit_event_service.rb'
- 'lib/declarative_enum.rb'
- 'lib/gitlab/auth/ldap/dn.rb'
diff --git a/ee/app/models/elastic/migration_record.rb b/ee/app/models/elastic/migration_record.rb
index ea9984590266e7e1043520873ffdf7965ab85961..7e9bc63efefe15802ae35a86b34e13c8ebce5d58 100644
--- a/ee/app/models/elastic/migration_record.rb
+++ b/ee/app/models/elastic/migration_record.rb
@@ -20,13 +20,18 @@ def save!(completed:)
data = { completed: completed, state: load_state, name: name }.merge(timestamps(completed: completed))
- client.index index: index_name, type: '_doc', id: version, body: data
+ client.index index: index_name, refresh: true, type: '_doc', id: version, body: data
end
def save_state!(state)
- completed = load_completed_from_index
+ source = load_from_index&.dig('_source')&.with_indifferent_access || {}
+ current_state = source['state']&.with_indifferent_access || {}
+ completed = source['completed']
- client.index index: index_name, refresh: true, type: '_doc', id: version, body: { completed: completed, state: load_state.merge(state) }
+ source.delete(:state)
+ body = source.merge(state: current_state.merge(state), completed: completed)
+
+ client.index index: index_name, refresh: true, type: '_doc', id: version, body: body
end
def started?
@@ -107,12 +112,18 @@ def method_missing(method, *args, &block)
end
end
+ def respond_to_missing?(method, include_private = false)
+ migration.respond_to?(method) || super
+ end
+
def self.load_versions(completed:)
helper = Gitlab::Elastic::Helper.default
+ body = { query: { term: { completed: completed } }, size: ELASTICSEARCH_SIZE }
helper.client
- .search(index: helper.migrations_index_name, body: { query: { term: { completed: completed } }, size: ELASTICSEARCH_SIZE })
+ .search(index: helper.migrations_index_name, body: body)
.dig('hits', 'hits')
.map { |v| v['_id'].to_i }
+ .sort
end
def self.completed_versions
diff --git a/ee/lib/gitlab/elastic/helper.rb b/ee/lib/gitlab/elastic/helper.rb
index 0a8847369c1ae6bba6d75190ef36dbae81e6d4dd..affd57ced6280bbf57a469967c81fa9ab92dc870 100644
--- a/ee/lib/gitlab/elastic/helper.rb
+++ b/ee/lib/gitlab/elastic/helper.rb
@@ -114,6 +114,9 @@ def create_migrations_index
},
completed_at: {
type: 'date'
+ },
+ name: {
+ type: 'keyword'
}
}
}
diff --git a/ee/spec/models/elastic/migration_record_spec.rb b/ee/spec/models/elastic/migration_record_spec.rb
index b0c7955c48c7beafcb1f2db42f00667dcef367db..3648455271f45d793f46e39ca5d30a7384be19c9 100644
--- a/ee/spec/models/elastic/migration_record_spec.rb
+++ b/ee/spec/models/elastic/migration_record_spec.rb
@@ -2,14 +2,22 @@
require 'spec_helper'
-RSpec.describe Elastic::MigrationRecord, :elastic_clean, feature_category: :global_search do
+RSpec.describe Elastic::MigrationRecord, :elastic_delete_by_query, feature_category: :global_search do
using RSpec::Parameterized::TableSyntax
+ let_it_be(:helper) { Gitlab::Elastic::Helper.default }
+
+ let(:migration) { Class.new }
let(:record) { described_class.new(version: Time.now.to_i, name: 'ExampleMigration', filename: nil) }
+ before do
+ allow(record).to receive(:load_migration).and_return(migration)
+ allow(Gitlab::Elastic::Helper).to receive(:default).and_return(helper)
+ end
+
describe '#save!' do
- it 'creates an index if it is not found' do
- es_helper.delete_migrations_index
+ it 'raises an error if the migrations index is not found' do
+ allow(helper).to receive(:index_exists?).with(index_name: 'gitlab-test-migrations').and_return(false)
expect { record.save!(completed: true) }.to raise_error(/index is not found/)
end
@@ -51,13 +59,13 @@
describe '#load_from_index' do
it 'does not raise an exception when connection refused' do
- allow(Gitlab::Elastic::Helper.default).to receive(:get).and_raise(Faraday::ConnectionFailed)
+ allow(helper.client).to receive(:get).and_raise(Faraday::ConnectionFailed)
expect(record.load_from_index).to be_nil
end
it 'does not raise an exception when record does not exist' do
- allow(Gitlab::Elastic::Helper.default).to receive(:get).and_raise(Elasticsearch::Transport::Transport::Errors::NotFound)
+ allow(helper.client).to receive(:get).and_raise(Elasticsearch::Transport::Transport::Errors::NotFound)
expect(record.load_from_index).to be_nil
end
@@ -88,7 +96,7 @@
end
describe '#started?' do
- it 'changes on object save' do
+ it 'changes on first save to the index' do
expect { record.save!(completed: true) }.to change { record.started? }.from(false).to(true)
end
end
@@ -98,12 +106,12 @@
let(:in_progress_migration) { described_class.new(version: 10, name: 10, filename: nil) }
before do
- es_helper.delete_migrations_index
- es_helper.create_migrations_index
+ helper.delete_migrations_index
+ helper.create_migrations_index
completed_versions.each { |migration| migration.save!(completed: true) }
in_progress_migration.save!(completed: false)
- es_helper.refresh_index(index_name: es_helper.migrations_index_name)
+ helper.refresh_index(index_name: helper.migrations_index_name)
end
it 'loads all records' do
@@ -112,7 +120,7 @@
end
it 'raises an exception if no index present' do
- es_helper.delete_migrations_index
+ allow(Gitlab::Elastic::Helper.default.client).to receive(:search).and_raise(Elasticsearch::Transport::Transport::Errors::NotFound)
expect { described_class.load_versions(completed: true) }.to raise_exception(Elasticsearch::Transport::Transport::Errors::NotFound)
expect { described_class.load_versions(completed: false) }.to raise_exception(Elasticsearch::Transport::Transport::Errors::NotFound)
@@ -276,4 +284,42 @@
it { is_expected.to eq(true) }
end
end
+
+ describe '#save_state!' do
+ it 'only modifies the state field' do
+ record.save!(completed: false)
+
+ original_source = record.load_from_index['_source']
+
+ record.save_state!({ projects: [1], remaining_documents: 500 })
+
+ source = record.load_from_index['_source']
+ expected_state = original_source['state'].merge({ 'projects' => [1], 'remaining_documents' => 500 })
+
+ expect(source['state']).to eq(expected_state)
+
+ source.delete('state')
+ source.each_key do |key|
+ expect(source[key]).to eq(original_source[key])
+ end
+ end
+
+ it 'only overwrites the state fields provided to the method' do
+ record.save_state!({ remaining_documents: 5_000, halted: false })
+
+ original_source = record.load_from_index['_source']
+
+ record.save_state!({ projects: [1], remaining_documents: 100 })
+
+ source = record.load_from_index['_source']
+ expected_state = { 'halted' => false, 'projects' => [1], 'remaining_documents' => 100 }
+
+ expect(source['state']).to eq(expected_state)
+
+ source.delete('state')
+ source.each_key do |key|
+ expect(source[key]).to eq(original_source[key])
+ end
+ end
+ end
end