Fix ArcGIS imports for good

NEWS.md

@@ -41,6 +41,7 @@ Development
 - Fix broken links on the public footer [#16308](https://github.com/CartoDB/cartodb/pull/16308)
 - Fix search in _Filter by Column Value_ analysis [#16310](https://github.com/CartoDB/cartodb/pull/16310)
 - Use Google Maps provider if the base layer is Google [#16314](https://github.com/CartoDB/cartodb/pull/16314)
+- Fix empty ArcGIS imports [#16321](https://github.com/CartoDB/cartodb/pull/16321)
 
 4.45.0 (2021-04-14)
 -------------------

app/models/data_import.rb

@@ -125,7 +125,15 @@ class DataImport < Sequel::Model
     log.append("Running on server #{server} with PID: #{Process.pid}")
 
     begin
-      success = !!dispatch
+      self.state = STATE_UPLOADING
+
+      success = if table_copy.present? || from_query.present?
+                  from_table
+                elsif service_name == 'connector'
+                  !!execute_importer(*new_importer_with_connector)
+                else
+                  !!execute_importer(*new_importer)
+                end
     rescue TokenExpiredOrInvalidError => ex
       success = false
       begin
@@ -345,18 +353,6 @@ class DataImport < Sequel::Model
     end
   end
 
-  def dispatch
-    self.state = STATE_UPLOADING
-    return from_table         if table_copy.present? || from_query.present?
-
-    if service_name == 'connector'
-      importer, runner, datasource_provider, manual_fields = new_importer_with_connector
-    else
-      importer, runner, datasource_provider, manual_fields = new_importer
-    end
-    execute_importer importer, runner, datasource_provider, manual_fields
-  end
-
   def running_import_ids
     Resque::Worker.all.map do |worker|
       next unless worker.job['queue'] == 'imports'

services/datasources/lib/datasources/url/arcgis.rb

@@ -123,6 +123,14 @@ module CartoDB
           end
         end
 
+        def build_empty_initial_stream
+          {
+            geometryType: @metadata[:geometry_type],
+            fields: @metadata[:fields],
+            features: []
+          }.to_json
+        end
+
         # Initial stream, to be used for container creation (table usually)
         # @param id string
         # @return String
@@ -132,6 +140,8 @@ module CartoDB
 
           @ids = get_ids_list(@url)
 
+          return build_empty_initial_stream if empty?
+
           @ids_total = @ids.length
 
           first_item = get_by_ids(@url, [@ids.slice!(0)], @metadata[:fields])
@@ -148,7 +158,7 @@ module CartoDB
         # @param id string
         # @return String|nil Nil if no more items
         def stream_resource(id)
-          return nil if @ids.empty?
+          return nil if empty?
 
           retries = 0
           begin
@@ -236,6 +246,10 @@ module CartoDB
           is_multiresource?(id)
         end
 
+        def empty?
+          @ids.empty?
+        end
+
         private
 
         def http_client
@@ -391,8 +405,6 @@ module CartoDB
             raise ResponseError.new("Missing data: #{exception.to_s} #{request_url} #{exception.backtrace}")
           end
 
-          raise ResponseError.new("Empty ids list #{request_url}") if data.length == 0
-
           data
         end
 

services/datasources/spec/unit/arcgis_spec.rb

@@ -258,9 +258,13 @@ describe Url::ArcGIS do
       expect {
         arcgis.send(:get_ids_list, id)
       }.to raise_error ResponseError
+    end
+
+    it 'does not break when importing an empty feature list' do
+      arcgis = Url::ArcGIS.get_new(@user)
+
+      id = arcgis.send(:sanitize_id, @url)
 
-      # 'objectIds' empty
-      Typhoeus::Expectation.clear
       Typhoeus.stub(/\/arcgis\/rest\//) do
         body = File.read(File.join(File.dirname(__FILE__), "../fixtures/arcgis_ids_list.json"))
 
@@ -273,10 +277,7 @@ describe Url::ArcGIS do
         )
       end
 
-      expect {
-        arcgis.send(:get_ids_list, id)
-      }.to raise_error ResponseError
-
+      arcgis.send(:get_ids_list, id)
     end
 
     it 'tests the get_ids_list() private method' do

services/importer/lib/importer/connector_runner.rb

@@ -117,6 +117,13 @@ module CartoDB
         connector.provider_name
       end
 
+      def success?
+        return true unless remote_data_updated?
+
+        visualizations_count = @visualizations ? @visualizations.count : 0
+        (results.count(&:success?) + visualizations_count).positive? || @collision_strategy == SKIP
+      end
+
       private
 
       # Parse @json_params and extract @params

services/importer/lib/importer/datasource_downloader.rb

@@ -123,6 +123,8 @@ module CartoDB
       end
 
       def store_retrieved_data(filename, resource_data, available_quota_in_bytes)
+        return if resource_data.nil?
+
         if resource_data.is_a?(StringIO)
           return if resource_data.size.zero?
           data = resource_data

services/importer/lib/importer/runner.rb

@@ -31,6 +31,8 @@ module CartoDB
       # Hard-limit on number of spawned tables (zip files, KMLs and so on)
       MAX_TABLES_PER_IMPORT = 10
 
+      attr_writer :success
+
       # @param options Hash
       # {
       #   :pg Hash { ... }
@@ -146,6 +148,10 @@ module CartoDB
         @tracker || lambda { |state| state }
       end
 
+      def success?
+        remote_data_updated? ? @success : true
+      end
+
       attr_reader :results, :log, :loader, :stats, :downloader, :warnings, :visualizations
 
       private
@@ -242,6 +248,12 @@ module CartoDB
         @importer_stats.timing('resource') do
           @importer_stats.timing('download') do
             @downloader.run(available_quota)
+
+            if @downloader.datasource && @downloader.datasource.empty?
+              self.success = true
+              return
+            end
+
             return self unless remote_data_updated?
           end
 
@@ -286,6 +298,8 @@ module CartoDB
               @visualizations.push(visualization)
               log.store
             end
+
+            self.success = (results.count(&:success?) + @visualizations.size).positive? || @collision_strategy == SKIP
           end
         end
       ensure

services/importer/lib/importer/runner_helper.rb

@@ -9,13 +9,6 @@ module CartoDB
         !should_skip?(table_name)
       end
 
-      def success?
-        # TODO: Change this, "runner" can be ok even if no data has changed, should expose "data_changed" attribute
-        return true unless remote_data_updated?
-        visualizations_count = @visualizations ? @visualizations.count : 0
-        (results.count(&:success?) + visualizations_count) > 0 || @collision_strategy == SKIP
-      end
-
       private
 
       def should_skip?(table_name)

spec/models/table_spec.rb

@@ -29,7 +29,7 @@ def create_import(user, file_name, name=nil)
   end
 
   @data_import.data_source =  file_name
-  @data_import.send :dispatch
+  @data_import.run_import!
   @data_import
 end
 

spec/support/factories/users.rb

@@ -193,7 +193,7 @@ module CartoDB
       end
 
       data_import.data_source = file_name
-      data_import.send :dispatch
+      data_import.run_import!
       data_import
     end