Merge pull request #280 from alphagov/refac-sync-2

Refactor sync to be less repetitive

app/services/discovery_engine/sync/delete.rb

@@ -1,47 +1,31 @@
 module DiscoveryEngine::Sync
-  class Delete
-    include DocumentName
-    include Locking
-    include Logging
-
-    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
-
+  class Delete < Operation
     def call
-      with_locked_document(content_id, payload_version:) do
-        if outdated_payload_version?(content_id, payload_version:)
+      with_locked_document do
+        if outdated_payload_version?
           log(
             Logger::Severity::INFO,
-            "Ignored as newer version (#{latest_synced_version(content_id)}) already synced",
-            content_id:, payload_version:,
+            "Ignored as newer version (#{latest_synced_version}) already synced",
           )
           Metrics::Exported.increment_counter(
             :discovery_engine_requests, type: "delete", status: "ignored_outdated"
           )
           return
         end
 
-        client.delete_document(name: document_name(content_id))
+        client.delete_document(name: document_name)
 
-        set_latest_synced_version(content_id, payload_version)
+        set_latest_synced_version
       end
 
-      log(Logger::Severity::INFO, "Successfully deleted", content_id:, payload_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}).",
-        content_id:, payload_version:,
       )
       Metrics::Exported.increment_counter(
         :discovery_engine_requests, type: "delete", status: "already_not_present"
@@ -50,16 +34,11 @@ def call
       log(
         Logger::Severity::ERROR,
         "Failed to delete document due to an error (#{e.message})",
-        content_id:, payload_version:,
       )
       GovukError.notify(e)
       Metrics::Exported.increment_counter(
         :discovery_engine_requests, type: "delete", status: "error"
       )
     end
-
-  private
-
-    attr_reader :content_id, :payload_version, :client
   end
 end

app/services/discovery_engine/sync/locking.rb

@@ -16,7 +16,7 @@ module Locking
 
     # Locks a document while a critical section block is executed to avoid multiple workers
     # competing to update the same document.
-    def with_locked_document(content_id, payload_version:, &critical_section)
+    def with_locked_document(&critical_section)
       redlock_client.lock!(
         "#{LOCK_KEY_PREFIX}:#{content_id}",
         DOCUMENT_LOCK_TTL,
@@ -49,30 +49,29 @@ def with_locked_document(content_id, payload_version:, &critical_section)
       critical_section.call
     end
 
-    def outdated_payload_version?(content_id, payload_version:)
+    def outdated_payload_version?
       # Sense check: This shouldn't ever come through as nil from Publishing API, but if it does,
       # the only really useful thing we can do is ignore this check entirely because we can't
       # meaningfully make a comparison.
       return false if payload_version.nil?
 
       # If there is no remote version yet, our version is always newer by definition
-      remote_version = latest_synced_version(content_id)
-      return false if remote_version.nil?
+      return false if latest_synced_version.nil?
 
-      remote_version.to_i >= payload_version.to_i
+      latest_synced_version.to_i >= payload_version.to_i
     end
 
     # Gets the latest synced version for a document from Redis
-    def latest_synced_version(content_id)
+    def latest_synced_version
       Rails.application.config.redis_pool.with do |redis|
         redis.get("#{VERSION_KEY_PREFIX}:#{content_id}")&.to_i
       end
     end
 
     # Sets the latest synced version for a document in Redis
-    def set_latest_synced_version(content_id, version)
+    def set_latest_synced_version
       Rails.application.config.redis_pool.with do |redis|
-        redis.set("#{VERSION_KEY_PREFIX}:#{content_id}", version)
+        redis.set("#{VERSION_KEY_PREFIX}:#{content_id}", payload_version)
       end
     end
 

app/services/discovery_engine/sync/operation.rb

@@ -0,0 +1,30 @@
+module DiscoveryEngine::Sync
+  class Operation
+    include Locking
+
+    def initialize(content_id, payload_version: nil, client: nil)
+      @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
+
+    def document_name
+      "#{Rails.configuration.discovery_engine_datastore_branch}/documents/#{content_id}"
+    end
+
+    def log(level, message)
+      combined_message = sprintf(
+        "[%s] %s content_id:%s payload_version:%d",
+        self.class.name,
+        message,
+        content_id,
+        payload_version,
+      )
+      Rails.logger.add(level, combined_message)
+    end
+  end
+end

app/services/discovery_engine/sync/put.rb

@@ -1,30 +1,20 @@
 module DiscoveryEngine::Sync
-  class Put
+  class Put < Operation
     MIME_TYPE = "text/html".freeze
 
-    include DocumentName
-    include Locking
-    include Logging
+    def initialize(content_id, metadata = nil, content: "", payload_version: nil, client: nil)
+      super(content_id, payload_version:, client:)
 
-    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
-      with_locked_document(content_id, payload_version:) do
-        if outdated_payload_version?(content_id, payload_version:)
+      with_locked_document do
+        if outdated_payload_version?
           log(
             Logger::Severity::INFO,
-            "Ignored as newer version (#{latest_synced_version(content_id)}) already synced",
-            content_id:, payload_version:,
+            "Ignored as newer version (#{latest_synced_version}) already synced",
           )
           Metrics::Exported.increment_counter(
             :discovery_engine_requests, type: "put", status: "ignored_outdated"
@@ -35,7 +25,7 @@ def call
         client.update_document(
           document: {
             id: content_id,
-            name: document_name(content_id),
+            name: document_name,
             json_data: metadata.merge(payload_version:).to_json,
             content: {
               mime_type: MIME_TYPE,
@@ -46,25 +36,24 @@ def call
           allow_missing: true,
         )
 
-        set_latest_synced_version(content_id, payload_version)
+        set_latest_synced_version
       end
 
-      log(Logger::Severity::INFO, "Successfully added/updated", content_id:, payload_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})",
-        content_id:, payload_version:,
       )
       GovukError.notify(e)
       Metrics::Exported.increment_counter(:discovery_engine_requests, type: "put", status: "error")
     end
 
   private
 
-    attr_reader :content_id, :metadata, :content, :payload_version, :client
+    attr_reader :metadata, :content
   end
 end

spec/services/discovery_engine/sync/locking_spec.rb

@@ -1,8 +1,9 @@
 RSpec.describe DiscoveryEngine::Sync::Locking do
-  subject(:lockable) { Class.new.include(described_class).new }
+  subject(:lockable) { DiscoveryEngine::Sync::Operation.new(content_id, payload_version:, client:) }
 
   let(:content_id) { "some-content-id" }
   let(:payload_version) { 10 }
+  let(:client) { double("Google::Cloud::DiscoveryEngine::V1::DocumentService") }
 
   let(:redis_client) { double("Redis Client") }
 
@@ -11,7 +12,7 @@
   end
 
   describe "#outdated_payload_version?" do
-    subject(:outdated_payload_version) { lockable.outdated_payload_version?(content_id, payload_version:) }
+    subject(:outdated_payload_version) { lockable.outdated_payload_version? }
 
     let(:remote_version) { 42 }