From 52e393bb5a0183a290f173e79ec12c03082c41bf Mon Sep 17 00:00:00 2001 From: "Michael J. Giarlo" Date: Wed, 15 Jan 2025 13:33:01 -0800 Subject: [PATCH] Add more context to HB when editing via QuickMARC Errors from the QuickMARC API were previously being eaten. This gives us more actionable exceptions and alerts. --- .rubocop_todo.yml | 4 +-- app/services/catalog/folio_writer.rb | 33 ++++++++++++++++------ spec/services/catalog/folio_writer_spec.rb | 31 ++++++++++++++------ 3 files changed, 49 insertions(+), 19 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index d90b9b2f0..37255caba 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -17,10 +17,10 @@ Lint/SuppressedException: Metrics/AbcSize: Max: 39 -# Offense count: 6 +# Offense count: 7 # Configuration parameters: CountComments, CountAsOne. Metrics/ClassLength: - Max: 114 + Max: 118 # Offense count: 15 # Configuration parameters: AllowedMethods, AllowedPatterns. diff --git a/app/services/catalog/folio_writer.rb b/app/services/catalog/folio_writer.rb index a064417bd..d6af37b57 100644 --- a/app/services/catalog/folio_writer.rb +++ b/app/services/catalog/folio_writer.rb @@ -19,6 +19,12 @@ def initialize(cocina_object:, marc_856_data:) def save return if catalog_record_ids.empty? && previous_catalog_record_ids.empty? + Honeybadger.context( + druid: cocina_object.externalIdentifier, + catalog_record_ids:, + previous_catalog_record_ids: + ) + # remove 856 for previous catkeys previous_catalog_record_ids.each { |previous_id| delete_previous_ids(catalog_record_id: previous_id, ignore_not_found: true) } @@ -31,10 +37,12 @@ def save attr_reader :marc_856_data, :cocina_object def delete_previous_ids(catalog_record_id:, ignore_not_found: false) - FolioClient.edit_marc_json(hrid: catalog_record_id) do |marc_json| + response = FolioClient.edit_marc_json(hrid: catalog_record_id) do |marc_json| marc_json['fields'].reject! { |field| (field['tag'] == '856') && field['content'].include?(purl_no_protocol) } end + validate_quickmarc_response!(response) + retry_lookup do # check that update has completed in FOLIO raise StandardError, 'PURL still found in instance record after update.' if instance_has_purl?(catalog_record_id:) @@ -47,11 +55,13 @@ def delete_previous_ids(catalog_record_id:, ignore_not_found: false) end def update_current_ids(catalog_record_id:) - FolioClient.edit_marc_json(hrid: catalog_record_id) do |marc_json| + response = FolioClient.edit_marc_json(hrid: catalog_record_id) do |marc_json| marc_json['fields'].reject! { |field| (field['tag'] == '856') && field['content'].include?(purl_no_protocol) } marc_json['fields'] << marc_856_field if ReleaseTagService.released_to_searchworks?(cocina_object:) end + validate_quickmarc_response!(response) + retry_lookup do if ReleaseTagService.released_to_searchworks?(cocina_object:) raise StandardError, 'No matching PURL found in instance record after update.' unless instance_has_purl?(catalog_record_id:) @@ -64,24 +74,29 @@ def update_current_ids(catalog_record_id:) end end + # @param [Hash,NilClass] response quickmarc response e.g., + # {"message"=>"Record is too long to be a valid MARC binary record, it's length would be 100106 which is more than 99999 bytes", "type"=>"-4", "code"=>"INTERNAL_SERVER_ERROR"} + def validate_quickmarc_response!(response) + return if response.nil? + + Honeybadger.context(quickmarc_response: response) + + raise response['message'] + end + def retry_lookup @try_count ||= 0 yield rescue StandardError => e @try_count += 1 Rails.logger.warn "Retrying Folio client operation for #{cocina_object.externalIdentifier} (#{@try_count} tries)" + if @try_count <= MAX_TRIES sleep Settings.catalog.folio.sleep_seconds retry end - Honeybadger.notify( - 'Error updating Folio record', - error_message: e.message, - context: { - druid: cocina_object.externalIdentifier - } - ) + Honeybadger.notify('Error updating Folio record', error_message: e.message) raise StandardError, 'FOLIO update not completed.' end diff --git a/spec/services/catalog/folio_writer_spec.rb b/spec/services/catalog/folio_writer_spec.rb index fd2e3cad6..de3c6b1c4 100644 --- a/spec/services/catalog/folio_writer_spec.rb +++ b/spec/services/catalog/folio_writer_spec.rb @@ -103,7 +103,7 @@ before do allow(CocinaObjectStore).to receive(:find).and_return(cocina_object) - allow(FolioClient).to receive(:edit_marc_json).and_yield(folio_response_json) + allow(FolioClient).to receive(:edit_marc_json).and_yield(folio_response_json).and_return(nil) allow(ReleaseTagService).to receive(:released_to_searchworks?).and_return(release_data) allow(FolioClient).to receive_messages(fetch_instance_info: instance_record, fetch_marc_hash: source_record) allow(Honeybadger).to receive(:notify) @@ -140,6 +140,24 @@ end end + context 'when edit operation returns errors' do + let(:error_response) do + JSON.parse({ + message: "Record is too long to be a valid MARC binary record, it's length would be 100106 which is more than 99999 bytes", + type: '-4', + code: 'INTERNAL_SERVER_ERROR' + }.to_json) + end + + before do + allow(FolioClient).to receive(:edit_marc_json).and_raise(FolioClient::ResourceNotFound).and_return(error_response) + end + + it 'raises a RuntimeError with the given response message' do + expect { folio_writer.save }.to raise_error(/Record is too long to be a valid MARC binary record/) + end + end + context 'when existing 856 PURL uses http://' do let(:folio_response_json) do { parsedRecordId: '1ab23862-46db-4da9-af5b-633adbf5f90f', @@ -275,8 +293,7 @@ expect(Honeybadger).to have_received(:notify) .with( 'Error updating Folio record', - error_message: 'No matching PURL found in instance record after update.', - context: { druid: } + error_message: 'No matching PURL found in instance record after update.' ) .once end @@ -310,8 +327,7 @@ expect(Honeybadger).to have_received(:notify) .with( 'Error updating Folio record', - error_message: 'More than one matching field with a PURL found on FOLIO record.', - context: { druid: } + error_message: 'More than one matching field with a PURL found on FOLIO record.' ) .once end @@ -354,8 +370,7 @@ expect(Honeybadger).to have_received(:notify) .with( 'Error updating Folio record', - error_message: 'PURL still found in instance record after update.', - context: { druid: } + error_message: 'PURL still found in instance record after update.' ) .once end @@ -459,7 +474,7 @@ before do allow(FolioClient).to receive(:fetch_instance_info).and_return(instance_record_unreleased) allow(FolioClient).to receive(:edit_marc_json).with(hrid: 'a8832160').and_raise(FolioClient::ResourceNotFound) - allow(FolioClient).to receive(:edit_marc_json).with(hrid: 'a8832161').and_yield(folio_response_json) + allow(FolioClient).to receive(:edit_marc_json).with(hrid: 'a8832161').and_yield(folio_response_json).and_return(nil) end it 'updates MARC record that exists to not include the 856' do