DEV: Correctly validate multiple uploads in object site setting (#36639)

janzenisaac · web-flow · commit 4223288017db · 2025-12-11T14:48:44.000-06:00
# Bug The Setup: - Object site setting has 2 upload fields: UPLOAD_FIELD_1 and UPLOAD_FIELD_2 - Frontend sends URLs for both fields to the backend - Backend needs to convert URLs → IDs and validate them ## The Problem Validation loop processes properties one by one: ### 1. Validate UPLOAD_FIELD_1 - Convert URL → ID (123) - Check if ID is valid by querying: Upload.where(id: fetch_all_upload_values) - fetch_all_upload_values returns: [123, "http://mobile.png"] ← MIXED TYPES! - Query only finds Upload 123 - Cache valid_ids = [123] ← MEMOIZED ### 2. Validate UPLOAD_FIELD_2 - Convert URL → ID (456) - Check if ID is valid using cached valid_ids - valid_ids = [123] ← Still cached from step 1! - ID 456 not in [123] → VALIDATION FAILS ❌ Why it failed: When validating the first upload, the validation logic tried to fetch ALL upload values to check validity. But the second upload was still a URL string, not an ID yet. This created a mixed array [123, "http://..."] that couldn't be properly queried. The cached result then excluded the second upload, causing it to fail validation. # Fix Convert ALL URLs → IDs in first pass, THEN validate everything in second pass. Now when the validation queries run, all values are already integers.
diff --git a/lib/schema_settings_object_validator.rb b/lib/schema_settings_object_validator.rb
@@ -80,6 +80,13 @@ def initialize(schema:, object:, json_pointer_prefix: "", errors: {}, valid_ids_
   end
 
   def validate
+    # Convert all upload URLs to IDs before validation
+    @properties.each do |property_name, property_attributes|
+      next if property_attributes[:type] != "upload"
+      normalize_upload_value(property_name, property_attributes)
+    end
+
+    # Validate all properties
     @properties.each do |property_name, property_attributes|
       if property_attributes[:type] == "objects"
         validate_child_objects(
@@ -101,6 +108,15 @@ def property_values_of_type(type)
 
   private
 
+  def normalize_upload_value(property_name, property_attributes)
+    value = @object[property_name]
+    return unless value.is_a?(String) && value.present?
+
+    if upload = Upload.get_from_url(value)
+      @object[property_name] = upload.id
+    end
+  end
+
   def validate_child_objects(objects, property_name:, schema:)
     return if objects.blank?
 
@@ -137,18 +153,8 @@ def has_valid_property_value_type?(property_attributes, property_name)
       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
+        # Upload URLs are converted to IDs in normalize_upload_value
+        value.is_a?(Integer)
       when "float"
         value.is_a?(Float) || value.is_a?(Integer)
       when "boolean"
diff --git a/spec/lib/schema_settings_object_validator_spec.rb b/spec/lib/schema_settings_object_validator_spec.rb
@@ -800,6 +800,30 @@ def schema(required: false)
         # only 1 SQL query should be executed to check if upload ids are valid
         expect(queries.length).to eq(1)
       end
+
+      it "should convert multiple upload URLs to IDs and validate successfully" do
+        upload1 = Fabricate(:upload)
+        upload2 = Fabricate(:upload)
+        schema = {
+          name: "section",
+          properties: {
+            upload_property_1: {
+              type: "upload",
+            },
+            upload_property_2: {
+              type: "upload",
+            },
+          },
+        }
+
+        object = { upload_property_1: upload1.url, upload_property_2: upload2.url }
+        validator = described_class.new(schema: schema, object: object)
+        errors = validator.validate
+
+        expect(errors).to eq({})
+        expect(validator.instance_variable_get(:@object)[:upload_property_1]).to eq(upload1.id)
+        expect(validator.instance_variable_get(:@object)[:upload_property_2]).to eq(upload2.id)
+      end
     end
 
     context "for tag properties" do
