Generate smaller image sizes for Designs

Adds a new DesignV432x230Uploader to generate and serve smaller
versions of design files.

The original (full-sized) design files are stored in Git LFS, and so
have a different uploader, `LfsObjectUploader`.

The decision to make a separate uploader to handle just the smaller
image versions came out of a discussion in
https://gitlab.com/gitlab-org/gitlab/merge_requests/20511. Initially,
the direction taken was to make LfsObjectUploader conditionally generate
smaller images for LfsObject files. The feedback was that this was
adding logic that was too specific to design management into an uploader
that is very general, and that we should use a separate uploader to
handle just the smaller image versions.

A service now runs after a new DesignManagement::Version has been
created, which downloads the original design files from their Git LFS
storage and generates smaller versions, storing them in the regular
uploads storage.

This change is behind the :design_management_resize_images feature flag.

https://gitlab.com/gitlab-org/gitlab/issues/12577
https://gitlab.com/gitlab-org/gitlab/issues/13815

changelogs/unreleased/12577-generate-smaller-image-sizes-for-designs.yml

+---
+title: Add id and image_v432x230 columns to design_management_designs_versions
+merge_request: 22860
+author:
+type: changed

db/migrate/20191206014412_add_image_to_design_management_designs_versions.rb

+# frozen_string_literal: true
+
+class AddImageToDesignManagementDesignsVersions < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :design_management_designs_versions, :image_v432x230, :string, limit: 255
+  end
+end

db/migrate/20200203015140_add_id_to_design_management_designs_versions.rb

+# frozen_string_literal: true
+
+class AddIdToDesignManagementDesignsVersions < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def change
+    add_column :design_management_designs_versions, :id, :primary_key
+  end
+end

db/schema.rb

@@ -1417,10 +1417,11 @@ ActiveRecord::Schema.define(version: 2020_02_14_034836) do
     t.index ["project_id"], name: "index_design_management_designs_on_project_id"
   end
 
-  create_table "design_management_designs_versions", id: false, force: :cascade do |t|
+  create_table "design_management_designs_versions", force: :cascade do |t|
     t.bigint "design_id", null: false
     t.bigint "version_id", null: false
     t.integer "event", limit: 2, default: 0, null: false
+    t.string "image_v432x230", limit: 255
     t.index ["design_id", "version_id"], name: "design_management_designs_versions_uniqueness", unique: true
     t.index ["design_id"], name: "index_design_management_designs_versions_on_design_id"
     t.index ["event"], name: "index_design_management_designs_versions_on_event"

doc/administration/raketasks/uploads/migrate.md

@@ -73,6 +73,9 @@ gitlab-rake "gitlab:uploads:migrate[FileUploader, Project]"
 gitlab-rake "gitlab:uploads:migrate[PersonalFileUploader, Snippet]"
 gitlab-rake "gitlab:uploads:migrate[NamespaceFileUploader, Snippet]"
 gitlab-rake "gitlab:uploads:migrate[FileUploader, MergeRequest]"
+
+# Design Management design thumbnails (EE)
+gitlab-rake "gitlab:uploads:migrate[DesignManagement::DesignV432x230Uploader, DesignManagement::Action, :image_v432x230]"
 ```
 
 **Source Installation**
@@ -102,6 +105,8 @@ sudo -u git -H bundle exec rake "gitlab:uploads:migrate[PersonalFileUploader, Sn
 sudo -u git -H bundle exec rake "gitlab:uploads:migrate[NamespaceFileUploader, Snippet]"
 sudo -u git -H bundle exec rake "gitlab:uploads:migrate[FileUploader, MergeRequest]"
 
+# Design Management design thumbnails (EE)
+sudo -u git -H bundle exec rake "gitlab:uploads:migrate[DesignManagement::DesignV432x230Uploader, DesignManagement::Action]"
 ```
 
 ## Migrate legacy uploads out of deprecated paths

doc/development/file_storage.md

@@ -21,6 +21,7 @@ There are many places where file uploading is used, according to contexts:
   - CI Artifacts (archive, metadata, trace)
   - LFS Objects
   - Merge request diffs
+  - Design Management design thumbnails (EE)
 
 ## Disk storage
 
@@ -37,6 +38,7 @@ they are still not 100% standardized. You can see them below:
 | Project avatars                       | yes    | uploads/-/system/project/avatar/:id/:filename               | `AvatarUploader`       | Project    |
 | Issues/MR/Notes Markdown attachments  | yes    | uploads/:project_path_with_namespace/:random_hex/:filename  | `FileUploader`         | Project    |
 | Issues/MR/Notes Legacy Markdown attachments | no | uploads/-/system/note/attachment/:id/:filename            | `AttachmentUploader`   | Note       |
+| Design Management design thumbnails (EE) | yes | uploads/-/system/design_management/action/image_v432x230/:id/:filename | `DesignManagement::DesignV432x230Uploader` | DesignManagement::Action |
 | CI Artifacts (CE)                     | yes    | `shared/artifacts/:disk_hash[0..1]/:disk_hash[2..3]/:disk_hash/:year_:month_:date/:job_id/:job_artifact_id` (:disk_hash is SHA256 digest of project_id) | `JobArtifactUploader`  | Ci::JobArtifact  |
 | LFS Objects  (CE)                     | yes    | shared/lfs-objects/:hex/:hex/:object_hash                   | `LfsObjectUploader`    | LfsObject  |
 | External merge request diffs          | yes    | shared/external-diffs/merge_request_diffs/mr-:parent_id/diff-:id | `ExternalDiffUploader` | MergeRequestDiff |

ee/app/models/design_management/action.rb

@@ -2,8 +2,12 @@
 
 module DesignManagement
   class Action < ApplicationRecord
+    include WithUploads
+
     self.table_name = "#{DesignManagement.table_name_prefix}designs_versions"
 
+    mount_uploader :image_v432x230, DesignManagement::DesignV432x230Uploader
+
     belongs_to :design, class_name: "DesignManagement::Design", inverse_of: :actions
     belongs_to :version, class_name: "DesignManagement::Version", inverse_of: :actions
 

ee/app/models/design_management/version.rb

@@ -53,7 +53,7 @@ module DesignManagement
     scope :for_designs, -> (designs) do
       where(id: ::DesignManagement::Action.where(design_id: designs).select(:version_id)).distinct
     end
-    scope :earlier_or_equal_to, -> (version) { where('id <= ?', version) }
+    scope :earlier_or_equal_to, -> (version) { where("(#{table_name}.id) <= ?", version) } # rubocop:disable GitlabSecurity/SqlInjection
     scope :ordered, -> { order(id: :desc) }
     scope :for_issue, -> (issue) { where(issue: issue) }
     scope :by_sha, -> (sha) { where(sha: sha) }

ee/app/services/design_management/generate_image_versions_service.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  # This service generates smaller image versions for `DesignManagement::Design`
+  # records within a given `DesignManagement::Version`.
+  class GenerateImageVersionsService < DesignService
+    # We limit processing to only designs with file sizes that don't
+    # exceed `MAX_DESIGN_SIZE`.
+    #
+    # Note, we may be able to remove checking this limit, if when we come to
+    # implement a file size limit for designs, there are no designs that
+    # exceed 40MB on GitLab.com
+    #
+    # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/22860#note_281780387
+    MAX_DESIGN_SIZE = 40.megabytes.freeze
+
+    def initialize(version)
+      super(version.project, version.author, issue: version.issue)
+
+      @version = version
+    end
+
+    def execute
+      # rubocop: disable CodeReuse/ActiveRecord
+      version.actions.includes(:design).each do |action|
+        generate_image(action)
+      end
+      # rubocop: enable CodeReuse/ActiveRecord
+
+      success(version: version)
+    end
+
+    private
+
+    attr_reader :version
+
+    def generate_image(action)
+      raw_file = get_raw_file(action)
+
+      unless raw_file
+        log_error("No design file found for Action: #{action.id}")
+        return
+      end
+
+      # Skip attempting to process images that would be rejected by CarrierWave.
+      return unless DesignManagement::DesignV432x230Uploader::MIME_TYPE_WHITELIST.include?(raw_file.content_type)
+
+      # Store and process the file
+      action.image_v432x230.store!(raw_file)
+      action.save!
+    rescue CarrierWave::UploadError => e
+      Gitlab::ErrorTracking.track_exception(e, project_id: project.id, design_id: action.design_id, version_id: action.version_id)
+      log_error(e.message)
+    end
+
+    # Returns the `CarrierWave::SanitizedFile` of the original design file
+    def get_raw_file(action)
+      raw_files_by_path[action.design.full_path]
+    end
+
+    # Returns the `Carrierwave:SanitizedFile` instances for all of the original
+    # design files, mapping to { design.filename => `Carrierwave::SanitizedFile` }.
+    #
+    # As design files are stored in Git LFS, the only way to retrieve their original
+    # files is to first fetch the LFS pointer file data from the Git design repository.
+    # The LFS pointer file data contains an "OID" that lets us retrieve `LfsObject`
+    # records, which have an Uploader (`LfsObjectUploader`) for the original design file.
+    def raw_files_by_path
+      @raw_files_by_path ||= begin
+        LfsObject.for_oids(blobs_by_oid.keys).each_with_object({}) do |lfs_object, h|
+          blob = blobs_by_oid[lfs_object.oid]
+          file = lfs_object.file.file
+          # The `CarrierWave::SanitizedFile` is loaded without knowing the `content_type`
+          # of the file, due to the file not having an extension.
+          #
+          # Set the content_type from the `Blob`.
+          file.content_type = blob.content_type
+          h[blob.path] = file
+        end
+      end
+    end
+
+    # Returns the `Blob`s that correspond to the design files in the repository.
+    #
+    # All design `Blob`s are LFS Pointer files, and are therefore small amounts
+    # of data to load.
+    #
+    # `Blob`s whose size are above a certain threshold: `MAX_DESIGN_SIZE`
+    # are filtered out.
+    def blobs_by_oid
+      @blobs ||= begin
+        items = version.designs.map { |design| [version.sha, design.full_path] }
+        blobs = repository.blobs_at(items)
+        blobs.reject! { |blob| blob.lfs_size > MAX_DESIGN_SIZE }
+        blobs.index_by(&:lfs_oid)
+      end
+    end
+  end
+end

ee/app/uploaders/design_management/design_v432x230_uploader.rb

+# frozen_string_literal: true
+
+module DesignManagement
+  # This Uploader is used to generate and serve the smaller versions of
+  # the design files.
+  #
+  # The original (full-sized) design files are stored in Git LFS, and so
+  # have a different uploader, `LfsObjectUploader`.
+  class DesignV432x230Uploader < GitlabUploader
+    include CarrierWave::MiniMagick
+    include RecordsUploads::Concern
+    include ObjectStorage::Concern
+    prepend ObjectStorage::Extension::RecordsUploads
+
+    # We currently cannot resize `image/ico` or `image/svg+xml` mime types.
+    # See https://gitlab.com/gitlab-org/gitlab/issues/207069
+    MIME_TYPE_WHITELIST = %w(image/png image/jpeg image/bmp image/gif image/tiff).freeze
+
+    process resize_to_fit: [432, 230]
+
+    # Allow CarrierWave to reject files without correct mimetypes.
+    def content_type_whitelist
+      MIME_TYPE_WHITELIST
+    end
+
+    # Override `GitlabUploader` and always return false, otherwise local
+    # `LfsObject` files would be deleted.
+    # https://github.com/carrierwaveuploader/carrierwave/blob/f84672a/lib/carrierwave/uploader/cache.rb#L131-L135
+    def move_to_cache
+      false
+    end
+
+    private
+
+    def dynamic_segment
+      File.join(model.class.underscore, mounted_as.to_s, model.id.to_s)
+    end
+  end
+end

ee/app/workers/all_queues.yml

@@ -441,7 +441,7 @@
   :feature_category: :design_management
   :has_external_dependencies: 
   :latency_sensitive: 
-  :resource_boundary: :unknown
+  :resource_boundary: :memory
   :weight: 1
   :idempotent: 
 - :name: elastic_batch_project_indexer

ee/app/workers/design_management/new_version_worker.rb

@@ -5,11 +5,15 @@ module DesignManagement
     include ApplicationWorker
 
     feature_category :design_management
+    # Declare this worker as memory bound due to
+    # `GenerateImageVersionsService` resizing designs
+    worker_resource_boundary :memory
 
     def perform(version_id)
       version = DesignManagement::Version.find(version_id)
 
       add_system_note(version)
+      generate_image_versions(version)
     rescue ActiveRecord::RecordNotFound => e
       Sidekiq.logger.warn(e)
     end
@@ -19,5 +23,11 @@ module DesignManagement
     def add_system_note(version)
       SystemNoteService.design_version_added(version)
     end
+
+    def generate_image_versions(version)
+      return unless ::Feature.enabled?(:design_management_resize_images, version.project)
+
+      DesignManagement::GenerateImageVersionsService.new(version).execute
+    end
   end
 end

ee/lib/ee/gitlab/uploads/migration_helper.rb

+# frozen_string_literal: true
+
+module EE
+  module Gitlab
+    module Uploads
+      module MigrationHelper
+        extend ActiveSupport::Concern
+
+        EE_CATEGORIES = [
+          %w(DesignManagement::DesignV432x230Uploader DesignManagement::Action :image_v432x230)
+        ].freeze
+
+        class_methods do
+          extend ::Gitlab::Utils::Override
+
+          override :categories
+          def categories
+            super + EE_CATEGORIES
+          end
+        end
+      end
+    end
+  end
+end

ee/spec/factories/design_management/actions.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :design_action, class: 'DesignManagement::Action' do
+    design
+    association :version, factory: :design_version
+    event { :creation }
+
+    trait :with_image_v432x230 do
+      image_v432x230 { fixture_file_upload('spec/fixtures/dk.png') }
+    end
+  end
+end

ee/spec/factories/design_management/designs.rb

@@ -53,11 +53,20 @@ FactoryBot.define do
       design.clear_version_cache
     end
 
+    # Use this trait to build designs that are backed by Git LFS, committed
+    # to the repository, and with an LfsObject correctly created for it.
     trait :with_lfs_file do
       with_file
 
       transient do
-        file { Gitlab::Git::LfsPointerFile.new('').pointer }
+        raw_file { fixture_file_upload('spec/fixtures/dk.png', 'image/png') }
+        lfs_pointer { Gitlab::Git::LfsPointerFile.new(SecureRandom.random_bytes) }
+        file { lfs_pointer.pointer }
+      end
+
+      after :create do |design, evaluator|
+        lfs_object = create(:lfs_object, file: evaluator.raw_file, oid: evaluator.lfs_pointer.sha256, size: evaluator.lfs_pointer.size)
+        create(:lfs_objects_project, project: design.project, lfs_object: lfs_object, repository_type: :design)
       end
     end
 
@@ -82,8 +91,8 @@ FactoryBot.define do
       end
     end
 
-    # Use this trait if you want your designs to be as true-to-life as possible,
-    # with correctly made commits in the repository and files that can be retrieved.
+    # Use this trait to build designs that have commits in the repository
+    # and files that can be retrieved.
     trait :with_file do
       transient do
         deleted { false }

ee/spec/factories/design_management/versions.rb

@@ -71,6 +71,23 @@ FactoryBot.define do
       version.actions.reset
     end
 
+    # Use this trait to build versions with designs that are backed by Git LFS, committed
+    # to the repository, and with an LfsObject correctly created for it.
+    trait :with_lfs_file do
+      committed
+
+      transient do
+        raw_file { fixture_file_upload('spec/fixtures/dk.png', 'image/png') }
+        lfs_pointer { Gitlab::Git::LfsPointerFile.new(SecureRandom.random_bytes) }
+        file { lfs_pointer.pointer }
+      end
+
+      after :create do |version, evaluator|
+        lfs_object = create(:lfs_object, file: evaluator.raw_file, oid: evaluator.lfs_pointer.sha256, size: evaluator.lfs_pointer.size)
+        create(:lfs_objects_project, project: version.project, lfs_object: lfs_object, repository_type: :design)
+      end
+    end
+
     # This trait is for versions that must be present in the git repository.
     trait :committed do
       transient do

ee/spec/factories/uploads.rb

+# frozen_string_literal: true
+
+FactoryBot.modify do
+  factory :upload do
+    trait :design_action_image_v432x230_upload do
+      mount_point { :image_v432x230 }
+      model { create(:design_action) }
+      uploader { ::DesignManagement::DesignV432x230Uploader.name }
+    end
+  end
+end

ee/spec/models/design_management/version_spec.rb

@@ -56,7 +56,7 @@ describe DesignManagement::Version do
         expect(described_class.earlier_or_equal_to(version_2)).to contain_exactly(version_1, version_2)
       end
 
-      it 'can be passed either a DesignManagement::Design or an ID' do
+      it 'can be passed either a DesignManagement::Version or an ID' do
         [version_1, version_1.id].each do |arg|
           expect(described_class.earlier_or_equal_to(arg)).to eq([version_1])
         end

ee/spec/services/design_management/generate_image_versions_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DesignManagement::GenerateImageVersionsService do
+  let_it_be(:project) { create(:project) }
+  let_it_be(:issue) { create(:issue, project: project) }
+  let_it_be(:version) { create(:design, :with_lfs_file, issue: issue).versions.first }
+  let_it_be(:action) { version.actions.first }
+
+  describe '#execute' do
+    it 'generates the image' do
+      expect { described_class.new(version).execute }
+        .to change { action.reload.image_v432x230.file }
+        .from(nil).to(CarrierWave::SanitizedFile)
+    end
+
+    it 'skips generating image versions if the design file size is too large' do
+      stub_const("#{described_class.name}::MAX_DESIGN_SIZE", 1.byte)
+
+      described_class.new(version).execute
+
+      expect(action.reload.image_v432x230.file).to eq(nil)
+    end
+
+    it 'returns the status' do
+      result = described_class.new(version).execute
+
+      expect(result[:status]).to eq(:success)
+    end
+
+    it 'returns the version' do
+      result = described_class.new(version).execute
+
+      expect(result[:version]).to eq(version)
+    end
+
+    it 'logs if the raw image cannot be found' do
+      version.designs.first.update(filename: 'foo.png')
+
+      expect(Gitlab::AppLogger).to receive(:error).with("No design file found for Action: #{action.id}")
+
+      described_class.new(version).execute
+    end
+
+    context 'when an error is encountered when generating the image versions' do
+      before do
+        expect_next_instance_of(DesignManagement::DesignV432x230Uploader) do |uploader|
+          expect(uploader).to receive(:cache!).and_raise(CarrierWave::DownloadError, 'foo')
+        end
+      end
+
+      it 'logs the error' do
+        expect(Gitlab::AppLogger).to receive(:error).with('foo')
+
+        described_class.new(version).execute
+      end
+
+      it 'tracks the error' do
+        expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
+          instance_of(CarrierWave::DownloadError),
+          project_id: project.id, version_id: version.id, design_id: version.designs.first.id
+        )
+
+        described_class.new(version).execute
+      end
+    end
+  end
+end

ee/spec/tasks/gitlab/uploads/migrate_rake_spec.rb

+# frozen_string_literal: true
+
+require 'rake_helper'
+
+describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
+  let(:batch_size) { 3 }
+
+  before do
+    stub_env('MIGRATION_BATCH_SIZE', batch_size.to_s)
+    stub_uploads_object_storage(uploader_class)
+    Rake.application.rake_require 'tasks/gitlab/uploads/migrate'
+
+    allow(ObjectStorage::MigrateUploadsWorker).to receive(:perform_async)
+  end
+
+  context "for DesignManagement::DesignV432x230Uploader" do
+    let(:uploader_class) { DesignManagement::DesignV432x230Uploader }
+    let(:model_class) {  DesignManagement::Action }
+    let(:mounted_as) { :image_v432x230 }
+
+    before do
+      create_list(:design_action, 10, :with_image_v432x230)
+    end
+
+    it_behaves_like 'enqueue upload migration jobs in batch', batch: 4
+  end
+end

ee/spec/uploaders/design_management/design_v432x230_uploader_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe DesignManagement::DesignV432x230Uploader do
+  include CarrierWave::Test::Matchers
+
+  let(:model) { create(:design_action, :with_image_v432x230) }
+  let(:upload) { create(:upload, :design_action_image_v432x230_upload, model: model) }
+
+  subject(:uploader) { described_class.new(model, :image_v432x230) }
+
+  it_behaves_like 'builds correct paths',
+                  store_dir: %r[uploads/-/system/design_management/action/image_v432x230/],
+                  upload_path: %r[uploads/-/system/design_management/action/image_v432x230/],
+                  relative_path: %r[uploads/-/system/design_management/action/image_v432x230/],
+                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/design_management/action/image_v432x230/]
+
+  context 'object_store is REMOTE' do
+    before do
+      stub_uploads_object_storage
+    end
+
+    include_context 'with storage', described_class::Store::REMOTE
+
+    it_behaves_like 'builds correct paths',
+                    store_dir: %r[design_management/action/image_v432x230/],
+                    upload_path: %r[design_management/action/image_v432x230/],
+                    relative_path: %r[design_management/action/image_v432x230/]
+  end
+
+  describe "#migrate!" do
+    before do
+      uploader.store!(fixture_file_upload('spec/fixtures/dk.png'))
+      stub_uploads_object_storage
+    end
+
+    it_behaves_like 'migrates', to_store: described_class::Store::REMOTE
+    it_behaves_like 'migrates', from_store: described_class::Store::REMOTE, to_store: described_class::Store::LOCAL
+  end
+
+  it 'resizes images', :aggregate_failures do
+    image_loader = CarrierWave::Test::Matchers::ImageLoader
+    original_file = fixture_file_upload('spec/fixtures/dk.png')
+    uploader.store!(original_file)
+
+    expect(
+      image_loader.load_image(original_file.tempfile.path)
+    ).to have_attributes(
+      width: 460,
+      height: 322
+    )
+    expect(
+      image_loader.load_image(uploader.file.file)
+    ).to have_attributes(
+      width: 329,
+      height: 230
+    )
+  end
+
+  context 'uploading a whitelisted file extension' do
+    it 'stores the image successfully' do
+      fixture_file = fixture_file_upload('spec/fixtures/dk.png')
+
+      expect { uploader.cache!(fixture_file) }.to change { uploader.file }.from(nil).to(kind_of(CarrierWave::SanitizedFile))
+    end
+  end
+
+  context 'uploading a non-whitelisted file' do
+    it 'will deny the upload' do
+      fixture_file = fixture_file_upload('spec/fixtures/logo_sample.svg', 'image/svg+xml')
+
+      expect { uploader.cache!(fixture_file) }.to raise_exception(
+        CarrierWave::IntegrityError,
+        'You are not allowed to upload image/svg+xml files'
+      )
+    end
+  end
+end

ee/spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ObjectStorage::MigrateUploadsWorker do
+  let(:model_class) { Project }
+  let(:uploads) { Upload.all }
+  let(:to_store) { ObjectStorage::Store::REMOTE }
+
+  context 'for DesignManagement::DesignV432x230Uploader' do
+    let(:model_class) { DesignManagement::Action }
+    let!(:design_actions) { create_list(:design_action, 10, :with_image_v432x230) }
+    let(:mounted_as) { :image_v432x230 }
+
+    before do
+      stub_uploads_object_storage(DesignManagement::DesignV432x230Uploader)
+    end
+
+    it_behaves_like 'uploads migration worker'
+  end
+end

ee/spec/workers/design_management/new_version_worker_spec.rb

@@ -6,6 +6,10 @@ describe DesignManagement::NewVersionWorker do
   describe '#perform' do
     let(:worker) { described_class.new }
 
+    before do
+      stub_feature_flags(design_management_resize_images: true)
+    end
+
     context 'the id is wrong or out-of-date' do
       let(:version_id) { -1 }
 
@@ -15,6 +19,12 @@ describe DesignManagement::NewVersionWorker do
         worker.perform(version_id)
       end
 
+      it 'does not invoke GenerateImageVersionsService' do
+        expect(DesignManagement::GenerateImageVersionsService).not_to receive(:new)
+
+        worker.perform(version_id)
+      end
+
       it 'logs the reason for this failure' do
         expect(Sidekiq.logger).to receive(:warn)
           .with(an_instance_of(ActiveRecord::RecordNotFound))
@@ -24,23 +34,43 @@ describe DesignManagement::NewVersionWorker do
     end
 
     context 'the version id is valid' do
-      let_it_be(:version) { create(:design_version, :committed, designs_count: 2) }
+      let_it_be(:version) { create(:design_version, :with_lfs_file, designs_count: 2) }
 
       it 'creates a system note' do
         expect { worker.perform(version.id) }.to change { Note.system.count }.by(1)
       end
 
+      it 'invokes GenerateImageVersionsService' do
+        expect_next_instance_of(DesignManagement::GenerateImageVersionsService) do |service|
+          expect(service).to receive(:execute)
+        end
+
+        worker.perform(version.id)
+      end
+
       it 'does not log anything' do
         expect(Sidekiq.logger).not_to receive(:warn)
 
         worker.perform(version.id)
       end
+
+      context 'when the `design_management_resize_images` flag is disabled' do
+        before do
+          stub_feature_flags(design_management_resize_images: false)
+        end
+
+        it 'does not invoke GenerateImageVersionsService' do
+          expect(DesignManagement::GenerateImageVersionsService).not_to receive(:new)
+
+          worker.perform(version.id)
+        end
+      end
     end
 
     context 'the version includes multiple types of action' do
       let_it_be(:version) do
-        create(:design_version, :committed,
-               created_designs: create_list(:design, 1),
+        create(:design_version, :with_lfs_file,
+               created_designs: create_list(:design, 1, :with_lfs_file),
                modified_designs: create_list(:design, 1))
       end
 

lib/gitlab/file_type_detection.rb

 # frozen_string_literal: true
 
-# The method `filename` must be defined in classes that use this module.
+# The method `filename` must be defined in classes that mix in this module.
 #
 # This module is intended to be used as a helper and not a security gate
 # to validate that a file is safe, as it identifies files only by the
@@ -35,6 +35,13 @@ module Gitlab
     DANGEROUS_VIDEO_EXT = [].freeze # None, yet
     DANGEROUS_AUDIO_EXT = [].freeze # None, yet
 
+    def self.extension_match?(filename, extensions)
+      return false unless filename.present?
+
+      extension = File.extname(filename).delete('.')
+      extensions.include?(extension.downcase)
+    end
+
     def image?
       extension_match?(SAFE_IMAGE_EXT)
     end
@@ -74,10 +81,7 @@ module Gitlab
     private
 
     def extension_match?(extensions)
-      return false unless filename
-
-      extension = File.extname(filename).delete('.')
-      extensions.include?(extension.downcase)
+      ::Gitlab::FileTypeDetection.extension_match?(filename, extensions)
     end
   end
 end

lib/gitlab/import_export/import_export.yml

@@ -377,3 +377,6 @@ ee:
       - protected_environments:
         - :deploy_access_levels
       - :service_desk_setting
+  excluded_attributes:
+    actions:
+      - image_v432x230

lib/gitlab/uploads/migration_helper.rb

@@ -21,6 +21,10 @@ module Gitlab
         prepare_variables(args, logger)
       end
 
+      def self.categories
+        CATEGORIES
+      end
+
       def migrate_to_remote_storage
         @to_store = ObjectStorage::Store::REMOTE
 
@@ -70,3 +74,5 @@ module Gitlab
     end
   end
 end
+
+Gitlab::Uploads::MigrationHelper.prepend_if_ee('EE::Gitlab::Uploads::MigrationHelper')

lib/tasks/gitlab/uploads/migrate.rake

@@ -3,7 +3,7 @@ namespace :gitlab do
     namespace :migrate do
       desc "GitLab | Uploads | Migrate all uploaded files to object storage"
       task all: :environment do
-        Gitlab::Uploads::MigrationHelper::CATEGORIES.each do |args|
+        Gitlab::Uploads::MigrationHelper.categories.each do |args|
           Rake::Task["gitlab:uploads:migrate"].invoke(*args)
           Rake::Task["gitlab:uploads:migrate"].reenable
         end
@@ -20,7 +20,7 @@ namespace :gitlab do
     namespace :migrate_to_local do
       desc "GitLab | Uploads | Migrate all uploaded files to local storage"
       task all: :environment do
-        Gitlab::Uploads::MigrationHelper::CATEGORIES.each do |args|
+        Gitlab::Uploads::MigrationHelper.categories.each do |args|
           Rake::Task["gitlab:uploads:migrate_to_local"].invoke(*args)
           Rake::Task["gitlab:uploads:migrate_to_local"].reenable
         end

spec/lib/gitlab/file_type_detection_spec.rb

@@ -2,6 +2,35 @@
 require 'spec_helper'
 
 describe Gitlab::FileTypeDetection do
+  describe '.extension_match?' do
+    let(:extensions) { %w[foo bar] }
+
+    it 'returns false when filename is blank' do
+      expect(described_class.extension_match?(nil, extensions)).to eq(false)
+      expect(described_class.extension_match?('', extensions)).to eq(false)
+    end
+
+    it 'returns true when filename matches extensions' do
+      expect(described_class.extension_match?('file.foo', extensions)).to eq(true)
+      expect(described_class.extension_match?('file.bar', extensions)).to eq(true)
+    end
+
+    it 'returns false when filename does not match extensions' do
+      expect(described_class.extension_match?('file.baz', extensions)).to eq(false)
+    end
+
+    it 'can match case insensitive filenames' do
+      expect(described_class.extension_match?('file.FOO', extensions)).to eq(true)
+    end
+
+    it 'can match filenames with periods' do
+      expect(described_class.extension_match?('my.file.foo', extensions)).to eq(true)
+    end
+
+    it 'can match filenames with directories' do
+      expect(described_class.extension_match?('my/file.foo', extensions)).to eq(true)
+    end
+  end
   context 'when class is an uploader' do
     let(:uploader) do
       example_uploader = Class.new(CarrierWave::Uploader::Base) do

spec/lib/gitlab/import_export/all_models.yml

@@ -567,6 +567,8 @@ designs: *design
 actions:
 - design
 - version
+- uploads
+- file_uploads
 versions: &version
 - author
 - issue

spec/lib/gitlab/import_export/safe_model_attributes.yml

@@ -769,7 +769,9 @@ DesignManagement::Design:
 - project_id
 - filename
 DesignManagement::Action:
+- id
 - event
+- image_v432x230
 DesignManagement::Version:
 - id
 - created_at

spec/models/lfs_object_spec.rb

@@ -14,7 +14,7 @@ describe LfsObject do
       end
     end
 
-    describe 'for_oids' do
+    describe '.for_oids' do
       it 'returns the correct LfsObjects' do
         lfs_object_1, lfs_object_2 = create_list(:lfs_object, 2)
 

spec/support/shared_examples/tasks/gitlab/uploads/migration_shared_examples.rb

+# frozen_string_literal: true
+
+# Expects the calling spec to define:
+# - uploader_class
+# - model_class
+# - mounted_as
+RSpec.shared_examples 'enqueue upload migration jobs in batch' do |batch:|
+  def run(task)
+    args = [uploader_class.to_s, model_class.to_s, mounted_as].compact
+    run_rake_task(task, *args)
+  end
+
+  it 'migrates local storage to remote object storage' do
+    expect(ObjectStorage::MigrateUploadsWorker)
+      .to receive(:perform_async).exactly(batch).times
+      .and_return("A fake job.")
+
+    run('gitlab:uploads:migrate')
+  end
+
+  it 'migrates remote object storage to local storage' do
+    expect(Upload).to receive(:where).exactly(batch + 1).times { Upload.all }
+    expect(ObjectStorage::MigrateUploadsWorker)
+      .to receive(:perform_async)
+      .with(anything, model_class.name, mounted_as, ObjectStorage::Store::LOCAL)
+      .exactly(batch).times
+      .and_return("A fake job.")
+
+    run('gitlab:uploads:migrate_to_local')
+  end
+end

spec/support/shared_examples/uploaders/workers/object_storage/migrate_uploads_shared_examples.rb

+# frozen_string_literal: true
+
+# Expects the calling spec to define:
+# - model_class
+# - mounted_as
+# - to_store
+RSpec.shared_examples 'uploads migration worker' do
+  def perform(uploads, store = nil)
+    described_class.new.perform(uploads.ids, model_class.to_s, mounted_as, store || to_store)
+  rescue ObjectStorage::MigrateUploadsWorker::Report::MigrationFailures
+    # swallow
+  end
+
+  describe '.enqueue!' do
+    def enqueue!
+      described_class.enqueue!(uploads, model_class, mounted_as, to_store)
+    end
+
+    it 'is guarded by .sanity_check!' do
+      expect(described_class).to receive(:perform_async)
+      expect(described_class).to receive(:sanity_check!)
+
+      enqueue!
+    end
+
+    context 'sanity_check! fails' do
+      include_context 'sanity_check! fails'
+
+      it 'does not enqueue a job' do
+        expect(described_class).not_to receive(:perform_async)
+
+        expect { enqueue! }.to raise_error(described_class::SanityCheckError)
+      end
+    end
+  end
+
+  describe '.sanity_check!' do
+    shared_examples 'raises a SanityCheckError' do |expected_message|
+      let(:mount_point) { nil }
+
+      it do
+        expect { described_class.sanity_check!(uploads, model_class, mount_point) }
+          .to raise_error(described_class::SanityCheckError).with_message(expected_message)
+      end
+    end
+
+    context 'uploader types mismatch' do
+      let!(:outlier) { create(:upload, uploader: 'GitlabUploader') }
+
+      include_examples 'raises a SanityCheckError', /Multiple uploaders found/
+    end
+
+    context 'mount point not found' do
+      include_examples 'raises a SanityCheckError', /Mount point [a-z:]+ not found in/ do
+        let(:mount_point) { :potato }
+      end
+    end
+  end
+
+  describe '#perform' do
+    shared_examples 'outputs correctly' do |success: 0, failures: 0|
+      total = success + failures
+
+      if success > 0
+        it 'outputs the reports' do
+          expect(Rails.logger).to receive(:info).with(%r{Migrated #{success}/#{total} files})
+
+          perform(uploads)
+        end
+      end
+
+      if failures > 0
+        it 'outputs upload failures' do
+          expect(Rails.logger).to receive(:warn).with(/Error .* I am a teapot/)
+
+          perform(uploads)
+        end
+      end
+    end
+
+    it_behaves_like 'outputs correctly', success: 10
+
+    it 'migrates files to remote storage' do
+      perform(uploads)
+
+      expect(Upload.where(store: ObjectStorage::Store::LOCAL).count).to eq(0)
+    end
+
+    context 'reversed' do
+      let(:to_store) { ObjectStorage::Store::LOCAL }
+
+      before do
+        perform(uploads, ObjectStorage::Store::REMOTE)
+      end
+
+      it 'migrates files to local storage' do
+        expect(Upload.where(store: ObjectStorage::Store::REMOTE).count).to eq(10)
+
+        perform(uploads)
+
+        expect(Upload.where(store: ObjectStorage::Store::LOCAL).count).to eq(10)
+      end
+    end
+
+    context 'migration is unsuccessful' do
+      before do
+        allow_any_instance_of(ObjectStorage::Concern)
+          .to receive(:migrate!).and_raise(CarrierWave::UploadError, 'I am a teapot.')
+      end
+
+      it_behaves_like 'outputs correctly', failures: 10
+    end
+  end
+end
+
+RSpec.shared_context 'sanity_check! fails' do
+  before do
+    expect(described_class).to receive(:sanity_check!).and_raise(described_class::SanityCheckError)
+  end
+end

spec/tasks/gitlab/uploads/migrate_rake_spec.rb

@@ -16,32 +16,6 @@ describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
     allow(ObjectStorage::MigrateUploadsWorker).to receive(:perform_async)
   end
 
-  def run(task)
-    args = [uploader_class.to_s, model_class.to_s, mounted_as].compact
-    run_rake_task(task, *args)
-  end
-
-  shared_examples 'enqueue jobs in batch' do |batch:|
-    it 'migrates local storage to remote object storage' do
-      expect(ObjectStorage::MigrateUploadsWorker)
-        .to receive(:perform_async).exactly(batch).times
-        .and_return("A fake job.")
-
-      run('gitlab:uploads:migrate')
-    end
-
-    it 'migrates remote object storage to local storage' do
-      expect(Upload).to receive(:where).exactly(batch + 1).times { Upload.all }
-      expect(ObjectStorage::MigrateUploadsWorker)
-        .to receive(:perform_async)
-        .with(anything, model_class.name, mounted_as, ObjectStorage::Store::LOCAL)
-        .exactly(batch).times
-        .and_return("A fake job.")
-
-      run('gitlab:uploads:migrate_to_local')
-    end
-  end
-
   context "for AvatarUploader" do
     let(:uploader_class) { AvatarUploader }
     let(:mounted_as) { :avatar }
@@ -50,7 +24,7 @@ describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
       let(:model_class) { Project }
       let!(:projects) { create_list(:project, 10, :with_avatar) }
 
-      it_behaves_like 'enqueue jobs in batch', batch: 4
+      it_behaves_like 'enqueue upload migration jobs in batch', batch: 4
     end
 
     context "for Group" do
@@ -60,7 +34,7 @@ describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
         create_list(:group, 10, :with_avatar)
       end
 
-      it_behaves_like 'enqueue jobs in batch', batch: 4
+      it_behaves_like 'enqueue upload migration jobs in batch', batch: 4
     end
 
     context "for User" do
@@ -70,7 +44,7 @@ describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
         create_list(:user, 10, :with_avatar)
       end
 
-      it_behaves_like 'enqueue jobs in batch', batch: 4
+      it_behaves_like 'enqueue upload migration jobs in batch', batch: 4
     end
   end
 
@@ -85,7 +59,7 @@ describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
         create_list(:note, 10, :with_attachment)
       end
 
-      it_behaves_like 'enqueue jobs in batch', batch: 4
+      it_behaves_like 'enqueue upload migration jobs in batch', batch: 4
     end
 
     context "for Appearance" do
@@ -97,7 +71,7 @@ describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
       end
 
       %i(logo header_logo).each do |mount|
-        it_behaves_like 'enqueue jobs in batch', batch: 1 do
+        it_behaves_like 'enqueue upload migration jobs in batch', batch: 1 do
           let(:mounted_as) { mount }
         end
       end
@@ -115,7 +89,7 @@ describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
       end
     end
 
-    it_behaves_like 'enqueue jobs in batch', batch: 4
+    it_behaves_like 'enqueue upload migration jobs in batch', batch: 4
   end
 
   context "for PersonalFileUploader" do
@@ -129,7 +103,7 @@ describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
       end
     end
 
-    it_behaves_like 'enqueue jobs in batch', batch: 4
+    it_behaves_like 'enqueue upload migration jobs in batch', batch: 4
   end
 
   context "for NamespaceFileUploader" do
@@ -143,6 +117,6 @@ describe 'gitlab:uploads:migrate and migrate_to_local rake tasks' do
       end
     end
 
-    it_behaves_like 'enqueue jobs in batch', batch: 4
+    it_behaves_like 'enqueue upload migration jobs in batch', batch: 4
   end
 end

spec/uploaders/workers/object_storage/migrate_uploads_worker_spec.rb

@@ -3,12 +3,6 @@
 require 'spec_helper'
 
 describe ObjectStorage::MigrateUploadsWorker do
-  shared_context 'sanity_check! fails' do
-    before do
-      expect(described_class).to receive(:sanity_check!).and_raise(described_class::SanityCheckError)
-    end
-  end
-
   let(:model_class) { Project }
   let(:uploads) { Upload.all }
   let(:to_store) { ObjectStorage::Store::REMOTE }
@@ -19,109 +13,6 @@ describe ObjectStorage::MigrateUploadsWorker do
     # swallow
   end
 
-  shared_examples "uploads migration worker" do
-    describe '.enqueue!' do
-      def enqueue!
-        described_class.enqueue!(uploads, Project, mounted_as, to_store)
-      end
-
-      it 'is guarded by .sanity_check!' do
-        expect(described_class).to receive(:perform_async)
-        expect(described_class).to receive(:sanity_check!)
-
-        enqueue!
-      end
-
-      context 'sanity_check! fails' do
-        include_context 'sanity_check! fails'
-
-        it 'does not enqueue a job' do
-          expect(described_class).not_to receive(:perform_async)
-
-          expect { enqueue! }.to raise_error(described_class::SanityCheckError)
-        end
-      end
-    end
-
-    describe '.sanity_check!' do
-      shared_examples 'raises a SanityCheckError' do |expected_message|
-        let(:mount_point) { nil }
-
-        it do
-          expect { described_class.sanity_check!(uploads, model_class, mount_point) }
-            .to raise_error(described_class::SanityCheckError).with_message(expected_message)
-        end
-      end
-
-      context 'uploader types mismatch' do
-        let!(:outlier) { create(:upload, uploader: 'GitlabUploader') }
-
-        include_examples 'raises a SanityCheckError', /Multiple uploaders found/
-      end
-
-      context 'mount point not found' do
-        include_examples 'raises a SanityCheckError', /Mount point [a-z:]+ not found in/ do
-          let(:mount_point) { :potato }
-        end
-      end
-    end
-
-    describe '#perform' do
-      shared_examples 'outputs correctly' do |success: 0, failures: 0|
-        total = success + failures
-
-        if success > 0
-          it 'outputs the reports' do
-            expect(Rails.logger).to receive(:info).with(%r{Migrated #{success}/#{total} files})
-
-            perform(uploads)
-          end
-        end
-
-        if failures > 0
-          it 'outputs upload failures' do
-            expect(Rails.logger).to receive(:warn).with(/Error .* I am a teapot/)
-
-            perform(uploads)
-          end
-        end
-      end
-
-      it_behaves_like 'outputs correctly', success: 10
-
-      it 'migrates files to remote storage' do
-        perform(uploads)
-
-        expect(Upload.where(store: ObjectStorage::Store::LOCAL).count).to eq(0)
-      end
-
-      context 'reversed' do
-        let(:to_store) { ObjectStorage::Store::LOCAL }
-
-        before do
-          perform(uploads, ObjectStorage::Store::REMOTE)
-        end
-
-        it 'migrates files to local storage' do
-          expect(Upload.where(store: ObjectStorage::Store::REMOTE).count).to eq(10)
-
-          perform(uploads)
-
-          expect(Upload.where(store: ObjectStorage::Store::LOCAL).count).to eq(10)
-        end
-      end
-
-      context 'migration is unsuccessful' do
-        before do
-          allow_any_instance_of(ObjectStorage::Concern)
-            .to receive(:migrate!).and_raise(CarrierWave::UploadError, "I am a teapot.")
-        end
-
-        it_behaves_like 'outputs correctly', failures: 10
-      end
-    end
-  end
-
   context "for AvatarUploader" do
     let!(:projects) { create_list(:project, 10, :with_avatar) }
     let(:mounted_as) { :avatar }