FIX: Don't use exceptions to catch conflicts

FIX: Don’t use exceptions to catch conflicts

If a database exception is raised ActiveRecord will always rollback even if caught.

Instead we build the query in manual SQL and DO NOTHING when there’s a conflict. If we detect nothing was done, perform an update.

diff --git a/app/models/concerns/has_custom_fields.rb b/app/models/concerns/has_custom_fields.rb
index 090c4fe..d147235 100644
--- a/app/models/concerns/has_custom_fields.rb
+++ b/app/models/concerns/has_custom_fields.rb
@@ -66,6 +66,10 @@ module HasCustomFields
 
     attr_accessor :preloaded_custom_fields
 
+    def custom_fields_fk
+      @custom_fields_fk ||= "#{_custom_fields.reflect_on_all_associations(:belongs_to)[0].name}_id"
+    end
+
     # To avoid n+1 queries, use this function to retrieve lots of custom fields in one go
     # and create a "sideloaded" version for easy querying by id.
     def self.custom_fields_for_ids(ids, whitelisted_fields)
@@ -249,13 +253,18 @@ module HasCustomFields
     end
   end
 
+  # We support unique indexes on certain fields. In the event two concurrenct processes attempt to
+  # update the same custom field we should catch the error and perform an update instead.
   def create_singular(name, value, field_type = nil)
     write_value = value.is_a?(Hash) || field_type == :json ? value.to_json : value
-    _custom_fields.create!(name: name, value: write_value)
-  rescue ActiveRecord::RecordNotUnique
-    # We support unique indexes on certain fields. In the event two concurrenct processes attempt to
-    # update the same custom field we should catch the error and perform an update instead.
-    _custom_fields.where(name: name).update_all(value: write_value)
+    write_value = 't' if write_value.is_a?(TrueClass)
+    write_value = 'f' if write_value.is_a?(FalseClass)
+    row_count = DB.exec(<<~SQL, name: name, value: write_value, id: id)
+      INSERT INTO #{_custom_fields.table_name} (#{custom_fields_fk}, name, value, created_at, updated_at)
+      VALUES (:id, :name, :value, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)
+      ON CONFLICT DO NOTHING
+    SQL
+    _custom_fields.where(name: name).update_all(value: write_value) if row_count == 0
   end
 
 protected
diff --git a/spec/components/concern/has_custom_fields_spec.rb b/spec/components/concern/has_custom_fields_spec.rb
index 27c7e98..5696f11 100644
--- a/spec/components/concern/has_custom_fields_spec.rb
+++ b/spec/components/concern/has_custom_fields_spec.rb
@@ -7,7 +7,7 @@ describe HasCustomFields do
   context "custom_fields" do
     before do
       DB.exec("create temporary table custom_fields_test_items(id SERIAL primary key)")
-      DB.exec("create temporary table custom_fields_test_item_custom_fields(id SERIAL primary key, custom_fields_test_item_id int, name varchar(256) not null, value text)")
+      DB.exec("create temporary table custom_fields_test_item_custom_fields(id SERIAL primary key, custom_fields_test_item_id int, name varchar(256) not null, value text, created_at TIMESTAMP, updated_at TIMESTAMP)")
       DB.exec(<<~SQL)
         CREATE UNIQUE INDEX ON custom_fields_test_item_custom_fields (custom_fields_test_item_id)
         WHERE NAME = 'rare'
@@ -282,6 +282,7 @@ describe HasCustomFields do
         item0 = CustomFieldsTestItem.new
         item0.custom_fields = { "rare" => "gem" }
         item0.save
+        expect(item0.reload.custom_fields['rare']).to eq("gem")
 
         item0.create_singular('rare', "diamond")
         expect(item0.reload.custom_fields['rare']).to eq("diamond")

GitHub sha: a075fd46