Code review comments appliedfj-rename-project-pipelines-relationship

5 files changed, 66 insertions, 41 deletions

diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb
index 56ab474f729..a56ec65b9f1 100644
--- a/lib/gitlab/import_export/project_tree_restorer.rb
+++ b/lib/gitlab/import_export/project_tree_restorer.rb
@@ -26,7 +26,7 @@ module Gitlab
 
         @project_members = @tree_hash.delete('project_members')
 
-        RelationRenameFactory.rename(@tree_hash)
+        RelationRenameService.rename(@tree_hash)
 
         ActiveRecord::Base.uncached do
           ActiveRecord::Base.no_touching do
diff --git a/lib/gitlab/import_export/project_tree_saver.rb b/lib/gitlab/import_export/project_tree_saver.rb
index d951590a2e2..a7a8bb1197e 100644
--- a/lib/gitlab/import_export/project_tree_saver.rb
+++ b/lib/gitlab/import_export/project_tree_saver.rb
@@ -34,7 +34,7 @@ module Gitlab
 
         project_json['project_members'] += group_members_json
 
-        RelationRenameFactory.add_renames(project_json)
+        RelationRenameService.add_renames(project_json)
 
         project_json.to_json
       end
diff --git a/lib/gitlab/import_export/relation_rename_factory.rb b/lib/gitlab/import_export/relation_rename_factory.rb
deleted file mode 100644
index e9bb9cac729..00000000000
--- a/lib/gitlab/import_export/relation_rename_factory.rb
+++ /dev/null
@@ -1,29 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
-  module ImportExport
-    class RelationRenameFactory
-      RENAMES = { 'pipelines' => 'ci_pipelines' }.freeze
-
-      def self.rename(tree_hash)
-        return if tree_hash&.empty?
-
-        RENAMES.each do |old_name, new_name|
-          old_entry = tree_hash.delete(old_name)
-
-          next if tree_hash[new_name] || !old_entry
-
-          tree_hash[new_name] = old_entry
-        end
-      end
-
-      def self.add_renames(tree_hash)
-        RENAMES.each do |old_name, new_name|
-          next if tree_hash.key?(old_name)
-
-          tree_hash[old_name] = tree_hash[new_name]
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/import_export/relation_rename_service.rb b/lib/gitlab/import_export/relation_rename_service.rb
new file mode 100644
index 00000000000..bdd0afc773d
--- /dev/null
+++ b/lib/gitlab/import_export/relation_rename_service.rb
@@ -0,0 +1,48 @@
+# frozen_string_literal: true
+
+# This class is intended to help with relation renames within Gitlab versions
+# and allow compatibility between versions.
+# If you have to change one relationship name that is imported/exported,
+# you should add it to the RENAMES constant indicating the old name and the
+# new one.
+# The behavior of these renamed relationships should be transient and it should
+# only last one release until you completely remove the renaming from the list.
+#
+# When importing, this class will check the project hash and:
+# - if only the old relationship name is found, it will rename it with the new one
+# - if only the new relationship name is found, it will do nothing
+# - if it finds both, it will use the new relationship data
+#
+# When exporting, this class will duplicate the keys in the resulting file.
+# This way, if we open the file in an old version of the exporter it will work
+# and also it will with the newer versions.
+module Gitlab
+  module ImportExport
+    class RelationRenameService
+      RENAMES = {
+        'pipelines' => 'ci_pipelines' # Added in 11.6, remove in 11.7
+      }.freeze
+
+      def self.rename(tree_hash)
+        return if tree_hash&.empty?
+
+        RENAMES.each do |old_name, new_name|
+          old_entry = tree_hash.delete(old_name)
+
+          next if tree_hash[new_name]
+          next unless old_entry
+
+          tree_hash[new_name] = old_entry
+        end
+      end
+
+      def self.add_renames(tree_hash)
+        RENAMES.each do |old_name, new_name|
+          next if tree_hash.key?(old_name)
+
+          tree_hash[old_name] = tree_hash[new_name]
+        end
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/import_export/relation_rename_factory_spec.rb b/spec/lib/gitlab/import_export/relation_rename_service_spec.rb
index 3e7159042c8..4606c824799 100644
--- a/spec/lib/gitlab/import_export/relation_rename_factory_spec.rb
+++ b/spec/lib/gitlab/import_export/relation_rename_service_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Gitlab::ImportExport::RelationRenameFactory do
+describe Gitlab::ImportExport::RelationRenameService do
   let(:renames) do
     {
       'example_relation1' => 'new_example_relation1',
@@ -33,8 +33,8 @@ describe Gitlab::ImportExport::RelationRenameFactory do
 
     context 'when the file has only old relationship names' do
       before do
-        renames.each do |k, v|
-          json_file[k.to_s] = []
+        renames.each do |old_name, _|
+          json_file[old_name.to_s] = []
         end
       end
 
@@ -50,9 +50,9 @@ describe Gitlab::ImportExport::RelationRenameFactory do
 
     context 'when the file has both the old and new relationships' do
       before do
-        renames.each do |k, v|
-          json_file[k.to_s] = [1]
-          json_file[v.to_s] = [2]
+        renames.each do |old_name, new_name|
+          json_file[old_name.to_s] = [1]
+          json_file[new_name.to_s] = [2]
         end
       end
 
@@ -69,8 +69,8 @@ describe Gitlab::ImportExport::RelationRenameFactory do
 
     context 'when the file has only new relationship names' do
       before do
-        renames.each do |k, v|
-          json_file[v.to_s] = []
+        renames.each do |_, new_name|
+          json_file[new_name.to_s] = []
         end
       end
 
@@ -89,9 +89,15 @@ describe Gitlab::ImportExport::RelationRenameFactory do
     let(:project_tree) { project_tree_saver.send(:project_json) }
 
     it 'adds old relationships to the exported file' do
-      project_tree.merge!(renames.values.map { |v| [v, []] }.to_h)
+      project_tree.merge!(renames.values.map { |new_name| [new_name, []] }.to_h)
 
-      saved_data = ActiveSupport::JSON.decode(project_tree_saver.send(:project_json_tree))
+      allow(project_tree_saver).to receive(:save) do |arg|
+        project_tree_saver.send(:project_json_tree)
+      end
+
+      result = project_tree_saver.save
+
+      saved_data = ActiveSupport::JSON.decode(result)
 
       expect(saved_data.keys).to include(*(renames.keys + renames.values))
     end