Merge pull request #5773 from betagouv/even_less_api_entreprise_errors

Even less api entreprise errors

app/controllers/champs/siret_controller.rb

@@ -16,7 +16,7 @@ class Champs::SiretController < ApplicationController
 
     begin
       etablissement = find_etablissement_with_siret
-    rescue ApiEntreprise::API::RequestFailed, ApiEntreprise::API::ServiceUnavailable
+    rescue ApiEntreprise::API::Error::RequestFailed, ApiEntreprise::API::Error::ServiceUnavailable
       return siret_error(:network_error)
     end
     if etablissement.nil?

app/controllers/users/dossiers_controller.rb

@@ -105,7 +105,7 @@ module Users
       sanitized_siret = siret_model.siret
       begin
         etablissement = ApiEntrepriseService.create_etablissement(@dossier, sanitized_siret, current_user.id)
-      rescue ApiEntreprise::API::RequestFailed, ApiEntreprise::API::BadGateway
+      rescue ApiEntreprise::API::Error::RequestFailed, ApiEntreprise::API::Error::BadGateway, ApiEntreprise::API::Error::TimedOut
         return render_siret_error(t('errors.messages.siret_network_error'))
       end
       if etablissement.nil?

app/jobs/api_entreprise/exercices_job.rb

@@ -1,5 +1,5 @@
 class ApiEntreprise::ExercicesJob < ApiEntreprise::Job
-  rescue_from(ApiEntreprise::API::BadFormatRequest) do |exception|
+  rescue_from(ApiEntreprise::API::Error::BadFormatRequest) do |exception|
   end
 
   def perform(etablissement_id, procedure_id)

app/jobs/api_entreprise/job.rb

@@ -1,21 +1,31 @@
 class ApiEntreprise::Job < ApplicationJob
+  DEFAULT_MAX_ATTEMPTS_API_ENTREPRISE_JOBS = 5
+
   queue_as :api_entreprise
 
-  retry_on ApiEntreprise::API::ServiceUnavailable,
-    ApiEntreprise::API::BadGateway,
+  # BadGateway could mean
+  # - acoss: réessayer ultérieurement
+  # - bdf: erreur interne
+  # so we retry every day for 5 days
+  # same logic for ServiceUnavailable
+  retry_on ApiEntreprise::API::Error::ServiceUnavailable,
+    ApiEntreprise::API::Error::BadGateway,
     wait: 1.day
 
-  DEFAULT_MAX_ATTEMPTS_API_ENTREPRISE_JOBS = 5
+  # We guess the backend is slow but not broken
+  # and the information we are looking for is available
+  # so we retry few seconds later (exponentially to avoid overload)
+  retry_on ApiEntreprise::API::Error::TimedOut, wait: :exponentially_longer
 
   # If by the time the job runs the Etablissement has been deleted
   # (it can happen through EtablissementUpdateJob for instance), ignore the job
   discard_on ActiveRecord::RecordNotFound
 
-  rescue_from(ApiEntreprise::API::ResourceNotFound) do |exception|
+  rescue_from(ApiEntreprise::API::Error::ResourceNotFound) do |exception|
     error(self, exception)
   end
 
-  rescue_from(ApiEntreprise::API::BadFormatRequest) do |exception|
+  rescue_from(ApiEntreprise::API::Error::BadFormatRequest) do |exception|
     error(self, exception)
   end
 

app/lib/api_entreprise/adapter.rb

@@ -9,7 +9,7 @@ class ApiEntreprise::Adapter
   def data_source
     begin
       @data_source ||= get_resource
-    rescue ApiEntreprise::API::ResourceNotFound
+    rescue ApiEntreprise::API::Error::ResourceNotFound
       @data_source = nil
     end
   end

app/lib/api_entreprise/api.rb

@@ -10,22 +10,7 @@ class ApiEntreprise::API
   BILANS_BDF_RESOURCE_NAME = "bilans_entreprises_bdf"
   PRIVILEGES_RESOURCE_NAME = "privileges"
 
-  TIMEOUT = 15
-
-  class ResourceNotFound < StandardError
-  end
-
-  class RequestFailed < StandardError
-  end
-
-  class BadFormatRequest < StandardError
-  end
-
-  class BadGateway < StandardError
-  end
-
-  class ServiceUnavailable < StandardError
-  end
+  TIMEOUT = 20
 
   def self.entreprise(siren, procedure_id)
     call_with_siret(ENTREPRISE_RESOURCE_NAME, siren, procedure_id)
@@ -81,7 +66,7 @@ class ApiEntreprise::API
     if response.success?
       JSON.parse(response.body, symbolize_names: true)
     else
-      raise RequestFailed, "HTTP Error Code: #{response.code} for #{url}\nheaders: #{response.headers}\nbody: #{response.body}"
+      raise RequestFailed.new(response)
     end
   end
 
@@ -97,22 +82,17 @@ class ApiEntreprise::API
     if response.success?
       JSON.parse(response.body, symbolize_names: true)
     elsif response.code&.between?(401, 499)
-      raise ResourceNotFound, "url: #{url}"
+      raise Error::ResourceNotFound.new(response)
     elsif response.code == 400
-      raise BadFormatRequest, "url: #{url}"
+      raise Error::BadFormatRequest.new(response)
     elsif response.code == 502
-      raise	BadGateway, "url: #{url}"
+      raise	Error::BadGateway.new(response)
     elsif response.code == 503
-      raise ServiceUnavailable, "url: #{url}"
+      raise Error::ServiceUnavailable.new(response)
+    elsif response.timed_out?
+      raise Error::TimedOut.new(response)
     else
-      raise RequestFailed,
-        <<~TEXT
-          HTTP Error Code: #{response.code} for #{url}
-          headers: #{response.headers}
-          body: #{response.body}
-          curl message: #{response.return_message}
-          timeout: #{response.timed_out?}
-        TEXT
+      raise Error::RequestFailed.new(response)
     end
   end
 

app/lib/api_entreprise/api/error.rb

@@ -0,0 +1,18 @@
+class ApiEntreprise::API::Error < ::StandardError
+  def initialize(response)
+    # use uri to avoid sending token
+    uri = URI.parse(response.effective_url)
+
+    msg = <<~TEXT
+      url: #{uri.host}#{uri.path}
+      HTTP error code: #{response.code}
+      body: #{CGI.escape(response.body)}
+      curl message: #{response.return_message}
+      total time: #{response.total_time}
+      connect time: #{response.connect_time}
+      response headers: #{response.headers}
+    TEXT
+
+    super(msg)
+  end
+end

app/lib/api_entreprise/api/error/bad_format_request.rb

@@ -0,0 +1,4 @@
+class ApiEntreprise::API::Error
+  class BadFormatRequest < ApiEntreprise::API::Error
+  end
+end

app/lib/api_entreprise/api/error/bad_gateway.rb

@@ -0,0 +1,4 @@
+class ApiEntreprise::API::Error
+  class BadGateway < ApiEntreprise::API::Error
+  end
+end

app/lib/api_entreprise/api/error/request_failed.rb

@@ -0,0 +1,4 @@
+class ApiEntreprise::API::Error
+  class RequestFailed < ApiEntreprise::API::Error
+  end
+end

app/lib/api_entreprise/api/error/resource_not_found.rb

@@ -0,0 +1,4 @@
+class ApiEntreprise::API::Error
+  class ResourceNotFound < ApiEntreprise::API::Error
+  end
+end

app/lib/api_entreprise/api/error/service_unavailable.rb

@@ -0,0 +1,4 @@
+class ApiEntreprise::API::Error
+  class ServiceUnavailable < ApiEntreprise::API::Error
+  end
+end

app/lib/api_entreprise/api/error/timed_out.rb

@@ -0,0 +1,4 @@
+class ApiEntreprise::API::Error
+  class TimedOut < ApiEntreprise::API::Error
+  end
+end

app/services/api_entreprise_service.rb

@@ -5,7 +5,7 @@ class ApiEntrepriseService
   #
   # Returns nil if the SIRET is unknown
   #
-  # Raises a ApiEntreprise::API::RequestFailed exception on transient errors
+  # Raises a ApiEntreprise::API::Error::RequestFailed exception on transient errors
   # (timeout, 5XX HTTP error code, etc.)
   def self.create_etablissement(dossier_or_champ, siret, user_id = nil)
     etablissement_params = ApiEntreprise::EtablissementAdapter.new(siret, dossier_or_champ.procedure.id).to_params

spec/jobs/api_entreprise/job_spec.rb

@@ -1,43 +1,53 @@
 include ActiveJob::TestHelper
 
 RSpec.describe ApiEntreprise::Job, type: :job do
-  # https://api.rubyonrails.org/classes/ActiveJob/Exceptions/ClassMethods.html#method-i-retry_on
-  context 'when an exception is raised' do
-    subject do
-      assert_performed_jobs(try) do
-        ExceptionJob.perform_later(error) rescue StandardError
+  # https://api.rubyonrails.org/classes/ActiveJob/Exceptions/ClassMethods.html
+  # #method-i-retry_on
+  describe '#perform' do
+    context 'when a un retryable error is raised' do
+      let(:errors) { [:standard_error] }
+
+      it 'does not retry' do
+        ensure_errors_force_n_retry(errors, 1)
       end
     end
 
-    context 'when it is a service_unavaible' do
-      let(:error) { :standard_error }
-      let(:try) { 1 }
+    context 'when a retryable error is raised' do
+      let(:errors) { [:service_unavaible, :bad_gateway, :timed_out] }
 
-      it { subject }
+      it 'retries 5 times' do
+        ensure_errors_force_n_retry(errors, 5)
+      end
     end
 
-    context 'when it is a service_unavaible' do
-      let(:error) { :service_unavaible }
-      let(:try) { 5 }
-
-      it { subject }
-    end
-
-    context 'when it is a bad gateway' do
-      let(:error) { :bad_gateway }
-      let(:try) { 5 }
-
-      it { subject }
+    def ensure_errors_force_n_retry(errors, retry_nb)
+      errors.each do |error|
+        assert_performed_jobs(retry_nb) do
+          ErrorJob.perform_later(error) rescue StandardError
+        end
+      end
     end
   end
 
-  class ExceptionJob < ApiEntreprise::Job
-    def perform(exception)
-      case exception
+  class ErrorJob < ApiEntreprise::Job
+    def perform(error)
+      response = OpenStruct.new(
+        effective_url: 'http://host.com/path',
+        code: '666',
+        body: 'body',
+        return_message: 'return_message',
+        total_time: 10,
+        connect_time: 20,
+        headers: 'headers'
+      )
+
+      case error
       when :service_unavaible
-        raise ApiEntreprise::API::ServiceUnavailable
+        raise ApiEntreprise::API::Error::ServiceUnavailable.new(response)
       when :bad_gateway
-        raise ApiEntreprise::API::BadGateway
+        raise ApiEntreprise::API::Error::BadGateway.new(response)
+      when :timed_out
+        raise ApiEntreprise::API::Error::TimedOut.new(response)
       else
         raise StandardError
       end

spec/lib/api_entreprise/api_spec.rb

@@ -17,8 +17,8 @@ describe ApiEntreprise::API do
       let(:status) { 502 }
       let(:body) { File.read('spec/fixtures/files/api_entreprise/entreprises_unavailable.json') }
 
-      it 'raises ApiEntreprise::API::RequestFailed' do
-        expect { subject }.to raise_error(ApiEntreprise::API::BadGateway)
+      it 'raises ApiEntreprise::API::Error::RequestFailed' do
+        expect { subject }.to raise_error(ApiEntreprise::API::Error::BadGateway)
       end
     end
 
@@ -27,8 +27,8 @@ describe ApiEntreprise::API do
       let(:status) { 404 }
       let(:body) { File.read('spec/fixtures/files/api_entreprise/entreprises_not_found.json') }
 
-      it 'raises ApiEntreprise::API::ResourceNotFound' do
-        expect { subject }.to raise_error(ApiEntreprise::API::ResourceNotFound)
+      it 'raises ApiEntreprise::API::Error::ResourceNotFound' do
+        expect { subject }.to raise_error(ApiEntreprise::API::Error::ResourceNotFound)
       end
     end
 
@@ -37,8 +37,8 @@ describe ApiEntreprise::API do
       let(:status) { 400 }
       let(:body) { File.read('spec/fixtures/files/api_entreprise/entreprises_not_found.json') }
 
-      it 'raises ApiEntreprise::API::BadFormatRequest' do
-        expect { subject }.to raise_error(ApiEntreprise::API::BadFormatRequest)
+      it 'raises ApiEntreprise::API::Error::BadFormatRequest' do
+        expect { subject }.to raise_error(ApiEntreprise::API::Error::BadFormatRequest)
       end
     end
 
@@ -47,8 +47,8 @@ describe ApiEntreprise::API do
       let(:status) { 403 }
       let(:body) { File.read('spec/fixtures/files/api_entreprise/entreprises_private.json') }
 
-      it 'raises ApiEntreprise::API::ResourceNotFound' do
-        expect { subject }.to raise_error(ApiEntreprise::API::ResourceNotFound)
+      it 'raises ApiEntreprise::API::Error::ResourceNotFound' do
+        expect { subject }.to raise_error(ApiEntreprise::API::Error::ResourceNotFound)
       end
     end
 
@@ -97,8 +97,8 @@ describe ApiEntreprise::API do
       let(:status) { 404 }
       let(:body) { '' }
 
-      it 'raises ApiEntreprise::API::ResourceNotFound' do
-        expect { subject }.to raise_error(ApiEntreprise::API::ResourceNotFound)
+      it 'raises ApiEntreprise::API::Error::ResourceNotFound' do
+        expect { subject }.to raise_error(ApiEntreprise::API::Error::ResourceNotFound)
       end
     end
 
@@ -127,8 +127,8 @@ describe ApiEntreprise::API do
       let(:status) { 404 }
       let(:body) { '' }
 
-      it 'raises ApiEntreprise::API::ResourceNotFound' do
-        expect { subject }.to raise_error(ApiEntreprise::API::ResourceNotFound)
+      it 'raises ApiEntreprise::API::Error::ResourceNotFound' do
+        expect { subject }.to raise_error(ApiEntreprise::API::Error::ResourceNotFound)
       end
     end
 
@@ -159,8 +159,8 @@ describe ApiEntreprise::API do
       let(:status) { 404 }
       let(:body) { '' }
 
-      it 'raises ApiEntreprise::API::ResourceNotFound' do
-        expect { subject }.to raise_error(ApiEntreprise::API::ResourceNotFound)
+      it 'raises ApiEntreprise::API::Error::ResourceNotFound' do
+        expect { subject }.to raise_error(ApiEntreprise::API::Error::ResourceNotFound)
       end
     end
 

spec/lib/api_entreprise/entreprise_adapter_spec.rb

@@ -84,7 +84,7 @@ describe ApiEntreprise::EntrepriseAdapter do
     let(:status) { 500 }
 
     it 'raises an exception' do
-      expect { subject }.to raise_error(ApiEntreprise::API::RequestFailed)
+      expect { subject }.to raise_error(ApiEntreprise::API::Error::RequestFailed)
     end
   end
 end

spec/services/api_entreprise_service_spec.rb

@@ -38,8 +38,8 @@ describe ApiEntrepriseService do
       let(:etablissements_status) { 504 }
       let(:etablissements_body) { '' }
 
-      it 'should raise ApiEntreprise::API::RequestFailed' do
-        expect { subject }.to raise_error(ApiEntreprise::API::RequestFailed)
+      it 'should raise ApiEntreprise::API::Error::RequestFailed' do
+        expect { subject }.to raise_error(ApiEntreprise::API::Error::RequestFailed)
       end
     end