Merge branch '209025-design-filename-limit-migrations' into 'master'

Add migration to limit Design filename

See merge request gitlab-org/gitlab!33565

changelogs/unreleased/209025-design-filename-limit-migrations.yml

+---
+title: Add database migrations to design_management_designs.filename to enforce
+  a 255 character limit, and modify any filenames that exceed that limit
+merge_request: 33565
+author:
+type: changed

db/post_migrate/20200602013900_add_limit_to_designs_filename.rb

+# frozen_string_literal: true
+
+class AddLimitToDesignsFilename < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_text_limit(:design_management_designs, :filename, 255, validate: false)
+  end
+
+  def down
+    remove_text_limit(:design_management_designs, :filename)
+  end
+end

db/post_migrate/20200602013901_cap_designs_filename_length_to_new_limit.rb

+# frozen_string_literal: true
+
+class CapDesignsFilenameLengthToNewLimit < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  CHAR_LENGTH = 255
+  MODIFIED_NAME = 'gitlab-modified-'
+  MODIFIED_EXTENSION = '.jpg'
+
+  def up
+    arel_table = Arel::Table.new(:design_management_designs)
+
+    # Design filenames larger than the limit will be renamed to "gitlab-modified-{id}.jpg",
+    # which will be valid and unique. The design file itself will appear broken, as it is
+    # understood that no designs with filenames that exceed this limit will be legitimate.
+    # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/33565/diffs#note_355789080
+    new_value_clause = Arel::Nodes::NamedFunction.new(
+      'CONCAT',
+      [
+        Arel::Nodes.build_quoted(MODIFIED_NAME),
+        arel_table[:id],
+        Arel::Nodes.build_quoted(MODIFIED_EXTENSION)
+      ]
+    )
+    where_clause = Arel::Nodes::NamedFunction.new(
+      'CHAR_LENGTH',
+      [arel_table[:filename]]
+    ).gt(CHAR_LENGTH)
+
+    update_arel = Arel::UpdateManager.new.table(arel_table)
+                                         .set([[arel_table[:filename], new_value_clause]])
+                                         .where(where_clause)
+
+    ActiveRecord::Base.connection.execute(update_arel.to_sql)
+  end
+
+  def down
+    # no-op : the original filename is lost forever
+  end
+end

db/structure.sql

@@ -8121,6 +8121,9 @@ ALTER TABLE ONLY public.chat_names
 ALTER TABLE ONLY public.chat_teams
     ADD CONSTRAINT chat_teams_pkey PRIMARY KEY (id);
 
+ALTER TABLE public.design_management_designs
+    ADD CONSTRAINT check_07155e2715 CHECK ((char_length((filename)::text) <= 255)) NOT VALID;
+
 ALTER TABLE public.ci_job_artifacts
     ADD CONSTRAINT check_27f0f6dbab CHECK ((file_store IS NOT NULL)) NOT VALID;
 
@@ -13839,6 +13842,8 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200528125905
 20200528171933
 20200601210148
+20200602013900
+20200602013901
 20200603073101
 20200604143628
 \.

spec/migrations/cap_designs_filename_length_to_new_limit_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200602013901_cap_designs_filename_length_to_new_limit')
+
+describe CapDesignsFilenameLengthToNewLimit, :migration, schema: 20200528125905 do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:issues) { table(:issues) }
+  let(:designs) { table(:design_management_designs) }
+
+  let(:filename_below_limit) { generate_filename(254) }
+  let(:filename_at_limit) { generate_filename(255) }
+  let(:filename_above_limit) { generate_filename(256) }
+
+  let!(:namespace) { namespaces.create!(name: 'foo', path: 'foo') }
+  let!(:project) { projects.create!(name: 'gitlab', path: 'gitlab-org/gitlab', namespace_id: namespace.id) }
+  let!(:issue) { issues.create!(description: 'issue', project_id: project.id) }
+
+  def generate_filename(length, extension: '.png')
+    name = 'a' * (length - extension.length)
+
+    "#{name}#{extension}"
+  end
+
+  def create_design(filename)
+    designs.create!(
+      issue_id: issue.id,
+      project_id: project.id,
+      filename: filename
+    )
+  end
+
+  it 'correctly sets filenames that are above the limit' do
+    [
+      filename_below_limit,
+      filename_at_limit,
+      filename_above_limit
+    ].each(&method(:create_design))
+
+    migrate!
+
+    expect(designs.find(1).filename).to eq(filename_below_limit)
+    expect(designs.find(2).filename).to eq(filename_at_limit)
+    expect(designs.find(3).filename).to eq([described_class::MODIFIED_NAME, 3, described_class::MODIFIED_EXTENSION].join)
+  end
+
+  it 'runs after filename limit has been set' do
+    # This spec file uses the `schema:` keyword to run these tests
+    # against a schema version before the one that sets the limit,
+    # as otherwise we can't create the design data with filenames greater
+    # than the limit.
+    #
+    # For this test, we migrate any skipped versions up to this migration.
+    migration_context.migrate(20200602013901)
+
+    create_design(filename_at_limit)
+    expect { create_design(filename_above_limit) }.to raise_error(ActiveRecord::StatementInvalid)
+  end
+end