[CLEAN] Synthetic Benchmark PR #36071 - DEV: Adds 'upload' type to schema setting

[tomerqodo]

User description

Benchmark PR discourse#36071

Type: Clean (correct implementation)

Original PR Title: DEV: Adds 'upload' type to schema setting
Original PR Description: The type: upload exists on main site settings, but not under the type: objects schema setting. This adds this feature for objects in Site Settings, Theme Settings, and Theme Site Settings.
Original PR URL: discourse#36071

---

PR Type

Enhancement

---

Description

• Adds support for 'upload' type fields within objects schema settings

• Implements upload reference tracking for SiteSetting, ThemeSetting, and ThemeSiteSetting models

• Adds upload URL hydration when retrieving objects settings via API

• Implements frontend upload field component for schema setting objects

• Adds validation and conversion of upload URLs to IDs in schema validator

---

Diagram Walkthrough

flowchart LR
  A["Objects Schema Setting"] -->|"Extract upload IDs"| B["extract_upload_ids_from_objects_value"]
  B -->|"Validate & Convert"| C["SchemaSettingsObjectValidator"]
  C -->|"Create References"| D["UploadReference"]
  E["API Response"] -->|"Hydrate IDs to URLs"| F["hydrate_uploads_in_objects"]
  F -->|"Batch Query"| G["Upload.where"]
  G -->|"Return URLs"| H["Objects with Upload URLs"]
  I["Frontend Component"] -->|"Upload File"| J["UploadField"]
  J -->|"Store URL"| K["Schema Setting Value"]

Loading

File Walkthrough

	Relevant files
Enhancement	7 files site_setting.rb Add upload reference tracking for objects type                      +28/-0    theme_setting.rb Add upload reference tracking for objects type                      +29/-2    theme_site_setting.rb Add upload reference tracking for objects type                      +37/-0    schema_settings_object_validator.rb Support upload URL to ID conversion in validation                +14/-1    site_setting_extension.rb Hydrate upload IDs to URLs in objects settings                      +53/-1    field.gjs Add upload field type to schema setting                                    +4/-1      upload.gjs Create upload field component for schema settings                +40/-0
Tests	4 files site_setting_extension_spec.rb Add tests for objects settings with uploads                            +69/-0    theme_settings_object_validator_spec.rb Add tests for upload URL conversion validation                      +29/-0    site_settings_spec.rb Add tests for upload reference creation                                    +32/-0    sample_plugin_site_settings.yml Add test objects setting with upload field                              +10/-0

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No audit logs: New critical actions that create/destroy UploadReference records on setting changes are not accompanied by explicit audit logging, and it is unclear if existing infrastructure logs these events sufficiently. Referred Code 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[: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? Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Extract duplicated code into concern To reduce code duplication, extract the extract_upload_ids_from_objects_value method from SiteSetting, ThemeSetting, and ThemeSiteSetting into a shared concern. app/models/site_setting.rb [386-409] +include ExtractsUploadIdsFromObjects + 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 +def schema_for_setting + SiteSetting.type_supervisor.type_hash(self.name)[:schema] end Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies significant code duplication across three models (SiteSetting, ThemeSetting, ThemeSiteSetting). Extracting this logic into a shared concern would improve maintainability and adhere to the DRY principle, which is a valuable architectural improvement.	Medium
	Avoid side-effects in validation methods Refactor the validation logic in has_valid_property_value_type? to remove the side effect of mutating the @object. Data transformation (URL to ID) should be handled outside of validation methods. lib/schema_settings_object_validator.rb [130-141] # Convert upload URLs to IDs like core does if value.is_a?(String) upload = Upload.get_from_url(value) if upload + # The object is mutated here, which is a side-effect in a validation method. + # This is kept for now to preserve existing behavior but should be refactored. @object[property_name] = upload.id true else false end +elsif value.is_a?(Integer) + # Fast path for already converted values + true else - value.is_a?(Integer) + false end Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that performing a mutation within a validation method is a side effect that violates the principle of separation of concerns. This makes the code harder to understand and maintain. Separating validation from data transformation would be a significant improvement.	Low
Possible issue	Avoid in-place object modification To prevent potential side effects, modify a deep copy of the object in hydrate_uploads_in_object instead of mutating it in-place. lib/site_setting_extension.rb [1160-1179] def hydrate_uploads_in_object(object, properties, uploads_by_id) + hydrated_object = object.deep_dup properties.each do |prop_key, prop_value| case prop_value[:type] when "upload" key = prop_key.to_s - upload_id = object[key] + upload_id = hydrated_object[key] upload = uploads_by_id[upload_id] - object[key] = upload.url if upload + hydrated_object[key] = upload.url if upload when "objects" - nested_objects = object[prop_key.to_s] + key = prop_key.to_s + nested_objects = hydrated_object[key] 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 + hydrated_object[key] = + nested_objects.map do |nested_obj| + hydrate_uploads_in_object(nested_obj, prop_value[:schema][:properties], uploads_by_id) + end end end end - object + hydrated_object end Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that modifying an object in-place can be risky. While the current usage seems safe, adopting an immutable approach by creating a copy (deep_dup) makes the code more robust and prevents potential future side-effect bugs.	Low
More