Merge pull request #251 from alphagov/fix-payload-version-check

Fix incorrect document locking version check

app/services/discovery_engine/sync/delete.rb

@@ -10,7 +10,7 @@ def initialize(client: ::Google::Cloud::DiscoveryEngine.document_service(version
 
     def call(content_id, payload_version: nil)
       with_locked_document(content_id, payload_version:) do
-        if payload_newer_than_remote?(content_id, payload_version:)
+        if outdated_payload_version?(content_id, payload_version:)
           log(
             Logger::Severity::INFO,
             "Ignored as newer version (#{latest_synced_version(content_id)}) already synced",

app/services/discovery_engine/sync/locking.rb

@@ -49,15 +49,15 @@ def with_locked_document(content_id, payload_version:, &critical_section)
       critical_section.call
     end
 
-    def payload_newer_than_remote?(content_id, payload_version:)
+    def outdated_payload_version?(content_id, 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 true if payload_version.nil?
+      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 true if remote_version.nil?
+      return false if remote_version.nil?
 
       remote_version.to_i >= payload_version.to_i
     end

app/services/discovery_engine/sync/put.rb

@@ -12,7 +12,7 @@ def initialize(client: ::Google::Cloud::DiscoveryEngine.document_service(version
 
     def call(content_id, metadata, content: "", payload_version: nil)
       with_locked_document(content_id, payload_version:) do
-        if payload_newer_than_remote?(content_id, payload_version:)
+        if outdated_payload_version?(content_id, payload_version:)
           log(
             Logger::Severity::INFO,
             "Ignored as newer version (#{latest_synced_version(content_id)}) already synced",

spec/services/discovery_engine/sync/delete_spec.rb

@@ -68,6 +68,34 @@
     end
   end
 
+  context "when there is no remote version yet" do
+    before do
+      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")
+    end
+
+    it "deletes the document" do
+      expect(client).to have_received(:delete_document)
+        .with(name: "branch/documents/some_content_id")
+    end
+
+    it "sets the new latest remote version" do
+      expect(redis_client).to have_received(:set).with(
+        "search_api_v2:latest_synced_version:some_content_id",
+        "1",
+      )
+    end
+
+    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",
+      )
+    end
+  end
+
   context "when locking the document fails" do
     let(:error) { Redlock::LockError.new("resource") }
 

spec/services/discovery_engine/sync/locking_spec.rb

@@ -0,0 +1,54 @@
+RSpec.describe DiscoveryEngine::Sync::Locking do
+  subject(:lockable) { Class.new.include(described_class).new }
+
+  let(:content_id) { "some-content-id" }
+  let(:payload_version) { 10 }
+
+  let(:redis_client) { double("Redis Client") }
+
+  before do
+    allow(Rails.application.config.redis_pool).to receive(:with).and_yield(redis_client)
+  end
+
+  describe "#outdated_payload_version?" do
+    subject(:outdated_payload_version) { lockable.outdated_payload_version?(content_id, payload_version:) }
+
+    let(:remote_version) { 42 }
+
+    before do
+      allow(redis_client).to receive(:get)
+        .with("search_api_v2:latest_synced_version:some-content-id")
+        .and_return(remote_version.to_s)
+    end
+
+    context "when payload_version is nil" do
+      let(:payload_version) { nil }
+
+      it { is_expected.to be(false) }
+    end
+
+    context "when there is no remote version" do
+      let(:remote_version) { nil }
+
+      it { is_expected.to be(false) }
+    end
+
+    context "when remote version is equal to payload version" do
+      let(:remote_version) { payload_version }
+
+      it { is_expected.to be(true) }
+    end
+
+    context "when remote version is greater than payload version" do
+      let(:remote_version) { payload_version + 1 }
+
+      it { is_expected.to be(true) }
+    end
+
+    context "when remote version is less than payload version" do
+      let(:remote_version) { payload_version - 1 }
+
+      it { is_expected.to be(false) }
+    end
+  end
+end

spec/services/discovery_engine/sync/put_spec.rb

@@ -99,6 +99,38 @@
     end
   end
 
+  context "when there is no remote version yet" do
+    before do
+      allow(redis_client).to receive(:get)
+        .with("search_api_v2:latest_synced_version:some_content_id").and_return(nil)
+
+      put.call(
+        "some_content_id",
+        { foo: "bar" },
+        content: "some content",
+        payload_version: "1",
+      )
+    end
+
+    it "updates the document" do
+      expect(client).to have_received(:update_document)
+    end
+
+    it "sets the new latest remote version" do
+      expect(redis_client).to have_received(:set).with(
+        "search_api_v2:latest_synced_version:some_content_id",
+        "1",
+      )
+    end
+
+    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",
+      )
+    end
+  end
+
   context "when locking the document fails" do
     let(:error) { Redlock::LockError.new("resource") }