Merge pull request #98 from af83/4633-import-handle-incorrect-zip

4633 import handle incorrect zip Messages added

11 files changed, 206 insertions, 78 deletions

diff --git a/INSTALL.md b/INSTALL.md
index 1da342e39..6e497b580 100644
--- a/INSTALL.md
+++ b/INSTALL.md
@@ -27,11 +27,6 @@ Otherwise please make sure to use a compatible version, still best to use the sa
 
 Then install yarn (`brew install yarn` does nicely on macOS).
 
-## Webpack
-
-... is managed by the webpacker gem, thusly at latest
- before starting your server and tests, setup webpacker with `bundle exec rake webpacker:install`.
-
 ### Installation Caveats
 
 #### Node Related Issue, libv8
diff --git a/app/services/zip_service.rb b/app/services/zip_service.rb
index cab301b01..7a4bdad1b 100644
--- a/app/services/zip_service.rb
+++ b/app/services/zip_service.rb
@@ -1,10 +1,9 @@
 class ZipService
-  # TODO: Remove me before merge https://github.com/rubyzip/rubyzip
 
-  class Subdir < Struct.new(:name, :stream)
+  class Subdir < Struct.new(:name, :stream, :spurious)
   end
 
-  attr_reader :current_key, :current_output, :yielder
+  attr_reader :current_key, :current_output, :current_spurious, :yielder
 
   def initialize data
     @zip_data       = StringIO.new(data)
@@ -36,6 +35,7 @@ class ZipService
   end
 
   def add_to_current_output entry
+    return if is_spurious! entry.name
     current_output.put_next_entry entry.name
     write_to_current_output entry.get_input_stream
   end
@@ -51,7 +51,8 @@ class ZipService
       @yielder  << Subdir.new(
         current_key,
         # Second part of the solution, yield the closed stream
-        current_output.close_buffer)
+        current_output.close_buffer,
+        current_spurious)
     end
   end
 
@@ -59,10 +60,19 @@ class ZipService
     @current_key    = entry_key
     # First piece of the solution, use internal way to create a Zip::OutputStream
     @current_output = Zip::OutputStream.new(StringIO.new(''), true, nil)
+    @current_spurious = []
   end
 
   def entry_key entry
     # last dir name File.dirname.split("/").last
     entry.name.split('/', -1)[-2]
   end
+
+  def is_spurious! entry_name
+    segments = entry_name.split('/', 3)
+    return false if segments.size < 3
+
+    current_spurious << segments.second
+    return true
+  end
 end
diff --git a/app/workers/workbench_import_worker.rb b/app/workers/workbench_import_worker.rb
index 994493944..300fad9e2 100644
--- a/app/workers/workbench_import_worker.rb
+++ b/app/workers/workbench_import_worker.rb
@@ -14,11 +14,13 @@ class WorkbenchImportWorker
     zip_service = ZipService.new(downloaded)
     upload zip_service
     @workbench_import.update(ended_at: Time.now)
+  rescue Zip::Error
+    handle_corrupt_zip_file
   end
 
   def download
     logger.info  "HTTP GET #{import_url}"
-    @zipfile_data = HTTPService.get_resource(
+    HTTPService.get_resource(
       host: import_host,
       path: import_path,
       params: {token: @workbench_import.token_download}).body
@@ -32,6 +34,10 @@ class WorkbenchImportWorker
       params: params(eg_file, eg_name))
   end
 
+  def handle_corrupt_zip_file
+    @workbench_import.messages.create(criticity: :error, message_key: 'corrupt_zip_file', message_attributes: {import_name: @workbench_import.name})
+  end
+
   def upload zip_service
     entry_group_streams = zip_service.subdirs
     @workbench_import.update total_steps: entry_group_streams.size
@@ -42,11 +48,24 @@ class WorkbenchImportWorker
     raise
   end
 
-  def upload_entry_group entry_pair, element_count
-    @workbench_import.update( current_step: element_count.succ )
-    # status = retry_service.execute(&upload_entry_group_proc(entry_pair))
-    eg_name = entry_pair.name
-    eg_stream = entry_pair.stream
+  def update_object_state entry, count
+    @workbench_import.update( current_step: count )
+    unless entry.spurious.empty?
+      @workbench_import.messages.create(
+        criticity: :warning,
+        message_key: 'inconsistent_zip_file',
+        message_attributes: {
+          'import_name' => @workbench_import.name,
+          'spurious_dirs' => entry.spurious.join(', ')
+        }) 
+    end
+  end
+
+  def upload_entry_group entry, element_count
+    update_object_state entry, element_count.succ
+    # status = retry_service.execute(&upload_entry_group_proc(entry))
+    eg_name = entry.name
+    eg_stream = entry.stream
 
     FileUtils.mkdir_p(Rails.root.join('tmp', 'imports'))
 
diff --git a/config/locales/import_messages.en.yml b/config/locales/import_messages.en.yml
index 4009d7c77..528ab3477 100644
--- a/config/locales/import_messages.en.yml
+++ b/config/locales/import_messages.en.yml
@@ -1,6 +1,8 @@
 en:
   import_messages:
     compliance_check_messages:
+      corrupt_zip_file: "The zip file of WorkbenchImport %{import_name} is corrupted and cannot be read"
+      inconsistent_zip_file: "The zip file of WorkbenchImport %{import_name} contains the following spurious directories %{spurious_dirs}, which are ignored"
       referential_creation: "Le référentiel n'a pas pu être créé car un référentiel existe déjà sur les même périodes et lignes"
       1_netexstif_2: "Le fichier %{source_filename} ne respecte pas la syntaxe XML ou la XSD NeTEx : erreur '%{error_value}' rencontré"
       1_netexstif_5: "%{source_filename}-Ligne %{source_line_number}-Colonne %{source_column_number} : l'objet %{source_label} d'identifiant %{source_objectid} a une date de mise à jour dans le futur"
diff --git a/config/locales/import_messages.fr.yml b/config/locales/import_messages.fr.yml
index 085299bb4..15de6eed8 100644
--- a/config/locales/import_messages.fr.yml
+++ b/config/locales/import_messages.fr.yml
@@ -1,6 +1,8 @@
 fr:
   import_messages:
     compliance_check_messages:
+      corrupt_zip_file: "Le fichier zip du WorkbenchImport %{import_name} est corrompu, et ne peut être lu" 
+      inconsistent_zip_file: "Le fichier zip du WorkbenchImport %{import_name} contient les repertoirs illegeaux %{spurious_dirs} qui seront ignorés"
       referential_creation: "Le référentiel n'a pas pu être créé car un référentiel existe déjà sur les même périodes et lignes"
       1_netexstif_2: "Le fichier %{source_filename} ne respecte pas la syntaxe XML ou la XSD NeTEx : erreur '%{error_value}' rencontré"
       1_netexstif_5: "%{source_filename}-Ligne %{source_line_number}-Colonne %{source_column_number} : l'objet %{source_label} d'identifiant %{source_objectid} a une date de mise à jour dans le futur"
diff --git a/spec/fixtures/OFFRE_WITH_EXTRA.zip b/spec/fixtures/OFFRE_WITH_EXTRA.zip
new file mode 100644
index 000000000..97ea3f513
--- /dev/null
+++ b/spec/fixtures/OFFRE_WITH_EXTRA.zip
diff --git a/spec/services/zip_service/regression_4273_spec.rb b/spec/services/zip_service/regression_4273_spec.rb
deleted file mode 100644
index 4fe0f6539..000000000
--- a/spec/services/zip_service/regression_4273_spec.rb
+++ /dev/null
@@ -1,59 +0,0 @@
-RSpec.describe ZipService do
-  describe 'Regression Issue # 4273 https://projects.af83.io/issues/4273' do
-    let( :zip_service ){ described_class }
-    let( :unzipper ){ zip_service.new(zip_data) }
-    let( :zip_data ){ File.read zip_file }
-
-    context 'real test data' do
-      let( :subdir_names ){ %w<OFFRE_TRANSDEV_20170301122517 OFFRE_TRANSDEV_20170301122519>  }
-      let( :expected_chksums ){
-        checksum_trees( subdir_names.map{ |sn| subdir_file(sn, prefix: 'source_') } )
-      }
-
-      let( :zip_file ){ fixtures_path 'OFFRE_TRANSDEV_2017030112251.zip' }
-      #
-      # Remove potential test artefacts
-      before do
-        subdir_names.each do | subdir_name |
-          File.unlink( subdir_file subdir_name, suffix: '.zip' ) rescue nil
-          Dir.unlink( subdir_file subdir_name ) rescue nil
-        end
-      end
-
-      it "yields the correct content" do
-        subdir_contents = {}
-        # Write ZipService Streams to files and inflate them to file system
-        unzipper.subdirs.each do | subdir |
-          File.open(subdir_file( subdir.name, suffix: '.zip' ), 'wb'){ |f| f.write subdir.stream.string }
-          unzip_subdir subdir
-        end
-        # Represent the inflated file_system as a checksum tree
-        actual_checksums = 
-          checksum_trees( subdir_names.map{ |sn| subdir_file(sn, prefix: 'target/') } )
-        expect( actual_checksums ).to eq( expected_chksums )
-      end
-
-    end
-
-  end
-
-  def checksum_trees *dirs
-    dirs.flatten.inject({},&method(:checksum_tree))
-  end
-  def checksum_tree repr, dir
-    Dir.glob("#{dir}/**/*").each do |file|
-      if !File.directory?(file)
-        repr.merge!( File.basename(file) => %x{cksum #{file}}.split.first ){ |_, ov, nv| Array(ov) << nv }
-      end
-    end
-    repr
-  end
-
-  def subdir_file( subdir, prefix: 'target_', suffix: '' )
-    fixtures_path("#{prefix}#{subdir}#{suffix}")
-  end
-
-  def unzip_subdir subdir
-    %x{unzip -oqq #{subdir_file subdir.name, suffix: '.zip'} -d #{fixture_path}/target}
-  end
-end
diff --git a/spec/services/zip_service_spec.rb b/spec/services/zip_service_spec.rb
new file mode 100644
index 000000000..98cb9026d
--- /dev/null
+++ b/spec/services/zip_service_spec.rb
@@ -0,0 +1,68 @@
+RSpec.describe ZipService do
+
+  let( :zip_service ){ described_class }
+  let( :unzipper ){ zip_service.new(zip_data) }
+  let( :zip_data ){ File.read zip_file }
+
+
+  context 'correct test data' do
+    before do
+      subdir_names.each do | subdir_name |
+        File.unlink( subdir_file subdir_name, suffix: '.zip' ) rescue nil
+        Dir.unlink( subdir_file subdir_name ) rescue nil
+      end
+    end
+    let( :subdir_names ){ %w<OFFRE_TRANSDEV_20170301122517 OFFRE_TRANSDEV_20170301122519>  }
+    let( :expected_chksums ){
+      checksum_trees( subdir_names.map{ |sn| subdir_file(sn, prefix: 'source_') } )
+    }
+
+    let( :zip_file ){ fixtures_path 'OFFRE_TRANSDEV_2017030112251.zip' }
+    #
+    # Remove potential test artefacts
+
+    it 'yields the correct content' do
+      # Write ZipService Streams to files and inflate them to file system
+      unzipper.subdirs.each do | subdir |
+        expect( subdir.spurious ).to be_empty
+        File.open(subdir_file( subdir.name, suffix: '.zip' ), 'wb'){ |f| f.write subdir.stream.string }
+        unzip_subdir subdir
+      end
+      # Represent the inflated file_system as a checksum tree
+      actual_checksums = 
+        checksum_trees( subdir_names.map{ |sn| subdir_file(sn, prefix: 'target/') } )
+      expect( actual_checksums ).to eq( expected_chksums )
+    end
+
+  end
+
+  context 'test data with spurious directories' do 
+    let( :zip_file ){ fixtures_path 'OFFRE_WITH_EXTRA.zip' }
+
+    it 'returns the extra dir in the spurious field of the entry' do
+      expect( unzipper.subdirs.first.spurious ).to eq(%w{EXTRA})
+    end
+  end
+
+
+  def checksum_trees *dirs
+    dirs.flatten.inject({},&method(:checksum_tree))
+  end
+  def checksum_tree repr, dir
+    Dir.glob("#{dir}/**/*").each do |file|
+      if !File.directory?(file)
+        repr.merge!( File.basename(file) => %x{cksum #{file}}.split.first ){ |_, ov, nv| Array(ov) << nv }
+      end
+    end
+    repr
+  end
+
+  def subdir_file( subdir, prefix: 'target_', suffix: '' )
+    fixtures_path("#{prefix}#{subdir}#{suffix}")
+  end
+
+  def unzip_subdir subdir
+    %x{unzip -oqq #{subdir_file subdir.name, suffix: '.zip'} -d #{fixture_path}/target}
+  end
+end
+
diff --git a/spec/support/random.rb b/spec/support/random.rb
index 59e1a1475..0ebc2ee5e 100644
--- a/spec/support/random.rb
+++ b/spec/support/random.rb
@@ -22,6 +22,12 @@ module Support
     def random_string
       SecureRandom.urlsafe_base64
     end
+
+    def very_random(veryness=3, joiner: '-')
+      raise ArgumentError, 'not very random' unless veryness > 1
+      veryness.times.map{ SecureRandom.uuid }.join(joiner)
+    end
+
   end
 end
 
diff --git a/spec/workers/workbench_import/workbench_import_with_corrupt_zip_spec.rb b/spec/workers/workbench_import/workbench_import_with_corrupt_zip_spec.rb
new file mode 100644
index 000000000..5e34b208a
--- /dev/null
+++ b/spec/workers/workbench_import/workbench_import_with_corrupt_zip_spec.rb
@@ -0,0 +1,48 @@
+RSpec.describe WorkbenchImportWorker do
+
+
+  shared_examples_for 'corrupt zipfile data' do
+    subject { described_class.new }
+    let( :workbench_import ){ create :workbench_import, status: :pending }
+
+    before do
+      # Let us make sure that the name Enterprise will never be forgotten by history,
+      # ahem, I meant, that nothing is uploaded, by forbidding any message to be sent
+      # to HTTPService
+      expect_it.to receive(:download).and_return(downloaded)
+    end
+
+    it 'does not upload' do
+      stub_const 'HTTPService', double('HTTPService')
+      subject.perform(workbench_import.id)
+    end
+
+    it 'does create a message' do
+      expect{ subject.perform(workbench_import.id) }.to change{ workbench_import.messages.count }.by(1)
+
+      message = workbench_import.messages.last
+      expect( message.criticity ).to eq('error')
+      expect( message.message_key ).to eq('corrupt_zip_file')
+      expect( message.message_attributes ).to eq( 'import_name' => workbench_import.name )
+    end
+
+    it 'does not change current step' do
+      expect{ subject.perform(workbench_import.id) }.not_to change{ workbench_import.current_step }
+    end
+
+    it "sets the workbench_import.status to failed" do
+      subject.perform(workbench_import.id)
+      expect( workbench_import.reload.status ).to eq('failed')
+    end
+  end
+
+  context 'empty zip file' do 
+    let( :downloaded ){ '' }
+    it_should_behave_like 'corrupt zipfile data'
+  end
+
+  context 'corrupt data' do 
+    let( :downloaded ){ very_random }
+    it_should_behave_like 'corrupt zipfile data'
+  end
+end
diff --git a/spec/workers/workbench_import_worker_spec.rb b/spec/workers/workbench_import/workbench_import_worker_spec.rb
index a349b3433..9f860a6b3 100644
--- a/spec/workers/workbench_import_worker_spec.rb
+++ b/spec/workers/workbench_import/workbench_import_worker_spec.rb
@@ -5,7 +5,7 @@ RSpec.describe WorkbenchImportWorker, type: [:worker, :request] do
 
   let( :workbench ){ import.workbench }
   let( :referential ){ import.referential }
-  let( :api_key ){ build_stubbed :api_key, referential: referential, token: "#{referential.id}-#{SecureRandom.hex}" }
+  let( :api_key ){ build_stubbed :api_key, referential: referential, token: "#{referential.id}-#{random_hex}" }
 
   # http://www.example.com/workbenches/:workbench_id/imports/:id/download
   let( :host ){ Rails.configuration.rails_host }
@@ -13,16 +13,17 @@ RSpec.describe WorkbenchImportWorker, type: [:worker, :request] do
 
   let( :downloaded_zip ){ double("downloaded zip") }
   let( :download_zip_response ){ OpenStruct.new( body: downloaded_zip ) }
-  let( :download_token ){ SecureRandom.urlsafe_base64 }
-
+  let( :download_token ){ random_string }
 
   let( :upload_path ) { api_v1_netex_imports_path(format: :json) }
 
+  let( :spurious ){ [[], [], []] }
   let( :subdirs ) do
     entry_count.times.map do |i|
       ZipService::Subdir.new(
         "subdir #{i}",
-        double("subdir #{i}", rewind: 0, read: '')
+        double("subdir #{i}", rewind: 0, read: ''),
+        spurious[i]
       )
     end
   end
@@ -104,8 +105,44 @@ RSpec.describe WorkbenchImportWorker, type: [:worker, :request] do
       expect( import ).to receive(:update).with(current_step: 3, status: 'failed')
 
       expect { worker.perform import.id }.to raise_error(StopIteration)
+    end
+  end
+
+  context 'multireferential zipfile with spurious directories' do 
+    let( :entry_count ){ 2 }
+    let( :spurious1 ){ [random_string] }
+    let( :spurious2 ){ [random_string, random_string] }
+    let( :spurious ){ [spurious1, spurious2] }
+    let( :messages ){ double('messages') }
+    let( :message_attributes ){{criticity: :warning, message_key: 'inconsistent_zip_file'}}
+    let( :message1_attributes ){ message_attributes.merge(message_attributes: {'import_name' => import.name, 'spurious_dirs' => spurious1.inspect}) }
+    let( :message2_attributes ){ message_attributes.merge(message_attributes: {'import_name' => import.name, 'spurious_dirs' => spurious2.inspect}) }
+
+    before do
+      allow(import).to receive(:messages).and_return(messages)
+    end
+
+    it 'downloads a zip file, cuts it, and uploads all pieces and adds messages' do
+
+      expect(HTTPService).to receive(:get_resource)
+        .with(host: host, path: path, params: {token: download_token})
+        .and_return( download_zip_response )
+
+      subdirs.each do |subdir|
+        mock_post subdir, post_response_ok
+      end
+
+      expect( import ).to receive(:update).with(total_steps: 2)
+      expect( import ).to receive(:update).with(current_step: 1)
+      expect( messages ).to receive(:create).with(message1_attributes)
+      expect( import ).to receive(:update).with(current_step: 2)
+      expect( messages ).to receive(:create).with(message2_attributes)
+      expect( import ).to receive(:update).with(ended_at: Time.now)
+
+      worker.perform import.id
 
     end
+    
   end
 
   def mock_post subdir, response