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