Merge branch '24080-prevent-unmodified-Designs-from-being-versioned' into 'master'

Prevent unmodified designs from being versioned

See merge request gitlab-org/gitlab!16429

ee/app/graphql/mutations/design_management/upload.rb

@@ -15,6 +15,11 @@ module Mutations
             null: false,
             description: "The designs that were uploaded by the mutation"
 
+      field :skipped_designs, [Types::DesignManagement::DesignType],
+            null: false,
+            description: "Any designs that were skipped from the upload due to there " \
+                         "being no change to their content since their last version"
+
       def resolve(project_path:, iid:, files:)
         issue = authorized_find!(project_path: project_path, iid: iid)
         project = issue.project
@@ -24,6 +29,7 @@ module Mutations
 
         {
           designs: Array.wrap(result[:designs]),
+          skipped_designs: Array.wrap(result[:skipped_designs]),
           errors: Array.wrap(result[:message])
         }
       end

ee/app/services/design_management/runs_design_actions.rb

@@ -2,9 +2,15 @@
 
 module DesignManagement
   module RunsDesignActions
+    NoActions = Class.new(StandardError)
+
     # this concern requires the following methods to be implemented:
     #   current_user, target_branch, repository, commit_message
+    #
+    # @raise [NoActions] if actions are empty
     def run_actions(actions)
+      raise NoActions if actions.empty?
+
       repository.create_if_not_exists
       sha = repository.multi_action(current_user,
                                     branch_name: target_branch,

ee/app/services/design_management/save_designs_service.rb

@@ -17,10 +17,12 @@ module DesignManagement
       return error("Not allowed!") unless can_create_designs?
       return error("Only #{MAX_FILES} files are allowed simultaneously") if files.size > MAX_FILES
 
-      actions = build_actions
-      run_actions(actions)
+      repository.create_if_not_exists
+
+      uploaded_designs = upload_designs!
+      skipped_designs = designs - uploaded_designs
 
-      success({ designs: actions.map(&:design) })
+      success({ designs: uploaded_designs, skipped_designs: skipped_designs })
     rescue ::ActiveRecord::RecordInvalid => e
       error(e.message)
     end
@@ -28,31 +30,44 @@ module DesignManagement
     private
 
     attr_reader :files
-    attr_accessor :paths_in_repo
 
-    def build_actions
-      repository.create_if_not_exists
+    def upload_designs!
+      actions = build_actions
+      run_actions(actions) unless actions.empty?
 
-      designs = files.map do |file|
+      actions.map(&:design)
+    end
+
+    # Returns `Design` instances that correspond with `files`.
+    # New `Design`s will be created where a file name does not match
+    # an existing `Design`
+    def designs
+      @designs ||= files.map do |file|
         collection.find_or_create_design!(filename: file.original_filename)
       end
+    end
 
-      # Needs to be called before any call to build_design_action
-      cache_existence(designs)
-
-      files.zip(designs).map do |(file, design)|
-        build_design_action(file, design)
+    def build_actions
+      files.zip(designs).flat_map do |(file, design)|
+        Array.wrap(build_design_action(file, design))
       end
     end
 
     def build_design_action(file, design)
-      action = new_file?(design) ? :create : :update
       content = file_content(file, design.full_path)
+      return if design_unchanged?(design, content)
+
+      action = new_file?(design) ? :create : :update
       on_success { ::Gitlab::UsageCounters::DesignsCounter.count(action) }
 
       DesignManagement::DesignAction.new(design, action, content)
     end
 
+    # Returns true if the design file is the same as its latest version
+    def design_unchanged?(design, content)
+      content == existing_blobs[design]&.data
+    end
+
     def commit_message
       <<~MSG
       Updated #{files.size} #{'designs'.pluralize(files.size)}
@@ -74,11 +89,7 @@ module DesignManagement
     end
 
     def new_file?(design)
-      design.new_design? && !on_disk?(design)
-    end
-
-    def on_disk?(design)
-      paths_in_repo === design.full_path
+      !existing_blobs[design]
     end
 
     def file_content(file, full_path)
@@ -86,9 +97,17 @@ module DesignManagement
       transformer.new_file(full_path, file.to_io).content
     end
 
-    def cache_existence(designs)
-      paths = designs.map(&:full_path)
-      self.paths_in_repo = repository.blobs_metadata(paths).map(&:path).to_set
+    # Returns the latest blobs for the designs as a Hash of `{ Design => Blob }`
+    def existing_blobs
+      @existing_blobs ||= begin
+        items = designs.map { |d| ['HEAD', d.full_path] }
+
+        repository.blobs_at(items).each_with_object({}) do |blob, h|
+          design = designs.find { |d| d.full_path == blob.path }
+
+          h[design] = blob
+        end
+      end
     end
   end
 end

ee/spec/requests/api/graphql/mutations/design_management/upload_spec.rb

@@ -9,7 +9,7 @@ describe "uploading designs" do
   let(:current_user) { create(:user) }
   let(:issue) { create(:issue) }
   let(:project) { issue.project }
-  let(:files) { [fixture_file_upload('spec/fixtures/dk.png')] }
+  let(:files) { [fixture_file_upload("spec/fixtures/dk.png")] }
   let(:variables) { {} }
 
   let(:mutation) do
@@ -35,7 +35,7 @@ describe "uploading designs" do
     expect(graphql_errors).to be_present
   end
 
-  it 'succeeds (backward compatibility)' do
+  it "succeeds (backward compatibility)" do
     post_graphql_mutation(mutation, current_user: current_user)
 
     expect(graphql_errors).not_to be_present
@@ -57,8 +57,21 @@ describe "uploading designs" do
     post_graphql_mutation(mutation, current_user: current_user)
 
     expect(mutation_response).to include(
-      'designs' => a_collection_containing_exactly(
-        a_hash_including('filename' => 'dk.png')
+      "designs" => a_collection_containing_exactly(
+        a_hash_including("filename" => "dk.png")
+      )
+    )
+  end
+
+  it "can respond with skipped designs" do
+    2.times do
+      post_graphql_mutation(mutation, current_user: current_user)
+      files.each(&:rewind)
+    end
+
+    expect(mutation_response).to include(
+      "skippedDesigns" => a_collection_containing_exactly(
+        a_hash_including("filename" => "dk.png")
       )
     )
   end

ee/spec/services/design_management/save_designs_service_spec.rb

@@ -21,12 +21,26 @@ describe DesignManagement::SaveDesignsService do
   end
 
   def run_service(files_to_upload = nil)
+    design_files = files_to_upload || files
+    design_files.each(&:rewind)
+
     service = described_class.new(project, user,
                                   issue: issue,
-                                  files: files_to_upload || files)
+                                  files: design_files)
     service.execute
   end
 
+  # Randomly alter the content of files.
+  # This allows the files to be updated by the service, as unmodified
+  # files are rejected.
+  def touch_files(files_to_touch = nil)
+    design_files = files_to_touch || files
+
+    design_files.each do |f|
+      f.tempfile.write(SecureRandom.random_bytes)
+    end
+  end
+
   let(:response) { run_service }
 
   shared_examples 'a service error' do
@@ -126,11 +140,10 @@ describe DesignManagement::SaveDesignsService do
           end
         end
 
-        context 'when a design already exists' do
+        context 'when a design is being updated' do
           before do
-            # This makes sure the file is created in the repository.
-            # otherwise we'd have a database & repository that are not in sync.
             run_service
+            touch_files
           end
 
           it 'creates a new version for the existing design and updates the file' do
@@ -161,12 +174,30 @@ describe DesignManagement::SaveDesignsService do
           end
         end
 
+        context 'when a design has not changed since its previous version' do
+          before do
+            run_service
+          end
+
+          it 'does not create a new version' do
+            expect { run_service }.not_to change { issue.design_versions.count }
+          end
+
+          it 'returns the design in `skipped_designs` instead of `designs`' do
+            response = run_service
+
+            expect(response[:designs]).to be_empty
+            expect(response[:skipped_designs].size).to eq(1)
+          end
+        end
+
         context 'when doing a mixture of updates and creations' do
           let(:files) { [rails_sample, dk_png] }
 
           before do
             # Create just the first one, which we will later update.
             run_service([files.first])
+            touch_files([files.first])
           end
 
           it 'counts one creation and one update' do