Refactor document syncing

Extract common logic into `Operation` from subclasses, so they can focus
on the unique behaviour for each respective operation.

app/services/discovery_engine/sync/delete.rb

@@ -1,46 +1,19 @@
 module DiscoveryEngine::Sync
   class Delete < Operation
-    def call
-      lock.acquire
+    def initialize(content_id, payload_version: nil, client: nil)
+      super(:delete, content_id, payload_version:, client:)
+    end
 
-      unless version_cache.sync_required?
+    def call
+      sync do
+        client.delete_document(name: document_name)
+      rescue Google::Cloud::NotFoundError => e
+        increment_counter("already_not_present")
         log(
           Logger::Severity::INFO,
-          "Ignored as newer version already synced",
-        )
-        Metrics::Exported.increment_counter(
-          :discovery_engine_requests, type: "delete", status: "ignored_outdated"
+          "Did not delete document as it doesn't exist remotely (#{e.message}).",
         )
-        return
       end
-
-      client.delete_document(name: document_name)
-
-      version_cache.set_as_latest_synced_version
-
-      log(Logger::Severity::INFO, "Successfully deleted")
-      Metrics::Exported.increment_counter(
-        :discovery_engine_requests, type: "delete", status: "success"
-      )
-    rescue Google::Cloud::NotFoundError => e
-      log(
-        Logger::Severity::INFO,
-        "Did not delete document as it doesn't exist remotely (#{e.message}).",
-      )
-      Metrics::Exported.increment_counter(
-        :discovery_engine_requests, type: "delete", status: "already_not_present"
-      )
-    rescue Google::Cloud::Error => e
-      log(
-        Logger::Severity::ERROR,
-        "Failed to delete document due to an error (#{e.message})",
-      )
-      GovukError.notify(e)
-      Metrics::Exported.increment_counter(
-        :discovery_engine_requests, type: "delete", status: "error"
-      )
-    ensure
-      lock.release
     end
   end
 end

app/services/discovery_engine/sync/operation.rb

@@ -1,14 +1,37 @@
 module DiscoveryEngine::Sync
   class Operation
-    def initialize(content_id, payload_version: nil, client: nil)
+    def initialize(type, content_id, payload_version: nil, client: nil)
+      @type = type
       @content_id = content_id
       @payload_version = payload_version
       @client = client || ::Google::Cloud::DiscoveryEngine.document_service(version: :v1)
     end
 
   private
 
-    attr_reader :content_id, :payload_version, :client
+    attr_reader :type, :content_id, :payload_version, :client
+
+    def sync
+      lock.acquire
+
+      if version_cache.sync_required?
+        yield
+
+        version_cache.set_as_latest_synced_version
+
+        increment_counter("success")
+        log(Logger::Severity::INFO, "Successful #{type}")
+      else
+        increment_counter("ignored_outdated")
+        log(Logger::Severity::INFO, "Ignored as newer version already synced")
+      end
+    rescue Google::Cloud::Error => e
+      increment_counter("error")
+      log(Logger::Severity::ERROR, "Failed to #{type} document due to an error (#{e.message})")
+      GovukError.notify(e)
+    ensure
+      lock.release
+    end
 
     def lock
       @lock ||= Coordination::DocumentLock.new(content_id)
@@ -32,5 +55,9 @@ def log(level, message)
       )
       Rails.logger.add(level, combined_message)
     end
+
+    def increment_counter(status)
+      Metrics::Exported.increment_counter(:discovery_engine_requests, type:, status:)
+    end
   end
 end

app/services/discovery_engine/sync/put.rb

@@ -3,55 +3,28 @@ class Put < Operation
     MIME_TYPE = "text/html".freeze
 
     def initialize(content_id, metadata = nil, content: "", payload_version: nil, client: nil)
-      super(content_id, payload_version:, client:)
+      super(:put, content_id, payload_version:, client:)
 
       @metadata = metadata
       @content = content
     end
 
     def call
-      lock.acquire
-
-      unless version_cache.sync_required?
-        log(
-          Logger::Severity::INFO,
-          "Ignored as newer version already synced",
-        )
-        Metrics::Exported.increment_counter(
-          :discovery_engine_requests, type: "put", status: "ignored_outdated"
+      sync do
+        client.update_document(
+          document: {
+            id: content_id,
+            name: document_name,
+            json_data: metadata.merge(payload_version:).to_json,
+            content: {
+              mime_type: MIME_TYPE,
+              # The Google client expects an IO object to extract raw byte content from
+              raw_bytes: StringIO.new(content),
+            },
+          },
+          allow_missing: true,
         )
-        return
       end
-
-      client.update_document(
-        document: {
-          id: content_id,
-          name: document_name,
-          json_data: metadata.merge(payload_version:).to_json,
-          content: {
-            mime_type: MIME_TYPE,
-            # The Google client expects an IO object to extract raw byte content from
-            raw_bytes: StringIO.new(content),
-          },
-        },
-        allow_missing: true,
-      )
-
-      version_cache.set_as_latest_synced_version
-
-      log(Logger::Severity::INFO, "Successfully added/updated")
-      Metrics::Exported.increment_counter(
-        :discovery_engine_requests, type: "put", status: "success"
-      )
-    rescue Google::Cloud::Error => e
-      log(
-        Logger::Severity::ERROR,
-        "Failed to add/update document due to an error (#{e.message})",
-      )
-      GovukError.notify(e)
-      Metrics::Exported.increment_counter(:discovery_engine_requests, type: "put", status: "error")
-    ensure
-      lock.release
     end
 
   private

spec/services/discovery_engine/sync/delete_spec.rb

@@ -34,7 +34,7 @@
     it "logs the delete operation" do
       expect(logger).to have_received(:add).with(
         Logger::Severity::INFO,
-        "[DiscoveryEngine::Sync::Delete] Successfully deleted content_id:some_content_id payload_version:1",
+        "[DiscoveryEngine::Sync::Delete] Successful delete content_id:some_content_id payload_version:1",
       )
     end
 

spec/services/discovery_engine/sync/put_spec.rb

@@ -59,7 +59,7 @@
     it "logs the put operation" do
       expect(logger).to have_received(:add).with(
         Logger::Severity::INFO,
-        "[DiscoveryEngine::Sync::Put] Successfully added/updated content_id:some_content_id payload_version:1",
+        "[DiscoveryEngine::Sync::Put] Successful put content_id:some_content_id payload_version:1",
       )
     end
   end
@@ -123,7 +123,7 @@
     it "logs the failure" do
       expect(logger).to have_received(:add).with(
         Logger::Severity::ERROR,
-        "[DiscoveryEngine::Sync::Put] Failed to add/update document due to an error (Something went wrong) content_id:some_content_id payload_version:1",
+        "[DiscoveryEngine::Sync::Put] Failed to put document due to an error (Something went wrong) content_id:some_content_id payload_version:1",
       )
     end