Discourse/discourse
Discourse/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-10-03 17:21:20 +08:00
Code Packages Activity Actions 7

DEV: Move convert video job to upload after create (#35013)

Browse source 
This moves the logic for creating the convert_video job to the upload
after_create hook so that we ensure there is an upload_id. When this
logic
was in the s3 store and direct to s3 uploads was enabled the convert
video job
would never fire because we didn't have an upload_id.
This commit is contained in:
Blake Erickson 2025-09-30 10:58:07 -06:00 • committed by GitHub
parent e61dbabcc4
commit d501952f25
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 146 additions and 66 deletions
Download patch file Download diff file Expand all files Collapse all files

1
app/models/optimized_video.rb
View file

@ -21,6 +21,7 @@ class OptimizedVideo < ActiveRecord::Base
extension: options[:extension] || "mp4",
url: options[:url],
etag: options[:etag],
skip_video_conversion: true,
)

optimized_video =

12
app/models/upload.rb
View file

@ -30,6 +30,7 @@ class Upload < ActiveRecord::Base
has_many :posts, through: :upload_references, source: :target, source_type: "Post"
has_many :topic_thumbnails
has_many :badges, foreign_key: :image_upload_id, dependent: :nullify
after_create :enqueue_video_conversion_job, if: :should_convert_video?

attr_accessor :for_group_message
attr_accessor :for_theme
@ -38,6 +39,7 @@ class Upload < ActiveRecord::Base
attr_accessor :for_site_setting
attr_accessor :for_gravatar
attr_accessor :validate_file_size
attr_accessor :skip_video_conversion

validates_presence_of :filesize
validates_presence_of :original_filename
@ -660,6 +662,16 @@ class Upload < ActiveRecord::Base
def short_url_basename
"#{Upload.base62_sha1(sha1)}#{extension.present? ? ".#{extension}" : ""}"
end

def should_convert_video?
!skip_video_conversion && SiteSetting.video_conversion_enabled &&
SiteSetting.Upload.enable_s3_uploads && FileHelper.is_supported_video?(original_filename) &&
!OptimizedVideo.exists?(upload_id: id)
end

def enqueue_video_conversion_job
Jobs.enqueue(:convert_video, upload_id: id)
end
end

# == Schema Information

5
lib/file_store/s3_store.rb
View file

@ -116,11 +116,6 @@ module FileStore
path, etag = s3_helper.upload(file, path, options)
end

if opts[:upload_id] && FileHelper.is_supported_video?(opts[:filename]) &&
SiteSetting.video_conversion_enabled
Jobs.enqueue(:convert_video, upload_id: opts[:upload_id])
end

# return the upload url and etag
[File.join(absolute_base_url, path), etag]
end

61
spec/lib/file_store/s3_store_spec.rb
View file

@ -204,67 +204,6 @@ RSpec.describe FileStore::S3Store do
)
end
end

context "when video conversion is enabled" do
let(:video_file) { file_from_fixtures("small.mp4", "media") }
let(:video_upload) do
Fabricate.build(:upload, original_filename: "small.mp4", extension: "mp4", id: 42)
end

before do
# Set up required MediaConvert settings
SiteSetting.video_conversion_service = "aws_mediaconvert"
SiteSetting.mediaconvert_role_arn = "arn:aws:iam::123456789012:role/MediaConvertRole"
SiteSetting.video_conversion_enabled = true
# Default stub that returns false for any argument
allow(FileHelper).to receive(:is_supported_video?).and_return(false)
# Override for the specific video file case
allow(FileHelper).to receive(:is_supported_video?).with("small.mp4").and_return(true)
allow(store.s3_helper).to receive(:upload).and_return(["some/path.mp4", "\"etag\""])
# Setup Jobs as a spy
allow(Jobs).to receive(:enqueue)
end

it "enqueues a convert_video job for supported video files" do
store.store_upload(video_file, video_upload)

expect(Jobs).to have_received(:enqueue).with(:convert_video, upload_id: video_upload.id)
end

it "does not enqueue a convert_video job for unsupported video files" do
allow(FileHelper).to receive(:is_supported_video?).with("small.mp4").and_return(false)

store.store_upload(video_file, video_upload)

expect(Jobs).not_to have_received(:enqueue).with(
:convert_video,
upload_id: video_upload.id,
)
end

it "does not enqueue a convert_video job when video conversion is disabled" do
SiteSetting.video_conversion_enabled = false

store.store_upload(video_file, video_upload)

expect(Jobs).not_to have_received(:enqueue).with(
:convert_video,
upload_id: video_upload.id,
)
end

it "does not enqueue a convert_video job for non-video files" do
non_video_upload =
Fabricate.build(:upload, original_filename: "image.png", extension: "png", id: 43)

store.store_upload(uploaded_file, non_video_upload)

expect(Jobs).not_to have_received(:enqueue).with(
:convert_video,
upload_id: non_video_upload.id,
)
end
end
end

describe "#store_optimized_image" do

133
spec/models/upload_spec.rb
View file

@ -47,6 +47,139 @@ RSpec.describe Upload do
end
end

describe "video conversion" do
let(:user) { Fabricate(:user) }

before do
# Add mp4 to authorized extensions for video uploads
extensions = SiteSetting.authorized_extensions.split("|")
SiteSetting.authorized_extensions = (extensions | ["mp4"]).join("|")

SiteSetting.video_conversion_service = "aws_mediaconvert"
SiteSetting.mediaconvert_role_arn = "arn:aws:iam::123456789012:role/MediaConvertRole"
SiteSetting.enable_s3_uploads = true
SiteSetting.s3_use_iam_profile = true
SiteSetting.video_conversion_enabled = true
end

context "when video conversion is enabled" do
it "enqueues a convert_video job for supported video files on create" do
allow(FileHelper).to receive(:is_supported_video?).with("small.mp4").and_return(true)

upload = nil
expect_enqueued_with(job: :convert_video, args: {}) do
upload = Fabricate(:upload, original_filename: "small.mp4", extension: "mp4", user: user)
end
expect_job_enqueued(job: :convert_video, args: { upload_id: upload.id })
end

it "does not enqueue a convert_video job for unsupported video files" do
allow(FileHelper).to receive(:is_supported_video?).with("small.mp4").and_return(false)

expect_not_enqueued_with(job: :convert_video) do
Fabricate(:upload, original_filename: "small.mp4", extension: "mp4", user: user)
end
end

it "does not enqueue a convert_video job when video conversion is disabled" do
SiteSetting.video_conversion_enabled = false
allow(FileHelper).to receive(:is_supported_video?).with("small.mp4").and_return(true)

expect_not_enqueued_with(job: :convert_video) do
Fabricate(:upload, original_filename: "small.mp4", extension: "mp4", user: user)
end
end

it "does not enqueue a convert_video job when S3 uploads are disabled" do
SiteSetting.enable_s3_uploads = false
allow(FileHelper).to receive(:is_supported_video?).with("small.mp4").and_return(true)

expect_not_enqueued_with(job: :convert_video) do
Fabricate(:upload, original_filename: "small.mp4", extension: "mp4", user: user)
end
end

it "does not enqueue a convert_video job for non-video files" do
expect_not_enqueued_with(job: :convert_video) do
Fabricate(:upload, original_filename: "image.png", extension: "png", user: user)
end
end

it "does not enqueue a convert_video job if OptimizedVideo already exists" do
allow(FileHelper).to receive(:is_supported_video?).with("small.mp4").and_return(true)
allow(FileHelper).to receive(:is_supported_video?).with("video_converted.mp4").and_return(
true,
)

# Create original upload
upload = Fabricate(:upload, original_filename: "small.mp4", extension: "mp4", user: user)

# Create OptimizedVideo record for the original upload
optimized_video = Fabricate(:optimized_video, upload: upload)

# Update the original upload to trigger after_commit
expect_not_enqueued_with(job: :convert_video) { upload.update!(filesize: 12_345) }
end

it "does not enqueue a convert_video job on update, only on create" do
allow(FileHelper).to receive(:is_supported_video?).with("small.mp4").and_return(true)

upload = Fabricate(:upload, original_filename: "small.mp4", extension: "mp4", user: user)

# Update the upload - should not trigger video conversion since it's after_create, not after_update
expect_not_enqueued_with(job: :convert_video) { upload.update!(filesize: 12_345) }
end

it "does not enqueue a convert_video job for optimized video uploads to prevent infinite loop" do
allow(FileHelper).to receive(:is_supported_video?).with("small.mp4").and_return(true)
allow(FileHelper).to receive(:is_supported_video?).with("video_converted.mp4").and_return(
true,
)

# Create original upload
upload = Fabricate(:upload, original_filename: "small.mp4", extension: "mp4", user: user)

# Use OptimizedVideo.create_for to simulate the real flow
# This creates the optimized upload and then the OptimizedVideo record
optimized_video = nil
optimized_upload = nil
expect_not_enqueued_with(job: :convert_video) do
optimized_video =
OptimizedVideo.create_for(
upload,
"video_converted.mp4",
user.id,
filesize: 1000,
sha1: "abcdef1234567890",
url: "https://example.com/video_converted.mp4",
adapter: "aws_mediaconvert",
)
end

expect(optimized_video).not_to be_nil
end

it "enqueues a convert_video job for user uploads with _converted in filename" do
allow(FileHelper).to receive(:is_supported_video?).with(
"my_video_converted.mp4",
).and_return(true)

# User uploads a file with "_converted" in the name - should still be converted
upload = nil
expect_enqueued_with(job: :convert_video, args: {}) do
upload =
Fabricate(
:upload,
original_filename: "my_video_converted.mp4",
extension: "mp4",
user: user,
)
end
expect_job_enqueued(job: :convert_video, args: { upload_id: upload.id })
end
end
end

describe ".create_thumbnail!" do
it "does not create a thumbnail when disabled" do
SiteSetting.create_thumbnails = false