FIX: Ensure chat channel slug uniqueness at DB level (#19277)

There must have been a small loophole that allowed
setting the channel slug in the DB which has led to
conflicts in some cases.

This commit fixes the conflicting chat channel
slugs and then changes the channel slug index
to a unique one in the DB.

plugins/chat/app/models/chat_channel.rb

@@ -144,6 +144,6 @@ end
 #
 #  index_chat_channels_on_chatable_id                    (chatable_id)
 #  index_chat_channels_on_chatable_id_and_chatable_type  (chatable_id,chatable_type)
-#  index_chat_channels_on_slug                           (slug)
+#  index_chat_channels_on_slug                           (slug) UNIQUE
 #  index_chat_channels_on_status                         (status)
 #

plugins/chat/db/migrate/20221201024458_make_channel_slugs_unique_with_index.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+
+class MakeChannelSlugsUniqueWithIndex < ActiveRecord::Migration[7.0]
+  def up
+    DB.exec("CREATE TEMPORARY TABLE tmp_chat_channel_slugs_conflicts(id int, slug text)")
+
+    channels_with_conflicting_slugs = DB.query(<<~SQL)
+      SELECT chat_channels.id, subquery.slug
+      FROM (
+        SELECT slug, count(*)
+        FROM chat_channels
+        GROUP BY slug
+        HAVING count(*) > 1
+      ) subquery
+      INNER JOIN chat_channels ON chat_channels.slug = subquery.slug
+      ORDER BY slug, created_at ASC
+    SQL
+
+    current_slug = nil
+    slugs_to_update = []
+    channels_with_conflicting_slugs.each do |channel|
+      if current_slug != channel.slug
+        current_slug = channel.slug
+
+        # Continue since we want to keep the slug for the first
+        # matching channel and just update subsequent channels.
+        next
+      end
+
+      # Deduplicate slugs with the channel IDs, we can always improve
+      # slugs later on.
+      slugs_to_update << [channel.id, "#{channel.slug}-#{channel.id}"]
+    end
+
+    values_to_insert =
+      slugs_to_update.map do |channel_pair|
+        "(#{channel_pair[0]}, '#{PG::Connection.escape_string(channel_pair[1])}')"
+      end
+
+    if values_to_insert.any?
+      DB.exec(
+        "INSERT INTO tmp_chat_channel_slugs_conflicts
+        VALUES #{values_to_insert.join(",\n")}",
+      )
+
+      DB.exec(<<~SQL)
+        UPDATE chat_channels cc
+        SET slug = tmp.slug
+        FROM tmp_chat_channel_slugs_conflicts tmp
+        WHERE cc.id = tmp.id
+      SQL
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/chat/db/migrate/20221201035918_add_slug_unique_index_for_chat_channels.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class AddSlugUniqueIndexForChatChannels < ActiveRecord::Migration[7.0]
+  def change
+    remove_index :chat_channels, :slug
+    add_index :chat_channels, :slug, unique: true
+  end
+end

plugins/chat/db/post_migrate/20221201032830_drop_tmp_chat_slug_tables.rb

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class DropTmpChatSlugTables < ActiveRecord::Migration[7.0]
+  def up
+    DB.exec("DROP TABLE IF EXISTS tmp_chat_channel_slugs")
+    DB.exec("DROP TABLE IF EXISTS tmp_chat_channel_slugs_conflicts")
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/chat/spec/models/category_channel_spec.rb

@@ -88,9 +88,7 @@ RSpec.describe CategoryChannel do
     subject(:channel) { Fabricate(:category_channel) }
 
     context "when slug is not provided" do
-      before do
-        channel.slug = nil
-      end
+      before { channel.slug = nil }
 
       it "uses channel name when present" do
         channel.name = "Some Cool Stuff"
@@ -129,12 +127,20 @@ RSpec.describe CategoryChannel do
       end
 
       context "when there is a duplicate slug" do
-        before { Fabricate(:category_channel, slug: "awesome-channel") }
+        fab!(:awesome_channel) { Fabricate(:category_channel, slug: "awesome-channel") }
 
         it "adds a validation error" do
           channel.slug = "awesome-channel"
           channel.validate
-          expect(channel.errors.full_messages.first).to include(I18n.t("chat.category_channel.errors.is_already_in_use"))
+          expect(channel.errors.full_messages.first).to include(
+            I18n.t("chat.category_channel.errors.is_already_in_use"),
+          )
+        end
+
+        it "does not allow setting the slug conflict directly in SQL" do
+          expect {
+            DB.exec("UPDATE chat_channels SET slug = 'awesome-channel' WHERE id = #{channel.id}")
+          }.to raise_error(PG::UniqueViolation)
         end
       end
 
@@ -144,7 +150,9 @@ RSpec.describe CategoryChannel do
         it "fails if slug contains non-ascii characters" do
           channel.slug = "sem-acentuação"
           channel.validate
-          expect(channel.errors.full_messages.first).to match(/#{I18n.t("chat.category_channel.errors.slug_contains_non_ascii_chars")}/)
+          expect(channel.errors.full_messages.first).to match(
+            /#{I18n.t("chat.category_channel.errors.slug_contains_non_ascii_chars")}/,
+          )
         end
       end
     end