From 0eeadb2dd2cf20ab1c12f1b0e86fa1227e044b41 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= <ayufan@ayufan.eu>
Date: Thu, 5 Sep 2019 15:51:27 +0200
Subject: Normalize import_export structure

This brings a significant refactor to how
we handle `import_export.yml`, merge it with EE
and how we handle that for reader and saver.

This is meant to simplify the code, and remove
a ton of conditions to handle different models
of the structure.

This is also meant to prepare the structure
to extend it much easier, like adding `preload:`
or additional object types when needed.

This does not change the behavior of import/export,
rather unifies and simplifies the current implementation.
---
 .../import_export/attribute_configuration_spec.rb  |   2 +-
 .../gitlab/import_export/attributes_finder_spec.rb | 195 +++++++++++++++
 spec/lib/gitlab/import_export/config_spec.rb       | 266 ++++++++++-----------
 .../import_export/model_configuration_spec.rb      |   2 +-
 spec/lib/gitlab/import_export/reader_spec.rb       | 105 +++-----
 .../import_export/relation_rename_service_spec.rb  |  27 ++-
 6 files changed, 370 insertions(+), 227 deletions(-)
 create mode 100644 spec/lib/gitlab/import_export/attributes_finder_spec.rb

(limited to 'spec/lib')

diff --git a/spec/lib/gitlab/import_export/attribute_configuration_spec.rb b/spec/lib/gitlab/import_export/attribute_configuration_spec.rb
index fef84c87509..cc8ca1d87e3 100644
--- a/spec/lib/gitlab/import_export/attribute_configuration_spec.rb
+++ b/spec/lib/gitlab/import_export/attribute_configuration_spec.rb
@@ -12,7 +12,7 @@ describe 'Import/Export attribute configuration' do
 
   let(:config_hash) { Gitlab::ImportExport::Config.new.to_h.deep_stringify_keys }
   let(:relation_names) do
-    names = names_from_tree(config_hash['project_tree'])
+    names = names_from_tree(config_hash.dig('tree', 'project'))
 
     # Remove duplicated or add missing models
     # - project is not part of the tree, so it has to be added manually.
diff --git a/spec/lib/gitlab/import_export/attributes_finder_spec.rb b/spec/lib/gitlab/import_export/attributes_finder_spec.rb
new file mode 100644
index 00000000000..208b60844e3
--- /dev/null
+++ b/spec/lib/gitlab/import_export/attributes_finder_spec.rb
@@ -0,0 +1,195 @@
+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+
+describe Gitlab::ImportExport::AttributesFinder do
+  describe '#find_root' do
+    subject { described_class.new(config: config).find_root(model_key) }
+
+    let(:test_config) { 'spec/support/import_export/import_export.yml' }
+    let(:config) { Gitlab::ImportExport::Config.new.to_h }
+    let(:model_key) { :project }
+
+    let(:project_tree_hash) do
+      {
+        except: [:id, :created_at],
+        include: [
+          { issues: { include: [] } },
+          { labels: { include: [] } },
+          { merge_requests: {
+            except: [:iid],
+            include: [
+              { merge_request_diff: {
+                include: []
+              } },
+              { merge_request_test: { include: [] } }
+            ],
+            only: [:id]
+            } },
+          { commit_statuses: {
+              include: [{ commit: { include: [] } }]
+            } },
+          { project_members: {
+              include: [{ user: { include: [],
+                                  only: [:email] } }]
+          } }
+        ]
+      }
+    end
+
+    before do
+      allow_any_instance_of(Gitlab::ImportExport).to receive(:config_file).and_return(test_config)
+    end
+
+    it 'generates hash from project tree config' do
+      is_expected.to match(project_tree_hash)
+    end
+
+    context 'individual scenarios' do
+      it 'generates the correct hash for a single project relation' do
+        setup_yaml(tree: { project: [:issues] })
+
+        is_expected.to match(
+          include: [{ issues: { include: [] } }]
+        )
+      end
+
+      it 'generates the correct hash for a single project feature relation' do
+        setup_yaml(tree: { project: [:project_feature] })
+
+        is_expected.to match(
+          include: [{ project_feature: { include: [] } }]
+        )
+      end
+
+      it 'generates the correct hash for a multiple project relation' do
+        setup_yaml(tree: { project: [:issues, :snippets] })
+
+        is_expected.to match(
+          include: [{ issues: { include: [] } },
+                    { snippets: { include: [] } }]
+        )
+      end
+
+      it 'generates the correct hash for a single sub-relation' do
+        setup_yaml(tree: { project: [issues: [:notes]] })
+
+        is_expected.to match(
+          include: [{ issues: { include: [{ notes: { include: [] } }] } }]
+        )
+      end
+
+      it 'generates the correct hash for a multiple sub-relation' do
+        setup_yaml(tree: { project: [merge_requests: [:notes, :merge_request_diff]] })
+
+        is_expected.to match(
+          include: [{ merge_requests:
+                      { include: [{ notes: { include: [] } },
+                                  { merge_request_diff: { include: [] } }] } }]
+        )
+      end
+
+      it 'generates the correct hash for a sub-relation with another sub-relation' do
+        setup_yaml(tree: { project: [merge_requests: [notes: [:author]]] })
+
+        is_expected.to match(
+          include: [{ merge_requests: {
+                      include: [{ notes: { include: [{ author: { include: [] } }] } }]
+                    } }]
+        )
+      end
+
+      it 'generates the correct hash for a relation with included attributes' do
+        setup_yaml(tree: { project: [:issues] },
+                  included_attributes: { issues: [:name, :description] })
+
+        is_expected.to match(
+          include: [{ issues: { include: [],
+                                only: [:name, :description] } }]
+        )
+      end
+
+      it 'generates the correct hash for a relation with excluded attributes' do
+        setup_yaml(tree: { project: [:issues] },
+                  excluded_attributes: { issues: [:name] })
+
+        is_expected.to match(
+          include: [{ issues: { except: [:name],
+                                include: [] } }]
+        )
+      end
+
+      it 'generates the correct hash for a relation with both excluded and included attributes' do
+        setup_yaml(tree: { project: [:issues] },
+                  excluded_attributes: { issues: [:name] },
+                  included_attributes: { issues: [:description] })
+
+        is_expected.to match(
+          include: [{ issues: { except: [:name],
+                                include: [],
+                                only: [:description] } }]
+        )
+      end
+
+      it 'generates the correct hash for a relation with custom methods' do
+        setup_yaml(tree: { project: [:issues] },
+                  methods: { issues: [:name] })
+
+        is_expected.to match(
+          include: [{ issues: { include: [],
+                                methods: [:name] } }]
+        )
+      end
+
+      def setup_yaml(hash)
+        allow(YAML).to receive(:load_file).with(test_config).and_return(hash)
+      end
+    end
+  end
+
+  describe '#find_relations_tree' do
+    subject { described_class.new(config: config).find_relations_tree(model_key) }
+
+    let(:tree) { { project: { issues: {} } } }
+    let(:model_key) { :project }
+
+    context 'when initialized with config including tree' do
+      let(:config) { { tree: tree } }
+
+      context 'when relation is in top-level keys of the tree' do
+        it { is_expected.to eq({ issues: {} }) }
+      end
+
+      context 'when the relation is not in top-level keys' do
+        let(:model_key) { :issues }
+
+        it { is_expected.to be_nil }
+      end
+    end
+
+    context 'when tree is not present in config' do
+      let(:config) { {} }
+
+      it { is_expected.to be_nil }
+    end
+  end
+
+  describe '#find_excluded_keys' do
+    subject { described_class.new(config: config).find_excluded_keys(klass_name) }
+
+    let(:klass_name) { 'project' }
+
+    context 'when initialized with excluded_attributes' do
+      let(:config) { { excluded_attributes: excluded_attributes } }
+      let(:excluded_attributes) { { project: [:name, :path], issues: [:milestone_id] } }
+
+      it { is_expected.to eq(%w[name path]) }
+    end
+
+    context 'when excluded_attributes are not present in config' do
+      let(:config) { {} }
+
+      it { is_expected.to eq([]) }
+    end
+  end
+end
diff --git a/spec/lib/gitlab/import_export/config_spec.rb b/spec/lib/gitlab/import_export/config_spec.rb
index cf396dba382..e53db37def4 100644
--- a/spec/lib/gitlab/import_export/config_spec.rb
+++ b/spec/lib/gitlab/import_export/config_spec.rb
@@ -1,163 +1,159 @@
 # frozen_string_literal: true
 
-require 'spec_helper'
+require 'fast_spec_helper'
+require 'rspec-parameterized'
 
 describe Gitlab::ImportExport::Config do
   let(:yaml_file) { described_class.new }
 
   describe '#to_h' do
-    context 'when using CE' do
-      before do
-        allow(yaml_file)
-          .to receive(:merge?)
-          .and_return(false)
+    subject { yaml_file.to_h }
+
+    context 'when using default config' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:ee) do
+        [true, false]
       end
 
-      it 'just returns the parsed Hash without the EE section' do
-        expected = YAML.load_file(Gitlab::ImportExport.config_file)
-        expected.delete('ee')
+      with_them do
+        before do
+          allow(Gitlab).to receive(:ee?) { ee }
+        end
 
-        expect(yaml_file.to_h).to eq(expected)
+        it 'parses default config' do
+          expect { subject }.not_to raise_error
+          expect(subject).to be_a(Hash)
+          expect(subject.keys).to contain_exactly(
+            :tree, :excluded_attributes, :included_attributes, :methods)
+        end
       end
     end
 
-    context 'when using EE' do
-      before do
-        allow(yaml_file)
-          .to receive(:merge?)
-          .and_return(true)
-      end
+    context 'when using custom config' do
+      let(:config) do
+        <<-EOF.strip_heredoc
+          tree:
+            project:
+              - labels:
+                - :priorities
+              - milestones:
+                - events:
+                  - :push_event_payload
 
-      it 'merges the EE project tree into the CE project tree' do
-        allow(yaml_file)
-          .to receive(:parse_yaml)
-          .and_return({
-            'project_tree' => [
-              {
-                'issues' => [
-                  :id,
-                  :title,
-                  { 'notes' => [:id, :note, { 'author' => [:name] }] }
-                ]
-              }
-            ],
-            'ee' => {
-              'project_tree' => [
-                {
-                  'issues' => [
-                    :description,
-                    { 'notes' => [:date, { 'author' => [:email] }] }
-                  ]
-                },
-                { 'foo' => [{ 'bar' => %i[baz] }] }
-              ]
-            }
-          })
+          included_attributes:
+            user:
+              - :id
 
-        expect(yaml_file.to_h).to eq({
-          'project_tree' => [
-            {
-              'issues' => [
-                :id,
-                :title,
-                {
-                  'notes' => [
-                    :id,
-                    :note,
-                    { 'author' => [:name, :email] },
-                    :date
-                  ]
-                },
-                :description
-              ]
-            },
-            { 'foo' => [{ 'bar' => %i[baz] }] }
-          ]
-        })
+          excluded_attributes:
+            project:
+              - :name
+
+          methods:
+            labels:
+              - :type
+            events:
+              - :action
+
+          ee:
+            tree:
+              project:
+                protected_branches:
+                  - :unprotect_access_levels
+            included_attributes:
+              user:
+                - :name_ee
+            excluded_attributes:
+              project:
+                - :name_without_ee
+            methods:
+              labels:
+                - :type_ee
+              events_ee:
+                - :action_ee
+        EOF
       end
 
-      it 'merges the excluded attributes list' do
-        allow(yaml_file)
-          .to receive(:parse_yaml)
-          .and_return({
-            'project_tree' => [],
-            'excluded_attributes' => {
-              'project' => %i[id title],
-              'notes' => %i[id]
-            },
-            'ee' => {
-              'project_tree' => [],
-              'excluded_attributes' => {
-                'project' => %i[date],
-                'foo' => %i[bar baz]
-              }
-            }
-          })
-
-        expect(yaml_file.to_h).to eq({
-          'project_tree' => [],
-          'excluded_attributes' => {
-            'project' => %i[id title date],
-            'notes' => %i[id],
-            'foo' => %i[bar baz]
-          }
-        })
+      let(:config_hash) { YAML.safe_load(config, [Symbol]) }
+
+      before do
+        allow_any_instance_of(described_class).to receive(:parse_yaml) do
+          config_hash.deep_dup
+        end
       end
 
-      it 'merges the included attributes list' do
-        allow(yaml_file)
-          .to receive(:parse_yaml)
-          .and_return({
-            'project_tree' => [],
-            'included_attributes' => {
-              'project' => %i[id title],
-              'notes' => %i[id]
-            },
-            'ee' => {
-              'project_tree' => [],
-              'included_attributes' => {
-                'project' => %i[date],
-                'foo' => %i[bar baz]
+      context 'when using CE' do
+        before do
+          allow(Gitlab).to receive(:ee?) { false }
+        end
+
+        it 'just returns the normalized Hash' do
+          is_expected.to eq(
+            {
+              tree: {
+                project: {
+                  labels: {
+                    priorities: {}
+                  },
+                  milestones: {
+                    events: {
+                      push_event_payload: {}
+                    }
+                  }
+                }
+              },
+              included_attributes: {
+                user: [:id]
+              },
+              excluded_attributes: {
+                project: [:name]
+              },
+              methods: {
+                labels: [:type],
+                events: [:action]
               }
             }
-          })
-
-        expect(yaml_file.to_h).to eq({
-          'project_tree' => [],
-          'included_attributes' => {
-            'project' => %i[id title date],
-            'notes' => %i[id],
-            'foo' => %i[bar baz]
-          }
-        })
+          )
+        end
       end
 
-      it 'merges the methods list' do
-        allow(yaml_file)
-          .to receive(:parse_yaml)
-          .and_return({
-            'project_tree' => [],
-            'methods' => {
-              'project' => %i[id title],
-              'notes' => %i[id]
-            },
-            'ee' => {
-              'project_tree' => [],
-              'methods' => {
-                'project' => %i[date],
-                'foo' => %i[bar baz]
+      context 'when using EE' do
+        before do
+          allow(Gitlab).to receive(:ee?) { true }
+        end
+
+        it 'just returns the normalized Hash' do
+          is_expected.to eq(
+            {
+              tree: {
+                project: {
+                  labels: {
+                    priorities: {}
+                  },
+                  milestones: {
+                    events: {
+                      push_event_payload: {}
+                    }
+                  },
+                  protected_branches: {
+                    unprotect_access_levels: {}
+                  }
+                }
+              },
+              included_attributes: {
+                user: [:id, :name_ee]
+              },
+              excluded_attributes: {
+                project: [:name, :name_without_ee]
+              },
+              methods: {
+                labels: [:type, :type_ee],
+                events: [:action],
+                events_ee: [:action_ee]
               }
             }
-          })
-
-        expect(yaml_file.to_h).to eq({
-          'project_tree' => [],
-          'methods' => {
-            'project' => %i[id title date],
-            'notes' => %i[id],
-            'foo' => %i[bar baz]
-          }
-        })
+          )
+        end
       end
     end
   end
diff --git a/spec/lib/gitlab/import_export/model_configuration_spec.rb b/spec/lib/gitlab/import_export/model_configuration_spec.rb
index 5ed9fef1597..3442e22c11f 100644
--- a/spec/lib/gitlab/import_export/model_configuration_spec.rb
+++ b/spec/lib/gitlab/import_export/model_configuration_spec.rb
@@ -8,7 +8,7 @@ describe 'Import/Export model configuration' do
 
   let(:config_hash) { Gitlab::ImportExport::Config.new.to_h.deep_stringify_keys }
   let(:model_names) do
-    names = names_from_tree(config_hash['project_tree'])
+    names = names_from_tree(config_hash.dig('tree', 'project'))
 
     # Remove duplicated or add missing models
     # - project is not part of the tree, so it has to be added manually.
diff --git a/spec/lib/gitlab/import_export/reader_spec.rb b/spec/lib/gitlab/import_export/reader_spec.rb
index f93ff074770..87f665bd995 100644
--- a/spec/lib/gitlab/import_export/reader_spec.rb
+++ b/spec/lib/gitlab/import_export/reader_spec.rb
@@ -2,96 +2,45 @@ require 'spec_helper'
 
 describe Gitlab::ImportExport::Reader do
   let(:shared) { Gitlab::ImportExport::Shared.new(nil) }
-  let(:test_config) { 'spec/support/import_export/import_export.yml' }
-  let(:project_tree_hash) do
-    {
-      except: [:id, :created_at],
-      include: [:issues, :labels,
-                { merge_requests: {
-                  only: [:id],
-                  except: [:iid],
-                  include: [:merge_request_diff, :merge_request_test]
-                } },
-                { commit_statuses: { include: :commit } },
-                { project_members: { include: { user: { only: [:email] } } } }]
-    }
-  end
-
-  before do
-    allow_any_instance_of(Gitlab::ImportExport).to receive(:config_file).and_return(test_config)
-  end
-
-  it 'generates hash from project tree config' do
-    expect(described_class.new(shared: shared).project_tree).to match(project_tree_hash)
-  end
-
-  context 'individual scenarios' do
-    it 'generates the correct hash for a single project relation' do
-      setup_yaml(project_tree: [:issues])
-
-      expect(described_class.new(shared: shared).project_tree).to match(include: [:issues])
-    end
-
-    it 'generates the correct hash for a single project feature relation' do
-      setup_yaml(project_tree: [:project_feature])
 
-      expect(described_class.new(shared: shared).project_tree).to match(include: [:project_feature])
-    end
+  describe '#project_tree' do
+    subject { described_class.new(shared: shared).project_tree }
 
-    it 'generates the correct hash for a multiple project relation' do
-      setup_yaml(project_tree: [:issues, :snippets])
+    it 'delegates to AttributesFinder#find_root' do
+      expect_any_instance_of(Gitlab::ImportExport::AttributesFinder)
+        .to receive(:find_root)
+        .with(:project)
 
-      expect(described_class.new(shared: shared).project_tree).to match(include: [:issues, :snippets])
+      subject
     end
 
-    it 'generates the correct hash for a single sub-relation' do
-      setup_yaml(project_tree: [issues: [:notes]])
+    context 'when exception raised' do
+      before do
+        expect_any_instance_of(Gitlab::ImportExport::AttributesFinder)
+          .to receive(:find_root)
+          .with(:project)
+          .and_raise(StandardError)
+      end
 
-      expect(described_class.new(shared: shared).project_tree).to match(include: [{ issues: { include: :notes } }])
-    end
-
-    it 'generates the correct hash for a multiple sub-relation' do
-      setup_yaml(project_tree: [merge_requests: [:notes, :merge_request_diff]])
-
-      expect(described_class.new(shared: shared).project_tree).to match(include: [{ merge_requests: { include: [:notes, :merge_request_diff] } }])
-    end
+      it { is_expected.to be false }
 
-    it 'generates the correct hash for a sub-relation with another sub-relation' do
-      setup_yaml(project_tree: [merge_requests: [notes: :author]])
+      it 'logs the error' do
+        expect(shared).to receive(:error).with(instance_of(StandardError))
 
-      expect(described_class.new(shared: shared).project_tree).to match(include: [{ merge_requests: { include: { notes: { include: :author } } } }])
+        subject
+      end
     end
+  end
 
-    it 'generates the correct hash for a relation with included attributes' do
-      setup_yaml(project_tree: [:issues], included_attributes: { issues: [:name, :description] })
-
-      expect(described_class.new(shared: shared).project_tree).to match(include: [{ issues: { only: [:name, :description] } }])
-    end
-
-    it 'generates the correct hash for a relation with excluded attributes' do
-      setup_yaml(project_tree: [:issues], excluded_attributes: { issues: [:name] })
-
-      expect(described_class.new(shared: shared).project_tree).to match(include: [{ issues: { except: [:name] } }])
-    end
-
-    it 'generates the correct hash for a relation with both excluded and included attributes' do
-      setup_yaml(project_tree: [:issues], excluded_attributes: { issues: [:name] }, included_attributes: { issues: [:description] })
-
-      expect(described_class.new(shared: shared).project_tree).to match(include: [{ issues: { except: [:name], only: [:description] } }])
-    end
-
-    it 'generates the correct hash for a relation with custom methods' do
-      setup_yaml(project_tree: [:issues], methods: { issues: [:name] })
-
-      expect(described_class.new(shared: shared).project_tree).to match(include: [{ issues: { methods: [:name] } }])
-    end
+  describe '#group_members_tree' do
+    subject { described_class.new(shared: shared).group_members_tree }
 
-    it 'generates the correct hash for group members' do
-      expect(described_class.new(shared: shared).group_members_tree).to match({ include: { user: { only: [:email] } } })
-    end
+    it 'delegates to AttributesFinder#find_root' do
+      expect_any_instance_of(Gitlab::ImportExport::AttributesFinder)
+        .to receive(:find_root)
+        .with(:group_members)
 
-    def setup_yaml(hash)
-      allow(YAML).to receive(:load_file).with(test_config).and_return(hash)
+      subject
     end
   end
 end
diff --git a/spec/lib/gitlab/import_export/relation_rename_service_spec.rb b/spec/lib/gitlab/import_export/relation_rename_service_spec.rb
index 15748407f0c..17bb5bcc155 100644
--- a/spec/lib/gitlab/import_export/relation_rename_service_spec.rb
+++ b/spec/lib/gitlab/import_export/relation_rename_service_spec.rb
@@ -12,7 +12,7 @@ describe Gitlab::ImportExport::RelationRenameService do
 
   let(:user) { create(:admin) }
   let(:group) { create(:group, :nested) }
-  let!(:project) { create(:project, :builds_disabled, :issues_disabled, name: 'project', path: 'project') }
+  let!(:project) { create(:project, :builds_disabled, :issues_disabled, group: group, name: 'project', path: 'project') }
   let(:shared) { project.import_export_shared }
 
   before do
@@ -24,7 +24,6 @@ describe Gitlab::ImportExport::RelationRenameService do
     let(:import_path) { 'spec/lib/gitlab/import_export' }
     let(:file_content) { IO.read("#{import_path}/project.json") }
     let!(:json_file) { ActiveSupport::JSON.decode(file_content) }
-    let(:tree_hash) { project_tree_restorer.instance_variable_get(:@tree_hash) }
 
     before do
       allow(shared).to receive(:export_path).and_return(import_path)
@@ -92,21 +91,25 @@ describe Gitlab::ImportExport::RelationRenameService do
   end
 
   context 'when exporting' do
-    let(:project_tree_saver) { Gitlab::ImportExport::ProjectTreeSaver.new(project: project, current_user: user, shared: shared) }
-    let(:project_tree) { project_tree_saver.send(:project_json) }
+    let(:export_content_path) { project_tree_saver.full_path }
+    let(:export_content_hash) { ActiveSupport::JSON.decode(File.read(export_content_path)) }
+    let(:injected_hash) { renames.values.product([{}]).to_h }
 
-    it 'adds old relationships to the exported file' do
-      project_tree.merge!(renames.values.map { |new_name| [new_name, []] }.to_h)
+    let(:project_tree_saver) do
+      Gitlab::ImportExport::ProjectTreeSaver.new(
+        project: project, current_user: user, shared: shared)
+    end
 
-      allow(project_tree_saver).to receive(:save) do |arg|
-        project_tree_saver.send(:project_json_tree)
+    it 'adds old relationships to the exported file' do
+      # we inject relations with new names that should be rewritten
+      expect(project_tree_saver).to receive(:serialize_project_tree).and_wrap_original do |method, *args|
+        method.call(*args).merge(injected_hash)
       end
 
-      result = project_tree_saver.save
-
-      saved_data = ActiveSupport::JSON.decode(result)
+      expect(project_tree_saver.save).to eq(true)
 
-      expect(saved_data.keys).to include(*(renames.keys + renames.values))
+      expect(export_content_hash.keys).to include(*renames.keys)
+      expect(export_content_hash.keys).to include(*renames.values)
     end
   end
 end
-- 
cgit v1.2.3