mirror of
https://github.com/discourse/discourse.git
synced 2025-01-18 13:43:16 +08:00
DEV: Drop deprecated Badge#image column (#25536)
We just completed the 3.2 release, which marks a good time to drop some previously deprecated columns. Since the column has been marked in ignored_columns, it has been inaccessible to application code since then. There's a tiny risk that this might break a Data Explorer query, but given the nature of the column, the years of disuse, and the fact that such a breakage wouldn't be critical, we accept it.
This commit is contained in:
Ted Johansson
GitHub
parent
2da7c74e60
commit
e071b74a79
|
|
@ -1,98 +0,0 @@
|
|||
# frozen_string_literal: true
|
||||
require "uri"
|
||||
|
||||
module Jobs
|
||||
class MigrateBadgeImageToUploads < ::Jobs::Onceoff
|
||||
def execute_onceoff(args)
|
||||
column_exists = DB.exec(<<~SQL) == 1
|
||||
SELECT 1
|
||||
FROM INFORMATION_SCHEMA.COLUMNS
|
||||
WHERE
|
||||
table_schema = 'public' AND
|
||||
table_name = 'badges' AND
|
||||
column_name = 'image_upload_id'
|
||||
SQL
|
||||
return unless column_exists
|
||||
|
||||
Badge
|
||||
.where.not(image: nil)
|
||||
.select(:id, :image_upload_id, :image)
|
||||
.each do |badge|
|
||||
if badge.image_upload.present?
|
||||
DB.exec("UPDATE badges SET image = NULL WHERE id = ?", badge.id)
|
||||
next
|
||||
end
|
||||
|
||||
image_url = badge[:image]
|
||||
next if image_url.blank? || image_url !~ URI.regexp
|
||||
|
||||
count = 0
|
||||
file = nil
|
||||
sleep_interval = 5
|
||||
|
||||
loop do
|
||||
url = UrlHelper.absolute_without_cdn(image_url)
|
||||
|
||||
begin
|
||||
file =
|
||||
FileHelper.download(
|
||||
url,
|
||||
max_file_size: [SiteSetting.max_image_size_kb.kilobytes, 20.megabytes].max,
|
||||
tmp_file_name: "tmp_badge_image_upload",
|
||||
skip_rate_limit: true,
|
||||
follow_redirect: true,
|
||||
)
|
||||
rescue OpenURI::HTTPError,
|
||||
OpenSSL::SSL::SSLError,
|
||||
Net::OpenTimeout,
|
||||
Net::ReadTimeout,
|
||||
Errno::ECONNREFUSED,
|
||||
EOFError,
|
||||
SocketError,
|
||||
Discourse::InvalidParameters => e
|
||||
logger.error(
|
||||
"Error encountered when trying to download from URL '#{image_url}' " +
|
||||
"for badge '#{badge[:id]}'.\n#{e.class}: #{e.message}\n#{e.backtrace.join("\n")}",
|
||||
)
|
||||
end
|
||||
|
||||
count += 1
|
||||
break if file
|
||||
|
||||
logger.warn(
|
||||
"Failed to download image from #{url} for badge '#{badge[:id]}'. Retrying (#{count}/3)...",
|
||||
)
|
||||
break if count >= 3
|
||||
sleep(count * sleep_interval)
|
||||
end
|
||||
|
||||
next if file.blank?
|
||||
|
||||
upload =
|
||||
UploadCreator.new(
|
||||
file,
|
||||
"image_for_badge_#{badge[:id]}",
|
||||
origin: UrlHelper.absolute(image_url),
|
||||
).create_for(Discourse.system_user.id)
|
||||
|
||||
if upload.errors.count > 0 || upload&.id.blank?
|
||||
logger.error(
|
||||
"Failed to create an upload for the image of badge '#{badge[:id]}'. Error: #{upload.errors.full_messages}",
|
||||
)
|
||||
else
|
||||
DB.exec(
|
||||
"UPDATE badges SET image = NULL, image_upload_id = ? WHERE id = ?",
|
||||
upload.id,
|
||||
badge[:id],
|
||||
)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def logger
|
||||
Rails.logger
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -1,9 +1,6 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class Badge < ActiveRecord::Base
|
||||
# TODO: Drop in July 2021
|
||||
self.ignored_columns = %w[image]
|
||||
|
||||
include GlobalPath
|
||||
include HasSanitizableFields
|
||||
|
||||
|
|
|
|||
13
db/post_migrate/20240202052058_drop_badge_image_column.rb
Normal file
13
db/post_migrate/20240202052058_drop_badge_image_column.rb
Normal file
|
|
@ -0,0 +1,13 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class DropBadgeImageColumn < ActiveRecord::Migration[7.0]
|
||||
DROPPED_COLUMNS ||= { badges: %i[image] }
|
||||
|
||||
def up
|
||||
DROPPED_COLUMNS.each { |table, columns| Migration::ColumnDropper.execute_drop(table, columns) }
|
||||
end
|
||||
|
||||
def down
|
||||
raise ActiveRecord::IrreversibleMigration
|
||||
end
|
||||
end
|
||||
|
|
@ -1,58 +0,0 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
RSpec.describe Jobs::MigrateBadgeImageToUploads do
|
||||
let(:image_url) { "https://omg.aws.somestack/test.png" }
|
||||
let(:badge) { Fabricate(:badge) }
|
||||
|
||||
before do
|
||||
@orig_logger = Rails.logger
|
||||
Rails.logger = @fake_logger = FakeLogger.new
|
||||
end
|
||||
|
||||
after { Rails.logger = @orig_logger }
|
||||
|
||||
it "should migrate to the new badge `image_upload_id` column correctly" do
|
||||
stub_request(:get, image_url).to_return(
|
||||
status: 200,
|
||||
body: file_from_fixtures("smallest.png").read,
|
||||
)
|
||||
DB.exec(<<~SQL, flair_url: image_url, id: badge.id)
|
||||
UPDATE badges SET image = :flair_url WHERE id = :id
|
||||
SQL
|
||||
|
||||
expect do described_class.new.execute_onceoff({}) end.to change { Upload.count }.by(1)
|
||||
|
||||
badge.reload
|
||||
upload = Upload.last
|
||||
expect(badge.image_upload).to eq(upload)
|
||||
expect(badge.image_url).to eq(upload.url)
|
||||
expect(badge[:image]).to eq(nil)
|
||||
end
|
||||
|
||||
it "should skip badges with invalid flair URLs" do
|
||||
DB.exec("UPDATE badges SET image = 'abc' WHERE id = ?", badge.id)
|
||||
described_class.new.execute_onceoff({})
|
||||
expect(@fake_logger.warnings.count).to eq(0)
|
||||
expect(@fake_logger.errors.count).to eq(0)
|
||||
end
|
||||
|
||||
# this case has a couple of hacks that are needed to test this behavior, so if it
|
||||
# starts failing randomly in the future, I'd just delete it and not bother with it
|
||||
it "should not keep retrying forever if download fails" do
|
||||
stub_request(:get, image_url).to_return(status: 403)
|
||||
instance = described_class.new
|
||||
instance.expects(:sleep).times(2)
|
||||
|
||||
DB.exec(<<~SQL, flair_url: image_url, id: badge.id)
|
||||
UPDATE badges SET image = :flair_url WHERE id = :id
|
||||
SQL
|
||||
|
||||
expect do instance.execute_onceoff({}) end.not_to change { Upload.count }
|
||||
|
||||
badge.reload
|
||||
expect(badge.image_upload).to eq(nil)
|
||||
expect(badge.image_url).to eq(nil)
|
||||
expect(Badge.where(id: badge.id).select(:image).first[:image]).to eq(image_url)
|
||||
expect(@fake_logger.warnings.count).to eq(3)
|
||||
end
|
||||
end
|
||||
Loading…
Reference in New Issue
Block a user