diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb index 705132890f5..e530540fab8 100644 --- a/app/controllers/admin/themes_controller.rb +++ b/app/controllers/admin/themes_controller.rb @@ -160,6 +160,8 @@ class Admin::ThemesController < Admin::AdminController render_json_error I18n.t("themes.import_error.unknown_file_type"), status: :unprocessable_entity end + rescue Theme::SettingsMigrationError => err + render_json_error err.message end def index @@ -226,10 +228,14 @@ class Admin::ThemesController < Admin::AdminController @theme.remote_theme.update_remote_version if params[:theme][:remote_check] - @theme.remote_theme.update_from_remote if params[:theme][:remote_update] + if params[:theme][:remote_update] + @theme.remote_theme.update_from_remote(raise_if_theme_save_fails: false) + else + @theme.save + end respond_to do |format| - if @theme.save + if @theme.errors.blank? update_default_theme @theme = Theme.include_relations.find(@theme.id) @@ -253,6 +259,8 @@ class Admin::ThemesController < Admin::AdminController end rescue RemoteTheme::ImportError => e render_json_error e.message + rescue Theme::SettingsMigrationError => e + render_json_error e.message end def destroy diff --git a/app/models/remote_theme.rb b/app/models/remote_theme.rb index 8f62a0a1aa4..32db3d99ca4 100644 --- a/app/models/remote_theme.rb +++ b/app/models/remote_theme.rb @@ -110,30 +110,33 @@ class RemoteTheme < ActiveRecord::Base remote_theme = new remote_theme.theme = theme remote_theme.remote_url = "" - remote_theme.update_from_remote(importer, skip_update: true) - theme.save! + do_update_child_components = false + theme.transaction do + remote_theme.update_from_remote(importer, skip_update: true, already_in_transaction: true) - if existing && update_components.present? && update_components != "none" - child_components = child_components.map { |url| ThemeStore::GitImporter.new(url.strip).url } + if existing && update_components.present? && update_components != "none" + child_components = child_components.map { |url| ThemeStore::GitImporter.new(url.strip).url } - if update_components == "sync" - ChildTheme - .joins(child_theme: :remote_theme) - .where("remote_themes.remote_url NOT IN (?)", child_components) - .delete_all + if update_components == "sync" + ChildTheme + .joins(child_theme: :remote_theme) + .where("remote_themes.remote_url NOT IN (?)", child_components) + .delete_all + end + + child_components -= + theme + .child_themes + .joins(:remote_theme) + .where("remote_themes.remote_url IN (?)", child_components) + .pluck("remote_themes.remote_url") + theme.child_components = child_components + do_update_child_components = true end - - child_components -= - theme - .child_themes - .joins(:remote_theme) - .where("remote_themes.remote_url IN (?)", child_components) - .pluck("remote_themes.remote_url") - theme.child_components = child_components - theme.update_child_components end + theme.update_child_components if do_update_child_components theme ensure begin @@ -160,9 +163,9 @@ class RemoteTheme < ActiveRecord::Base remote_theme.private_key = private_key remote_theme.branch = branch remote_theme.remote_url = importer.url + remote_theme.update_from_remote(importer) - theme.save! theme ensure begin @@ -209,7 +212,12 @@ class RemoteTheme < ActiveRecord::Base end end - def update_from_remote(importer = nil, skip_update: false) + def update_from_remote( + importer = nil, + skip_update: false, + raise_if_theme_save_fails: true, + already_in_transaction: false + ) cleanup = false unless importer @@ -333,12 +341,6 @@ class RemoteTheme < ActiveRecord::Base updated_fields << theme.set_field(**opts.merge(value: value)) end - theme.convert_settings - - # Destroy fields that no longer exist in the remote theme - field_ids_to_destroy = theme.theme_fields.pluck(:id) - updated_fields.map { |tf| tf&.id } - ThemeField.where(id: field_ids_to_destroy).destroy_all - if !skip_update self.remote_updated_at = Time.zone.now self.remote_version = importer.version @@ -346,9 +348,28 @@ class RemoteTheme < ActiveRecord::Base self.commits_behind = 0 end - update_theme_color_schemes(theme, theme_info["color_schemes"]) unless theme.component + transaction_block = -> do + # Destroy fields that no longer exist in the remote theme + field_ids_to_destroy = theme.theme_fields.pluck(:id) - updated_fields.map { |tf| tf&.id } + ThemeField.where(id: field_ids_to_destroy).destroy_all + + update_theme_color_schemes(theme, theme_info["color_schemes"]) unless theme.component + + self.save! + if raise_if_theme_save_fails + theme.save! + else + raise ActiveRecord::Rollback if !theme.save + end + theme.migrate_settings(start_transaction: false) + end + + if already_in_transaction + transaction_block.call + else + self.transaction(&transaction_block) + end - self.save! self ensure begin diff --git a/app/models/theme.rb b/app/models/theme.rb index bdba5fc67a3..29d3a4566fe 100644 --- a/app/models/theme.rb +++ b/app/models/theme.rb @@ -8,6 +8,9 @@ class Theme < ActiveRecord::Base BASE_COMPILER_VERSION = 77 + class SettingsMigrationError < StandardError + end + attr_accessor :child_components def self.cache @@ -33,6 +36,7 @@ class Theme < ActiveRecord::Base through: :parent_theme_relation, source: :parent_theme has_many :color_schemes + has_many :theme_settings_migrations belongs_to :remote_theme, dependent: :destroy has_one :theme_modifier_set, dependent: :destroy has_one :theme_svg_sprite, dependent: :destroy @@ -59,6 +63,9 @@ class Theme < ActiveRecord::Base has_many :builder_theme_fields, -> { where("name IN (?)", ThemeField.scss_fields) }, class_name: "ThemeField" + has_many :migration_fields, + -> { where(target_id: Theme.targets[:migrations]) }, + class_name: "ThemeField" validate :component_validations validate :validate_theme_fields @@ -380,6 +387,7 @@ class Theme < ActiveRecord::Base extra_scss: 5, extra_js: 6, tests_js: 7, + migrations: 8, ) end @@ -828,22 +836,51 @@ class Theme < ActiveRecord::Base contents end - def convert_settings - settings.each do |setting| - setting_row = ThemeSetting.where(theme_id: self.id, name: setting.name.to_s).first + def migrate_settings(start_transaction: true) + block = -> do + runner = ThemeSettingsMigrationsRunner.new(self) + results = runner.run - if setting_row && setting_row.data_type != setting.type - if ( - setting_row.data_type == ThemeSetting.types[:list] && - setting.type == ThemeSetting.types[:string] && setting.json_schema.present? - ) - convert_list_to_json_schema(setting_row, setting) + next if results.blank? + + old_settings = self.theme_settings.pluck(:name) + self.theme_settings.destroy_all + + final_result = results.last + final_result[:settings_after].each do |key, val| + self.update_setting(key.to_sym, val) + rescue Discourse::NotFound + if old_settings.include?(key) + final_result[:settings_after].delete(key) else - Rails.logger.warn( - "Theme setting type has changed but cannot be converted. \n\n #{setting.inspect}", - ) + raise Theme::SettingsMigrationError.new( + I18n.t( + "themes.import_error.migrations.unknown_setting_returned_by_migration", + name: final_result[:original_name], + setting_name: key, + ), + ) end end + + results.each do |res| + record = + ThemeSettingsMigration.new( + theme_id: self.id, + version: res[:version], + name: res[:name], + theme_field_id: res[:theme_field_id], + ) + record.calculate_diff(res[:settings_before], res[:settings_after]) + record.save! + end + self.save! + end + + if start_transaction + self.transaction(&block) + else + block.call end end diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 36bd6948316..6be9efb600d 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -1,12 +1,17 @@ # frozen_string_literal: true class ThemeField < ActiveRecord::Base + MIGRATION_NAME_PART_MAX_LENGTH = 150 + belongs_to :upload has_one :javascript_cache, dependent: :destroy has_one :upload_reference, as: :target, dependent: :destroy + has_one :theme_settings_migration validates :value, { length: { maximum: 1024**2 } } + validate :migration_filename_is_valid, if: :migration_field? + after_save do if self.type_id == ThemeField.types[:theme_upload_var] && saved_change_to_upload_id? UploadReference.ensure_exist!(upload_ids: [self.upload_id], target: self) @@ -340,7 +345,7 @@ class ThemeField < ActiveRecord::Base types[:scss] elsif target.to_s == "extra_scss" types[:scss] - elsif target.to_s == "extra_js" + elsif %w[migrations extra_js].include?(target.to_s) types[:js] elsif target.to_s == "settings" || target.to_s == "translations" types[:yaml] @@ -394,6 +399,10 @@ class ThemeField < ActiveRecord::Base self.name == SvgSprite.theme_sprite_variable_name end + def migration_field? + Theme.targets[:migrations] == self.target_id + end + def ensure_baked! needs_baking = !self.value_baked || compiler_version != Theme.compiler_version return unless needs_baking @@ -422,6 +431,9 @@ class ThemeField < ActiveRecord::Base self.error = validate_svg_sprite_xml self.value_baked = "baked" self.compiler_version = Theme.compiler_version + elsif migration_field? + self.value_baked = "baked" + self.compiler_version = Theme.compiler_version end if self.will_save_change_to_value_baked? || self.will_save_change_to_compiler_version? || @@ -603,6 +615,13 @@ class ThemeField < ActiveRecord::Base targets: :common, canonical: ->(h) { "assets/#{h[:name]}#{File.extname(h[:filename])}" }, ), + ThemeFileMatcher.new( + regex: %r{\Amigrations/settings/(?[^/]+)\.js\z}, + names: nil, + types: :js, + targets: :migrations, + canonical: ->(h) { "migrations/settings/#{h[:name]}.js" }, + ), ] # For now just work for standard fields @@ -716,6 +735,28 @@ class ThemeField < ActiveRecord::Base true end end + + def migration_filename_is_valid + if !name.match?(/\A\d{4}-[a-zA-Z0-9]+/) + self.errors.add( + :base, + I18n.t("themes.import_error.migrations.invalid_filename", filename: name), + ) + return + end + + # the 5 here is the length of the first 4 digits and the dash that follows + # them + if name.size - 5 > MIGRATION_NAME_PART_MAX_LENGTH + self.errors.add( + :base, + I18n.t( + "themes.import_error.migrations.name_too_long", + count: MIGRATION_NAME_PART_MAX_LENGTH, + ), + ) + end + end end # == Schema Information diff --git a/app/models/theme_settings_migration.rb b/app/models/theme_settings_migration.rb new file mode 100644 index 00000000000..a93d11db670 --- /dev/null +++ b/app/models/theme_settings_migration.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +class ThemeSettingsMigration < ActiveRecord::Base + belongs_to :theme + belongs_to :theme_field + + validates :theme_id, presence: true + validates :theme_field_id, presence: true + + validates :version, presence: true + validates :name, presence: true + validates :diff, presence: true + + def calculate_diff(settings_before, settings_after) + diff = { additions: [], deletions: [] } + + before_keys = settings_before.keys + after_keys = settings_after.keys + + removed_keys = before_keys - after_keys + removed_keys.each { |key| diff[:deletions] << { key: key, val: settings_before[key] } } + + added_keys = after_keys - before_keys + added_keys.each { |key| diff[:additions] << { key: key, val: settings_after[key] } } + + common_keys = before_keys & after_keys + common_keys.each do |key| + if settings_before[key] != settings_after[key] + diff[:deletions] << { key: key, val: settings_before[key] } + diff[:additions] << { key: key, val: settings_after[key] } + end + end + + self.diff = diff + end +end + +# == Schema Information +# +# Table name: theme_settings_migrations +# +# id :bigint not null, primary key +# theme_id :integer not null +# theme_field_id :integer not null +# version :integer not null +# name :string(150) not null +# diff :json not null +# created_at :datetime not null +# +# Indexes +# +# index_theme_settings_migrations_on_theme_field_id (theme_field_id) UNIQUE +# index_theme_settings_migrations_on_theme_id_and_version (theme_id,version) UNIQUE +# diff --git a/app/services/theme_settings_migrations_runner.rb b/app/services/theme_settings_migrations_runner.rb new file mode 100644 index 00000000000..fd9c30e5f55 --- /dev/null +++ b/app/services/theme_settings_migrations_runner.rb @@ -0,0 +1,185 @@ +# frozen_string_literal: true + +class ThemeSettingsMigrationsRunner + Migration = Struct.new(:version, :name, :original_name, :code, :theme_field_id) + + MIGRATION_ENTRY_POINT_JS = <<~JS + const migrate = require("discourse/theme/migration")?.default; + function main(settingsObj) { + if (!migrate) { + throw new Error("no_exported_migration_function"); + } + if (typeof migrate !== "function") { + throw new Error("default_export_is_not_a_function"); + } + const map = new Map(Object.entries(settingsObj)); + const updatedMap = migrate(map); + if (!updatedMap) { + throw new Error("migration_function_no_returned_value"); + } + if (!(updatedMap instanceof Map)) { + throw new Error("migration_function_wrong_return_type"); + } + return Object.fromEntries(updatedMap.entries()); + } + JS + + private_constant :Migration, :MIGRATION_ENTRY_POINT_JS + + def self.loader_js_lib_content + @loader_js_lib_content ||= + File.read( + File.join( + Rails.root, + "app/assets/javascripts/node_modules/loader.js/dist/loader/loader.js", + ), + ) + end + + def initialize(theme, limit: 100, timeout: 100, memory: 2.megabytes) + @theme = theme + @limit = limit + @timeout = timeout + @memory = memory + end + + def run + fields = lookup_pending_migrations_fields + + count = fields.count + return [] if count == 0 + + raise_error("themes.import_error.migrations.too_many_pending_migrations") if count > @limit + + migrations = convert_fields_to_migrations(fields) + migrations.sort_by!(&:version) + + current_migration_version = + @theme.theme_settings_migrations.order(version: :desc).pick(:version) + current_migration_version ||= -Float::INFINITY + + current_settings = lookup_overriden_settings + + migrations.map do |migration| + if migration.version <= current_migration_version + raise_error( + "themes.import_error.migrations.out_of_sequence", + name: migration.original_name, + current: current_migration_version, + ) + end + + migrated_settings = execute(migration, current_settings) + results = { + version: migration.version, + name: migration.name, + original_name: migration.original_name, + theme_field_id: migration.theme_field_id, + settings_before: current_settings, + settings_after: migrated_settings, + } + current_settings = migrated_settings + current_migration_version = migration.version + results + rescue DiscourseJsProcessor::TranspileError => error + raise_error( + "themes.import_error.migrations.syntax_error", + name: migration.original_name, + error: error.message, + ) + rescue MiniRacer::V8OutOfMemoryError + raise_error( + "themes.import_error.migrations.exceeded_memory_limit", + name: migration.original_name, + ) + rescue MiniRacer::ScriptTerminatedError + raise_error("themes.import_error.migrations.timed_out", name: migration.original_name) + rescue MiniRacer::RuntimeError => error + message = error.message + if message.include?("no_exported_migration_function") + raise_error( + "themes.import_error.migrations.no_exported_function", + name: migration.original_name, + ) + elsif message.include?("default_export_is_not_a_function") + raise_error( + "themes.import_error.migrations.default_export_not_a_function", + name: migration.original_name, + ) + elsif message.include?("migration_function_no_returned_value") + raise_error( + "themes.import_error.migrations.no_returned_value", + name: migration.original_name, + ) + elsif message.include?("migration_function_wrong_return_type") + raise_error( + "themes.import_error.migrations.wrong_return_type", + name: migration.original_name, + ) + else + raise_error( + "themes.import_error.migrations.runtime_error", + name: migration.original_name, + error: message, + ) + end + end + end + + private + + def lookup_pending_migrations_fields + @theme + .migration_fields + .left_joins(:theme_settings_migration) + .where(theme_settings_migration: { id: nil }) + end + + def convert_fields_to_migrations(fields) + fields.map do |field| + match_data = /\A(?\d{4})-(?.+)/.match(field.name) + + if !match_data + raise_error("themes.import_error.migrations.invalid_filename", filename: field.name) + end + + version = match_data[:version].to_i + name = match_data[:name] + original_name = field.name + + Migration.new( + version: version, + name: name, + original_name: original_name, + code: field.value, + theme_field_id: field.id, + ) + end + end + + def lookup_overriden_settings + hash = {} + @theme.theme_settings.each { |row| hash[row.name] = ThemeSettingsManager.cast_row_value(row) } + hash + end + + def execute(migration, settings) + context = MiniRacer::Context.new(timeout: @timeout, max_memory: @memory) + + context.eval(self.class.loader_js_lib_content, filename: "loader.js") + + context.eval( + DiscourseJsProcessor.transpile(migration.code, "", "discourse/theme/migration"), + filename: "theme-#{@theme.id}-migration.js", + ) + + context.eval(MIGRATION_ENTRY_POINT_JS, filename: "migration-entrypoint.js") + context.call("main", settings) + ensure + context&.dispose + end + + def raise_error(message_key, **i18n_args) + raise Theme::SettingsMigrationError.new(I18n.t(message_key, **i18n_args)) + end +end diff --git a/app/services/themes_install_task.rb b/app/services/themes_install_task.rb index 3eaa931944b..3ce8c50b8e9 100644 --- a/app/services/themes_install_task.rb +++ b/app/services/themes_install_task.rb @@ -74,8 +74,7 @@ class ThemesInstallTask end def update - @remote_theme.update_from_remote - @theme.save + @remote_theme.update_from_remote(raise_if_theme_save_fails: false) add_component_to_all_themes @remote_theme.last_error_text.nil? end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index e3b7b754de0..035425934a4 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -92,6 +92,22 @@ en: not_allowed_theme: "`%{repo}` is not in the list of allowed themes (check `allowed_theme_repos` global setting)." ssh_key_gone: "You waited too long to install the theme and SSH key expired. Please try again." too_many_files: "The number of files (%{count}) in the theme has exceeded the maximum allowed number of files (%{limit})" + migrations: + invalid_filename: "Invalid filename for migration file: %{filename}. Filenames must begin with 4 digits followed by a hyphen and then a name that only contains alphanumeric characters with hyphens." + name_too_long: + one: "Migration name is too long. It shouldn't exceed %{count} character." + other: "Migration name is too long. It shouldn't exceed %{count} characters." + too_many_pending_migrations: "There are too many pending migrations in this theme. Themes are not allowed to introduce more than 100 migrations in a single update" + out_of_sequence: "Migration '%{name}' is out of sequence. The last migration for this theme had version number %{current} which is higher than the new migration" + syntax_error: "Failed to run migration '%{name}' because it has a syntax error: %{error}" + exceeded_memory_limit: "Migration '%{name}' failed because it exceeded the memory limit" + timed_out: "Migration '%{name}' timed out" + no_exported_function: "Migration '%{name}' doesn't export a function to perform the migration" + default_export_not_a_function: "Migration '%{name}' has a default export that's not a function. The default export must be the function that performs the migration" + no_returned_value: "Migration '%{name}' didn't return any value (or returned null or undefined). It must return a Map object" + wrong_return_type: "Migration '%{name}' returned an unknown data type. It must return a Map object" + runtime_error: "Migration '%{name}' encountered the following runtime error: %{error}" + unknown_setting_returned_by_migration: "Migrations '%{name}' returned a setting '%{setting_name}' which is not declared in the theme's settings.yml file" errors: component_no_user_selectable: "Theme components can't be user-selectable" component_no_default: "Theme components can't be default theme" diff --git a/db/migrate/20230907225057_create_theme_settings_migrations.rb b/db/migrate/20230907225057_create_theme_settings_migrations.rb new file mode 100644 index 00000000000..491c4cdaf2e --- /dev/null +++ b/db/migrate/20230907225057_create_theme_settings_migrations.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class CreateThemeSettingsMigrations < ActiveRecord::Migration[7.0] + def change + create_table :theme_settings_migrations do |t| + t.integer :theme_id, null: false + t.integer :theme_field_id, null: false + t.integer :version, null: false + t.string :name, null: false, limit: 150 + t.json :diff, null: false + t.datetime :created_at, null: false + end + + add_index :theme_settings_migrations, %i[theme_id version], unique: true + add_index :theme_settings_migrations, :theme_field_id, unique: true + end +end diff --git a/lib/tasks/themes.rake b/lib/tasks/themes.rake index 9fb8cfec0b1..e1fc052ac54 100644 --- a/lib/tasks/themes.rake +++ b/lib/tasks/themes.rake @@ -70,8 +70,7 @@ def update_themes 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 - theme.save! + remote_theme.update_from_remote(already_in_transaction: true) else puts "up to date" end diff --git a/lib/theme_settings_manager.rb b/lib/theme_settings_manager.rb index 7a5f987ab8e..88e2ea32831 100644 --- a/lib/theme_settings_manager.rb +++ b/lib/theme_settings_manager.rb @@ -7,12 +7,22 @@ class ThemeSettingsManager ThemeSetting.types end + def self.cast_row_value(row) + type_name = self.types.invert[row.data_type].downcase.capitalize + klass = "ThemeSettingsManager::#{type_name}".constantize + klass.cast(row.value) + end + def self.create(name, default, type, theme, opts = {}) type_name = self.types.invert[type].downcase.capitalize klass = "ThemeSettingsManager::#{type_name}".constantize klass.new(name, default, theme, opts) end + def self.cast(value) + value + end + def initialize(name, default, theme, opts = {}) @name = name.to_sym @default = default @@ -128,23 +138,31 @@ class ThemeSettingsManager end class Bool < self + def self.cast(value) + [true, "true"].include?(value) + end + def value - [true, "true"].include?(super) + self.class.cast(super) end def value=(new_value) - new_value = ([true, "true"].include?(new_value)).to_s + new_value = (self.class.cast(new_value)).to_s super(new_value) end end class Integer < self + def self.cast(value) + value.to_i + end + def value - super.to_i + self.class.cast(super) end def value=(new_value) - super(new_value.to_i) + super(self.class.cast(new_value)) end def is_valid_value?(new_value) @@ -153,12 +171,16 @@ class ThemeSettingsManager end class Float < self + def self.cast(value) + value.to_f + end + def value - super.to_f + self.class.cast(super) end def value=(new_value) - super(new_value.to_f) + super(self.class.cast(new_value)) end def is_valid_value?(new_value) diff --git a/spec/fabricators/theme_field_fabricator.rb b/spec/fabricators/theme_field_fabricator.rb index 688cfc58306..095a95744dc 100644 --- a/spec/fabricators/theme_field_fabricator.rb +++ b/spec/fabricators/theme_field_fabricator.rb @@ -7,3 +7,24 @@ Fabricator(:theme_field) do value { ".test {color: blue;}" } error { nil } end + +Fabricator(:migration_theme_field, from: :theme_field) do + transient :version + type_id ThemeField.types[:js] + target_id Theme.targets[:migrations] + name do |attrs| + version = attrs[:version] || sequence("theme_#{attrs[:theme].id}_migration_field", 1) + "#{version.to_s.rjust(4, "0")}-some-name" + end + value <<~JS + export default function migrate(settings) { + return settings; + } + JS +end + +Fabricator(:settings_theme_field, from: :theme_field) do + type_id ThemeField.types[:yaml] + target_id Theme.targets[:settings] + name "yaml" +end diff --git a/spec/fabricators/theme_settings_migration_fabricator.rb b/spec/fabricators/theme_settings_migration_fabricator.rb new file mode 100644 index 00000000000..afb6e1fdd44 --- /dev/null +++ b/spec/fabricators/theme_settings_migration_fabricator.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +Fabricator(:theme_settings_migration) do + theme + theme_field + version { |attrs| sequence("theme_#{attrs[:theme].id}_migrations", 1) } + name { |attrs| "migration-n-#{attrs[:version]}" } + diff { { "additions" => [], "deletions" => [] } } +end diff --git a/spec/lib/theme_store/zip_exporter_spec.rb b/spec/lib/theme_store/zip_exporter_spec.rb index 5c885f8ee86..ff59893325c 100644 --- a/spec/lib/theme_store/zip_exporter_spec.rb +++ b/spec/lib/theme_store/zip_exporter_spec.rb @@ -29,6 +29,14 @@ RSpec.describe ThemeStore::ZipExporter do upload_id: upload.id, type: :theme_upload_var, ) + + theme.set_field(target: :migrations, name: "0201-some-migration", type: :js, value: <<~JS) + export default function migrate(settings) { + settings.set("aa", 1); + return settings; + } + JS + theme.build_remote_theme( remote_url: "", about_url: "abouturl", @@ -94,7 +102,14 @@ RSpec.describe ThemeStore::ZipExporter do `rm #{file}` folders = Dir.glob("**/*").reject { |f| File.file?(f) } - expect(folders).to contain_exactly("assets", "common", "locales", "mobile") + expect(folders).to contain_exactly( + "assets", + "common", + "locales", + "mobile", + "migrations", + "migrations/settings", + ) files = Dir.glob("**/*").reject { |f| File.directory?(f) } expect(files).to contain_exactly( @@ -105,6 +120,7 @@ RSpec.describe ThemeStore::ZipExporter do "locales/en.yml", "mobile/mobile.scss", "settings.yml", + "migrations/settings/0201-some-migration.js", ) expect(JSON.parse(File.read("about.json")).deep_symbolize_keys).to eq( @@ -145,6 +161,12 @@ RSpec.describe ThemeStore::ZipExporter do expect(File.read("locales/en.yml")).to eq( { en: { key: "value" } }.deep_stringify_keys.to_yaml, ) + expect(File.read("migrations/settings/0201-some-migration.js")).to eq(<<~JS) + export default function migrate(settings) { + settings.set("aa", 1); + return settings; + } + JS theme.update!(name: "Discourse Header Icons") exporter = ThemeStore::ZipExporter.new(theme) diff --git a/spec/models/remote_theme_spec.rb b/spec/models/remote_theme_spec.rb index 93c7dd2b700..81b7fa47fcb 100644 --- a/spec/models/remote_theme_spec.rb +++ b/spec/models/remote_theme_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.describe RemoteTheme do - describe "#import_remote" do + describe "#import_theme" do def about_json( love_color: "FAFAFA", tertiary_low_color: "FFFFFF", @@ -35,6 +35,12 @@ RSpec.describe RemoteTheme do "@font-face { font-family: magic; src: url($font)}; body {color: $color; content: $name;}" end + let(:migration_js) { <<~JS } + export default function migrate(settings) { + return settings; + } + JS + let :initial_repo do setup_git_repo( "about.json" => about_json, @@ -51,6 +57,7 @@ RSpec.describe RemoteTheme do "assets/font.woff2" => "FAKE FONT", "settings.yaml" => "boolean_setting: true", "locales/en.yml" => "sometranslations", + "migrations/settings/0001-some-migration.js" => migration_js, ) end @@ -62,14 +69,102 @@ RSpec.describe RemoteTheme do around(:each) { |group| MockGitImporter.with_mock { group.run } } + it "run pending theme settings migrations" do + add_to_git_repo(initial_repo, "migrations/settings/0002-another-migration.js" => <<~JS) + export default function migrate(settings) { + settings.set("boolean_setting", false); + return settings; + } + JS + theme = RemoteTheme.import_theme(initial_repo_url) + migrations = theme.theme_settings_migrations.order(:version) + + expect(migrations.size).to eq(2) + + first_migration = migrations[0] + second_migration = migrations[1] + + expect(first_migration.version).to eq(1) + expect(second_migration.version).to eq(2) + + expect(first_migration.name).to eq("some-migration") + expect(second_migration.name).to eq("another-migration") + + expect(first_migration.diff).to eq("additions" => [], "deletions" => []) + expect(second_migration.diff).to eq( + "additions" => [{ "key" => "boolean_setting", "val" => false }], + "deletions" => [], + ) + + expect(theme.get_setting(:boolean_setting)).to eq(false) + + expect(first_migration.theme_field.value).to eq(<<~JS) + export default function migrate(settings) { + return settings; + } + JS + expect(second_migration.theme_field.value).to eq(<<~JS) + export default function migrate(settings) { + settings.set("boolean_setting", false); + return settings; + } + JS + end + + it "doesn't create theme if a migration fails" do + add_to_git_repo(initial_repo, "migrations/settings/0002-another-migration.js" => <<~JS) + export default function migrate(s) { + return null; + } + JS + expect do RemoteTheme.import_theme(initial_repo_url) end.to raise_error( + Theme::SettingsMigrationError, + ).and not_change(Theme, :count).and not_change(RemoteTheme, :count) + end + + it "doesn't partially update the theme when a migration fails" do + theme = RemoteTheme.import_theme(initial_repo_url) + + add_to_git_repo( + initial_repo, + "about.json" => + JSON + .parse(about_json(about_url: "https://updated.site.com")) + .tap { |h| h[:component] = true } + .to_json, + "stylesheets/file.scss" => ".class3 { color: green; }", + "common/header.html" => "I AM UPDATED HEADER", + "migrations/settings/0002-new-failing-migration.js" => <<~JS, + export default function migrate(settings) { + null.toString(); + return settings; + } + JS + ) + + expect do theme.remote_theme.update_from_remote end.to raise_error( + Theme::SettingsMigrationError, + ) + + theme.reload + + expect(theme.component).to eq(false) + expect(theme.remote_theme.about_url).to eq("https://www.site.com/about") + + expect(theme.theme_fields.find_by(name: "header").value).to eq("I AM HEADER") + expect( + theme.theme_fields.find_by(type_id: ThemeField.types[:scss], name: "file").value, + ).to eq(".class1{color:red}") + end + it "can correctly import a remote theme" do time = Time.new("2000") freeze_time time - @theme = RemoteTheme.import_theme(initial_repo_url) - remote = @theme.remote_theme + theme = RemoteTheme.import_theme(initial_repo_url) + remote = theme.remote_theme - expect(@theme.name).to eq("awesome theme") + expect(theme.name).to eq("awesome theme") expect(remote.remote_url).to eq(initial_repo_url) expect(remote.remote_version).to eq(`cd #{initial_repo} && git rev-parse HEAD`.strip) expect(remote.local_version).to eq(`cd #{initial_repo} && git rev-parse HEAD`.strip) @@ -79,11 +174,11 @@ RSpec.describe RemoteTheme do expect(remote.theme_version).to eq("1.0") expect(remote.minimum_discourse_version).to eq("1.0.0") - expect(@theme.theme_modifier_set.serialize_topic_excerpts).to eq(true) + expect(theme.theme_modifier_set.serialize_topic_excerpts).to eq(true) - expect(@theme.theme_fields.length).to eq(11) + expect(theme.theme_fields.length).to eq(12) - mapped = Hash[*@theme.theme_fields.map { |f| ["#{f.target_id}-#{f.name}", f.value] }.flatten] + mapped = Hash[*theme.theme_fields.map { |f| ["#{f.target_id}-#{f.name}", f.value] }.flatten] expect(mapped["0-header"]).to eq("I AM HEADER") expect(mapped["1-scss"]).to eq(scss_data) @@ -96,21 +191,24 @@ RSpec.describe RemoteTheme do expect(mapped["4-en"]).to eq("sometranslations") expect(mapped["7-acceptance/theme-test.js"]).to eq("assert.ok(true);") + expect(mapped["8-0001-some-migration"]).to eq( + "export default function migrate(settings) {\n return settings;\n}\n", + ) - expect(mapped.length).to eq(11) + expect(mapped.length).to eq(12) - expect(@theme.settings.length).to eq(1) - expect(@theme.settings.first.value).to eq(true) + expect(theme.settings.length).to eq(1) + expect(theme.settings.first.value).to eq(true) expect(remote.remote_updated_at).to eq_time(time) - scheme = ColorScheme.find_by(theme_id: @theme.id) + scheme = ColorScheme.find_by(theme_id: theme.id) expect(scheme.name).to eq("Amazing") expect(scheme.colors.find_by(name: "love").hex).to eq("fafafa") expect(scheme.colors.find_by(name: "tertiary-low").hex).to eq("ffffff") - expect(@theme.color_scheme_id).to eq(scheme.id) - @theme.update(color_scheme_id: nil) + expect(theme.color_scheme_id).to eq(scheme.id) + theme.update(color_scheme_id: nil) File.write("#{initial_repo}/common/header.html", "I AM UPDATED") File.write( @@ -133,15 +231,14 @@ RSpec.describe RemoteTheme do expect(remote.remote_version).to eq(`cd #{initial_repo} && git rev-parse HEAD`.strip) remote.update_from_remote - @theme.save! - @theme.reload + theme.reload - scheme = ColorScheme.find_by(theme_id: @theme.id) + scheme = ColorScheme.find_by(theme_id: theme.id) expect(scheme.name).to eq("Amazing") expect(scheme.colors.find_by(name: "love").hex).to eq("eaeaea") - expect(@theme.color_scheme_id).to eq(nil) # Should only be set on first import + expect(theme.color_scheme_id).to eq(nil) # Should only be set on first import - mapped = Hash[*@theme.theme_fields.map { |f| ["#{f.target_id}-#{f.name}", f.value] }.flatten] + mapped = Hash[*theme.theme_fields.map { |f| ["#{f.target_id}-#{f.name}", f.value] }.flatten] # Scss file was deleted expect(mapped["5-file"]).to eq(nil) @@ -149,8 +246,8 @@ RSpec.describe RemoteTheme do expect(mapped["0-header"]).to eq("I AM UPDATED") expect(mapped["1-scss"]).to eq(scss_data) - expect(@theme.settings.length).to eq(1) - expect(@theme.settings.first.value).to eq(32) + expect(theme.settings.length).to eq(1) + expect(theme.settings.first.value).to eq(32) expect(remote.remote_updated_at).to eq_time(time) expect(remote.about_url).to eq("https://newsite.com/about") @@ -163,13 +260,12 @@ RSpec.describe RemoteTheme do `cd #{initial_repo} && git commit -am "update"` remote.update_from_remote - @theme.save - @theme.reload + theme.reload - scheme_count = ColorScheme.where(theme_id: @theme.id).count + scheme_count = ColorScheme.where(theme_id: theme.id).count expect(scheme_count).to eq(1) - scheme = ColorScheme.find_by(theme_id: @theme.id) + scheme = ColorScheme.find_by(theme_id: theme.id) expect(scheme.colors.find_by(name: "tertiary_low_color")).to eq(nil) end @@ -196,11 +292,44 @@ RSpec.describe RemoteTheme do expect(remote.reload.commits_behind).to eq(-1) end + it "runs only new migrations when updating a theme" do + add_to_git_repo(initial_repo, "settings.yaml" => <<~YAML) + first_integer_setting: 1 + second_integer_setting: 2 + YAML + add_to_git_repo(initial_repo, "migrations/settings/0002-another-migration.js" => <<~JS) + export default function migrate(settings) { + settings.set("first_integer_setting", 101); + return settings; + } + JS + + theme = RemoteTheme.import_theme(initial_repo_url) + + expect(theme.get_setting(:first_integer_setting)).to eq(101) + expect(theme.get_setting(:second_integer_setting)).to eq(2) + + theme.update_setting(:first_integer_setting, 110) + + add_to_git_repo(initial_repo, "migrations/settings/0003-yet-another-migration.js" => <<~JS) + export default function migrate(settings) { + settings.set("second_integer_setting", 201); + return settings; + } + JS + + theme.remote_theme.update_from_remote + theme.reload + + expect(theme.get_setting(:first_integer_setting)).to eq(110) + expect(theme.get_setting(:second_integer_setting)).to eq(201) + end + it "fails if theme has too many files" do stub_const(RemoteTheme, "MAX_THEME_FILE_COUNT", 1) do expect { RemoteTheme.import_theme(initial_repo_url) }.to raise_error( RemoteTheme::ImportError, - I18n.t("themes.import_error.too_many_files", count: 14, limit: 1), + I18n.t("themes.import_error.too_many_files", count: 15, limit: 1), ) end end diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index 020b24e793d..90fd36c5dde 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -775,4 +775,67 @@ HTML expect(theme.scss_variables).not_to include("theme_uploads") end end + + describe "migration JavaScript field" do + it "must match a specific format for filename" do + field = Fabricate(:migration_theme_field, theme: theme) + field.name = "12-some-name" + + expect(field.valid?).to eq(false) + expect(field.errors.full_messages).to contain_exactly( + I18n.t("themes.import_error.migrations.invalid_filename", filename: "12-some-name"), + ) + + field.name = "00012-some-name" + + expect(field.valid?).to eq(false) + expect(field.errors.full_messages).to contain_exactly( + I18n.t("themes.import_error.migrations.invalid_filename", filename: "00012-some-name"), + ) + + field.name = "0012some-name" + + expect(field.valid?).to eq(false) + expect(field.errors.full_messages).to contain_exactly( + I18n.t("themes.import_error.migrations.invalid_filename", filename: "0012some-name"), + ) + + field.name = "0012" + + expect(field.valid?).to eq(false) + expect(field.errors.full_messages).to contain_exactly( + I18n.t("themes.import_error.migrations.invalid_filename", filename: "0012"), + ) + + field.name = "0012-something" + + expect(field.valid?).to eq(true) + end + + it "doesn't allow weird characters in the name" do + field = Fabricate(:migration_theme_field, theme: theme) + field.name = "0012-ëèard" + + expect(field.valid?).to eq(false) + expect(field.errors.full_messages).to contain_exactly( + I18n.t("themes.import_error.migrations.invalid_filename", filename: "0012-ëèard"), + ) + end + + it "imposes a limit on the name part in the filename" do + stub_const(ThemeField, "MIGRATION_NAME_PART_MAX_LENGTH", 10) do + field = Fabricate(:migration_theme_field, theme: theme) + field.name = "0012-#{"a" * 11}" + + expect(field.valid?).to eq(false) + expect(field.errors.full_messages).to contain_exactly( + I18n.t("themes.import_error.migrations.name_too_long", count: 10), + ) + + field.name = "0012-#{"a" * 10}" + + expect(field.valid?).to eq(true) + end + end + end end diff --git a/spec/models/theme_settings_migration_spec.rb b/spec/models/theme_settings_migration_spec.rb new file mode 100644 index 00000000000..8026a1c1f95 --- /dev/null +++ b/spec/models/theme_settings_migration_spec.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +RSpec.describe ThemeSettingsMigration do + describe "Validations" do + subject(:badge) { Fabricate.build(:theme_settings_migration) } + + it { is_expected.to validate_presence_of(:theme_id) } + it { is_expected.to validate_presence_of(:theme_field_id) } + + it { is_expected.to validate_presence_of(:version) } + it { is_expected.to validate_presence_of(:name) } + it { is_expected.to validate_presence_of(:diff) } + end +end diff --git a/spec/models/theme_spec.rb b/spec/models/theme_spec.rb index 27498852276..955f89dc263 100644 --- a/spec/models/theme_spec.rb +++ b/spec/models/theme_spec.rb @@ -12,7 +12,7 @@ RSpec.describe Theme do let(:guardian) { Guardian.new(user) } - let(:theme) { Fabricate(:theme, user: user) } + fab!(:theme) { Fabricate(:theme, user: user) } let(:child) { Fabricate(:theme, user: user, component: true) } it "can properly clean up color schemes" do @@ -440,9 +440,8 @@ HTML end it "correctly caches theme ids" do - Theme.destroy_all + Theme.where.not(id: theme.id).destroy_all - theme theme2 = Fabricate(:theme) expect(Theme.theme_ids).to contain_exactly(theme.id, theme2.id) @@ -562,7 +561,7 @@ HTML end it "includes theme_uploads in settings" do - Theme.destroy_all + Theme.where.not(id: theme.id).destroy_all upload = UploadCreator.new(file_from_fixtures("logo.png"), "logo.png").create_for(-1) theme.set_field(type: :theme_upload_var, target: :common, name: "bob", upload_id: upload.id) @@ -574,7 +573,7 @@ HTML end it "does not break on missing uploads in settings" do - Theme.destroy_all + Theme.where.not(id: theme.id).destroy_all upload = UploadCreator.new(file_from_fixtures("logo.png"), "logo.png").create_for(-1) theme.set_field(type: :theme_upload_var, target: :common, name: "bob", upload_id: upload.id) @@ -589,7 +588,7 @@ HTML it "uses CDN url for theme_uploads in settings" do set_cdn_url("http://cdn.localhost") - Theme.destroy_all + Theme.where.not(id: theme.id).destroy_all upload = UploadCreator.new(file_from_fixtures("logo.png"), "logo.png").create_for(-1) theme.set_field(type: :theme_upload_var, target: :common, name: "bob", upload_id: upload.id) @@ -602,7 +601,7 @@ HTML it "uses CDN url for settings of type upload" do set_cdn_url("http://cdn.localhost") - Theme.destroy_all + Theme.where.not(id: theme.id).destroy_all upload = UploadCreator.new(file_from_fixtures("logo.png"), "logo.png").create_for(-1) theme.set_field(target: :settings, name: "yaml", value: <<~YAML) @@ -623,99 +622,6 @@ HTML expect(json["my_upload"]).to eq("http://cdn.localhost#{upload.url}") end - describe "convert_settings" do - it "can migrate a list field to a string field with json schema" do - theme.set_field( - target: :settings, - name: :yaml, - value: "valid_json_schema_setting:\n default: \"green,globe\"\n type: \"list\"", - ) - theme.save! - - setting = theme.settings.find { |s| s.name == :valid_json_schema_setting } - setting.value = "red,globe|green,cog|brown,users" - theme.save! - - expect(setting.type).to eq(ThemeSetting.types[:list]) - - yaml = File.read("#{Rails.root}/spec/fixtures/theme_settings/valid_settings.yaml") - theme.set_field(target: :settings, name: "yaml", value: yaml) - theme.save! - - theme.convert_settings - setting = theme.settings.find { |s| s.name == :valid_json_schema_setting } - - expect(JSON.parse(setting.value)).to eq( - JSON.parse( - '[{"color":"red","icon":"globe"},{"color":"green","icon":"cog"},{"color":"brown","icon":"users"}]', - ), - ) - expect(setting.type).to eq(ThemeSetting.types[:string]) - end - - it "does not update setting if data does not validate against json schema" do - theme.set_field( - target: :settings, - name: :yaml, - value: "valid_json_schema_setting:\n default: \"green,globe\"\n type: \"list\"", - ) - theme.save! - - setting = theme.settings.find { |s| s.name == :valid_json_schema_setting } - - # json_schema_settings.yaml defines only two properties per object and disallows additionalProperties - setting.value = "red,globe,hey|green,cog,hey|brown,users,nay" - theme.save! - - yaml = File.read("#{Rails.root}/spec/fixtures/theme_settings/valid_settings.yaml") - theme.set_field(target: :settings, name: "yaml", value: yaml) - theme.save! - - expect { theme.convert_settings }.to raise_error("Schema validation failed") - - setting.value = "red,globe|green,cog|brown" - theme.save! - - expect { theme.convert_settings }.not_to raise_error - - setting = theme.settings.find { |s| s.name == :valid_json_schema_setting } - expect(setting.type).to eq(ThemeSetting.types[:string]) - end - - it "warns when the theme has modified the setting type but data cannot be converted" do - begin - @orig_logger = Rails.logger - Rails.logger = @fake_logger = FakeLogger.new - - theme.set_field( - target: :settings, - name: :yaml, - value: "valid_json_schema_setting:\n default: \"\"\n type: \"list\"", - ) - theme.save! - - setting = theme.settings.find { |s| s.name == :valid_json_schema_setting } - setting.value = "red,globe" - theme.save! - - theme.set_field( - target: :settings, - name: :yaml, - value: "valid_json_schema_setting:\n default: \"\"\n type: \"string\"", - ) - theme.save! - - theme.convert_settings - expect(setting.value).to eq("red,globe") - expect(@fake_logger.warnings[0]).to include( - "Theme setting type has changed but cannot be converted.", - ) - ensure - Rails.logger = @orig_logger - end - end - end - describe "theme translations" do it "can list working theme_translation_manager objects" do en_translation = @@ -1125,6 +1031,234 @@ HTML end end + describe "#migrate_settings" do + fab!(:settings_field) { Fabricate(:settings_theme_field, theme: theme, value: <<~YAML) } + integer_setting: 1 + list_setting: "aa,bb" + YAML + + fab!(:migration_field) { Fabricate(:migration_theme_field, theme: theme, version: 1) } + + it "persists the results of the last pending migration to the database" do + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + settings.set("integer_setting", 1033); + settings.set("list_setting", "cc,dd"); + return settings; + } + JS + + Fabricate(:migration_theme_field, theme: theme, value: <<~JS, version: 2) + export default function migrate(settings) { + settings.set("integer_setting", 9909); + settings.set("list_setting", "ee,ff"); + return settings; + } + JS + + theme.migrate_settings + expect(theme.get_setting("integer_setting")).to eq(9909) + expect(theme.get_setting("list_setting")).to eq("ee,ff") + end + + it "doesn't allow arbitrary settings to be saved in the database" do + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + settings.set("unknown_setting", 8834); + return settings; + } + JS + expect do theme.migrate_settings end.to raise_error( + Theme::SettingsMigrationError, + I18n.t( + "themes.import_error.migrations.unknown_setting_returned_by_migration", + name: "0001-some-name", + setting_name: "unknown_setting", + ), + ) + end + + it "allows changing a setting's type" do + theme.update_setting(:list_setting, "zz,aa") + theme.save! + + setting_record = theme.theme_settings.where(name: "list_setting").first + expect(setting_record.data_type).to eq(ThemeSetting.types[:string]) + expect(setting_record.value).to eq("zz,aa") + + settings_field.update!(value: <<~YAML) + integer_setting: 1 + list_setting: + default: aa|bb + type: list + YAML + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + settings.set("list_setting", "zz|aa"); + return settings; + } + JS + theme.reload + + theme.migrate_settings + + expect(theme.theme_settings.where(name: "list_setting").count).to eq(1) + setting_record = theme.theme_settings.where(name: "list_setting").first + + expect(setting_record.data_type).to eq(ThemeSetting.types[:list]) + expect(setting_record.value).to eq("zz|aa") + + expect( + theme.theme_settings_migrations.where(theme_field_id: migration_field.id).first.diff, + ).to eq( + "additions" => [{ "key" => "list_setting", "val" => "zz|aa" }], + "deletions" => [{ "key" => "list_setting", "val" => "zz,aa" }], + ) + end + + it "allows renaming a setting" do + theme.update_setting(:integer_setting, 11) + theme.save! + + setting_record = theme.theme_settings.where(name: "integer_setting").first + expect(setting_record.value).to eq("11") + + settings_field.update!(value: <<~YAML) + integer_setting_updated: 1 + list_setting: "aa,bb" + YAML + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + settings.set("integer_setting_updated", settings.get("integer_setting")); + return settings; + } + JS + + theme.reload + + theme.migrate_settings + + expect(theme.theme_settings.where(name: "integer_setting").exists?).to eq(false) + + setting_record = theme.theme_settings.where(name: "integer_setting_updated").first + expect(setting_record.value).to eq("11") + + expect( + theme.theme_settings_migrations.where(theme_field_id: migration_field.id).first.diff, + ).to eq( + "additions" => [{ "key" => "integer_setting_updated", "val" => 11 }], + "deletions" => [{ "key" => "integer_setting", "val" => 11 }], + ) + end + + it "creates a ThemeSettingsMigration record for each migration" do + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + settings.set("integer_setting", 2); + settings.set("list_setting", "cc,dd"); + return settings; + } + JS + + second_migration_field = + Fabricate(:migration_theme_field, theme: theme, value: <<~JS, version: 2) + export default function migrate(settings) { + settings.set("integer_setting", 3); + settings.set("list_setting", "ee,ff"); + return settings; + } + JS + + third_migration_field = + Fabricate(:migration_theme_field, theme: theme, value: <<~JS, version: 3) + export default function migrate(settings) { + settings.set("integer_setting", 4); + settings.set("list_setting", "gg,hh"); + return settings; + } + JS + + theme.migrate_settings + + records = theme.theme_settings_migrations.order(:version) + + expect(records.count).to eq(3) + + expect(records[0].version).to eq(1) + expect(records[0].name).to eq("some-name") + expect(records[0].theme_field_id).to eq(migration_field.id) + expect(records[0].diff).to eq( + "additions" => [ + { "key" => "integer_setting", "val" => 2 }, + { "key" => "list_setting", "val" => "cc,dd" }, + ], + "deletions" => [], + ) + + expect(records[1].version).to eq(2) + expect(records[1].name).to eq("some-name") + expect(records[1].theme_field_id).to eq(second_migration_field.id) + expect(records[1].diff).to eq( + "additions" => [ + { "key" => "integer_setting", "val" => 3 }, + { "key" => "list_setting", "val" => "ee,ff" }, + ], + "deletions" => [ + { "key" => "integer_setting", "val" => 2 }, + { "key" => "list_setting", "val" => "cc,dd" }, + ], + ) + + expect(records[2].version).to eq(3) + expect(records[2].name).to eq("some-name") + expect(records[2].theme_field_id).to eq(third_migration_field.id) + expect(records[2].diff).to eq( + "additions" => [ + { "key" => "integer_setting", "val" => 4 }, + { "key" => "list_setting", "val" => "gg,hh" }, + ], + "deletions" => [ + { "key" => "integer_setting", "val" => 3 }, + { "key" => "list_setting", "val" => "ee,ff" }, + ], + ) + end + + it "allows removing an old setting that no longer exists" do + settings_field.update!(value: <<~YAML) + setting_that_will_be_removed: 1 + YAML + theme.update_setting(:setting_that_will_be_removed, 1023) + theme.save! + + settings_field.update!(value: <<~YAML) + new_setting: 1 + YAML + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + if (settings.get("setting_that_will_be_removed") !== 1023) { + throw new Error(`expected setting_that_will_be_removed to be 1023, but it was instead ${settings.get("setting_that_will_be_removed")}.`); + } + settings.delete("setting_that_will_be_removed"); + return settings; + } + JS + theme.reload + theme.migrate_settings + theme.reload + + expect(theme.theme_settings.count).to eq(0) + + records = theme.theme_settings_migrations + expect(records.size).to eq(1) + + expect(records[0].diff).to eq( + "additions" => [], + "deletions" => [{ "key" => "setting_that_will_be_removed", "val" => 1023 }], + ) + end + end + describe "development experience" do it "sends 'development-mode-theme-changed event when non-css fields are updated" do Theme.any_instance.stubs(:should_refresh_development_clients?).returns(true) diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb index fe1b4705943..6c4799887bb 100644 --- a/spec/requests/admin/themes_controller_spec.rb +++ b/spec/requests/admin/themes_controller_spec.rb @@ -235,6 +235,32 @@ RSpec.describe Admin::ThemesController do expect(response.status).to eq(201) end + it "responds with suitable error message when a migration fails" do + repo_path = + setup_git_repo( + "about.json" => { name: "test theme" }.to_json, + "settings.yaml" => "boolean_setting: true", + "migrations/settings/0001-some-migration.js" => <<~JS, + export default function migrate(settings) { + settings.set("unknown_setting", "dsad"); + return settings; + } + JS + ) + repo_url = MockGitImporter.register("https://example.com/initial_repo.git", repo_path) + + post "/admin/themes/import.json", params: { remote: repo_url } + + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to contain_exactly( + I18n.t( + "themes.import_error.migrations.unknown_setting_returned_by_migration", + name: "0001-some-migration", + setting_name: "unknown_setting", + ), + ) + end + it "fails to import with a failing status" do post "/admin/themes/import.json", params: { remote: "non-existent" } @@ -250,16 +276,9 @@ RSpec.describe Admin::ThemesController do it "can lookup a private key by public key" do Discourse.redis.setex("ssh_key_abcdef", 1.hour, "rsa private key") - ThemeStore::GitImporter.any_instance.stubs(:import!) - RemoteTheme.stubs(:extract_theme_info).returns( - "name" => "discourse-brand-header", - "component" => true, - ) - RemoteTheme.any_instance.stubs(:update_from_remote) - post "/admin/themes/import.json", params: { - remote: " https://github.com/discourse/discourse-brand-header.git ", + remote: " #{repo_url} ", public_key: "abcdef", } diff --git a/spec/services/theme_settings_migrations_runner_spec.rb b/spec/services/theme_settings_migrations_runner_spec.rb new file mode 100644 index 00000000000..a20f357572c --- /dev/null +++ b/spec/services/theme_settings_migrations_runner_spec.rb @@ -0,0 +1,285 @@ +# frozen_string_literal: true + +describe ThemeSettingsMigrationsRunner do + fab!(:theme) { Fabricate(:theme) } + fab!(:migration_field) { Fabricate(:migration_theme_field, version: 1, theme: theme) } + fab!(:settings_field) { Fabricate(:settings_theme_field, theme: theme, value: <<~YAML) } + integer_setting: 1 + boolean_setting: true + string_setting: "" + YAML + + describe "#run" do + it "passes values of overridden settings only to migrations" do + theme.update_setting(:integer_setting, 1) + theme.update_setting(:string_setting, "osama") + theme.save! + + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + if (settings.get("integer_setting") !== 1) { + throw new Error(`expected integer_setting to equal 1, but it's actually ${settings.get("integer_setting")}`); + } + if (settings.get("string_setting") !== "osama") { + throw new Error(`expected string_setting to equal "osama", but it's actually "${settings.get("string_setting")}"`); + } + if (settings.size !== 2) { + throw new Error(`expected the settings map to have only 2 keys, but instead got ${settings.size} keys`); + } + return settings; + } + JS + results = described_class.new(theme).run + expect(results.first[:theme_field_id]).to eq(migration_field.id) + expect(results.first[:settings_before]).to eq( + { "integer_setting" => 1, "string_setting" => "osama" }, + ) + end + + it "passes the output of the previous migration as input to the next one" do + theme.update_setting(:integer_setting, 1) + + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + settings.set("integer_setting", 111); + return settings; + } + JS + + another_migration_field = + Fabricate(:migration_theme_field, theme: theme, version: 2, value: <<~JS) + export default function migrate(settings) { + if (settings.get("integer_setting") !== 111) { + throw new Error(`expected integer_setting to equal 111, but it's actually ${settings.get("integer_setting")}`); + } + settings.set("integer_setting", 222); + return settings; + } + JS + + results = described_class.new(theme).run + + expect(results.size).to eq(2) + + expect(results[0][:theme_field_id]).to eq(migration_field.id) + expect(results[1][:theme_field_id]).to eq(another_migration_field.id) + + expect(results[0][:settings_before]).to eq({}) + expect(results[0][:settings_after]).to eq({ "integer_setting" => 111 }) + + expect(results[1][:settings_before]).to eq({ "integer_setting" => 111 }) + expect(results[1][:settings_after]).to eq({ "integer_setting" => 222 }) + end + + it "doesn't run migrations that have already been ran" do + Fabricate(:theme_settings_migration, theme: theme, theme_field: migration_field) + + pending_field = Fabricate(:migration_theme_field, theme: theme, version: 23) + + results = described_class.new(theme).run + + expect(results.size).to eq(1) + + expect(results.first[:version]).to eq(23) + expect(results.first[:theme_field_id]).to eq(pending_field.id) + end + + it "doesn't error when no migrations have been ran yet" do + results = described_class.new(theme).run + + expect(results.size).to eq(1) + expect(results.first[:version]).to eq(1) + expect(results.first[:theme_field_id]).to eq(migration_field.id) + end + + it "doesn't error when there are no pending migrations" do + Fabricate(:theme_settings_migration, theme: theme, theme_field: migration_field) + + results = described_class.new(theme).run + + expect(results.size).to eq(0) + end + + it "raises an error when there are too many pending migrations" do + Fabricate(:migration_theme_field, theme: theme, version: 2) + + expect do described_class.new(theme, limit: 1).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t("themes.import_error.migrations.too_many_pending_migrations"), + ) + end + + it "raises an error if a migration field has a badly formatted name" do + migration_field.update_attribute(:name, "020-some-name") + + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t("themes.import_error.migrations.invalid_filename", filename: "020-some-name"), + ) + + migration_field.update_attribute(:name, "0020some-name") + + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t("themes.import_error.migrations.invalid_filename", filename: "0020some-name"), + ) + + migration_field.update_attribute(:name, "0020") + + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t("themes.import_error.migrations.invalid_filename", filename: "0020"), + ) + end + + it "raises an error if a pending migration has version lower than the last ran migration" do + migration_field.update!(name: "0020-some-name") + Fabricate(:theme_settings_migration, theme: theme, theme_field: migration_field, version: 20) + + Fabricate(:migration_theme_field, theme: theme, version: 19, name: "0019-failing-migration") + + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t( + "themes.import_error.migrations.out_of_sequence", + name: "0019-failing-migration", + current: 20, + ), + ) + end + + it "detects bad syntax in migrations and raises an error" do + migration_field.update!(value: <<~JS) + export default function migrate() { + JS + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t( + "themes.import_error.migrations.syntax_error", + name: "0001-some-name", + error: + 'SyntaxError: "/discourse/theme/migration: Unexpected token (2:0)\n\n 1 | export default function migrate() {\n> 2 |\n | ^"', + ), + ) + end + + it "imposes memory limit on migrations and raises an error if they exceed the limit" do + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + let a = new Array(10000); + while(true) { + a = a.concat(new Array(10000)); + } + return settings; + } + JS + + expect do described_class.new(theme, memory: 10.kilobytes).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t("themes.import_error.migrations.exceeded_memory_limit", name: "0001-some-name"), + ) + end + + it "imposes time limit on migrations and raises an error if they exceed the limit" do + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + let a = 1; + while(true) { + a += 1; + } + return settings; + } + JS + + expect do described_class.new(theme, timeout: 10).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t("themes.import_error.migrations.timed_out", name: "0001-some-name"), + ) + end + + it "raises a clear error message when the migration file doesn't export anything" do + migration_field.update!(value: <<~JS) + function migrate(settings) { + return settings; + } + JS + + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t("themes.import_error.migrations.no_exported_function", name: "0001-some-name"), + ) + end + + it "raises a clear error message when the migration file exports the default as something that's not a function" do + migration_field.update!(value: <<~JS) + export function migrate(settings) { + return settings; + } + + const AA = 1; + export default AA; + JS + + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t( + "themes.import_error.migrations.default_export_not_a_function", + name: "0001-some-name", + ), + ) + end + + it "raises a clear error message when the migration function doesn't return anything" do + migration_field.update!(value: <<~JS) + export default function migrate(settings) {} + JS + + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t("themes.import_error.migrations.no_returned_value", name: "0001-some-name"), + ) + end + + it "raises a clear error message when the migration function doesn't return a Map" do + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + return {}; + } + JS + + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t("themes.import_error.migrations.wrong_return_type", name: "0001-some-name"), + ) + end + + it "surfaces runtime errors that occur within the migration" do + migration_field.update!(value: <<~JS) + export default function migrate(settings) { + null.toString(); + return settings; + } + JS + + expect do described_class.new(theme).run end.to raise_error( + Theme::SettingsMigrationError, + I18n.t( + "themes.import_error.migrations.runtime_error", + name: "0001-some-name", + error: "TypeError: Cannot read properties of null (reading 'toString')", + ), + ) + end + + it "returns a list of objects that each has data representing the migration and the results" do + results = described_class.new(theme).run + + expect(results[0][:version]).to eq(1) + expect(results[0][:name]).to eq("some-name") + expect(results[0][:original_name]).to eq("0001-some-name") + expect(results[0][:theme_field_id]).to eq(migration_field.id) + expect(results[0][:settings_before]).to eq({}) + expect(results[0][:settings_after]).to eq({}) + end + end +end diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb index 516fafc2be7..025089fbcc3 100644 --- a/spec/support/helpers.rb +++ b/spec/support/helpers.rb @@ -184,6 +184,16 @@ module Helpers repo_dir end + def add_to_git_repo(repo_dir, files) + files.each do |name, data| + FileUtils.mkdir_p(Pathname.new("#{repo_dir}/#{name}").dirname) + File.write("#{repo_dir}/#{name}", data) + `cd #{repo_dir} && git add #{name}` + end + `cd #{repo_dir} && git commit -am 'add #{files.size} files'` + repo_dir + end + def stub_const(target, const, value) old = target.const_get(const) target.send(:remove_const, const)