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

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