Remove releases with NULL tags and add not-NULL constraint

Update tests to always have release.tag defined

Changelog: changed

app/models/release.rb

@@ -5,6 +5,7 @@ class Release < ApplicationRecord
   include CacheMarkdownField
   include Importable
   include Gitlab::Utils::StrongMemoize
+  include EachBatch
 
   cache_markdown_field :description
 

db/post_migrate/20220222192524_create_not_null_constraint_releases_tag.rb

+# frozen_string_literal: true
+
+class CreateNotNullConstraintReleasesTag < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  def up
+    add_not_null_constraint :releases, :tag, constraint_name: 'releases_not_null_tag', validate: false
+  end
+
+  def down
+    remove_not_null_constraint :releases, :tag, constraint_name: 'releases_not_null_tag'
+  end
+end

db/post_migrate/20220222192525_remove_null_releases.rb

+# frozen_string_literal: true
+
+class RemoveNullReleases < Gitlab::Database::Migration[1.0]
+  disable_ddl_transaction!
+
+  class Release < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'releases'
+  end
+
+  def up
+    Release.all.each_batch(of: 25000) do |rel|
+      rel.where(tag: nil).delete_all
+    end
+  end
+
+  def down
+    # no-op
+    #
+    # releases with the same tag within a project have been removed
+    # and therefore the duplicate release data is no longer available
+  end
+end

db/schema_migrations/20220222192524

+b876119bb369a9831736cddf5326b72a74003ec2e17fe863654cb69497fcf236
\ No newline at end of file

db/schema_migrations/20220222192525

+f512ea4c4a2625c647c3d05765152fee963b56962b674f839180fd77c194ccb0
\ No newline at end of file

db/structure.sql

@@ -25064,6 +25064,9 @@ ALTER TABLE ONLY related_epic_links
 ALTER TABLE ONLY release_links
     ADD CONSTRAINT release_links_pkey PRIMARY KEY (id);
 
+ALTER TABLE releases
+    ADD CONSTRAINT releases_not_null_tag CHECK ((tag IS NOT NULL)) NOT VALID;
+
 ALTER TABLE ONLY releases
     ADD CONSTRAINT releases_pkey PRIMARY KEY (id);
 

spec/finders/releases_finder_spec.rb

@@ -33,18 +33,6 @@ RSpec.describe ReleasesFinder do
     end
   end
 
-  # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27716
-  shared_examples_for 'when tag is nil' do
-    before do
-      v1_0_0.update_column(:tag, nil)
-    end
-
-    it 'ignores rows with a nil tag' do
-      expect(subject.size).to eq(1)
-      expect(subject).to eq([v1_1_0])
-    end
-  end
-
   shared_examples_for 'when a tag parameter is passed' do
     let(:params) { { tag: 'v1.0.0' } }
 
@@ -116,7 +104,6 @@ RSpec.describe ReleasesFinder do
       end
 
       it_behaves_like 'preload'
-      it_behaves_like 'when tag is nil'
       it_behaves_like 'when a tag parameter is passed'
     end
   end

spec/migrations/20220222192524_create_not_null_constraint_releases_tag_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+require_migration!
+
+RSpec.describe CreateNotNullConstraintReleasesTag do
+  let_it_be(:releases) { table(:releases) }
+  let_it_be(:migration) { described_class.new }
+
+  before do
+    allow(migration).to receive(:transaction_open?).and_return(false)
+    allow(migration).to receive(:with_lock_retries).and_yield
+  end
+
+  it 'adds a check constraint to tags' do
+    constraint = releases.connection.check_constraints(:releases).find { |constraint| constraint.expression == "tag IS NOT NULL" }
+    expect(constraint).to be_nil
+
+    migration.up
+
+    constraint = releases.connection.check_constraints(:releases).find { |constraint| constraint.expression == "tag IS NOT NULL" }
+    expect(constraint).to be_a(ActiveRecord::ConnectionAdapters::CheckConstraintDefinition)
+  end
+end

spec/migrations/20220222192525_remove_null_releases_spec.rb

+# frozen_string_literal: true
+require 'spec_helper'
+
+require_migration!
+
+RSpec.describe RemoveNullReleases do
+  let(:releases) { table(:releases) }
+
+  before do
+    # we need to migrate to before previous migration so an invalid record can be created
+    migrate!
+    migration_context.down(previous_migration(3).version)
+
+    releases.create!(tag: 'good', name: 'good release', released_at: Time.now)
+    releases.create!(tag: nil, name: 'bad release', released_at: Time.now)
+  end
+
+  it 'deletes template records and associated data' do
+    expect { migrate! }
+      .to change { releases.count }.from(2).to(1)
+  end
+end

spec/support/helpers/migrations_helpers.rb

@@ -104,9 +104,9 @@ module MigrationsHelpers
     stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
   end
 
-  def previous_migration
-    migrations.each_cons(2) do |previous, migration|
-      break previous if migration.name == described_class.name
+  def previous_migration(steps_back = 2)
+    migrations.each_cons(steps_back) do |cons|
+      break cons.first if cons.last.name == described_class.name
     end
   end