From a075fd46fd4acfc5307c6a97bb4ab8f7de612b36 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Sat, 6 Jul 2019 14:42:03 -0400 Subject: [PATCH] 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. --- app/models/concerns/has_custom_fields.rb | 19 ++++++++++++++----- .../concern/has_custom_fields_spec.rb | 3 ++- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/app/models/concerns/has_custom_fields.rb b/app/models/concerns/has_custom_fields.rb index 090c4feea79..d1472350231 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 27c7e989084..5696f113f12 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")