Convert VirusScanner class into stateless service

app/workers/virus_scan_worker.rb

@@ -1,16 +1,14 @@
-require 'virus_scanner'
+require 'services'
 
 class VirusScanWorker
   include Sidekiq::Worker
 
   def perform(asset_id)
     asset = Asset.find(asset_id)
-    scanner = VirusScanner.new(asset.file.path)
-    if scanner.clean?
-      asset.scanned_clean
-    else
-      GovukError.notify(VirusScanner::InfectedFile.new, extra: { error_message: scanner.virus_info, id: asset.id, filename: asset.filename })
-      asset.scanned_infected
-    end
+    Services.virus_scanner.scan(asset.file.path)
+    asset.scanned_clean
+  rescue VirusScanner::InfectedFile => e
+    GovukError.notify(e, extra: { id: asset.id, filename: asset.filename })
+    asset.scanned_infected
   end
 end

lib/services.rb

@@ -1,7 +1,12 @@
 require 's3_storage'
+require 'virus_scanner'
 
 module Services
   def self.cloud_storage
     @cloud_storage ||= S3Storage.build
   end
+
+  def self.virus_scanner
+    @virus_scanner ||= VirusScanner.new
+  end
 end

lib/virus_scanner.rb

@@ -6,35 +6,17 @@
 # to either clamscan or clamdscan
 class VirusScanner
   class Error < StandardError; end
-
-  # Used for sending exception notices on infection
   class InfectedFile < StandardError; end
 
-  def initialize(file_path)
-    @file_path = file_path
-    @scanned = false
-  end
-
-  attr_reader :virus_info
-
-  def clean?
-    scan unless @scanned
-    @clean
-  end
-
-private
-
-  def scan
-    out_str, status = Open3.capture2e('govuk_clamscan', '--no-summary', @file_path)
+  def scan(file_path)
+    out_str, status = Open3.capture2e('govuk_clamscan', '--no-summary', file_path)
     case status.exitstatus
     when 0
-      @clean = true
+      return true
     when 1
-      @clean = false
-      @virus_info = out_str
+      raise InfectedFile, out_str
     else
       raise Error, out_str
     end
-    @scanned = true
   end
 end

spec/lib/virus_scanner_spec.rb

@@ -3,54 +3,51 @@
 
 RSpec.describe VirusScanner do
   describe "scanning a file" do
-    subject(:scanner) { described_class.new("/path/to/file") }
+    subject(:scanner) { described_class.new }
 
-    it "calls out to clamdscan" do
-      status = double("Process::Status", exitstatus: 0)
-      expect(Open3).to receive(:capture2e).with("govuk_clamscan", "--no-summary", "/path/to/file").and_return(["", status])
-
-      scanner.clean?
-    end
+    let(:file_path) { '/path/to/file' }
+    let(:output) { '' }
+    let(:status) { instance_double('Process::Status', exitstatus: exitstatus) }
+    let(:exitstatus) { 0 }
 
-    it "only scans the file once" do
-      status = double("Process::Status", exitstatus: 0)
-      allow(Open3).to receive(:capture2e).and_return(["/path/to/file: OK", status])
-
-      scanner.clean?
-      expect(Open3).not_to receive(:capture2e)
-
-      expect(scanner.clean?).to eq(true)
+    before do
+      allow(Open3).to receive(:capture2e).and_return([output, status])
     end
 
-    it "returns true if clamdscan detects no virus" do
-      status = double("Process::Status", exitstatus: 0)
-      allow(Open3).to receive(:capture2e).and_return(["/path/to/file: OK", status])
+    it "calls out to clamdscan" do
+      expect(Open3).to receive(:capture2e).with("govuk_clamscan", "--no-summary", file_path)
 
-      expect(scanner.clean?).to eq(true)
+      scanner.scan(file_path)
     end
 
-    it "returns false if clamdscan detects a virus" do
-      status = double("Process::Status", exitstatus: 1)
-      allow(Open3).to receive(:capture2e).and_return(["/path/to/file: Eicar-Test-Signature FOUND", status])
+    context 'when clamdscan detects no virus' do
+      let(:exitstatus) { 0 }
 
-      expect(scanner.clean?).to eq(false)
+      it "returns true" do
+        expect(scanner.scan(file_path)).to eq(true)
+      end
     end
 
-    it "makes virus info available after detecting a virus" do
-      status = double("Process::Status", exitstatus: 1)
-      allow(Open3).to receive(:capture2e).and_return(["/path/to/file: Eicar-Test-Signature FOUND", status])
+    context 'when clamdscan detects a virus' do
+      let(:exitstatus) { 1 }
+      let(:output) { "#{file_path}: Eicar-Test-Signature FOUND" }
 
-      scanner.clean?
-      expect(scanner.virus_info).to eq("/path/to/file: Eicar-Test-Signature FOUND")
+      it "raises InfectedFile exception with the output message" do
+        expect {
+          scanner.scan(file_path)
+        }.to raise_error(VirusScanner::InfectedFile, output)
+      end
     end
 
-    it "raises an error with the output message if clamdscan fails" do
-      status = double("Process::Status", exitstatus: 2)
-      allow(Open3).to receive(:capture2e).and_return(["ERROR: Can't access file /path/to/file", status])
+    context 'when clamdscan fails' do
+      let(:exitstatus) { 2 }
+      let(:output) { "ERROR: Can't access file #{file_path}" }
 
-      expect {
-        scanner.clean?
-      }.to raise_error(VirusScanner::Error, "ERROR: Can't access file /path/to/file")
+      it "raises Error exception with the output message" do
+        expect {
+          scanner.scan(file_path)
+        }.to raise_error(VirusScanner::Error, output)
+      end
     end
   end
 end

spec/workers/virus_scan_worker_spec.rb

@@ -1,30 +1,40 @@
 require 'rails_helper'
+require 'services'
 
 RSpec.describe VirusScanWorker do
   let(:worker) { described_class.new }
   let(:asset) { FactoryBot.create(:asset) }
+  let(:scanner) { instance_double('VirusScanner') }
+
+  before do
+    allow(Services).to receive(:virus_scanner).and_return(scanner)
+  end
 
   it "calls out to the VirusScanner to scan the file" do
-    scanner = double("VirusScanner")
-    expect(VirusScanner).to receive(:new).with(asset.file.path).and_return(scanner)
-    expect(scanner).to receive(:clean?).and_return(true)
+    expect(scanner).to receive(:scan).with(asset.file.path)
 
     worker.perform(asset.id)
   end
 
-  it "sets the state to clean if the file is clean" do
-    allow_any_instance_of(VirusScanner).to receive(:clean?).and_return(true)
+  context 'when the file is clean' do
+    before do
+      allow(scanner).to receive(:scan).and_return(true)
+    end
 
-    worker.perform(asset.id)
+    it "sets the state to clean" do
+      worker.perform(asset.id)
 
-    asset.reload
-    expect(asset.state).to eq('clean')
+      asset.reload
+      expect(asset.state).to eq('clean')
+    end
   end
 
   context "when a virus is found" do
+    let(:exception_message) { "/path/to/file: Eicar-Test-Signature FOUND" }
+    let(:exception) { VirusScanner::InfectedFile.new(exception_message) }
+
     before do
-      allow_any_instance_of(VirusScanner).to receive(:clean?).and_return(false)
-      allow_any_instance_of(VirusScanner).to receive(:virus_info).and_return("/path/to/file: Eicar-Test-Signature FOUND")
+      allow(scanner).to receive(:scan).and_raise(exception)
     end
 
     it "sets the state to infected if a virus is found" do
@@ -36,7 +46,7 @@
 
     it "sends an exception notification" do
       expect(GovukError).to receive(:notify).
-        with(VirusScanner::InfectedFile.new, extra: { error_message: "/path/to/file: Eicar-Test-Signature FOUND", id: asset.id, filename: asset.filename })
+        with(exception, extra: { id: asset.id, filename: asset.filename })
 
       worker.perform(asset.id)
     end