Simplify API client error handling

app/clients/discovery_engine/control_client.rb

@@ -10,47 +10,20 @@ def create(control)
         control_id: control.remote_resource_id,
         parent: control.parent.name,
       )
-    rescue Google::Cloud::Error => e
-      set_record_errors(control, e)
-      raise ClientError, "Could not create control: #{e.message}"
     end
 
     # Updates the corresponding resource for this control on Discovery Engine.
     def update(control)
       control_service.update_control(control: control.to_discovery_engine_control)
-    rescue Google::Cloud::Error => e
-      set_record_errors(control, e)
-      raise ClientError, "Could not update control: #{e.message}"
     end
 
     # Deletes the corresponding resource for this control on Discovery Engine.
     def delete(control)
       control_service.delete_control(name: control.name)
-    rescue Google::Cloud::Error => e
-      set_record_errors(control, e)
-      raise ClientError, "Could not delete control: #{e.message}"
     end
 
   private
 
     attr_reader :control
-
-    def set_record_errors(control, error)
-      # There is no way to extract structured error information from the Google API client, so we
-      # have to resort to regex matching to see if we can extract the cause of the error.
-      #
-      # In this case, we know that if the error message contains "filter syntax", the user probably
-      # made a mistake entering the filter expression and we can attach the error to that field.
-      # Otherwise, we consider it an unknown error and make sure to log it.
-      case error.message
-      when /filter syntax/i
-        control.action.errors.add(:filter_expression, error.details)
-      else
-        control.errors.add(:base, :remote_error)
-
-        GovukError.notify(error)
-        Rails.logger.error(error.message)
-      end
-    end
   end
 end

app/clients/discovery_engine/serving_config_client.rb

@@ -21,11 +21,6 @@ def update(serving_config)
         # Ensure no fields other than the ones specified in the payload are updated
         update_mask: { paths: payload.keys.excluding(:name) },
       )
-    rescue Google::Cloud::Error => e
-      serving_config.errors.add(:base, :remote_error)
-
-      GovukError.notify(e)
-      Rails.logger.error(e.message)
     end
 
     # Deletes the corresponding resource for this serving config on Discovery Engine.

app/clients/publishing_api/content_item_client.rb

@@ -16,18 +16,12 @@ def create(record)
         record.to_publishing_api_content_item,
       )
       publishing_api_service.publish(record.content_id)
-    rescue GdsApi::BaseError => e
-      record.errors.add(:base, :remote_error)
-      raise ClientError, "Could not publish content item on Publishing API: #{e.message}"
     end
     alias_method :update, :create
 
     # Deletes the corresponding content item for this record on Publishing API.
     def delete(record)
       publishing_api_service.unpublish(record.content_id, type: UNPUBLISH_TYPE)
-    rescue GdsApi::BaseError => e
-      record.errors.add(:base, :remote_error)
-      raise ClientError, "Could not unpublish content item on Publishing API: #{e.message}"
     end
 
   private

app/models/concerns/remote_synchronizable.rb

@@ -26,8 +26,6 @@ def save_and_sync
 
       sync
     end
-  rescue ClientError
-    false
   end
 
   # Destroys the record and deletes its corresponding remote resource.
@@ -37,8 +35,6 @@ def destroy_and_sync
 
       sync
     end
-  rescue ClientError
-    false
   end
 
   # Synchonises the record with its corresponding remote resource, creating, updating or deleting it
@@ -51,6 +47,12 @@ def sync
     else
       client.update(self) # rubocop:disable Rails/SaveBang (not an ActiveRecord model)
     end
+  rescue StandardError => e
+    GovukError.notify(e)
+    Rails.logger.error(e)
+
+    errors.add(:base, :remote_error, error_message: e.cause&.message || e.message)
+    raise ActiveRecord::Rollback
   end
 
 private

config/locales/en.yml

@@ -119,15 +119,9 @@ en:
         controls: Attached controls
 
     errors:
-      models:
-        control:
-          remote_error: |
-            We couldn't save this control on Vertex AI Search because of an unexpected error.
-            This error has been logged. Please try again later.
-        recommended_link:
-          remote_error: |
-            We couldn't publish this recommended link to Publishing API because of an unexpected
-            error. This error has been logged. Please try again later.
+      messages:
+        remote_error: |
+          There was an error saving this record remotely: %{error_message}
 
   controls:
     index:

spec/clients/discovery_engine/control_client_spec.rb

@@ -35,46 +35,6 @@
 
       subject.update(control) # rubocop:disable Rails/SaveBang (not an ActiveRecord model)
     end
-
-    context "when the operation raises an arbitrary error" do
-      before do
-        allow(discovery_engine_client).to receive(:update_control).and_raise(Google::Cloud::Error)
-      end
-
-      it "raises a ClientInternalError and adds a base validation error" do
-        expect { subject.update(control) }.to raise_error(ClientError)
-
-        expect(control.errors).to be_of_kind(:base, :remote_error)
-      end
-    end
-
-    context "when the operation raises an invalid argument error about filter expressions" do
-      before do
-        allow(discovery_engine_client)
-          .to receive(:update_control)
-          .and_raise(Google::Cloud::InvalidArgumentError, "The filter syntax is broken")
-      end
-
-      it "raises a ClientInternalError and adds a field validation error on action" do
-        expect { subject.update(control) }.to raise_error(ClientError)
-
-        expect(control.action.errors).to be_of_kind(:filter_expression, :invalid)
-      end
-    end
-
-    context "when the operation raises an invalid argument error about anything else" do
-      before do
-        allow(discovery_engine_client)
-          .to receive(:update_control)
-          .and_raise(Google::Cloud::InvalidArgumentError, "The splines are unreticulated")
-      end
-
-      it "raises a ClientInternalError and adds a base validation error" do
-        expect { subject.update(control) }.to raise_error(ClientError)
-
-        expect(control.errors).to be_of_kind(:base, :remote_error)
-      end
-    end
   end
 
   describe "#delete" do
@@ -85,17 +45,5 @@
 
       subject.delete(control)
     end
-
-    context "when the operation raises an arbitrary error" do
-      before do
-        allow(discovery_engine_client).to receive(:delete_control).and_raise(Google::Cloud::Error)
-      end
-
-      it "raises a ClientInternalError and adds a base validation error" do
-        expect { subject.delete(control) }.to raise_error(ClientError)
-
-        expect(control.errors).to be_of_kind(:base, :remote_error)
-      end
-    end
   end
 end

spec/clients/publishing_api/content_item_client_spec.rb

@@ -30,18 +30,6 @@ def to_publishing_api_content_item
       assert_publishing_api_put_content("f00", { foo: "bar" })
       assert_publishing_api_publish("f00")
     end
-
-    context "when the operation raises an arbitrary error" do
-      before do
-        stub_publishing_api_isnt_available
-      end
-
-      it "raises a ClientError and adds a base validation error" do
-        expect { client.create(content_itemable) }.to raise_error(ClientError)
-
-        expect(content_itemable.errors).to be_of_kind(:base, :remote_error)
-      end
-    end
   end
 
   describe "#update" do
@@ -51,18 +39,6 @@ def to_publishing_api_content_item
       assert_publishing_api_put_content("f00", { foo: "bar" })
       assert_publishing_api_publish("f00")
     end
-
-    context "when the operation raises an arbitrary error" do
-      before do
-        stub_publishing_api_isnt_available
-      end
-
-      it "raises a ClientError and adds a base validation error" do
-        expect { client.update(content_itemable) }.to raise_error(ClientError)
-
-        expect(content_itemable.errors).to be_of_kind(:base, :remote_error)
-      end
-    end
   end
 
   describe "#delete" do
@@ -71,17 +47,5 @@ def to_publishing_api_content_item
 
       assert_publishing_api_unpublish("f00", type: "gone")
     end
-
-    context "when the operation raises an arbitrary error" do
-      before do
-        stub_publishing_api_isnt_available
-      end
-
-      it "raises a ClientError and adds a base validation error" do
-        expect { client.delete(content_itemable) }.to raise_error(ClientError)
-
-        expect(content_itemable.errors).to be_of_kind(:base, :remote_error)
-      end
-    end
   end
 end

spec/support/shared_examples/concerns/remote_synchronizable.rb

@@ -25,7 +25,7 @@
       end
 
       context "when the remote resource creation fails" do
-        let(:error) { ClientError.new("Uh oh") }
+        let(:error) { RuntimeError.new("Uh oh") }
 
         before do
           allow(client).to receive(:create).and_raise(error)
@@ -36,6 +36,10 @@
         it "stops the record from being created" do
           expect(record).not_to be_persisted
         end
+
+        it "adds an error to the record" do
+          expect(record.errors).to be_of_kind(:base, :remote_error)
+        end
       end
     end
 
@@ -61,6 +65,10 @@
         it "stops the record from being persisted" do
           expect(record).to be_changed
         end
+
+        it "adds an error to the record" do
+          expect(record.errors).to be_of_kind(:base, :remote_error)
+        end
       end
     end
   end
@@ -86,6 +94,10 @@
       it "stops the record from being destroyed" do
         expect(described_class.exists?(record.id)).to be(true)
       end
+
+      it "adds an error to the record" do
+        expect(record.errors).to be_of_kind(:base, :remote_error)
+      end
     end
   end
 end