diff --git a/app/services/discovery_engine/sync/delete.rb b/app/services/discovery_engine/sync/delete.rb index 8ad71ea..ac1a6c3 100644 --- a/app/services/discovery_engine/sync/delete.rb +++ b/app/services/discovery_engine/sync/delete.rb @@ -1,26 +1,11 @@ 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" @@ -28,12 +13,12 @@ def call 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" ) @@ -41,7 +26,6 @@ def call 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 diff --git a/app/services/discovery_engine/sync/document_name.rb b/app/services/discovery_engine/sync/document_name.rb deleted file mode 100644 index e957222..0000000 --- a/app/services/discovery_engine/sync/document_name.rb +++ /dev/null @@ -1,7 +0,0 @@ -module DiscoveryEngine::Sync - module DocumentName - def document_name(content_id) - "#{Rails.configuration.discovery_engine_datastore_branch}/documents/#{content_id}" - end - end -end diff --git a/app/services/discovery_engine/sync/locking.rb b/app/services/discovery_engine/sync/locking.rb index 2cd2ee5..d3e64af 100644 --- a/app/services/discovery_engine/sync/locking.rb +++ b/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 diff --git a/app/services/discovery_engine/sync/logging.rb b/app/services/discovery_engine/sync/logging.rb deleted file mode 100644 index b0e96af..0000000 --- a/app/services/discovery_engine/sync/logging.rb +++ /dev/null @@ -1,14 +0,0 @@ -module DiscoveryEngine::Sync - module Logging - def log(level, message, content_id:, payload_version:) - 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 diff --git a/app/services/discovery_engine/sync/operation.rb b/app/services/discovery_engine/sync/operation.rb new file mode 100644 index 0000000..d798308 --- /dev/null +++ b/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 diff --git a/app/services/discovery_engine/sync/put.rb b/app/services/discovery_engine/sync/put.rb index 45d1737..157ca17 100644 --- a/app/services/discovery_engine/sync/put.rb +++ b/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,10 +36,10 @@ 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" ) @@ -57,7 +47,6 @@ def call 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") @@ -65,6 +54,6 @@ def call private - attr_reader :content_id, :metadata, :content, :payload_version, :client + attr_reader :metadata, :content end end diff --git a/spec/services/discovery_engine/sync/locking_spec.rb b/spec/services/discovery_engine/sync/locking_spec.rb index 0b9ee97..3230ca3 100644 --- a/spec/services/discovery_engine/sync/locking_spec.rb +++ b/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 }