DEV: Use UploadReference instead of ChatUpload in chat (#19947)

We've had the UploadReference table for some time now in core,
but it was added after ChatUpload was and chat was just never
moved over to this new system.

This commit changes all chat code dealing with uploads to create/
update/delete/query UploadReference records instead of ChatUpload
records for consistency. At a later date we will drop the ChatUpload
table, but for now keeping it for data backup.

The migration + post migration are the same, we need both in case
any chat uploads are added/removed during deploy.

app/models/upload.rb

@@ -100,6 +100,10 @@ class Upload < ActiveRecord::Base
     self.url
   end
 
+  def to_markdown
+    UploadMarkdown.new(self).to_markdown
+  end
+
   def thumbnail(width = self.thumbnail_width, height = self.thumbnail_height)
     optimized_images.find_by(width: width, height: height)
   end

app/models/upload_reference.rb

@@ -4,6 +4,8 @@ class UploadReference < ActiveRecord::Base
   belongs_to :upload
   belongs_to :target, polymorphic: true
 
+  delegate :to_markdown, to: :upload
+
   def self.ensure_exist!(upload_ids: [], target: nil, target_type: nil, target_id: nil)
     if !target && !(target_type && target_id)
       raise "target OR target_type and target_id are required"

plugins/chat/app/jobs/regular/chat_channel_delete.rb

@@ -28,24 +28,30 @@ module Jobs
         Rails.logger.debug(
           "Deleting chat messages, mentions, revisions, and uploads for channel #{chat_channel.id}",
         )
-        ChatMessage.transaction do
+        chat_messages = ChatMessage.where(chat_channel: chat_channel)
-          chat_messages = ChatMessage.where(chat_channel: chat_channel)
+        delete_messages_and_related_records(chat_channel, chat_messages) if chat_messages.any?
-          message_ids = chat_messages.select(:id)
+      end
-          ChatMention.where(chat_message_id: message_ids).delete_all
+    end
-          ChatMessageRevision.where(chat_message_id: message_ids).delete_all
-          ChatMessageReaction.where(chat_message_id: message_ids).delete_all
 
-          # if the uploads are not used anywhere else they will be deleted
+    def delete_messages_and_related_records(chat_channel, chat_messages)
-          # by the CleanUpUploads job in core
+      message_ids = chat_messages.pluck(:id)
-          ChatUpload.where(chat_message_id: message_ids).delete_all
 
-          # only the messages and the channel are Trashable, everything else gets
+      ChatMessage.transaction do
-          # permanently destroyed
+        ChatMention.where(chat_message_id: message_ids).delete_all
-          chat_messages.update_all(
+        ChatMessageRevision.where(chat_message_id: message_ids).delete_all
-            deleted_by_id: chat_channel.deleted_by_id,
+        ChatMessageReaction.where(chat_message_id: message_ids).delete_all
-            deleted_at: Time.zone.now,
+
-          )
+        # if the uploads are not used anywhere else they will be deleted
-        end
+        # by the CleanUpUploads job in core
+        DB.exec("DELETE FROM chat_uploads WHERE chat_message_id IN (#{message_ids.join(",")})")
+        UploadReference.where(target_id: message_ids, target_type: "ChatMessage").delete_all
+
+        # only the messages and the channel are Trashable, everything else gets
+        # permanently destroyed
+        chat_messages.update_all(
+          deleted_by_id: chat_channel.deleted_by_id,
+          deleted_at: Time.zone.now,
+        )
       end
     end
   end

plugins/chat/app/models/chat_message.rb

@@ -14,8 +14,11 @@ class ChatMessage < ActiveRecord::Base
   has_many :revisions, class_name: "ChatMessageRevision", dependent: :destroy
   has_many :reactions, class_name: "ChatMessageReaction", dependent: :destroy
   has_many :bookmarks, as: :bookmarkable, dependent: :destroy
+  has_many :upload_references, as: :target, dependent: :destroy
+  has_many :uploads, through: :upload_references
+
+  # TODO (martin) Remove this when we drop the ChatUpload table
   has_many :chat_uploads, dependent: :destroy
-  has_many :uploads, through: :chat_uploads
   has_one :chat_webhook_event, dependent: :destroy
   has_one :chat_mention, dependent: :destroy
 
@@ -61,14 +64,20 @@ class ChatMessage < ActiveRecord::Base
   end
 
   def attach_uploads(uploads)
-    return if uploads.blank?
+    return if uploads.blank? || self.new_record?
 
     now = Time.now
-    record_attrs =
+    ref_record_attrs =
       uploads.map do |upload|
-        { upload_id: upload.id, chat_message_id: self.id, created_at: now, updated_at: now }
+        {
+          upload_id: upload.id,
+          target_id: self.id,
+          target_type: "ChatMessage",
+          created_at: now,
+          updated_at: now,
+        }
       end
-    ChatUpload.insert_all!(record_attrs)
+    UploadReference.insert_all!(ref_record_attrs)
   end
 
   def excerpt
@@ -91,20 +100,19 @@ class ChatMessage < ActiveRecord::Base
   end
 
   def to_markdown
-    markdown = []
+    upload_markdown =
+      self
+        .upload_references
+        .includes(:upload)
+        .order(:created_at)
+        .map(&:to_markdown)
+        .reject(&:empty?)
 
-    if self.message.present?
+    return self.message if upload_markdown.empty?
-      msg = self.message
 
-      self.chat_uploads.any? ? markdown << msg + "\n" : markdown << msg
+    return ["#{self.message}\n"].concat(upload_markdown).join("\n") if self.message.present?
-    end
 
-    self
+    upload_markdown.join("\n")
-      .chat_uploads
-      .order(:created_at)
-      .each { |chat_upload| markdown << UploadMarkdown.new(chat_upload.upload).to_markdown }
-
-    markdown.reject(&:empty?).join("\n")
   end
 
   def cook

plugins/chat/app/models/chat_upload.rb

@@ -1,8 +1,15 @@
 # frozen_string_literal: true
 
+# TODO (martin) DEPRECATED: Remove this model once UploadReference has been
+# in place for a couple of months, 2023-04-01
+#
+# NOTE: Do not use this model anymore, chat messages are linked to uploads via
+# the UploadReference table now, just like everything else.
 class ChatUpload < ActiveRecord::Base
   belongs_to :chat_message
   belongs_to :upload
+
+  deprecate *public_instance_methods(false)
 end
 
 # == Schema Information

plugins/chat/db/migrate/20230123020036_move_chat_uploads_to_upload_references.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class MoveChatUploadsToUploadReferences < ActiveRecord::Migration[7.0]
+  def up
+    execute <<~SQL
+      INSERT INTO upload_references(upload_id, target_type, target_id, created_at, updated_at)
+      SELECT chat_uploads.upload_id, 'ChatMessage', chat_uploads.chat_message_id, chat_uploads.created_at, chat_uploads.updated_at
+      FROM chat_uploads
+      INNER JOIN uploads ON uploads.id = chat_uploads.upload_id
+      ON CONFLICT DO NOTHING
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/chat/db/post_migrate/20230123025112_move_chat_uploads_to_upload_references_post.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class MoveChatUploadsToUploadReferencesPost < ActiveRecord::Migration[7.0]
+  def up
+    execute <<~SQL
+      INSERT INTO upload_references(upload_id, target_type, target_id, created_at, updated_at)
+      SELECT chat_uploads.upload_id, 'ChatMessage', chat_uploads.chat_message_id, chat_uploads.created_at, chat_uploads.updated_at
+      FROM chat_uploads
+      INNER JOIN uploads ON uploads.id = chat_uploads.upload_id
+      ON CONFLICT DO NOTHING
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/chat/lib/chat_message_updater.rb

@@ -83,7 +83,8 @@ class Chat::ChatMessageUpdater
   def update_uploads(upload_info)
     return unless upload_info[:changed]
 
-    ChatUpload.where(chat_message: @chat_message).destroy_all
+    DB.exec("DELETE FROM chat_uploads WHERE chat_message_id = #{@chat_message.id}")
+    UploadReference.where(target: @chat_message).destroy_all
     @chat_message.attach_uploads(upload_info[:uploads])
   end
 

plugins/chat/lib/chat_transcript_service.rb

@@ -147,7 +147,7 @@ class ChatTranscriptService
   def messages
     @messages ||=
       ChatMessage
-        .includes(:user, chat_uploads: :upload)
+        .includes(:user, upload_references: :upload)
         .where(id: @message_ids, chat_channel_id: @channel.id)
         .order(:created_at)
   end

plugins/chat/lib/message_mover.rb

@@ -125,10 +125,10 @@ class Chat::MessageMover
     SQL
 
     DB.exec(<<~SQL)
-      UPDATE chat_uploads cu
+      UPDATE upload_references uref
-      SET chat_message_id = mm.new_chat_message_id
+      SET target_id = mm.new_chat_message_id
       FROM moved_chat_messages mm
-      WHERE cu.chat_message_id = mm.old_chat_message_id
+      WHERE uref.target_id = mm.old_chat_message_id AND uref.target_type = 'ChatMessage'
     SQL
 
     DB.exec(<<~SQL)

plugins/chat/plugin.rb

@@ -372,14 +372,6 @@ after_initialize do
     end
   end
 
-  if respond_to?(:register_upload_unused)
-    register_upload_unused do |uploads|
-      uploads.joins("LEFT JOIN chat_uploads cu ON cu.upload_id = uploads.id").where(
-        "cu.upload_id IS NULL",
-      )
-    end
-  end
-
   if respond_to?(:register_upload_in_use)
     register_upload_in_use do |upload|
       ChatMessage.where(

plugins/chat/spec/components/chat_message_creator_spec.rb

@@ -461,7 +461,9 @@ describe Chat::ChatMessageCreator do
             content: "Beep boop",
             upload_ids: [upload1.id],
           )
-        }.to change { ChatUpload.where(upload_id: upload1.id).count }.by(1)
+        }.to not_change { chat_upload_count([upload1]) }.and change {
+                UploadReference.where(upload_id: upload1.id).count
+              }.by(1)
       end
 
       it "can attach multiple uploads to a new message" do
@@ -472,9 +474,9 @@ describe Chat::ChatMessageCreator do
             content: "Beep boop",
             upload_ids: [upload1.id, upload2.id],
           )
-        }.to change { ChatUpload.where(upload_id: upload1.id).count }.by(1).and change {
+        }.to not_change { chat_upload_count([upload1, upload2]) }.and change {
-                ChatUpload.where(upload_id: upload2.id).count
+                UploadReference.where(upload_id: [upload1.id, upload2.id]).count
-              }.by(1)
+              }.by(2)
       end
 
       it "filters out uploads that weren't uploaded by the user" do
@@ -485,7 +487,7 @@ describe Chat::ChatMessageCreator do
             content: "Beep boop",
             upload_ids: [private_upload.id],
           )
-        }.not_to change { ChatUpload.where(upload_id: private_upload.id).count }
+        }.not_to change { chat_upload_count([private_upload]) }
       end
 
       it "doesn't attach uploads when `chat_allow_uploads` is false" do
@@ -497,7 +499,9 @@ describe Chat::ChatMessageCreator do
             content: "Beep boop",
             upload_ids: [upload1.id],
           )
-        }.not_to change { ChatUpload.where(upload_id: upload1.id).count }
+        }.to not_change { chat_upload_count([upload1]) }.and not_change {
+                UploadReference.where(upload_id: upload1.id).count
+              }
       end
     end
   end
@@ -605,4 +609,11 @@ describe Chat::ChatMessageCreator do
       end
     end
   end
+
+  # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01
+  def chat_upload_count(uploads)
+    DB.query_single(
+      "SELECT COUNT(*) FROM chat_uploads WHERE upload_id IN (#{uploads.map(&:id).join(",")})",
+    ).first
+  end
 end

plugins/chat/spec/components/chat_message_updater_spec.rb

@@ -329,7 +329,7 @@ describe Chat::ChatMessageUpdater do
           new_content: "I guess this is different",
           upload_ids: [upload2.id, upload1.id],
         )
-      }.not_to change { ChatUpload.count }
+      }.to not_change { chat_upload_count }.and not_change { UploadReference.count }
     end
 
     it "removes uploads that should be removed" do
@@ -340,6 +340,16 @@ describe Chat::ChatMessageUpdater do
           public_chat_channel,
           upload_ids: [upload1.id, upload2.id],
         )
+
+      # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01
+      DB.exec(<<~SQL)
+        INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at)
+        VALUES(#{upload1.id}, #{chat_message.id}, NOW(), NOW())
+      SQL
+      DB.exec(<<~SQL)
+        INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at)
+        VALUES(#{upload2.id}, #{chat_message.id}, NOW(), NOW())
+      SQL
       expect {
         Chat::ChatMessageUpdater.update(
           guardian: guardian,
@@ -347,7 +357,9 @@ describe Chat::ChatMessageUpdater do
           new_content: "I guess this is different",
           upload_ids: [upload1.id],
         )
-      }.to change { ChatUpload.where(upload_id: upload2.id).count }.by(-1)
+      }.to change { chat_upload_count([upload2]) }.by(-1).and change {
+              UploadReference.where(upload_id: upload2.id).count
+            }.by(-1)
     end
 
     it "removes all uploads if they should be removed" do
@@ -358,6 +370,16 @@ describe Chat::ChatMessageUpdater do
           public_chat_channel,
           upload_ids: [upload1.id, upload2.id],
         )
+
+      # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01
+      DB.exec(<<~SQL)
+        INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at)
+        VALUES(#{upload1.id}, #{chat_message.id}, NOW(), NOW())
+      SQL
+      DB.exec(<<~SQL)
+        INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at)
+        VALUES(#{upload2.id}, #{chat_message.id}, NOW(), NOW())
+      SQL
       expect {
         Chat::ChatMessageUpdater.update(
           guardian: guardian,
@@ -365,7 +387,9 @@ describe Chat::ChatMessageUpdater do
           new_content: "I guess this is different",
           upload_ids: [],
         )
-      }.to change { ChatUpload.where(chat_message: chat_message).count }.by(-2)
+      }.to change { chat_upload_count([upload1, upload2]) }.by(-2).and change {
+              UploadReference.where(target: chat_message).count
+            }.by(-2)
     end
 
     it "adds one upload if none exist" do
@@ -377,7 +401,9 @@ describe Chat::ChatMessageUpdater do
           new_content: "I guess this is different",
           upload_ids: [upload1.id],
         )
-      }.to change { ChatUpload.where(chat_message: chat_message).count }.by(1)
+      }.to not_change { chat_upload_count([upload1]) }.and change {
+              UploadReference.where(target: chat_message).count
+            }.by(1)
     end
 
     it "adds multiple uploads if none exist" do
@@ -389,7 +415,9 @@ describe Chat::ChatMessageUpdater do
           new_content: "I guess this is different",
           upload_ids: [upload1.id, upload2.id],
         )
-      }.to change { ChatUpload.where(chat_message: chat_message).count }.by(2)
+      }.to not_change { chat_upload_count([upload1, upload2]) }.and change {
+              UploadReference.where(target: chat_message).count
+            }.by(2)
     end
 
     it "doesn't remove existing uploads when upload ids that do not exist are passed in" do
@@ -402,7 +430,9 @@ describe Chat::ChatMessageUpdater do
           new_content: "I guess this is different",
           upload_ids: [0],
         )
-      }.not_to change { ChatUpload.where(chat_message: chat_message).count }
+      }.to not_change { chat_upload_count }.and not_change {
+              UploadReference.where(target: chat_message).count
+            }
     end
 
     it "doesn't add uploads if `chat_allow_uploads` is false" do
@@ -415,7 +445,9 @@ describe Chat::ChatMessageUpdater do
           new_content: "I guess this is different",
           upload_ids: [upload1.id, upload2.id],
         )
-      }.not_to change { ChatUpload.where(chat_message: chat_message).count }
+      }.to not_change { chat_upload_count([upload1, upload2]) }.and not_change {
+              UploadReference.where(target: chat_message).count
+            }
     end
 
     it "doesn't remove existing uploads if `chat_allow_uploads` is false" do
@@ -434,7 +466,9 @@ describe Chat::ChatMessageUpdater do
           new_content: "I guess this is different",
           upload_ids: [],
         )
-      }.not_to change { ChatUpload.where(chat_message: chat_message).count }
+      }.to not_change { chat_upload_count }.and not_change {
+              UploadReference.where(target: chat_message).count
+            }
     end
 
     it "updates if upload is present even if length is less than `chat_minimum_message_length`" do
@@ -553,4 +587,12 @@ describe Chat::ChatMessageUpdater do
       end
     end
   end
+
+  # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01
+  def chat_upload_count(uploads = nil)
+    return DB.query_single("SELECT COUNT(*) FROM chat_uploads").first if !uploads
+    DB.query_single(
+      "SELECT COUNT(*) FROM chat_uploads WHERE upload_id IN (#{uploads.map(&:id).join(",")})",
+    ).first
+  end
 end

plugins/chat/spec/jobs/chat_channel_delete_spec.rb

@@ -17,10 +17,15 @@ describe Jobs::ChatChannelDelete do
     10.times { ChatMessageReaction.create(chat_message: messages.sample, user: users.sample) }
 
     10.times do
-      ChatUpload.create(
+      upload = Fabricate(:upload, user: users.sample)
-        upload: Fabricate(:upload, user: users.sample),
+      message = messages.sample
-        chat_message: messages.sample,
+
-      )
+      # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01
+      DB.exec(<<~SQL)
+        INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at)
+        VALUES(#{upload.id}, #{message.id}, NOW(), NOW())
+      SQL
+      UploadReference.create(target: message, upload: upload)
     end
 
     ChatMention.create(
@@ -52,27 +57,45 @@ describe Jobs::ChatChannelDelete do
     chat_channel.trash!
   end
 
+  def counts
+    {
+      incoming_webhooks: IncomingChatWebhook.where(chat_channel_id: chat_channel.id).count,
+      webhook_events:
+        ChatWebhookEvent.where(incoming_chat_webhook_id: @incoming_chat_webhook_id).count,
+      drafts: ChatDraft.where(chat_channel: chat_channel).count,
+      channel_memberships: UserChatChannelMembership.where(chat_channel: chat_channel).count,
+      revisions: ChatMessageRevision.where(chat_message_id: @message_ids).count,
+      mentions: ChatMention.where(chat_message_id: @message_ids).count,
+      chat_uploads:
+        DB.query_single(
+          "SELECT COUNT(*) FROM chat_uploads WHERE chat_message_id IN (#{@message_ids.join(",")})",
+        ).first,
+      upload_references:
+        UploadReference.where(target_id: @message_ids, target_type: "ChatMessage").count,
+      messages: ChatMessage.where(id: @message_ids).count,
+      reactions: ChatMessageReaction.where(chat_message_id: @message_ids).count,
+    }
+  end
+
   it "deletes all of the messages and related records completely" do
-    expect { described_class.new.execute(chat_channel_id: chat_channel.id) }.to change {
+    initial_counts = counts
-      IncomingChatWebhook.where(chat_channel_id: chat_channel.id).count
+    described_class.new.execute(chat_channel_id: chat_channel.id)
-    }.by(-1).and change {
+    new_counts = counts
-            ChatWebhookEvent.where(incoming_chat_webhook_id: @incoming_chat_webhook_id).count
+
-          }.by(-1).and change { ChatDraft.where(chat_channel: chat_channel).count }.by(
+    expect(new_counts[:incoming_webhooks]).to eq(initial_counts[:incoming_webhooks] - 1)
-                  -1,
+    expect(new_counts[:webhook_events]).to eq(initial_counts[:webhook_events] - 1)
-                ).and change {
+    expect(new_counts[:drafts]).to eq(initial_counts[:drafts] - 1)
-                        UserChatChannelMembership.where(chat_channel: chat_channel).count
+    expect(new_counts[:channel_memberships]).to eq(initial_counts[:channel_memberships] - 3)
-                      }.by(-3).and change {
+    expect(new_counts[:revisions]).to eq(initial_counts[:revisions] - 1)
-                              ChatMessageRevision.where(chat_message_id: @message_ids).count
+    expect(new_counts[:mentions]).to eq(initial_counts[:mentions] - 1)
-                            }.by(-1).and change {
+    expect(new_counts[:chat_uploads]).to eq(initial_counts[:chat_uploads] - 10)
-                                    ChatMention.where(chat_message_id: @message_ids).count
+    expect(new_counts[:upload_references]).to eq(initial_counts[:upload_references] - 10)
-                                  }.by(-1).and change {
+    expect(new_counts[:messages]).to eq(initial_counts[:messages] - 20)
-                                          ChatUpload.where(chat_message_id: @message_ids).count
+    expect(new_counts[:reactions]).to eq(initial_counts[:reactions] - 10)
-                                        }.by(-10).and change {
+  end
-                                                ChatMessage.where(id: @message_ids).count
+
-                                              }.by(-20).and change {
+  it "does not error if there are no messages in the channel" do
-                                                      ChatMessageReaction.where(
+    other_channel = Fabricate(:chat_channel)
-                                                        chat_message_id: @message_ids,
+    expect { described_class.new.execute(chat_channel_id: other_channel.id) }.not_to raise_error
-                                                      ).count
-                                                    }.by(-10)
   end
 end

plugins/chat/spec/lib/chat_channel_archive_service_spec.rb

@@ -152,7 +152,7 @@ describe Chat::ChatChannelArchiveService do
 
       it "successfully links uploads from messages to the post" do
         create_messages(3) && start_archive
-        ChatUpload.create(chat_message: ChatMessage.last, upload: Fabricate(:upload))
+        UploadReference.create(target: ChatMessage.last, upload: Fabricate(:upload))
         subject.new(@channel_archive).execute
         expect(@channel_archive.reload.complete?).to eq(true)
         expect(@channel_archive.destination_topic.posts.last.upload_references.count).to eq(1)

plugins/chat/spec/lib/chat_transcript_service_spec.rb

@@ -130,10 +130,10 @@ describe ChatTranscriptService do
         original_filename: "test_img.jpg",
         extension: "jpg",
       )
-    cu1 = ChatUpload.create(chat_message: message, created_at: 10.seconds.ago, upload: video)
+    UploadReference.create(target: message, created_at: 10.seconds.ago, upload: video)
-    cu2 = ChatUpload.create(chat_message: message, created_at: 9.seconds.ago, upload: audio)
+    UploadReference.create(target: message, created_at: 9.seconds.ago, upload: audio)
-    cu3 = ChatUpload.create(chat_message: message, created_at: 8.seconds.ago, upload: attachment)
+    UploadReference.create(target: message, created_at: 8.seconds.ago, upload: attachment)
-    cu4 = ChatUpload.create(chat_message: message, created_at: 7.seconds.ago, upload: image)
+    UploadReference.create(target: message, created_at: 7.seconds.ago, upload: image)
     video_markdown = UploadMarkdown.new(video).to_markdown
     audio_markdown = UploadMarkdown.new(audio).to_markdown
     attachment_markdown = UploadMarkdown.new(attachment).to_markdown
@@ -166,7 +166,7 @@ describe ChatTranscriptService do
         original_filename: "test_img.jpg",
         extension: "jpg",
       )
-    cu = ChatUpload.create(chat_message: message, created_at: 7.seconds.ago, upload: image)
+    UploadReference.create(target: message, created_at: 7.seconds.ago, upload: image)
     image_markdown = UploadMarkdown.new(image).to_markdown
 
     expect(service(message.id).generate_markdown).to eq(<<~MARKDOWN)

plugins/chat/spec/lib/message_mover_spec.rb

@@ -112,7 +112,7 @@ describe Chat::MessageMover do
 
     it "updates references for reactions, uploads, revisions, mentions, etc." do
       reaction = Fabricate(:chat_message_reaction, chat_message: message1)
-      upload = Fabricate(:chat_upload, chat_message: message1)
+      upload = Fabricate(:upload_reference, target: message1)
       mention = Fabricate(:chat_mention, chat_message: message2, user: acting_user)
       revision = Fabricate(:chat_message_revision, chat_message: message3)
       webhook_event = Fabricate(:chat_webhook_event, chat_message: message3)
@@ -121,7 +121,7 @@ describe Chat::MessageMover do
       moved_messages =
         ChatMessage.where(chat_channel: destination_channel).order("created_at ASC, id ASC").last(3)
       expect(reaction.reload.chat_message_id).to eq(moved_messages.first.id)
-      expect(upload.reload.chat_message_id).to eq(moved_messages.first.id)
+      expect(upload.reload.target_id).to eq(moved_messages.first.id)
       expect(mention.reload.chat_message_id).to eq(moved_messages.second.id)
       expect(revision.reload.chat_message_id).to eq(moved_messages.third.id)
       expect(webhook_event.reload.chat_message_id).to eq(moved_messages.third.id)

plugins/chat/spec/models/chat_message_spec.rb

@@ -309,7 +309,7 @@ describe ChatMessage do
       gif =
         Fabricate(:upload, original_filename: "cat.gif", width: 400, height: 300, extension: "gif")
       message = Fabricate(:chat_message, message: "")
-      ChatUpload.create(chat_message: message, upload: gif)
+      UploadReference.create(target: message, upload: gif)
 
       expect(message.excerpt).to eq "cat.gif"
     end
@@ -391,8 +391,8 @@ describe ChatMessage do
         )
       image2 =
         Fabricate(:upload, original_filename: "meme.jpg", width: 10, height: 10, extension: "jpg")
-      ChatUpload.create(chat_message: message, upload: image)
+      UploadReference.create!(target: message, upload: image)
-      ChatUpload.create(chat_message: message, upload: image2)
+      UploadReference.create!(target: message, upload: image2)
       expect(message.to_markdown).to eq(<<~MSG.chomp)
       hey friend, what's up?!
 
@@ -479,13 +479,20 @@ describe ChatMessage do
       expect { webhook_1.reload }.to raise_error(ActiveRecord::RecordNotFound)
     end
 
-    it "destroys chat_uploads" do
+    it "destroys upload_references and chat_uploads" do
       message_1 = Fabricate(:chat_message)
-      chat_upload_1 = Fabricate(:chat_upload, chat_message: message_1)
+      upload_reference_1 = Fabricate(:upload_reference, target: message_1)
+      upload_1 = Fabricate(:upload)
+      # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01
+      DB.exec(<<~SQL)
+        INSERT INTO chat_uploads(upload_id, chat_message_id, created_at, updated_at)
+        VALUES(#{upload_1.id}, #{message_1.id}, NOW(), NOW())
+      SQL
 
       message_1.destroy!
 
-      expect { chat_upload_1.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      expect(DB.query("SELECT * FROM chat_uploads WHERE upload_id = #{upload_1.id}")).to eq([])
+      expect { upload_reference_1.reload }.to raise_error(ActiveRecord::RecordNotFound)
     end
 
     describe "bookmarks" do
@@ -532,4 +539,39 @@ describe ChatMessage do
       end
     end
   end
+
+  describe "#attach_uploads" do
+    fab!(:chat_message) { Fabricate(:chat_message) }
+    fab!(:upload_1) { Fabricate(:upload) }
+    fab!(:upload_2) { Fabricate(:upload) }
+
+    it "creates an UploadReference record for the provided uploads" do
+      chat_message.attach_uploads([upload_1, upload_2])
+      upload_references = UploadReference.where(upload_id: [upload_1, upload_2])
+      expect(chat_upload_count([upload_1, upload_2])).to eq(0)
+      expect(upload_references.count).to eq(2)
+      expect(upload_references.map(&:target_id).uniq).to eq([chat_message.id])
+      expect(upload_references.map(&:target_type).uniq).to eq(["ChatMessage"])
+    end
+
+    it "does nothing if the message record is new" do
+      expect { ChatMessage.new.attach_uploads([upload_1, upload_2]) }.to not_change {
+        chat_upload_count
+      }.and not_change { UploadReference.count }
+    end
+
+    it "does nothing for an empty uploads array" do
+      expect { chat_message.attach_uploads([]) }.to not_change {
+        chat_upload_count
+      }.and not_change { UploadReference.count }
+    end
+  end
+
+  # TODO (martin) Remove this when we remove ChatUpload completely, 2023-04-01
+  def chat_upload_count(uploads = nil)
+    return DB.query_single("SELECT COUNT(*) FROM chat_uploads").first if !uploads
+    DB.query_single(
+      "SELECT COUNT(*) FROM chat_uploads WHERE upload_id IN (#{uploads.map(&:id).join(",")})",
+    ).first
+  end
 end

plugins/chat/spec/plugin_spec.rb

@@ -26,7 +26,7 @@ describe Chat do
       )
     end
 
-    it "marks uploads with ChatUpload in use" do
+    it "marks uploads with reference to ChatMessage via UploadReference in use" do
       unused_upload
 
       expect { Jobs::CleanUpUploads.new.execute({}) }.to change { Upload.count }.by(-1)
@@ -61,7 +61,7 @@ describe Chat do
       )
     end
 
-    it "marks uploads with ChatUpload in use" do
+    it "marks uploads with reference to ChatMessage via UploadReference in use" do
       draft_upload
       unused_upload
 

plugins/chat/spec/system/uploads_spec.rb

@@ -79,15 +79,21 @@ describe "Uploading files in chat messages", type: :system, js: true do
 
   context "when editing a message with uploads" do
     fab!(:message_2) { Fabricate(:chat_message, user: current_user, chat_channel: channel_1) }
-    fab!(:chat_upload) { Fabricate(:chat_upload, chat_message: message_2, user: current_user) }
+    fab!(:upload_reference) do
+      Fabricate(
+        :upload_reference,
+        target: message_2,
+        upload: Fabricate(:upload, user: current_user),
+      )
+    end
 
     before do
       channel_1.add(current_user)
       sign_in(current_user)
 
       file = file_from_fixtures("logo-dev.png", "images")
-      url = Discourse.store.store_upload(file, chat_upload.upload)
+      url = Discourse.store.store_upload(file, upload_reference.upload)
-      chat_upload.upload.update!(url: url, sha1: Upload.generate_digest(file))
+      upload_reference.upload.update!(url: url, sha1: Upload.generate_digest(file))
     end
 
     it "allows deleting uploads" do

spec/fabricators/upload_fabricator.rb

@@ -97,3 +97,8 @@ Fabricator(:secure_upload_s3, from: :upload_s3) do
   sha1 { SecureRandom.hex(20) }
   original_sha1 { sequence(:sha1) { |n| Digest::SHA1.hexdigest(n.to_s) } }
 end
+
+Fabricator(:upload_reference) do
+  target
+  upload
+end