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

[tomerqodo]

User description

Benchmark PR discourse#36071

Type: Corrupted (contains bugs)

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 ID to URL hydration for objects type settings

• Adds upload reference tracking for site, theme, and theme site settings

• Creates frontend upload field component for schema setting objects

---

Diagram Walkthrough

flowchart LR
  A["Objects Schema Settings"] -->|"Extract upload IDs"| B["SchemaSettingsObjectValidator"]
  B -->|"Convert URLs to IDs"| C["Upload ID Storage"]
  C -->|"Track references"| D["UploadReference"]
  D -->|"Hydrate on retrieval"| E["Upload URLs"]
  F["Frontend Upload Component"] -->|"Handle uploads"| A

Loading

File Walkthrough

	Relevant files
Enhancement	7 files site_setting.rb Extract and track upload IDs from objects                                +28/-0    theme_setting.rb Extract and track upload IDs from objects                                +29/-2    theme_site_setting.rb Add upload reference tracking for theme site settings        +37/-0    schema_settings_object_validator.rb Convert upload URLs to IDs during validation                          +14/-1    site_setting_extension.rb Hydrate upload IDs to URLs in objects settings                      +51/-1    field.gjs Add upload field type to schema settings                                  +4/-1      upload.gjs Create upload field component for schema settings                +40/-0
Tests	4 files site_setting_extension_spec.rb Test upload hydration in objects settings                                +69/-0    theme_settings_object_validator_spec.rb Test upload URL to ID conversion                                                  +29/-0    site_settings_spec.rb Test upload reference creation for objects                              +32/-0    sample_plugin_site_settings.yml Add test objects with uploads configuration                            +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: 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: Missing auditing: New critical actions creating/destroying UploadReference records on save lack explicit audit logging of actor, action, and outcome. Referred Code 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) end end 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: Unhandled JSON: JSON.parse on objects values is performed without rescue in all_settings which may raise and break settings hydration for malformed input. Referred Code # 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 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
High-level	Validator should not mutate input The SchemaSettingsObjectValidator should not mutate its input by converting upload URLs to IDs. This data transformation should be handled separately from the validation logic to improve code clarity and prevent side effects. Examples: lib/schema_settings_object_validator.rb [129-141] 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.to_s] = upload.id true else false end ... (clipped 3 lines) Solution Walkthrough: Before: class SchemaSettingsObjectValidator def initialize(schema:, object:) @object = object # ... end def has_valid_property_value_type?(property_attributes, property_name) value = @object[property_name] # ... when "upload" if value.is_a?(String) upload = Upload.get_from_url(value) if upload @object[property_name.to_s] = upload.id # MUTATION return true end end # ... end end After: # Data transformation should be a separate step before validation def coerce_upload_urls_to_ids(object, schema) # ... logic to find 'upload' properties and convert URLs to IDs ... # This method is responsible for mutation. return mutated_object end # The validator is now pure and only checks for validity. class SchemaSettingsObjectValidator def has_valid_property_value_type?(property_attributes, property_name) value = @object[property_name] # ... when "upload" # Only validates that the value is an integer ID. return value.is_a?(Integer) # ... end end Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant design flaw where the SchemaSettingsObjectValidator mutates its input, violating the principle of separation of concerns and potentially causing future bugs.	Medium
Possible issue	Validate existence of upload ID In has_valid_property_value_type?, when validating an 'upload' property, check for the existence of the upload record if the value is an integer by using Upload.exists?(value). 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 @object[property_name.to_s] = upload.id true else false end +elsif value.is_a?(Integer) + Upload.exists?(value) else - value.is_a?(Integer) + false end Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out a validation flaw where non-existent integer upload IDs are considered valid, which could lead to data integrity issues. Adding Upload.exists? makes the validation more robust and consistent.	Medium
	Handle missing uploads during hydration In hydrate_uploads_in_object, use the safe navigation operator (&.) to set the upload property to nil if the upload record is not found, preventing stale IDs from being sent to the client. lib/site_setting_extension.rb [1160-1177] 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 + object[key] = upload&.url 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 end Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that a missing upload record would leave a stale ID in the setting data, which could cause client-side errors. Using the safe navigation operator (&.) correctly handles this edge case by setting the value to nil.	Medium
General	Refactor duplicated code into a concern Refactor the duplicated extract_upload_ids_from_objects_value method from SiteSetting, ThemeSetting, and ThemeSiteSetting into a shared concern to reduce code duplication and improve maintainability. app/models/site_setting.rb [388-409] -def extract_upload_ids_from_objects_value - return [] if self.value.blank? +# In a new file, e.g., app/models/concerns/extracts_upload_ids.rb +module ExtractsUploadIds + extend ActiveSupport::Concern - type_hash = SiteSetting.type_supervisor.type_hash(self.name.to_sym) - return [] unless type_hash[:schema]&.dig(:properties) + private - begin - parsed_value = JSON.parse(self.value) - parsed_value = [parsed_value] unless parsed_value.is_a?(Array) - upload_ids = Set.new + def extract_upload_ids_from_objects_value + return [] if self.value.blank? - parsed_value.each do |obj| - validator = SchemaSettingsObjectValidator.new(schema: type_hash[:schema], object: obj) + schema = schema_for_setting + return [] unless schema&.dig(:properties) - upload_ids.merge(validator.property_values_of_type("upload")) + 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 - - upload_ids.to_a - rescue JSON::ParserError - [] end end +# In app/models/site_setting.rb +# include ExtractsUploadIds +# +# private +# +# def schema_for_setting +# SiteSetting.type_supervisor.type_hash(self.name.to_sym)[:schema] +# end + Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies significant code duplication across three models and proposes a sound refactoring strategy using a concern, which would improve maintainability.	Low
More