Store manifest import data outside of session

When a user uploads a manifest file to import (typically for the Android
Open Source Project), we store some data about that import in Redis.
Previously, we stored that in the session directly, but that meant that
as long as the session didn't expire, neither would this - quite large -
import status data.

This changes the import status data to be stored in its own dedicated
fields, with explicit expiry set. To handle existing imports, it will
still read data from the session if it can't find anything in the new
place.

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,22 +16,55 @@ 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
 
-  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(:repos) { [repo1, repo2] }
+    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')
+             }
 
-    before do
-      assign_session_group
+        expect(assigns(:errors)).to be_present
+      end
+    end
 
-      session[:manifest_import_repositories] = repos
+    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) { { id: 'test1', url: 'http://demo.host/test1' } }
+    let(:repo2) { { id: 'test2', url: 'http://demo.host/test2' } }
+    let(:repos) { [repo1, repo2] }
+
+    shared_examples 'status action' do
       it "returns variables for json request" do
         project = create(:project, import_type: 'manifest', creator_id: user.id)
 
@@ -39,19 +72,37 @@ RSpec.describe Import::ManifestController do
 
         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("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
 
       it "does not show already added project" do
-      project = create(:project, import_type: 'manifest', namespace: user.namespace, import_status: :finished, import_url: repo1.url)
+        project = create(:project, import_type: 'manifest', namespace: user.namespace, import_status: :finished, import_url: repo1[:url])
 
         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)
+        expect(json_response.dig("provider_repos", 0, "id")).not_to eq(repo1[:id])
+      end
+    end
+
+    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
+
+    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
+
+      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