Merge branch '12577-generate-smaller-image-sizes-for-designs' into 'master'

Generate smaller image sizes for Designs

See merge request gitlab-org/gitlab!22860

app/models/lfs_object.rb

@@ -11,6 +11,7 @@ class LfsObject < ApplicationRecord
 
   scope :with_files_stored_locally, -> { where(file_store: LfsObjectUploader::Store::LOCAL) }
   scope :with_files_stored_remotely, -> { where(file_store: LfsObjectUploader::Store::REMOTE) }
+  scope :for_oids, -> (oids) { where(oid: oids) }
 
   validates :oid, presence: true, uniqueness: true
 

app/services/projects/lfs_pointers/lfs_link_service.rb

@@ -25,7 +25,6 @@ module Projects
 
       private
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def link_existing_lfs_objects(oids)
         linked_existing_objects = []
         iterations = 0
@@ -33,7 +32,7 @@ module Projects
         oids.each_slice(BATCH_SIZE) do |oids_batch|
           # Load all existing LFS Objects immediately so we don't issue an extra
           # query for the `.any?`
-          existent_lfs_objects = LfsObject.where(oid: oids_batch).load
+          existent_lfs_objects = LfsObject.for_oids(oids_batch).load
           next unless existent_lfs_objects.any?
 
           rows = existent_lfs_objects
@@ -49,7 +48,6 @@ module Projects
 
         linked_existing_objects
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       def log_lfs_link_results(lfs_objects_linked_count, iterations)
         Gitlab::Import::Logger.info(

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_20_180944) 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/checks/lfs_integrity.rb

@@ -9,7 +9,6 @@ module Gitlab
         @time_left = time_left
       end
 
-      # rubocop: disable CodeReuse/ActiveRecord
       def objects_missing?
         return false unless @newrev && @project.lfs_enabled?
 
@@ -19,12 +18,11 @@ module Gitlab
         return false unless new_lfs_pointers.present?
 
         existing_count = @project.all_lfs_objects
-                                 .where(oid: new_lfs_pointers.map(&:lfs_oid))
+                                 .for_oids(new_lfs_pointers.map(&:lfs_oid))
                                  .count
 
         existing_count != new_lfs_pointers.count
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
   end
 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

@@ -13,6 +13,15 @@ describe LfsObject do
         expect(described_class.not_linked_to_project(project)).to contain_exactly(other_lfs_object)
       end
     end
+
+    describe '.for_oids' do
+      it 'returns the correct LfsObjects' do
+        lfs_object_1, lfs_object_2 = create_list(:lfs_object, 2)
+
+        expect(described_class.for_oids(lfs_object_1.oid)).to contain_exactly(lfs_object_1)
+        expect(described_class.for_oids([lfs_object_1.oid, lfs_object_2.oid])).to contain_exactly(lfs_object_1, lfs_object_2)
+      end
+    end
   end
 
   it 'has a distinct has_many :projects relation through lfs_objects_projects' do

spec/services/projects/lfs_pointers/lfs_link_service_spec.rb

@@ -60,8 +60,8 @@ describe Projects::LfsPointers::LfsLinkService do
       stub_const("#{described_class}::BATCH_SIZE", 1)
       oids = %w(one two)
 
-      expect(LfsObject).to receive(:where).with(oid: %w(one)).once.and_call_original
-      expect(LfsObject).to receive(:where).with(oid: %w(two)).once.and_call_original
+      expect(LfsObject).to receive(:for_oids).with(%w(one)).once.and_call_original
+      expect(LfsObject).to receive(:for_oids).with(%w(two)).once.and_call_original
 
       subject.execute(oids)
     end

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 }