Filename generator extraction refactor #11337

Makefile

@@ -68,6 +68,7 @@ WORKING_SPECS_1 = \
 	spec/lib/trending_maps_spec.rb \
 	spec/lib/explore_api_spec.rb \
 	spec/lib/user_account_creator_spec.rb \
+	spec/lib/carto/filename_generator_spec.rb \
 	spec/lib/carto/http_header_authentication_spec.rb \
 	spec/lib/carto/saml_service_spec.rb \
 	spec/lib/carto/users_metadata_redis_cache_spec.rb \

lib/carto/filename_generator.rb

@@ -0,0 +1,32 @@
+module Carto
+  module FilenameGenerator
+    def filename_from_url(url, supported_extensions)
+      filename = CGI.unescape(File.basename(URI(url).path))
+
+      extension = File.extname(filename)
+      if extension.present? && supported_extensions.include?(extension)
+        filename
+      else
+        # For non-conventional URLs (i.e: filename in params)
+        regex_match = url_filename_regex(supported_extensions).match(url).to_s
+
+        regex_match if regex_match.present?
+      end
+    end
+
+    private
+
+    def url_filename_regex(supported_extensions)
+      se_match_regex = Regexp.union(supported_extensions_match(supported_extensions))
+      Regexp.new("[[:word:]-]+#{se_match_regex}+", Regexp::IGNORECASE)
+    end
+
+    def supported_extensions_match(supported_extensions)
+      supported_extensions.map { |ext|
+        ext = ext.gsub('.', '\\.')
+        [/#{ext}$/i, /#{ext}(?=\.)/i, /#{ext}(?=\?)/i, /#{ext}(?=&)/i]
+      }.flatten
+    end
+
+  end
+end

services/importer/lib/importer/downloader.rb

@@ -14,6 +14,7 @@ require_relative './url_translator/kimono_labs'
 require_relative './unp'
 require_relative '../../../../lib/carto/http/client'
 require_relative '../../../../lib/carto/url_validator'
+require_relative '../../../../lib/carto/filename_generator'
 require_relative '../helpers/quota_check_helpers.rb'
 
 # NOTE: Beware that some methods and some parameters are kept since this class is supposed to be
@@ -21,8 +22,6 @@ require_relative '../helpers/quota_check_helpers.rb'
 # managed this might have been through inheritance, since Ruby doesn't provide interfaces.
 # The out-facing methods this class must implement for this purpose are:
 # - supported_extensions
-# - supported_extensions_match
-# - url_filename_regex
 # - provides_stream?
 # - http_download?
 # - multi_resource_import_supported?
@@ -37,7 +36,7 @@ require_relative '../helpers/quota_check_helpers.rb'
 module CartoDB
   module Importer2
     class Downloader
-      include CartoDB::Importer2::QuotaCheckHelpers, Carto::UrlValidator
+      include CartoDB::Importer2::QuotaCheckHelpers, Carto::UrlValidator, Carto::FilenameGenerator
 
       def self.supported_extensions
         @supported_extensions ||= CartoDB::Importer2::Unp::SUPPORTED_FORMATS
@@ -45,20 +44,6 @@ module CartoDB
                                   .sort_by(&:length).reverse
       end
 
-      def self.supported_extensions_match
-        @supported_extensions_match ||= supported_extensions.map { |ext|
-          ext = ext.gsub('.', '\\.')
-          [/#{ext}$/i, /#{ext}(?=\.)/i, /#{ext}(?=\?)/i, /#{ext}(?=&)/i]
-        }.flatten
-      end
-
-      def self.url_filename_regex
-        return @url_filename_regex if @url_filename_regex
-
-        se_match_regex = Regexp.union(supported_extensions_match)
-        @url_filename_regex = Regexp.new("[[:word:]-]+#{se_match_regex}+", Regexp::IGNORECASE)
-      end
-
       def provides_stream?
         false
       end
@@ -199,7 +184,7 @@ module CartoDB
 
         basename = @custom_filename ||
                    filename_from_headers ||
-                   filename_from_url ||
+                   filename_from_url(@translated_url, self.class.supported_extensions) ||
                    SecureRandom.urlsafe_base64
 
         @filename = filename_with_extension(basename)
@@ -344,23 +329,6 @@ module CartoDB
         parsed_filename if parsed_filename.present?
       end
 
-      def filename_from_url
-        filename = CGI.unescape(File.basename(URI(@translated_url).path))
-
-        extension = File.extname(filename)
-        if extension.present? && self.class.supported_extensions.include?(extension)
-          filename
-        else
-          # For non-conventional URLs (i.e: filename in params)
-          regex_match = self.class
-                            .url_filename_regex
-                            .match(@translated_url)
-                            .to_s
-
-          regex_match if regex_match.present?
-        end
-      end
-
       CONTENT_TYPES_MAPPING = [
         {
           content_types: ['text/plain'],

spec/lib/carto/filename_generator_spec.rb

@@ -0,0 +1,53 @@
+require 'active_support'
+require 'active_support/core_ext/object/blank'
+require_relative '../../../lib/carto/filename_generator'
+
+describe Carto::FilenameGenerator do
+  class TestFilenameGenerator
+    include Carto::FilenameGenerator
+  end
+
+  describe '#filename_from_url' do
+    let(:generator) { TestFilenameGenerator.new }
+    let(:supported_extensions) { %w(.csv .carto .zip) }
+
+    it 'returns the full url for just filenames' do
+      ['a_name.csv', 'a_name.carto'].each do |filename|
+        generator.filename_from_url(filename, supported_extensions).should eq filename
+      end
+    end
+
+    it 'returns nil for unsupported extensions' do
+      ['a_name.wadus', 'a_name'].each do |filename|
+        generator.filename_from_url(filename, supported_extensions).should be_nil
+      end
+    end
+
+    it 'returns the filename for normal urls' do
+      [%w(http://carto.com/name.csv name.csv), %w(http://carto.com/sub/file.carto file.carto)].each do |url, filename|
+        generator.filename_from_url(url, supported_extensions).should eq filename
+      end
+    end
+
+    it 'returns the filename for filenames with dots and other characters' do
+      name1 = 'name.something (one - two : three).carto'
+
+      [
+        %w(http://carto.com/name.something.csv name.something.csv),
+        ["http://carto.com/sub/#{name1.gsub(' ', '%20')}", name1]
+      ].each do |url, filename|
+        generator.filename_from_url(url, supported_extensions).should eq filename
+      end
+    end
+
+    it 'returns the filename for weirds URLs with filenames within the parameters (see #5704)' do
+      filename = '1259030001_MB_2011_ASGC_NSW_csv.zip'
+      weird_url = "http://www.abs.gov.au/ausstats/subscriber.nsf/log?openagent&"\
+                  "#{filename}"\
+                  "&1259.0.30.001&Data%20Cubes&2787F2FFF3F6E607CA2578CC001268ED&0&July%202011&05.10.2011&Latest"
+      [[weird_url, filename]].each do |url, filename|
+        generator.filename_from_url(url, supported_extensions).should eq filename
+      end
+    end
+  end
+end