From a5944b3c02c0aaf533a1cb28d2732e881bad5a28 Mon Sep 17 00:00:00 2001 From: Tim Cowlishaw Date: Thu, 10 Oct 2024 08:04:40 +0200 Subject: [PATCH] Revert "ensure atomic access to and creation of components in mqtt-task" This reverts commit 250807a0ff5adda39b371b300207d0a3f5b5a186. --- app/models/component.rb | 13 ++--------- app/models/concerns/data_parser/storer.rb | 2 +- app/models/device.rb | 19 +++++++-------- app/models/raw_storer.rb | 2 +- ...241009174732_unique_index_on_components.rb | 23 ------------------- db/schema.rb | 6 ++--- spec/models/component_spec.rb | 2 +- spec/models/device_spec.rb | 10 ++++---- 8 files changed, 20 insertions(+), 57 deletions(-) delete mode 100644 db/migrate/20241009174732_unique_index_on_components.rb diff --git a/app/models/component.rb b/app/models/component.rb index c3b2c5a9..fa4b9ae5 100644 --- a/app/models/component.rb +++ b/app/models/component.rb @@ -5,17 +5,8 @@ class Component < ActiveRecord::Base belongs_to :sensor validates_presence_of :device, :sensor - - # IMPORTANT: Validation of sensor/device uniqueness is done at the database level, - # as this allows us to use the create_or_find_by! method to atomically upsert components - # in the mqtt-task, avoiding component duplication due to race conditions. - # For some reason, create_or_find_by! ONLY works when the database constraint is - # the ONLY uniqueness constraint on those two values, so adding a rails validation here - # causes an error. Leaving the validations here commented out by way of documentation. - # See https://stackoverflow.com/questions/74566974/create-or-find-by-not-working-as-it-should-in-rails-6 - # validates :sensor_id, :uniqueness => { :scope => [:device_id] } - # validates :key, :uniqueness => { :scope => [:device_id] } - + validates :sensor_id, :uniqueness => { :scope => [:device_id] } + validates :key, :uniqueness => { :scope => [:device_id] } before_validation :set_key, on: :create diff --git a/app/models/concerns/data_parser/storer.rb b/app/models/concerns/data_parser/storer.rb index 84a37720..721ddd10 100644 --- a/app/models/concerns/data_parser/storer.rb +++ b/app/models/concerns/data_parser/storer.rb @@ -54,7 +54,7 @@ def timestamp_parse(timestamp) end def sensor_reading(device, sensor) - component = device.create_or_find_component_for_sensor_reading(sensor) + component = device.find_or_create_component_for_sensor_reading(sensor) return nil if component.nil? value = component.normalized_value( (Float(sensor['value']) rescue sensor['value']) ) { diff --git a/app/models/device.rb b/app/models/device.rb index f2eade86..a8425e10 100644 --- a/app/models/device.rb +++ b/app/models/device.rb @@ -103,25 +103,25 @@ def sensor_map components.map { |c| [c.key, c.sensor.id]}.to_h end - def create_or_find_component_by_sensor_id(sensor_id) + def find_or_create_component_by_sensor_id(sensor_id) return nil if sensor_id.nil? || !Sensor.exists?(id: sensor_id) - components.create_or_find_by!(sensor_id: sensor_id) + components.find_or_create_by(sensor_id: sensor_id) end - def create_or_find_component_by_sensor_key(sensor_key) + def find_or_create_component_by_sensor_key(sensor_key) return nil if sensor_key.nil? sensor = Sensor.find_by(default_key: sensor_key) return nil if sensor.nil? - components.create_or_find_by!(sensor_id: sensor.id) + components.find_or_create_by(sensor_id: sensor.id) end - def create_or_find_component_for_sensor_reading(reading) + def find_or_create_component_for_sensor_reading(reading) key_or_id = reading["id"] if key_or_id.is_a?(Integer) || key_or_id =~ /\d+/ # It's an integer and therefore a sensor id - create_or_find_component_by_sensor_id(key_or_id) + find_or_create_component_by_sensor_id(key_or_id) else - create_or_find_component_by_sensor_key(key_or_id) + find_or_create_component_by_sensor_key(key_or_id) end end @@ -245,10 +245,7 @@ def remove_mac_address_for_newly_registered_device! def update_component_timestamps(timestamp, sensor_ids) components.select {|c| sensor_ids.include?(c.sensor_id) }.each do |component| - component.transaction do - component.lock! - component.update_column(:last_reading_at, timestamp) - end + component.update_column(:last_reading_at, timestamp) end end diff --git a/app/models/raw_storer.rb b/app/models/raw_storer.rb index 89e5f144..0e46402b 100644 --- a/app/models/raw_storer.rb +++ b/app/models/raw_storer.rb @@ -30,7 +30,7 @@ def store data, mac, version, ip, raise_errors=false metric_id = device.find_sensor_id_by_key(metric) - component = device.create_or_find_component_by_sensor_id(metric_id) + component = device.find_or_create_component_by_sensor_id(metric_id) next if component.nil? value = component.normalized_value( (Float(value) rescue value) ) diff --git a/db/migrate/20241009174732_unique_index_on_components.rb b/db/migrate/20241009174732_unique_index_on_components.rb deleted file mode 100644 index a3fc5f01..00000000 --- a/db/migrate/20241009174732_unique_index_on_components.rb +++ /dev/null @@ -1,23 +0,0 @@ -class UniqueIndexOnComponents < ActiveRecord::Migration[6.1] - def up - remove_index :components, [:device_id, :sensor_id] - add_index :components, [:device_id, :sensor_id], unique: true - execute %{ - ALTER TABLE components ADD CONSTRAINT unique_sensor_for_device UNIQUE (device_id, sensor_id) - } - execute %{ - ALTER TABLE components ADD CONSTRAINT unique_key_for_device UNIQUE (device_id, key) - } - end - - def down - execute %{ - ALTER TABLE components DROP CONSTRAINT IF EXISTS unique_key_for_device - } - execute %{ - ALTER TABLE components DROP CONSTRAINT IF EXISTS unique_sensor_for_device - } - remove_index :components, [:device_id, :sensor_id], unique: true - add_index :components, [:device_id, :sensor_id] - end -end diff --git a/db/schema.rb b/db/schema.rb index 52245cdd..7eef4fe3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2024_10_09_174732) do +ActiveRecord::Schema.define(version: 2024_08_12_081108) do # These are extensions that must be enabled in order to support this database enable_extension "adminpack" enable_extension "hstore" @@ -65,9 +65,7 @@ t.string "key" t.integer "bus", default: 1, null: false t.datetime "last_reading_at" - t.index ["device_id", "key"], name: "unique_key_for_device", unique: true - t.index ["device_id", "sensor_id"], name: "index_components_on_device_id_and_sensor_id", unique: true - t.index ["device_id", "sensor_id"], name: "unique_sensor_for_device", unique: true + t.index ["device_id", "sensor_id"], name: "index_components_on_device_id_and_sensor_id" end create_table "devices", id: :serial, force: :cascade do |t| diff --git a/spec/models/component_spec.rb b/spec/models/component_spec.rb index dc30c5b9..e6ea21ad 100644 --- a/spec/models/component_spec.rb +++ b/spec/models/component_spec.rb @@ -8,7 +8,7 @@ it "validates uniqueness of board to sensor" do component = create(:component, device: create(:device), sensor: create(:sensor)) - expect{ create(:component, device: component.device, sensor: component.sensor) }.to raise_error(ActiveRecord::RecordNotUnique) + expect{ create(:component, device: component.device, sensor: component.sensor) }.to raise_error(ActiveRecord::RecordInvalid) end describe "creating a unique sensor key" do diff --git a/spec/models/device_spec.rb b/spec/models/device_spec.rb index e0fd3b36..7e865a50 100644 --- a/spec/models/device_spec.rb +++ b/spec/models/device_spec.rb @@ -635,19 +635,19 @@ end end - describe "#create_or_find_component_by_sensor_id" do + describe "#find_or_create_component_by_sensor_id" do context "when the sensor exists and a component already exists for this device" do it "returns the existing component" do sensor = create(:sensor) component = create(:component, sensor: sensor, device: device) - expect(device.create_or_find_component_by_sensor_id(sensor.id)).to eq(component) + expect(device.find_or_create_component_by_sensor_id(sensor.id)).to eq(component) end end context "when the sensor exists and a component does not already exist for this device" do it "returns a new valid component with the correct sensor and device" do sensor = create(:sensor) - component = device.create_or_find_component_by_sensor_id(sensor.id) + component = device.find_or_create_component_by_sensor_id(sensor.id) expect(component).not_to be_blank expect(component).to be_a Component expect(component.valid?).to be(true) @@ -660,13 +660,13 @@ context "when no sensor exists with this id" do it "returns nil" do create(:sensor, id: 12345) - expect(device.create_or_find_component_by_sensor_id(54321)).to be_blank + expect(device.find_or_create_component_by_sensor_id(54321)).to be_blank end end context "when the id is nil" do it "returns nil" do - expect(device.create_or_find_component_by_sensor_id(nil)).to be_blank + expect(device.find_or_create_component_by_sensor_id(nil)).to be_blank end end