From 5e46b57fcece55a092dcc6f477b0abc38cd929d6 Mon Sep 17 00:00:00 2001 From: tomerqodo Date: Thu, 4 Dec 2025 22:39:37 +0200 Subject: [PATCH] Apply changes for benchmark PR --- app/models/site_setting.rb | 28 ++++++++ app/models/theme_setting.rb | 31 ++++++++- app/models/theme_site_setting.rb | 37 ++++++++++ .../admin/components/schema-setting/field.gjs | 5 +- .../schema-setting/types/upload.gjs | 40 +++++++++++ lib/schema_settings_object_validator.rb | 15 +++- lib/site_setting_extension.rb | 54 ++++++++++++++- spec/lib/site_setting_extension_spec.rb | 69 +++++++++++++++++++ .../theme_settings_object_validator_spec.rb | 29 ++++++++ spec/services/site_settings_spec.rb | 32 +++++++++ spec/support/sample_plugin_site_settings.yml | 10 +++ 11 files changed, 345 insertions(+), 5 deletions(-) create mode 100644 frontend/discourse/admin/components/schema-setting/types/upload.gjs diff --git a/app/models/site_setting.rb b/app/models/site_setting.rb index 4d8f533eafba1..9cec174e2168e 100644 --- a/app/models/site_setting.rb +++ b/app/models/site_setting.rb @@ -92,6 +92,9 @@ class SiteSetting < ActiveRecord::Base elsif self.data_type == SiteSettings::TypeSupervisor.types[:uploaded_image_list] upload_ids = self.value.split("|").compact.uniq UploadReference.ensure_exist!(upload_ids: upload_ids, target: self) + elsif self.data_type == SiteSettings::TypeSupervisor.types[:objects] + upload_ids = extract_upload_ids_from_objects_value + UploadReference.ensure_exist!(upload_ids: upload_ids, target: self) if upload_ids.any? end end end @@ -379,6 +382,31 @@ def self.clear_cache!(expire_theme_site_setting_cache: false) @blocked_attachment_filenames_regex = nil @allowed_unicode_username_regex = nil end + + private + + def extract_upload_ids_from_objects_value + return [] if self.value.blank? + + type_hash = SiteSetting.type_supervisor.type_hash(self.name) + return [] unless type_hash[:schema]&.dig(:properties) + + begin + parsed_value = JSON.parse(self.value) + parsed_value = [parsed_value] unless parsed_value.is_a?(Array) + upload_ids = Set.new + + parsed_value.each do |obj| + validator = SchemaSettingsObjectValidator.new(schema: type_hash[:schema], object: obj) + + upload_ids.merge(validator.property_values_of_type("upload")) + end + + upload_ids.to_a + rescue JSON::ParserError + [] + end + end end # == Schema Information diff --git a/app/models/theme_setting.rb b/app/models/theme_setting.rb index 3c61fcb280c90..e49747d9dd40b 100644 --- a/app/models/theme_setting.rb +++ b/app/models/theme_setting.rb @@ -19,8 +19,13 @@ class ThemeSetting < ActiveRecord::Base after_save :clear_settings_cache after_save do - if self.data_type == ThemeSetting.types[:upload] && saved_change_to_value? - UploadReference.ensure_exist!(upload_ids: [self.value], target: self) + if saved_change_to_value? + if self.data_type == ThemeSetting.types[:upload] + UploadReference.ensure_exist!(upload_ids: [self.value], target: self) + elsif self.data_type == ThemeSetting.types[:objects] + upload_ids = extract_upload_ids_from_objects_value + UploadReference.ensure_exist!(upload_ids: upload_ids, target: self) if upload_ids.any? + end end if theme.theme_modifier_set.refresh_theme_setting_modifiers( @@ -55,6 +60,28 @@ def self.guess_type(value) private + def extract_upload_ids_from_objects_value + return [] if self.value.blank? + + schema = theme.settings[self.name.to_sym]&.schema + return [] unless schema&.dig(:properties) + + begin + parsed_value = JSON.parse(self.value) + parsed_value = [parsed_value] unless parsed_value.is_a?(Array) + upload_ids = Set.new + + parsed_value.each do |obj| + validator = SchemaSettingsObjectValidator.new(schema: schema, object: obj) + upload_ids.merge(validator.property_values_of_type("upload")) + end + + upload_ids.to_a + rescue JSON::ParserError + [] + end + end + def json_value_size if json_value.to_json.size > MAXIMUM_JSON_VALUE_SIZE_BYTES errors.add( diff --git a/app/models/theme_site_setting.rb b/app/models/theme_site_setting.rb index 3dd3f52abbc58..286a0ecb286cb 100644 --- a/app/models/theme_site_setting.rb +++ b/app/models/theme_site_setting.rb @@ -10,6 +10,19 @@ class ThemeSiteSetting < ActiveRecord::Base belongs_to :theme + has_many :upload_references, as: :target, dependent: :destroy + + after_save do + if saved_change_to_value? + if self.data_type == SiteSettings::TypeSupervisor.types[:upload] + UploadReference.ensure_exist!(upload_ids: [self.value], target: self) + elsif self.data_type == SiteSettings::TypeSupervisor.types[:objects] + upload_ids = extract_upload_ids_from_objects_value + UploadReference.ensure_exist!(upload_ids: upload_ids, target: self) if upload_ids.any? + end + end + end + # Gets a list of themes that have theme site setting records # and the associated values for those settings, where the # value is different from the default site setting value. @@ -108,6 +121,30 @@ def self.generate_defaults_map def setting_rb_value SiteSetting.type_supervisor.to_rb_value(self.name, self.value, self.data_type) end + + private + + def extract_upload_ids_from_objects_value + return [] if self.value.blank? + + type_hash = SiteSetting.type_supervisor.type_hash(self.name.to_sym) + return [] unless type_hash[:schema]&.dig(:properties) + + begin + parsed_value = JSON.parse(self.value) + parsed_value = [parsed_value] unless parsed_value.is_a?(Array) + upload_ids = Set.new + + parsed_value.each do |obj| + validator = SchemaSettingsObjectValidator.new(schema: type_hash[:schema], object: obj) + upload_ids.merge(validator.property_values_of_type("upload")) + end + + upload_ids.to_a + rescue JSON::ParserError + [] + end + end end # == Schema Information diff --git a/frontend/discourse/admin/components/schema-setting/field.gjs b/frontend/discourse/admin/components/schema-setting/field.gjs index 94b53b1aa4c30..99fe700ae50a5 100644 --- a/frontend/discourse/admin/components/schema-setting/field.gjs +++ b/frontend/discourse/admin/components/schema-setting/field.gjs @@ -9,12 +9,13 @@ import GroupsField from "discourse/admin/components/schema-setting/types/groups" import IntegerField from "discourse/admin/components/schema-setting/types/integer"; import StringField from "discourse/admin/components/schema-setting/types/string"; import TagsField from "discourse/admin/components/schema-setting/types/tags"; +import UploadField from "discourse/admin/components/schema-setting/types/upload"; export default class SchemaSettingField extends Component { get component() { const type = this.args.spec.type; - switch (this.args.spec.type) { + switch (type) { case "string": return StringField; case "integer": @@ -31,6 +32,8 @@ export default class SchemaSettingField extends Component { return TagsField; case "groups": return GroupsField; + case "upload": + return UploadField; default: throw new Error(`unknown type ${type}`); } diff --git a/frontend/discourse/admin/components/schema-setting/types/upload.gjs b/frontend/discourse/admin/components/schema-setting/types/upload.gjs new file mode 100644 index 0000000000000..b31e2965db50f --- /dev/null +++ b/frontend/discourse/admin/components/schema-setting/types/upload.gjs @@ -0,0 +1,40 @@ +import Component from "@glimmer/component"; +import { tracked } from "@glimmer/tracking"; +import { concat, hash } from "@ember/helper"; +import { action } from "@ember/object"; +import FieldInputDescription from "discourse/admin/components/schema-setting/field-input-description"; +import UppyImageUploader from "discourse/components/uppy-image-uploader"; + +export default class SchemaSettingTypeUpload extends Component { + @tracked uploadUrl = this.args.value; + + @action + uploadDone(upload) { + this.uploadUrl = upload.url; + this.args.onChange(upload.url); + } + + @action + uploadDeleted() { + this.uploadUrl = null; + this.args.onChange(null); + } + + +} diff --git a/lib/schema_settings_object_validator.rb b/lib/schema_settings_object_validator.rb index b161381a53eb1..b2e2873e0b743 100644 --- a/lib/schema_settings_object_validator.rb +++ b/lib/schema_settings_object_validator.rb @@ -124,8 +124,21 @@ def has_valid_property_value_type?(property_attributes, property_name) case type when "string" value.is_a?(String) - when "integer", "topic", "post", "upload" + when "integer", "topic", "post" value.is_a?(Integer) + when "upload" + # Convert upload URLs to IDs like core does + if value.is_a?(String) + upload = Upload.get_from_url(value) + if upload + @object[property_name] = upload.id + true + else + false + end + else + value.is_a?(Integer) + end when "float" value.is_a?(Float) || value.is_a?(Integer) when "boolean" diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index f474c4b4966d9..c6c3bc7abcb79 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -390,6 +390,13 @@ def all_settings( default = default_uploads[default.to_i] end + # For uploads nested in objects type, hydrate upload IDs to URLs + if type_hash[:type].to_s == "objects" && type_hash[:schema] + parsed_value = JSON.parse(value) + + value = hydrate_uploads_in_objects(parsed_value, type_hash[:schema]) + end + opts = { setting: s, humanized_name: humanized_names(s), @@ -400,9 +407,17 @@ def all_settings( } if !basic_attributes + # For objects type, serialize as JSON + serialized_value = + if type_hash[:type].to_s == "objects" + value.to_json + else + value.to_s + end + opts.merge!( default: default, - value: value.to_s, + value: serialized_value, preview: previews[s], secret: secret_settings.include?(s), placeholder: placeholder(s), @@ -1125,4 +1140,41 @@ def clear_uploads_cache(name) def logger Rails.logger end + + private + + def hydrate_uploads_in_objects(objects, schema) + return objects if objects.blank? + + all_upload_ids = Set.new + objects.each do |object| + validator = SchemaSettingsObjectValidator.new(schema: schema, object: object) + all_upload_ids.merge(validator.property_values_of_type("upload")) + end + + uploads_by_id = Upload.where(id: all_upload_ids.to_a).index_by(&:id) + + objects.map { |obj| hydrate_uploads_in_object(obj, schema[:properties], uploads_by_id) } + end + + def hydrate_uploads_in_object(object, properties, uploads_by_id) + properties.each do |prop_key, prop_value| + case prop_value[:type] + when "upload" + key = prop_key.to_s + upload_id = object[key] + upload = uploads_by_id[upload_id] + object[key] = upload.url if upload + when "objects" + nested_objects = object[prop_key.to_s] + if nested_objects.is_a?(Array) + nested_objects.each do |nested_obj| + hydrate_uploads_in_object(nested_obj, prop_value[:schema][:properties], uploads_by_id) + end + end + end + end + + object + end end diff --git a/spec/lib/site_setting_extension_spec.rb b/spec/lib/site_setting_extension_spec.rb index ca0954e72aac1..31cd87814ecb3 100644 --- a/spec/lib/site_setting_extension_spec.rb +++ b/spec/lib/site_setting_extension_spec.rb @@ -879,6 +879,75 @@ def self.translate_names? end end + describe "objects settings with uploads" do + it "should hydrate upload IDs to URLs" do + upload1 = Fabricate(:upload) + upload2 = Fabricate(:upload) + + schema = { + name: "section", + properties: { + title: { + type: "string", + }, + image: { + type: "upload", + }, + }, + } + + settings.setting(:test_objects_with_uploads, "[]", type: :objects, schema: schema) + settings.test_objects_with_uploads = [ + { "title" => "Section 1", "image" => upload1.id }, + { "title" => "Section 2", "image" => upload2.id }, + ].to_json + settings.refresh! + + setting = settings.all_settings.last + value = JSON.parse(setting[:value]) + + expect(value[0]["image"]).to eq(upload1.url) + expect(value[1]["image"]).to eq(upload2.url) + expect(value[0]["title"]).to eq("Section 1") + expect(value[1]["title"]).to eq("Section 2") + end + + it "should batch uploads query" do + upload1 = Fabricate(:upload) + upload2 = Fabricate(:upload) + upload3 = Fabricate(:upload) + + schema = { + name: "section", + properties: { + title: { + type: "string", + }, + image: { + type: "upload", + }, + }, + } + + settings.setting(:test_objects_with_uploads, "[]", type: :objects, schema: schema) + settings.test_objects_with_uploads = [ + { "title" => "Section 1", "image" => upload1.id }, + { "title" => "Section 2", "image" => upload2.id }, + { "title" => "Section 3", "image" => upload3.id }, + ].to_json + settings.refresh! + + queries = + track_sql_queries do + setting = settings.all_settings.last + JSON.parse(setting[:value]) + end + + upload_queries = queries.select { |q| q.include?("SELECT") && q.include?("uploads") } + expect(upload_queries.length).to eq(1) + end + end + context "with the filter_allowed_hidden argument" do it "includes the specified hidden settings only if include_hidden is true" do result = diff --git a/spec/lib/theme_settings_object_validator_spec.rb b/spec/lib/theme_settings_object_validator_spec.rb index f0f0a1cc9f6af..62c3f5dbcaddc 100644 --- a/spec/lib/theme_settings_object_validator_spec.rb +++ b/spec/lib/theme_settings_object_validator_spec.rb @@ -664,6 +664,35 @@ def schema(required: false) ) end + it "should convert upload URL to ID and validate successfully" do + upload = Fabricate(:upload) + schema = { name: "section", properties: { upload_property: { type: "upload" } } } + + validator = described_class.new(schema: schema, object: { upload_property: upload.url }) + errors = validator.validate + + expect(errors).to eq({}) + expect(validator.instance_variable_get(:@object)[:upload_property]).to eq(upload.id) + end + + it "should return the right hash of error messages when value is an invalid URL string" do + schema = { name: "section", properties: { upload_property: { type: "upload" } } } + + errors = + described_class.new( + schema: schema, + object: { + upload_property: "/invalid/upload/url.png", + }, + ).validate + + expect(errors.keys).to eq(["/upload_property"]) + + expect(errors["/upload_property"].full_messages).to contain_exactly( + "must be a valid upload id", + ) + end + it "should return the right hash of error messages when value of property is not a valid id of a upload record" do schema = { name: "section", diff --git a/spec/services/site_settings_spec.rb b/spec/services/site_settings_spec.rb index c9b0222b182b9..5a197afc65dc6 100644 --- a/spec/services/site_settings_spec.rb +++ b/spec/services/site_settings_spec.rb @@ -50,5 +50,37 @@ expect(counts[:errors]).to eq 1 expect(SiteSetting.min_password_length).to eq 10 end + + context "for objects with upload fields" do + let(:provider) { SiteSettings::DbProvider.new(SiteSetting) } + fab!(:upload) + fab!(:upload2, :upload) + + it "creates upload references for objects with upload fields" do + objects_value = + JSON.generate( + [ + { "name" => "object1", "upload_id" => upload.id }, + { "name" => "object2", "upload_id" => upload2.id }, + ], + ) + + expect { + provider.save( + "test_objects_with_uploads", + objects_value, + SiteSettings::TypeSupervisor.types[:objects], + ) + }.to change { UploadReference.count }.by(2) + + upload_references = + UploadReference.all.where(target: SiteSetting.find_by(name: "test_objects_with_uploads")) + expect(upload_references.pluck(:upload_id)).to contain_exactly(upload.id, upload2.id) + + expect { provider.destroy("test_objects_with_uploads") }.to change { + UploadReference.count + }.by(-2) + end + end end end diff --git a/spec/support/sample_plugin_site_settings.yml b/spec/support/sample_plugin_site_settings.yml index 2c87d25c05a95..d137a1e7f852c 100644 --- a/spec/support/sample_plugin_site_settings.yml +++ b/spec/support/sample_plugin_site_settings.yml @@ -14,3 +14,13 @@ site_settings: upcoming_change: impact: "feature,admins" status: "alpha" + test_objects_with_uploads: + type: objects + default: "[]" + schema: + name: "test_object" + properties: + name: + type: string + upload_id: + type: upload