ensure atomic access to and creation of components in mqtt-task

app/models/component.rb

@@ -5,8 +5,17 @@ class Component < ActiveRecord::Base
   belongs_to :sensor
 
   validates_presence_of :device, :sensor
-  validates :sensor_id, :uniqueness => { :scope => [:device_id] }
-  validates :key, :uniqueness => { :scope =>  [:device_id] }
+
+  # 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] }
+
 
   before_validation :set_key, on: :create
 

app/models/concerns/data_parser/storer.rb

@@ -54,7 +54,7 @@ def timestamp_parse(timestamp)
       end
 
       def sensor_reading(device, sensor)
-        component = device.find_or_create_component_for_sensor_reading(sensor)
+        component = device.create_or_find_component_for_sensor_reading(sensor)
         return nil if component.nil?
         value = component.normalized_value( (Float(sensor['value']) rescue sensor['value']) )
         {

app/models/device.rb

@@ -103,25 +103,25 @@ def sensor_map
     components.map { |c| [c.key, c.sensor.id]}.to_h
   end
 
-  def find_or_create_component_by_sensor_id(sensor_id)
+  def create_or_find_component_by_sensor_id(sensor_id)
     return nil if sensor_id.nil? || !Sensor.exists?(id: sensor_id)
-    components.find_or_create_by(sensor_id: sensor_id)
+    components.create_or_find_by!(sensor_id: sensor_id)
   end
 
-  def find_or_create_component_by_sensor_key(sensor_key)
+  def create_or_find_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.find_or_create_by(sensor_id: sensor.id)
+    components.create_or_find_by!(sensor_id: sensor.id)
   end
 
-  def find_or_create_component_for_sensor_reading(reading)
+  def create_or_find_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
-      find_or_create_component_by_sensor_id(key_or_id)
+      create_or_find_component_by_sensor_id(key_or_id)
     else
-      find_or_create_component_by_sensor_key(key_or_id)
+      create_or_find_component_by_sensor_key(key_or_id)
     end
   end
 
@@ -245,7 +245,10 @@ 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.update_column(:last_reading_at, timestamp)
+      component.transaction do
+        component.lock!
+        component.update_column(:last_reading_at, timestamp)
+      end
     end
   end
 

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.find_or_create_component_by_sensor_id(metric_id)
+        component = device.create_or_find_component_by_sensor_id(metric_id)
         next if component.nil?
 
         value = component.normalized_value( (Float(value) rescue value) )

db/migrate/20241009174732_unique_index_on_components.rb

@@ -0,0 +1,23 @@
+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

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_08_12_081108) do
+ActiveRecord::Schema.define(version: 2024_10_09_174732) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "adminpack"
   enable_extension "hstore"
@@ -65,7 +65,9 @@
     t.string "key"
     t.integer "bus", default: 1, null: false
     t.datetime "last_reading_at"
-    t.index ["device_id", "sensor_id"], name: "index_components_on_device_id_and_sensor_id"
+    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
   end
 
   create_table "devices", id: :serial, force: :cascade do |t|

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::RecordInvalid)
+    expect{ create(:component, device: component.device, sensor: component.sensor) }.to raise_error(ActiveRecord::RecordNotUnique)
   end
 
   describe "creating a unique sensor key" do

spec/models/device_spec.rb

@@ -635,19 +635,19 @@
     end
   end
 
-  describe "#find_or_create_component_by_sensor_id" do
+  describe "#create_or_find_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.find_or_create_component_by_sensor_id(sensor.id)).to eq(component)
+        expect(device.create_or_find_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.find_or_create_component_by_sensor_id(sensor.id)
+        component = device.create_or_find_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.find_or_create_component_by_sensor_id(54321)).to be_blank
+        expect(device.create_or_find_component_by_sensor_id(54321)).to be_blank
       end
     end
 
     context "when the id is nil" do
       it "returns nil" do
-        expect(device.find_or_create_component_by_sensor_id(nil)).to be_blank
+        expect(device.create_or_find_component_by_sensor_id(nil)).to be_blank
       end
     end