From e071b74a79d49900dbf6f4d74c44634c3dae8ffe Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Fri, 2 Feb 2024 14:09:55 +0800 Subject: [PATCH] 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. --- .../onceoff/migrate_badge_image_to_uploads.rb | 98 ------------------- app/models/badge.rb | 3 - .../20240202052058_drop_badge_image_column.rb | 13 +++ .../migrate_badge_image_to_uploads_spec.rb | 58 ----------- 4 files changed, 13 insertions(+), 159 deletions(-) delete mode 100644 app/jobs/onceoff/migrate_badge_image_to_uploads.rb create mode 100644 db/post_migrate/20240202052058_drop_badge_image_column.rb delete mode 100644 spec/jobs/migrate_badge_image_to_uploads_spec.rb diff --git a/app/jobs/onceoff/migrate_badge_image_to_uploads.rb b/app/jobs/onceoff/migrate_badge_image_to_uploads.rb deleted file mode 100644 index b45595498ff..00000000000 --- a/app/jobs/onceoff/migrate_badge_image_to_uploads.rb +++ /dev/null @@ -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 diff --git a/app/models/badge.rb b/app/models/badge.rb index a4688b48464..4ee2257d781 100644 --- a/app/models/badge.rb +++ b/app/models/badge.rb @@ -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 diff --git a/db/post_migrate/20240202052058_drop_badge_image_column.rb b/db/post_migrate/20240202052058_drop_badge_image_column.rb new file mode 100644 index 00000000000..20cc8bbedbb --- /dev/null +++ b/db/post_migrate/20240202052058_drop_badge_image_column.rb @@ -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 diff --git a/spec/jobs/migrate_badge_image_to_uploads_spec.rb b/spec/jobs/migrate_badge_image_to_uploads_spec.rb deleted file mode 100644 index 21c39338ea8..00000000000 --- a/spec/jobs/migrate_badge_image_to_uploads_spec.rb +++ /dev/null @@ -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