Merge pull request #279 from alphagov/refac-sync

Use new Put/Delete service instance per document

app/models/publishing_api_document.rb

@@ -7,8 +7,8 @@ class PublishingApiDocument
 
   def initialize(
     document_hash,
-    put_service: DiscoveryEngine::Sync::Put.new,
-    delete_service: DiscoveryEngine::Sync::Delete.new
+    put_service: DiscoveryEngine::Sync::Put,
+    delete_service: DiscoveryEngine::Sync::Delete
   )
     @document_hash = document_hash
     @put_service = put_service
@@ -25,11 +25,11 @@ def synchronize
     elsif sync?
       log("sync")
       Metrics::Exported.increment_counter(:documents_synced)
-      put_service.call(content_id, metadata, content:, payload_version:)
+      put_service.new(content_id, metadata, content:, payload_version:).call
     elsif desync?
       log("desync (#{action_reason}))")
       Metrics::Exported.increment_counter(:documents_desynced)
-      delete_service.call(content_id, payload_version:)
+      delete_service.new(content_id, payload_version:).call
     else
       raise "Cannot determine action for document: #{content_id}"
     end

app/services/discovery_engine/sync/delete.rb

@@ -4,11 +4,17 @@ class Delete
     include Locking
     include Logging
 
-    def initialize(client: ::Google::Cloud::DiscoveryEngine.document_service(version: :v1))
+    def initialize(
+      content_id = nil, payload_version: nil,
+      client: ::Google::Cloud::DiscoveryEngine.document_service(version: :v1)
+    )
+      @content_id = content_id
+      @payload_version = payload_version
+
       @client = client
     end
 
-    def call(content_id, payload_version: nil)
+    def call
       with_locked_document(content_id, payload_version:) do
         if outdated_payload_version?(content_id, payload_version:)
           log(
@@ -54,6 +60,6 @@ def call(content_id, payload_version: nil)
 
   private
 
-    attr_reader :client
+    attr_reader :content_id, :payload_version, :client
   end
 end

app/services/discovery_engine/sync/put.rb

@@ -6,11 +6,19 @@ class Put
     include Locking
     include Logging
 
-    def initialize(client: ::Google::Cloud::DiscoveryEngine.document_service(version: :v1))
+    def initialize(
+      content_id = nil, metadata = nil, content: "", payload_version: nil,
+      client: ::Google::Cloud::DiscoveryEngine.document_service(version: :v1)
+    )
+      @content_id = content_id
+      @metadata = metadata
+      @content = content
+      @payload_version = payload_version
+
       @client = client
     end
 
-    def call(content_id, metadata, content: "", payload_version: nil)
+    def call
       with_locked_document(content_id, payload_version:) do
         if outdated_payload_version?(content_id, payload_version:)
           log(
@@ -57,6 +65,6 @@ def call(content_id, metadata, content: "", payload_version: nil)
 
   private
 
-    attr_reader :client
+    attr_reader :content_id, :metadata, :content, :payload_version, :client
   end
 end

spec/integration/document_synchronization_spec.rb

@@ -19,7 +19,7 @@
     let(:payload) { json_fixture_as_hash("message_queue/press_release_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "5941cb22-5d52-4212-83b6-255d75d2c680",
         {
           content_id: "5941cb22-5d52-4212-83b6-255d75d2c680",
@@ -47,14 +47,15 @@
         content: a_string_including("<div class=\"govspeak\"><p>The UK was represented remotely"),
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for a 'travel_advice' message" do
     let(:payload) { json_fixture_as_hash("message_queue/travel_advice_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "b662d0a3-c20d-4167-8056-b9c7d058d860",
         {
           content_id: "b662d0a3-c20d-4167-8056-b9c7d058d860",
@@ -110,14 +111,15 @@
         content: a_string_including("<h1>Warnings and insurance</h1>\n<p>The Foreign"),
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for a historic 'news_story' message" do
     let(:payload) { json_fixture_as_hash("message_queue/historic_news_story_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "5c880596-7631-11e4-a3cb-005056011aef",
         {
           content_id: "5c880596-7631-11e4-a3cb-005056011aef",
@@ -148,14 +150,15 @@
         content: a_string_including("<div class=\"govspeak\"><p>In the UEFA Champions"),
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for a 'manual_section' message" do
     let(:payload) { json_fixture_as_hash("message_queue/manual_section_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "e1f47495-b58d-41ca-84bb-ccb2b751cc3f",
         {
           content_id: "e1f47495-b58d-41ca-84bb-ccb2b751cc3f",
@@ -179,14 +182,15 @@
         content: a_string_matching(/<h2 id="section-6-1">6\.1\. Structure.+<\/table>\n\n/m),
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for an 'hmrc_manual_section' message" do
     let(:payload) { json_fixture_as_hash("message_queue/hmrc_manual_section_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "f1cb1b1f-d619-5694-9822-0e5ff9bfcb09",
         {
           content_id: "f1cb1b1f-d619-5694-9822-0e5ff9bfcb09",
@@ -209,14 +213,15 @@
         content: a_string_starting_with("Pre-trading expenses – Overview \n<p>The normal rules"),
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for a 'service_manual_guide' message" do
     let(:payload) { json_fixture_as_hash("message_queue/service_manual_guide_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "174c41e0-3316-4e9d-be46-6555d52f3cb7",
         {
           content_id: "174c41e0-3316-4e9d-be46-6555d52f3cb7",
@@ -240,14 +245,15 @@
         content: a_string_matching(/Make sure everyone can use the service/),
         payload_version: 1989,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for an 'organisation' message" do
     let(:payload) { json_fixture_as_hash("message_queue/organisation_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "6ba90ae6-972d-4d48-ad66-693bbb31496d",
         {
           content_id: "6ba90ae6-972d-4d48-ad66-693bbb31496d",
@@ -271,14 +277,15 @@
         content: a_string_including("LAA\n<div class=\"govspeak\"><p>We provide civil"),
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for an 'worldwide_organisation' message" do
     let(:payload) { json_fixture_as_hash("message_queue/worldwide_organisation_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "f4c394f9-7a30-11e4-a3cb-005056011aef",
         {
           content_id: "f4c394f9-7a30-11e4-a3cb-005056011aef",
@@ -307,14 +314,15 @@
         content: a_string_including("maintains and develops relations between the UK and Austria"),
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for an 'independent_report' message" do
     let(:payload) { json_fixture_as_hash("message_queue/independent_report_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "5d315ee8-7631-11e4-a3cb-005056011aef",
         {
           content_id: "5d315ee8-7631-11e4-a3cb-005056011aef",
@@ -351,14 +359,15 @@
         TEXT
         payload_version: 54_321,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for a 'taxon' message that isn't ignorelisted" do
     let(:payload) { json_fixture_as_hash("message_queue/world_taxon_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "f1724368-504f-4b3c-9dc2-41121046de9f",
         {
           content_id: "f1724368-504f-4b3c-9dc2-41121046de9f",
@@ -383,14 +392,15 @@
         TEXT
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for a 'speech' message" do
     let(:payload) { json_fixture_as_hash("message_queue/speech_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "5fac6be0-146e-40ea-a899-c3299f62eff9",
         {
           content_id: "5fac6be0-146e-40ea-a899-c3299f62eff9",
@@ -421,14 +431,15 @@
         content: a_string_including("Service of thanksgiving for the life of Her Majesty Queen Elizabeth II"),
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for an 'html_publication' message" do
     let(:payload) { json_fixture_as_hash("message_queue/html_publication_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "1f1f2c96-5a14-4d2a-9d0c-be6ac6c62c3b",
         {
           content_id: "1f1f2c96-5a14-4d2a-9d0c-be6ac6c62c3b",
@@ -448,14 +459,15 @@
         content: a_string_starting_with("Access Consultation Forum terms of reference"),
         payload_version: 12_345,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
   describe "for an 'external_content' message" do
     let(:payload) { json_fixture_as_hash("message_queue/external_content_message.json") }
 
     it "is added to Discovery Engine through the Put service" do
-      expect(put_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Put).to have_received(:new).with(
         "526d5caf-221b-4c7b-9e74-b3e0b189fc8d",
         {
           content_id: "526d5caf-221b-4c7b-9e74-b3e0b189fc8d",
@@ -477,6 +489,7 @@
         content: a_string_including("Brighton & Hove City Council"),
         payload_version: 17,
       )
+      expect(put_service).to have_received(:call)
     end
   end
 
@@ -492,21 +505,23 @@
     let(:payload) { json_fixture_as_hash("message_queue/withdrawn_notice_message.json") }
 
     it "is proactively deleted from Discovery Engine through the Delete service" do
-      expect(delete_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Delete).to have_received(:new).with(
         "e3b7c15d-1928-4101-9912-c9b40a6d6e78",
         payload_version: 12_345,
       )
+      expect(delete_service).to have_received(:call)
     end
   end
 
   describe "for a 'gone' message" do
     let(:payload) { json_fixture_as_hash("message_queue/gone_message.json") }
 
     it "is deleted from Discovery Engine through the Delete service" do
-      expect(delete_service).to have_received(:call).with(
+      expect(DiscoveryEngine::Sync::Delete).to have_received(:new).with(
         "0dd44b7a-3c10-5a3b-8bda-6ccb736102e0",
         payload_version: 12_345,
       )
+      expect(delete_service).to have_received(:call)
     end
   end
 end

spec/services/discovery_engine/sync/delete_spec.rb

@@ -1,6 +1,4 @@
 RSpec.describe DiscoveryEngine::Sync::Delete do
-  subject(:delete) { described_class.new(client:) }
-
   let(:client) { double("DocumentService::Client", delete_document: nil) }
   let(:logger) { double("Logger", add: nil) }
   let(:redlock_client) { instance_double(Redlock::Client) }
@@ -21,7 +19,7 @@
 
   context "when the delete succeeds" do
     before do
-      delete.call("some_content_id", payload_version: "1")
+      described_class.new("some_content_id", payload_version: "1", client:).call
     end
 
     it "deletes the document" do
@@ -49,7 +47,7 @@
       allow(redis_client).to receive(:get)
         .with("search_api_v2:latest_synced_version:some_content_id").and_return("42")
 
-      delete.call("some_content_id", payload_version: "1")
+      described_class.new("some_content_id", payload_version: "1", client:).call
     end
 
     it "does not delete the document" do
@@ -73,7 +71,7 @@
       allow(redis_client).to receive(:get)
         .with("search_api_v2:latest_synced_version:some_content_id").and_return(nil)
 
-      delete.call("some_content_id", payload_version: "1")
+      described_class.new("some_content_id", payload_version: "1", client:).call
     end
 
     it "deletes the document" do
@@ -102,7 +100,7 @@
     before do
       allow(redlock_client).to receive(:lock!).and_raise(error)
 
-      delete.call("some_content_id", payload_version: "1")
+      described_class.new("some_content_id", payload_version: "1", client:).call
     end
 
     it "deletes the document regardless" do
@@ -127,7 +125,7 @@
     before do
       allow(client).to receive(:delete_document).and_raise(err)
 
-      delete.call("some_content_id", payload_version: "1")
+      described_class.new("some_content_id", payload_version: "1", client:).call
     end
 
     it "logs the failure" do
@@ -148,7 +146,7 @@
     before do
       allow(client).to receive(:delete_document).and_raise(err)
 
-      delete.call("some_content_id", payload_version: "1")
+      described_class.new("some_content_id", payload_version: "1", client:).call
     end
 
     it "logs the failure" do