Merge pull request #22 from culturecode/more-validation

More detailed shapefile archive validation

lib/spatial_features.rb

@@ -12,6 +12,7 @@
 require 'spatial_features/download'
 require 'spatial_features/unzip'
 require 'spatial_features/utils'
+require 'spatial_features/validation'
 
 require 'spatial_features/has_spatial_features'
 require 'spatial_features/has_spatial_features/queued_spatial_processing'

lib/spatial_features/download.rb

@@ -26,6 +26,12 @@ def self.normalize_file(file)
       end
     end
 
+    def self.entries(file)
+      file = Kernel.open(file)
+      file = normalize_file(file) if file.is_a?(StringIO)
+      Unzip.entries(file)
+    end
+
     def self.find_in_zip(file, find:, **unzip_options)
       return File.open(Unzip.paths(file, :find => find, **unzip_options))
     end

lib/spatial_features/importers/shapefile.rb

@@ -17,7 +17,13 @@ def cache_key
 
       private
 
+      def validate!
+        Validation.validate_shapefile_archive!(Download.entries(@data), default_proj4_projection: default_proj4_projection)
+      end
+
       def each_record(&block)
+        validate!
+
         RGeo::Shapefile::Reader.open(file.path) do |records|
           records.each do |record|
             yield OpenStruct.new data_from_wkt(record.geometry.as_text, proj4_projection).merge(:metadata => record.attributes) if record.geometry.present?

lib/spatial_features/validation.rb

@@ -0,0 +1,48 @@
+module SpatialFeatures
+  module Validation
+    # SHP file must come first
+    REQUIRED_SHAPEFILE_COMPONENT_EXTENSIONS = %w[shp shx dbf prj].freeze
+
+    # Check if a shapefile archive includes the required component files, otherwise
+    # raise an exception.
+    #
+    # @param [Zip::File] zip_file                 A Zip::File object
+    # @param [String] default_proj4_projection    Optional, if supplied we don't raise an exception when we're missing a .PRJ file
+    # @param [Boolean] allow_generic_zip_files    When true, we skip validation entirely if the archive does not contain a .SHP file
+    def self.validate_shapefile_archive!(zip_file, default_proj4_projection: nil, allow_generic_zip_files: false)
+      zip_file_entries = zip_file.entries.each_with_object({}) do |f, obj|
+        obj[File.extname(f.name).downcase[1..-1]] = File.basename(f.name, '.*')
+      end
+
+      shapefile_basename = zip_file_entries["shp"]
+      unless shapefile_basename
+        # not a shapefile archive but we don't care
+        return if allow_generic_zip_files
+
+        raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "Shapefile archive is missing a SHP file"
+      end
+
+      REQUIRED_SHAPEFILE_COMPONENT_EXTENSIONS[1..-1].each do |ext|
+        ext_basename = zip_file_entries[ext]
+        next if ext_basename&.casecmp?(shapefile_basename)
+
+        case ext
+        when "prj"
+          # special case for missing projection files to allow using default_proj4_projection
+          next if default_proj4_projection
+
+          raise ::SpatialFeatures::Importers::IndeterminateShapefileProjection, "Shapefile archive is missing a projection file: #{expected_component_path(shapefile_basename, ext)}"
+        else
+          # for all un-handled cases of missing files raise the more generic error
+          raise ::SpatialFeatures::Importers::IncompleteShapefileArchive, "Shapefile archive is missing a required file: #{expected_component_path(shapefile_basename, ext)}"
+        end
+      end
+
+      true
+    end
+
+    def self.expected_component_path(basename, ext)
+      "#{basename}.#{ext}"
+    end
+  end
+end

spec/lib/spatial_features/importers/shapefile_spec.rb

@@ -33,20 +33,44 @@
         it_behaves_like "a well formed shapefile"
       end
 
-      context 'when the shapefile is missing a required file' do
-        let(:subject) { SpatialFeatures::Importers::Shapefile.new(shapefile_with_missing_required_file) }
+      context 'when the shapefile is missing a SHX file' do
+        let(:subject) { SpatialFeatures::Importers::Shapefile.new(shapefile_without_shape_index) }
 
         it 'raises an exception' do
-          expect { subject.features }.to raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive)
+          expect { subject.features }.to raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive, /FirstNationReserves\.shx/)
         end
       end
 
-      context 'when the shapefile has no projection' do
+      context 'when the shapefile is missing a SHP file' do
+        let(:subject) { SpatialFeatures::Importers::Shapefile.new(shapefile_without_shape_format) }
+
+        it 'raises an exception' do
+          expect { subject.features }.to raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive, /missing a SHP file/)
+        end
+      end
+
+      context 'when the shapefile is missing a DBF file' do
+        let(:subject) { SpatialFeatures::Importers::Shapefile.new(shapefile_with_missing_dbf_file) }
+
+        it 'raises an exception' do
+          expect { subject.features }.to raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive, /FirstNationReserves\.dbf/)
+        end
+      end
+
+      context 'when the shapefile has an incorrect component basename' do
+        let(:subject) { SpatialFeatures::Importers::Shapefile.new(shapefile_with_incorrect_shx_basename) }
+
+        it 'raises an exception' do
+          expect { subject.features }.to raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive, /FirstNationReserves\.shx/)
+        end
+      end
+
+      context 'when the shapefile is missing a PRJ file' do
         let(:subject) { SpatialFeatures::Importers::Shapefile.new(shapefile_without_projection) }
 
         it 'raises an exception if there is no default projection' do
           allow(SpatialFeatures::Importers::Shapefile).to receive(:default_proj4_projection).and_return(nil)
-          expect { subject.features }.to raise_exception(SpatialFeatures::Importers::IndeterminateShapefileProjection)
+          expect { subject.features }.to raise_exception(SpatialFeatures::Importers::IndeterminateShapefileProjection, /FirstNationReserves\.prj/)
         end
 
         it 'is uses the `default_proj4_projection` when no projection can be determined from the shapefile' do

spec/lib/spatial_features/validation.rb

@@ -0,0 +1,16 @@
+require 'spec_helper'
+
+describe SpatialFeatures do
+  describe '::validate_shapefile_archive!!' do
+    let(:data) { shapefile_without_shape_format }
+
+    it 'skips validation with allow_generic_zip_files option' do
+      expect { SpatialFeatures::Validation.validate_shapefile_archive!(SpatialFeatures::Download.entries(data), allow_generic_zip_files: true) }.not_to raise_exception
+    end
+
+    it 'performs validation without allow_generic_zip_files option' do
+      expect { SpatialFeatures::Validation.validate_shapefile_archive!(SpatialFeatures::Download.entries(data), allow_generic_zip_files: false) }.to \
+        raise_exception(SpatialFeatures::Importers::IncompleteShapefileArchive, /FirstNationReserves\.shp/)
+    end
+  end
+end

spec/support/fixtures.rb

@@ -1,35 +1,51 @@
+def open_fixture_file(path)
+  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/#{path}")
+end
+
 def kml_file
-  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/test.kml")
+  open_fixture_file("test.kml")
 end
 
 def kmz_file
-  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/test.kmz")
+  open_fixture_file("test.kmz")
 end
 
 def kmz_file_features_without_placemarks
-  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/kmz_file_features_without_placemarks.kmz")
+  open_fixture_file("kmz_file_features_without_placemarks.kmz")
 end
 
 def kml_file_with_invalid_placemark
-  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/kml_file_with_invalid_placemark.kml")
+  open_fixture_file("kml_file_with_invalid_placemark.kml")
 end
 
 def shapefile
-  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/shapefile.zip")
+  open_fixture_file("shapefile.zip")
 end
 
 def shapefile_without_projection
-  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/shapefile_without_projection.zip")
+  open_fixture_file("shapefile_without_projection.zip")
 end
 
 def shapefile_with_upcase_shp
-  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/shapefile_with_upcase_shp.zip")
+  open_fixture_file("shapefile_with_upcase_shp.zip")
+end
+
+def shapefile_without_shape_format
+  open_fixture_file("shapefile_without_shape_format.zip")
+end
+
+def shapefile_without_shape_index
+  open_fixture_file("shapefile_without_shape_index.zip")
+end
+
+def shapefile_with_missing_dbf_file
+  open_fixture_file("shapefile_with_missing_dbf_file.zip")
 end
 
-def shapefile_with_missing_required_file
-  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/shapefile_with_missing_required_file.zip")
+def shapefile_with_incorrect_shx_basename
+  open_fixture_file("shapefile_with_incorrect_shx_basename.zip")
 end
 
 def archive_without_any_known_file
-  File.open("#{SpatialFeatures::Engine.root}/spec/fixtures/archive_without_any_known_file.zip")
+  open_fixture_file("archive_without_any_known_file.zip")
 end