FIX: themes:update rake task not rolling back transaction on error (#26750)

This commit fixes a bug in the `themes:update` rake task which resulted
in the ActiveRecord transaction not being rolled back when an error was
encountered. The transaction was first introduced in
7f0682f4f2 which changed a `begin..rescue`
block to `transaction do..rescue`. The problem with that change
prevented the transaction from ever rolling back as the code block
looks something like this:

```
transaction do
  begin
    update_theme
  rescue => e
    # surpress error
  end
end
```

From the transaction's point of view now, it will never rollback even if
an error was encountered when updating the remote theme because it will
never see the error.

Instead we should have done something like this if we wanted to surpress
the errors encountered while still ensuring that the transaction is
rolled back.

```
begin
  transaction do
    update_theme
  end
rescue => e
  # surpress error
end
```
This commit is contained in:
Alan Guo Xiang Tan 2024-04-25 15:19:23 +08:00 committed by GitHub
parent 9b829216b2
commit 35bc27a36d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 78 additions and 14 deletions

View File

@ -62,26 +62,28 @@ def update_themes
.includes(:remote_theme)
.where(enabled: true, auto_update: true)
.find_each do |theme|
theme.transaction do
remote_theme = theme.remote_theme
next if remote_theme.blank? || remote_theme.remote_url.blank?
begin
theme.transaction do
remote_theme = theme.remote_theme
next if remote_theme.blank? || remote_theme.remote_url.blank?
print "Checking '#{theme.name}' for '#{RailsMultisite::ConnectionManagement.current_db}'... "
print "Checking '#{theme.name}' for '#{RailsMultisite::ConnectionManagement.current_db}'... "
remote_theme.update_remote_version
remote_theme.update_remote_version
if remote_theme.out_of_date?
puts "updating from #{remote_theme.local_version[0..7]} to #{remote_theme.remote_version[0..7]}"
remote_theme.update_from_remote(already_in_transaction: true)
else
puts "up to date"
end
if remote_theme.out_of_date?
puts "updating from #{remote_theme.local_version[0..7]} to #{remote_theme.remote_version[0..7]}"
remote_theme.update_from_remote(already_in_transaction: true)
else
puts "up to date"
end
if remote_theme.last_error_text.present?
raise RemoteTheme::ImportError.new(remote_theme.last_error_text)
if remote_theme.last_error_text.present?
raise RemoteTheme::ImportError.new(remote_theme.last_error_text)
end
end
rescue => e
STDERR.puts "[#{RailsMultisite::ConnectionManagement.current_db}] Failed to update '#{theme.name}' (#{theme.id}): #{e}"
$stderr.puts "[#{RailsMultisite::ConnectionManagement.current_db}] Failed to update '#{theme.name}' (#{theme.id}): #{e}"
raise if ENV["RAISE_THEME_ERRORS"] == "1"
end
end

62
spec/tasks/themes_spec.rb Normal file
View File

@ -0,0 +1,62 @@
# frozen_string_literal: true
RSpec.describe "tasks/themes" do
before do
Rake::Task.clear
Discourse::Application.load_tasks
end
describe "themes:update" do
let(:initial_repo) do
about_json = <<~JSON
{
"name": "awesome theme",
"about_url": "https://www.site.com/about",
"license_url": "https://www.site.com/license",
"theme_version": "1.0",
"minimum_discourse_version": "1.0.0"
}
JSON
setup_git_repo("about.json" => about_json, "common/header.html" => "I AM A HEADER")
end
let(:initial_repo_url) do
MockGitImporter.register("https://example.com/initial_repo.git", initial_repo)
end
let!(:theme) { RemoteTheme.import_theme(initial_repo_url) }
around(:each) { |group| MockGitImporter.with_mock { group.run } }
after { `rm -fr #{initial_repo}` }
it "should not update the theme if a theme setting migration fails during the update" do
migration_content = <<~JS
export default function migrate(settings) {
throw "error";
}
JS
add_to_git_repo(
initial_repo,
"migrations/settings/0001-a-migration.js" => migration_content,
"common/header.html" => "I AM UPDATED HEADER",
)
original_remote_version = theme.remote_theme.remote_version
original_local_version = theme.remote_theme.local_version
capture_stderr { capture_stdout { Rake::Task["themes:update"].invoke } }
theme.reload
expect(theme.theme_fields.count).to eq(1)
expect(theme.theme_fields.where(name: "header").first.value).to eq("I AM A HEADER")
expect(theme.theme_settings_migrations.count).to eq(0)
expect(theme.remote_theme.commits_behind).to eq(0)
expect(theme.remote_theme.remote_version).to eq(original_remote_version)
expect(theme.remote_theme.local_version).to eq(original_local_version)
end
end
end