From 3d950971895a1ad2cb4fb94def8e731240e70b0f Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 27 Feb 2025 15:05:04 +0000 Subject: [PATCH] Simplify API client error handling The way we handle errors in API clients is a bit too complex, and too coupled to ActiveRecord (it sets model errors, possibly based on trying to parse an error string, and raises its own `ClientError`). This changes the clients to not rescue any errors from the remote API, but instead letting them bubble up to the caller, and moves the setting of record errors to the ActiveRecord-specific `RemoteSynchronizable` concern, paving the way for clients to be able to work with POROs as well. --- .../discovery_engine/control_client.rb | 27 ---------- .../discovery_engine/serving_config_client.rb | 5 -- .../publishing_api/content_item_client.rb | 6 --- app/models/concerns/remote_synchronizable.rb | 10 ++-- config/locales/en.yml | 12 ++--- .../discovery_engine/control_client_spec.rb | 52 ------------------- .../content_item_client_spec.rb | 36 ------------- .../concerns/remote_synchronizable.rb | 14 ++++- 8 files changed, 22 insertions(+), 140 deletions(-) diff --git a/app/clients/discovery_engine/control_client.rb b/app/clients/discovery_engine/control_client.rb index 8a609795..992dd92b 100644 --- a/app/clients/discovery_engine/control_client.rb +++ b/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 diff --git a/app/clients/discovery_engine/serving_config_client.rb b/app/clients/discovery_engine/serving_config_client.rb index d105bf61..6d985347 100644 --- a/app/clients/discovery_engine/serving_config_client.rb +++ b/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. diff --git a/app/clients/publishing_api/content_item_client.rb b/app/clients/publishing_api/content_item_client.rb index 7e6a6097..57332b31 100644 --- a/app/clients/publishing_api/content_item_client.rb +++ b/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 diff --git a/app/models/concerns/remote_synchronizable.rb b/app/models/concerns/remote_synchronizable.rb index 3c1209a8..2b70dc0e 100644 --- a/app/models/concerns/remote_synchronizable.rb +++ b/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 diff --git a/config/locales/en.yml b/config/locales/en.yml index 9cdde2f4..87b4ce57 100644 --- a/config/locales/en.yml +++ b/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: diff --git a/spec/clients/discovery_engine/control_client_spec.rb b/spec/clients/discovery_engine/control_client_spec.rb index 6126fa2f..1fd91a48 100644 --- a/spec/clients/discovery_engine/control_client_spec.rb +++ b/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 diff --git a/spec/clients/publishing_api/content_item_client_spec.rb b/spec/clients/publishing_api/content_item_client_spec.rb index fcdae056..cfb89acc 100644 --- a/spec/clients/publishing_api/content_item_client_spec.rb +++ b/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 diff --git a/spec/support/shared_examples/concerns/remote_synchronizable.rb b/spec/support/shared_examples/concerns/remote_synchronizable.rb index 45191029..5efa0fc0 100644 --- a/spec/support/shared_examples/concerns/remote_synchronizable.rb +++ b/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