Prevent versioning Designs that are unmodified

Our `SaveDesignsService` would previously version and commit designs
files that were not modified since their last version.

These would appear as updates in the version history for the design.

Besides the version history becoming a bit inaccurate and silly looking,
this may create future problems for us. For example, because annotations
use the existing `DiffNote` classes, we get errors when producing the
discussion threads for discussions started on designs that have not
changed in their "diff". There may be other issues lurking if we allow
unmodified files to be committed.

https://gitlab.com/gitlab-org/gitlab-ee/issues/24080

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|
-        collection.find_or_create_design!(filename: file.original_filename)
+      actions.map(&:design)
     end
 
-      # Needs to be called before any call to build_design_action
-      cache_existence(designs)
+    # 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
 
-      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

@@ -63,6 +63,19 @@ describe "uploading designs" do
     )
   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
+
   context "when the issue does not exist" do
     let(:variables) { { iid: "123" } }
 

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