diff --git a/lib/lift_wing_api.rb b/lib/lift_wing_api.rb index 0bf6ac162f..ff603c6a1c 100644 --- a/lib/lift_wing_api.rb +++ b/lib/lift_wing_api.rb @@ -66,17 +66,13 @@ def get_single_revision_parsed_data(rev_id) response = lift_wing_server.post(quality_query_url, body) parsed_response = Oj.load(response.body) # If the responses contain an error, do not try to calculate wp10 or features. - if parsed_response.key? 'error' - return { 'wp10' => nil, 'features' => nil, 'deleted' => deleted?(parsed_response), - 'prediction' => nil, 'error' => parsed_response.dig('error') } - end + return build_error_response parsed_response.dig('error') if parsed_response.key? 'error' build_successful_response(rev_id, parsed_response) rescue StandardError => e tries -= 1 retry unless tries.zero? @errors << e - return { 'wp10' => nil, 'features' => nil, 'deleted' => false, 'prediction' => nil, - 'error' => e } + build_error_response e.to_s end class InvalidProjectError < StandardError @@ -121,6 +117,15 @@ def build_successful_response(rev_id, response) } end + def build_error_response(error) + deleted = deleted?(error) + error_response = { 'wp10' => nil, 'features' => nil, 'deleted' => deleted, 'prediction' => nil } + # Add error field only if the revision was not deleted + error_response['error'] = error unless deleted + + error_response + end + # TODO: monitor production for errors, understand them, put benign ones here TYPICAL_ERRORS = [].freeze @@ -133,9 +138,9 @@ def log_error_batch(rev_ids) error_count: @errors.count }) end - def deleted?(response) + def deleted?(error) LiftWingApi::DELETED_REVISION_ERRORS.any? do |revision_error| - response.dig('error').include?(revision_error) + error.include?(revision_error) end end end diff --git a/lib/reference_counter_api.rb b/lib/reference_counter_api.rb index abe4aaeb19..5cd9f9ab1b 100644 --- a/lib/reference_counter_api.rb +++ b/lib/reference_counter_api.rb @@ -58,9 +58,8 @@ def get_number_of_references_from_revision_id(rev_id) response = toolforge_server.get(references_query_url(rev_id)) parsed_response = Oj.load(response.body) return { 'num_ref' => parsed_response['num_ref'] } if response.status == 200 - # If the response is bad request, then the language and/or the project is not supported. - # Leave the error empty in this case, as it is not a transient error. - return { 'num_ref' => nil } if response.status == 400 + # Leave the error empty if it is not a transient error. + return { 'num_ref' => nil } if non_transient_error? response.status # Log the error and return empty hash # Sentry.capture_message 'Non-200 response hitting references counter API', level: 'warning', # extra: { project_code: @project_code, language_code: @language_code, rev_id:, @@ -89,6 +88,16 @@ def toolforge_server ) end + BAD_REQUEST = 400 + FORBIDDEN = 403 + NOT_FOUND = 404 + # A bad request response indicates that the language and/or project is not supported. + # A forbidden response likely means we lack permission to access the revision, + # possibly because it was deleted or hidden. + def non_transient_error?(status) + [BAD_REQUEST, FORBIDDEN, NOT_FOUND].include? status + end + TYPICAL_ERRORS = [Faraday::TimeoutError, Faraday::ConnectionFailed].freeze diff --git a/spec/lib/importers/revision_score_importer_spec.rb b/spec/lib/importers/revision_score_importer_spec.rb index b478d5d0ae..ca8ef0780f 100644 --- a/spec/lib/importers/revision_score_importer_spec.rb +++ b/spec/lib/importers/revision_score_importer_spec.rb @@ -222,7 +222,7 @@ expect(revisions[2].wp10).to be_nil expect(revisions[2].features).to eq({}) # TODO: use another field for this. For now we use ithenticate_id as an error field - expect(revisions[2].ithenticate_id).to eq(1) + expect(revisions[2].ithenticate_id).to be_nil end end @@ -233,7 +233,7 @@ expect(revisions[3].wp10).to be_nil expect(revisions[3].features).to eq({}) # TODO: use another field for this. For now we use ithenticate_id as an error field - expect(revisions[3].ithenticate_id).to eq(1) + expect(revisions[3].ithenticate_id).to be_nil end end diff --git a/spec/lib/lift_wing_api_spec.rb b/spec/lib/lift_wing_api_spec.rb index b48ecdd07a..21be25551c 100644 --- a/spec/lib/lift_wing_api_spec.rb +++ b/spec/lib/lift_wing_api_spec.rb @@ -77,7 +77,7 @@ VCR.use_cassette 'liftwing_api/deleted_revision' do expect(subject2).to be_a(Hash) expect(subject2.dig('708326238', 'deleted')).to eq(true) - expect(subject2.dig('708326238', 'error').class).to be(String) + expect(subject2.dig('708326238').key?('error')).to eq(false) end end diff --git a/spec/lib/reference_counter_api_spec.rb b/spec/lib/reference_counter_api_spec.rb index fd2906d2cb..c665aab381 100644 --- a/spec/lib/reference_counter_api_spec.rb +++ b/spec/lib/reference_counter_api_spec.rb @@ -41,10 +41,34 @@ context 'fails silently' do before do stub_wiki_validation - stub_not_supported_wiki_reference_counter_response end it 'if response is 400 bad request' do + stub_400_wiki_reference_counter_response + ref_counter_api = described_class.new(not_supported) + response = ref_counter_api.get_number_of_references_from_revision_ids rev_ids + expect(response.dig('5006940', 'num_ref')).to be_nil + expect(response.dig('5006940').key?('error')).to eq(false) + expect(response.dig('5006942', 'num_ref')).to be_nil + expect(response.dig('5006942').key?('error')).to eq(false) + expect(response.dig('5006946', 'num_ref')).to be_nil + expect(response.dig('5006946').key?('error')).to eq(false) + end + + it 'if response is 403 forbidden' do + stub_403_wiki_reference_counter_response + ref_counter_api = described_class.new(not_supported) + response = ref_counter_api.get_number_of_references_from_revision_ids rev_ids + expect(response.dig('5006940', 'num_ref')).to be_nil + expect(response.dig('5006940').key?('error')).to eq(false) + expect(response.dig('5006942', 'num_ref')).to be_nil + expect(response.dig('5006942').key?('error')).to eq(false) + expect(response.dig('5006946', 'num_ref')).to be_nil + expect(response.dig('5006946').key?('error')).to eq(false) + end + + it 'if response is 404 not found' do + stub_404_wiki_reference_counter_response ref_counter_api = described_class.new(not_supported) response = ref_counter_api.get_number_of_references_from_revision_ids rev_ids expect(response.dig('5006940', 'num_ref')).to be_nil diff --git a/spec/support/request_helpers.rb b/spec/support/request_helpers.rb index 7003835b87..da574273d2 100644 --- a/spec/support/request_helpers.rb +++ b/spec/support/request_helpers.rb @@ -736,7 +736,7 @@ def stub_en_wikipedia_reference_counter_reponse ) end - def stub_not_supported_wiki_reference_counter_response + def stub_400_wiki_reference_counter_response stub_request(:get, %r{https://reference-counter.toolforge.org/api/v1/references/wikimedia/incubator/\d+}) .to_return( status: 400, @@ -744,4 +744,24 @@ def stub_not_supported_wiki_reference_counter_response headers: { 'Content-Type' => 'application/json' } ) end + + def stub_403_wiki_reference_counter_response + stub_request(:get, %r{https://reference-counter.toolforge.org/api/v1/references/wikimedia/incubator/\d+}) + .to_return( + status: 403, + body: { 'description' => "mwapi error: permissiondenied - You don't have permission to view\ + deleted text or changes between deleted revisions." }.to_json, + headers: { 'Content-Type' => 'application/json' } + ) + end + + def stub_404_wiki_reference_counter_response + stub_request(:get, %r{https://reference-counter.toolforge.org/api/v1/references/wikimedia/incubator/\d+}) + .to_return( + status: 404, + body: { 'description' => 'rest-nonexistent-revision -\ + The specified revision does not exist' }.to_json, + headers: { 'Content-Type' => 'application/json' } + ) + end end