Merge branch 'store-import-manifest-data-outside-session' into 'master'

Store manifest import data outside of session

See merge request gitlab-org/gitlab!42869

app/controllers/import/manifest_controller.rb

@@ -26,8 +26,7 @@ class Import::ManifestController < Import::BaseController
     manifest = Gitlab::ManifestImport::Manifest.new(params[:manifest].tempfile)
 
     if manifest.valid?
-      session[:manifest_import_repositories] = manifest.projects
-      session[:manifest_import_group_id] = group.id
+      manifest_import_metadata.save(manifest.projects, group.id)
 
       redirect_to status_import_manifest_path
     else
@@ -96,12 +95,16 @@ class Import::ManifestController < Import::BaseController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def group
-    @group ||= Group.find_by(id: session[:manifest_import_group_id])
+    @group ||= Group.find_by(id: manifest_import_metadata.group_id)
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
+  def manifest_import_metadata
+    @manifest_import_status ||= Gitlab::ManifestImport::Metadata.new(current_user, fallback: session)
+  end
+
   def repositories
-    @repositories ||= session[:manifest_import_repositories]
+    @repositories ||= manifest_import_metadata.repositories
   end
 
   def find_jobs

lib/gitlab/manifest_import/metadata.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module ManifestImport
+    class Metadata
+      EXPIRY_TIME = 1.week
+
+      attr_reader :user, :fallback
+
+      def initialize(user, fallback: {})
+        @user = user
+        @fallback = fallback
+      end
+
+      def save(repositories, group_id)
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.multi do
+            redis.set(key_for('repositories'), Gitlab::Json.dump(repositories), ex: EXPIRY_TIME)
+            redis.set(key_for('group_id'), group_id, ex: EXPIRY_TIME)
+          end
+        end
+      end
+
+      def repositories
+        redis_get('repositories').then do |repositories|
+          next unless repositories
+
+          Gitlab::Json.parse(repositories).map(&:symbolize_keys)
+        end || fallback[:manifest_import_repositories]
+      end
+
+      def group_id
+        redis_get('group_id')&.to_i || fallback[:manifest_import_group_id]
+      end
+
+      private
+
+      def key_for(field)
+        "manifest_import:metadata:user:#{user.id}:#{field}"
+      end
+
+      def redis_get(field)
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.get(key_for(field))
+        end
+      end
+    end
+  end
+end

spec/controllers/import/manifest_controller_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Import::ManifestController do
+RSpec.describe Import::ManifestController, :clean_gitlab_redis_shared_state do
   include ImportSpecHelper
 
   let_it_be(:user) { create(:user) }
@@ -16,42 +16,93 @@ RSpec.describe Import::ManifestController do
     sign_in(user)
   end
 
-  def assign_session_group
-    session[:manifest_import_repositories] = []
-    session[:manifest_import_group_id] = group.id
+  describe 'POST upload' do
+    context 'with a valid manifest' do
+      it 'saves the manifest and redirects to the status page', :aggregate_failures do
+        post :upload, params: {
+               group_id: group.id,
+               manifest: fixture_file_upload('spec/fixtures/aosp_manifest.xml')
+             }
+
+        metadata = Gitlab::ManifestImport::Metadata.new(user)
+
+        expect(metadata.group_id).to eq(group.id)
+        expect(metadata.repositories.size).to eq(660)
+        expect(metadata.repositories.first).to include(name: 'platform/build', path: 'build/make')
+
+        expect(response).to redirect_to(status_import_manifest_path)
+      end
+    end
+
+    context 'with an invalid manifest' do
+      it 'displays an error' do
+        post :upload, params: {
+               group_id: group.id,
+               manifest: fixture_file_upload('spec/fixtures/invalid_manifest.xml')
+             }
+
+        expect(assigns(:errors)).to be_present
+      end
+    end
+
+    context 'when the user cannot create projects in the group' do
+      it 'displays an error' do
+        sign_in(create(:user))
+
+        post :upload, params: {
+               group_id: group.id,
+               manifest: fixture_file_upload('spec/fixtures/aosp_manifest.xml')
+             }
+
+        expect(assigns(:errors)).to be_present
+      end
+    end
   end
 
   describe 'GET status' do
-    let(:repo1) { OpenStruct.new(id: 'test1', url: 'http://demo.host/test1') }
-    let(:repo2) { OpenStruct.new(id: 'test2', url: 'http://demo.host/test2') }
+    let(:repo1) { { id: 'test1', url: 'http://demo.host/test1' } }
+    let(:repo2) { { id: 'test2', url: 'http://demo.host/test2' } }
     let(:repos) { [repo1, repo2] }
 
-    before do
-      assign_session_group
+    shared_examples 'status action' do
+      it "returns variables for json request" do
+        project = create(:project, import_type: 'manifest', creator_id: user.id)
 
-      session[:manifest_import_repositories] = repos
-    end
+        get :status, format: :json
 
-    it "returns variables for json request" do
-      project = create(:project, import_type: 'manifest', creator_id: user.id)
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response.dig("imported_projects", 0, "id")).to eq(project.id)
+        expect(json_response.dig("provider_repos", 0, "id")).to eq(repo1[:id])
+        expect(json_response.dig("provider_repos", 1, "id")).to eq(repo2[:id])
+        expect(json_response.dig("namespaces", 0, "id")).to eq(group.id)
+      end
 
-      get :status, format: :json
+      it "does not show already added project" do
+        project = create(:project, import_type: 'manifest', namespace: user.namespace, import_status: :finished, import_url: repo1[:url])
 
-      expect(response).to have_gitlab_http_status(:ok)
-      expect(json_response.dig("imported_projects", 0, "id")).to eq(project.id)
-      expect(json_response.dig("provider_repos", 0, "id")).to eq(repo1.id)
-      expect(json_response.dig("provider_repos", 1, "id")).to eq(repo2.id)
-      expect(json_response.dig("namespaces", 0, "id")).to eq(group.id)
+        get :status, format: :json
+
+        expect(json_response.dig("imported_projects", 0, "id")).to eq(project.id)
+        expect(json_response.dig("provider_repos").length).to eq(1)
+        expect(json_response.dig("provider_repos", 0, "id")).not_to eq(repo1[:id])
+      end
     end
 
-    it "does not show already added project" do
-      project = create(:project, import_type: 'manifest', namespace: user.namespace, import_status: :finished, import_url: repo1.url)
+    context 'when the data is stored via Gitlab::ManifestImport::Metadata' do
+      before do
+        Gitlab::ManifestImport::Metadata.new(user).save(repos, group.id)
+      end
+
+      include_examples 'status action'
+    end
 
-      get :status, format: :json
+    context 'when the data is stored in the user session' do
+      before do
+        session[:manifest_import_repositories] = repos
+        session[:manifest_import_group_id] = group.id
+      end
 
-      expect(json_response.dig("imported_projects", 0, "id")).to eq(project.id)
-      expect(json_response.dig("provider_repos").length).to eq(1)
-      expect(json_response.dig("provider_repos", 0, "id")).not_to eq(repo1.id)
+      include_examples 'status action'
     end
   end
 end

spec/fixtures/invalid_manifest.xml

+<manifest>
+  <remote review="invalid-url" />
+  <project name="platform/build"/>
+</manifest>

spec/lib/gitlab/manifest_import/manifest_spec.rb

@@ -12,19 +12,7 @@ RSpec.describe Gitlab::ManifestImport::Manifest do
     end
 
     context 'missing or invalid attributes' do
-      let(:file) { Tempfile.new('foo') }
-
-      before do
-        content = <<~EOS
-          <manifest>
-            <remote review="invalid-url" />
-            <project name="platform/build"/>
-          </manifest>
-        EOS
-
-        file.write(content)
-        file.rewind
-      end
+      let(:file) { File.open(Rails.root.join('spec/fixtures/invalid_manifest.xml')) }
 
       it { expect(manifest.valid?).to be false }
 

spec/lib/gitlab/manifest_import/metadata_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::ManifestImport::Metadata, :clean_gitlab_redis_shared_state do
+  let(:user) { double(id: 1) }
+  let(:repositories) do
+    [
+      { id: 'test1', url: 'http://demo.host/test1' },
+      { id: 'test2', url: 'http://demo.host/test2' }
+    ]
+  end
+
+  describe '#save' do
+    it 'stores data in Redis with an expiry of EXPIRY_TIME' do
+      status = described_class.new(user)
+      repositories_key = 'manifest_import:metadata:user:1:repositories'
+      group_id_key = 'manifest_import:metadata:user:1:group_id'
+
+      status.save(repositories, 2)
+
+      Gitlab::Redis::SharedState.with do |redis|
+        expect(redis.ttl(repositories_key)).to be_within(5).of(described_class::EXPIRY_TIME)
+        expect(redis.ttl(group_id_key)).to be_within(5).of(described_class::EXPIRY_TIME)
+      end
+    end
+  end
+
+  describe '#repositories' do
+    it 'allows repositories to round-trip with symbol keys' do
+      status = described_class.new(user)
+
+      status.save(repositories, 2)
+
+      expect(status.repositories).to eq(repositories)
+    end
+
+    it 'uses the fallback when there is nothing in Redis' do
+      fallback = { manifest_import_repositories: repositories }
+      status = described_class.new(user, fallback: fallback)
+
+      expect(status.repositories).to eq(repositories)
+    end
+  end
+
+  describe '#group_id' do
+    it 'returns the group ID as an integer' do
+      status = described_class.new(user)
+
+      status.save(repositories, 2)
+
+      expect(status.group_id).to eq(2)
+    end
+
+    it 'uses the fallback when there is nothing in Redis' do
+      fallback = { manifest_import_group_id: 3 }
+      status = described_class.new(user, fallback: fallback)
+
+      expect(status.group_id).to eq(3)
+    end
+  end
+end