[Data rearchitecture] Do not propagate error if revision was deleted in reference-counter and liftwing APIs (#6176)

* Add 403 forbidden responses to non transient errors for reference counter API. Usually, this error means that we lack permission to access the revision, possibly because it was deleted or hidden. That is not atransient error, so we don't want to mark the timeslice to be reprocessed.

* Add 404 not found responses to non transient errors for reference counter API. This error occurs for some deleted revisions.

* Do not return error field in LiftWingAPI response if the revision was deleted. That's not a transient error so keeping retrying the lift wing request doesnt make sense.

Author: gabina

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

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
 

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
 

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
 

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

spec/support/request_helpers.rb

@@ -736,12 +736,32 @@ 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,
         body: { 'description' => 'Language incubator is not a valid language. ' }.to_json,
         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