fix: Cache invalidation on subject creation/deletion

Also add unit tests for this

app/models/concerns/spec_cache_updater.rb

@@ -4,14 +4,20 @@ module SpecCacheUpdater
   extend ActiveSupport::Concern
 
   included do
-    before_destroy :schedule_spec_cache_update
-    after_save :schedule_spec_cache_update
+    before_destroy :update_service_subject_spec_cache
+    around_save :update_service_subject_spec_cache_with_yield
   end
 
   private
-    def schedule_spec_cache_update(item_hint = nil)
+    def update_service_subject_spec_cache_with_yield
+      yield
+      update_service_subject_spec_cache
+    end
+
+    def update_service_subject_spec_cache(item_hint = nil)
       ServiceSubject.transaction do
         [self, item_hint].compact.flat_map(&method(:gather_subjects)).uniq.each do |subject|
+          next if subject.customization_spec_ids == subject.matched_spec_ids
           subject.update_columns(
             customization_spec_ids: subject.matched_spec_ids,
             updated_at: Time.current

app/models/concerns/vm_cache_buster.rb

@@ -4,21 +4,33 @@ module VmCacheBuster
   extend ActiveSupport::Concern
 
   included do
-    before_destroy :update_specs
-    after_save :update_specs
+    around_destroy :update_specs_before
+    around_save :update_specs_before
   end
 
   private
-    def update_specs
-      l_service = case self
-                  when Service
-                    self
-                  else
-                    service
-      end
-      exercise
-        .customization_specs
-        .where(id: l_service.cached_spec_ids)
-        .touch_all
+    def update_specs_before
+      spec_ids = Set.new
+      spec_ids.merge(get_service.cached_spec_ids)
+      yield
+      spec_ids.merge(get_service.cached_spec_ids)
+      update_specs(spec_ids)
+    end
+
+    def update_specs_after
+      update_specs(get_service.cached_spec_ids)
+    end
+
+    def update_specs(ids)
+      exercise.customization_specs.where(id: ids).touch_all
+    end
+
+    def get_service
+      case self
+      when Service
+        self
+      else
+        service
+      end.tap { |s| s.reload if s.persisted? }
     end
 end

app/models/service_subject.rb

@@ -3,6 +3,7 @@
 require 'service_subject_match_condition'
 
 class ServiceSubject < ApplicationRecord
+  include VmCacheBuster
   include SpecCacheUpdater
 
   has_paper_trail

spec/factories.rb

@@ -11,9 +11,9 @@
 
   factory :customization_spec do
     virtual_machine
-    name { 'MyString' }
+    mode { 'host' }
+    sequence(:name) { |n| "CoolSpec#{n}" }
     role_name { 'MyString' }
-    dns_name { 'MyString' }
     description { 'MyText' }
   end
 
@@ -44,7 +44,7 @@
   end
 
   factory :virtual_machine do
-    name { 'CoolTarget' }
+    sequence(:name) { |n| "CoolTarget#{n}" }
     actor
     operating_system
     exercise
@@ -91,4 +91,13 @@
   factory :api_token do
     user
   end
+
+  factory :service do
+    exercise
+    name { 'Some service' }
+  end
+
+  factory :service_subject do
+    service
+  end
 end

spec/models/service_subject_spec.rb

@@ -0,0 +1,82 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe ServiceSubject do
+
+  context 'spec cache' do
+    subject { create(:service_subject, service:) }
+    let(:service) { create(:service) }
+    let(:virtual_machine) { create(:virtual_machine, exercise: service.exercise) }
+
+    it 'should set correct cache when matcher condition added' do
+      service.service_subjects.reload
+      subject.match_conditions = [ServiceSubjectMatchCondition.new(
+        matcher_type: 'CustomizationSpec', matcher_id: virtual_machine.host_spec.id
+      )]
+      subject.save
+      subject.match_conditions[0].matcher_id = virtual_machine.host_spec.id
+      subject.save
+      subject.reload
+
+      expect(subject.customization_spec_ids).to eq ([virtual_machine.host_spec.id])
+    end
+
+    context 'with a pre-existing match condition' do
+      subject {
+        create(:service_subject, service:, match_conditions: [ServiceSubjectMatchCondition.new(
+          matcher_type: 'CustomizationSpec', matcher_id: virtual_machine.host_spec.id
+        )])
+      }
+
+      let(:virtual_machine2) { create(:virtual_machine, exercise: service.exercise) }
+
+      it 'should update cache when changing matcher id' do
+        subject.match_conditions[0].matcher_id = virtual_machine2.host_spec.id
+        subject.save
+
+        expect(subject.customization_spec_ids).to eq ([virtual_machine2.host_spec.id])
+      end
+
+      it 'should update cache key for previous vms when changing matcher id' do
+        subject.match_conditions[0].matcher_id = virtual_machine2.host_spec.id
+
+        expect {
+          subject.save
+          virtual_machine.host_spec.reload
+        }.to change(virtual_machine.host_spec, :cache_key_with_version)
+      end
+
+      it 'should update cache key for following vms when changing matcher id' do
+        subject.match_conditions[0].matcher_id = virtual_machine2.host_spec.id
+
+        expect {
+          subject.save
+          virtual_machine2.host_spec.reload
+        }.to change(virtual_machine2.host_spec, :cache_key_with_version)
+      end
+
+      it 'should empty cached ids when deleting matcher' do
+        subject.match_conditions[0].matcher_id = nil
+        subject.save
+        expect(subject.customization_spec_ids).to eq ([])
+      end
+
+      it 'should update cache key for previous vms when deleting matcher id' do
+        subject.match_conditions[0].matcher_id = nil
+
+        expect {
+          subject.save
+          virtual_machine.host_spec.reload
+        }.to change(virtual_machine.host_spec, :cache_key_with_version)
+      end
+
+      it 'should update cache key for previous vms when deleting subject' do
+        expect {
+          subject.destroy
+          virtual_machine.host_spec.reload
+        }.to change(virtual_machine.host_spec, :cache_key_with_version)
+      end
+    end
+  end
+end