From e364547ff709ad15d888bd4e87c121d4ac5e8505 Mon Sep 17 00:00:00 2001
From: Arpit Jalan <arpit@techapj.com>
Date: Thu, 13 Sep 2018 13:29:17 +0530
Subject: [PATCH] FIX: ignore and log bad json values for custom fields

---
 app/models/concerns/has_custom_fields.rb      | 22 ++++++++--
 .../concern/has_custom_fields_spec.rb         | 41 +++++++++++++++++++
 2 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/app/models/concerns/has_custom_fields.rb b/app/models/concerns/has_custom_fields.rb
index a2bc01b254d..0a0b2edebeb 100644
--- a/app/models/concerns/has_custom_fields.rb
+++ b/app/models/concerns/has_custom_fields.rb
@@ -42,13 +42,20 @@ module HasCustomFields
         case type
         when :boolean then !!CUSTOM_FIELD_TRUE.include?(value)
         when :integer then value.to_i
-        when :json    then ::JSON.parse(value)
+        when :json    then parse_json_value(value, key)
         else
           value
         end
 
       array ? [result] : result
     end
+
+    def self.parse_json_value(value, key)
+      ::JSON.parse(value)
+    rescue JSON::ParserError
+      Rails.logger.warn("Value '#{value}' for custom field '#{key}' is not json, it is being ignored.")
+      {}
+    end
   end
 
   included do
@@ -85,6 +92,11 @@ module HasCustomFields
       @custom_field_types[name] = type
     end
 
+    def self.get_custom_field_type(name)
+      @custom_field_types ||= {}
+      @custom_field_types[name]
+    end
+
     def self.preload_custom_fields(objects, fields)
       if objects.present?
         map = {}
@@ -186,7 +198,7 @@ module HasCustomFields
       array_fields = {}
 
       _custom_fields.reload.each do |f|
-        if dup[f.name].is_a? Array
+        if dup[f.name].is_a?(Array)
           # we need to collect Arrays fully before we can compare them
           if !array_fields.has_key?(f.name)
             array_fields[f.name] = [f]
@@ -221,12 +233,14 @@ module HasCustomFields
       end
 
       dup.each do |k, v|
-        if v.is_a? Array
+        field_type = self.class.get_custom_field_type(k)
+
+        if v.is_a?(Array) && field_type != :json
           v.each { |subv| _custom_fields.create!(name: k, value: subv) }
         else
           _custom_fields.create!(
             name: k,
-            value: v.is_a?(Hash) ? v.to_json : v
+            value: v.is_a?(Hash) || field_type == :json ? v.to_json : v
           )
         end
       end
diff --git a/spec/components/concern/has_custom_fields_spec.rb b/spec/components/concern/has_custom_fields_spec.rb
index 1a55fc9f7c4..61f342658fe 100644
--- a/spec/components/concern/has_custom_fields_spec.rb
+++ b/spec/components/concern/has_custom_fields_spec.rb
@@ -187,6 +187,47 @@ describe HasCustomFields do
       expect(test_item2.custom_fields).to eq("sixto" => "rodriguez", "de" => "la playa")
     end
 
+    it "supports arrays in json fields" do
+      field_type = "json_array"
+      CustomFieldsTestItem.register_custom_field_type(field_type, :json)
+
+      item = CustomFieldsTestItem.new
+      item.custom_fields = {
+        "json_array" => [{ a: "test" }, { b: "another" }]
+      }
+      item.save
+
+      item.reload
+
+      expect(item.custom_fields[field_type]).to eq(
+        [{ "a" => "test" }, { "b" => "another" }]
+      )
+
+      item.custom_fields["json_array"] = ['a', 'b']
+      item.save
+
+      item.reload
+
+      expect(item.custom_fields[field_type]).to eq(["a", "b"])
+    end
+
+    it "will not fail to load custom fields if json is corrupt" do
+
+      field_type = "bad_json"
+      CustomFieldsTestItem.register_custom_field_type(field_type, :json)
+
+      item = CustomFieldsTestItem.create!
+
+      CustomFieldsTestItemCustomField.create!(
+        custom_fields_test_item_id: item.id,
+        name: field_type,
+        value: "{test"
+      )
+
+      item = item.reload
+      expect(item.custom_fields[field_type]).to eq({})
+    end
+
     it "supports bulk retrieval with a list of ids" do
       item1 = CustomFieldsTestItem.new
       item1.custom_fields = { "a" => ["b", "c", "d"], 'not_whitelisted' => 'secret' }