Improve feed info error handling

app/services/feed_info.rb

@@ -1,60 +1,43 @@
 require 'net/http'
 
 class FeedInfo
+
   CACHE_EXPIRATION = 1.hour
 
-  def self.fetch(url, limit=10, &block)
-    # http://ruby-doc.org/stdlib-2.2.3/libdoc/net/http/rdoc/Net/HTTP.html
-    # You should choose a better exception.
-    raise ArgumentError.new('Too many redirects') if limit == 0
-    url = URI.parse(url)
-    Net::HTTP.start(url.host, url.port) do |http|
-      http.request_get(url.request_uri) do |response|
-        case response
-        when Net::HTTPSuccess then
-          yield response
-        when Net::HTTPRedirection then
-          fetch(response['location'], limit-1, &block)
-        else
-          raise response.value
-        end
-      end
-    end
+  def initialize(url: nil, path: nil)
+    fail ArgumentError.new('must provide url') unless url.present?
+    @url = url
+    @path = path
+    @gtfs = nil
   end
 
-  def self.download_to_tempfile(url, maxsize=nil)
-    fetch(url) do |response|
-      file = Tempfile.new('test.zip', Dir.tmpdir)
-      file.binmode
-      total = 0
-      begin
-        response.read_body do |chunk|
-          file.write(chunk)
-          total += chunk.size
-        end
-        raise IOError.new('Exceeds maximum file size') if (maxsize && total > maxsize)
-        file.close
-        yield file.path
-      ensure
-        file.close unless file.closed?
-        file.unlink
+  def open(&block)
+    if @gtfs
+      yield self
+    elsif @path
+      @gtfs = GTFS::Source.build(@path, {strict: false})
+      @gtfs.load_graph
+      yield self
+    elsif @url
+      FeedInfo.download_to_tempfile(@url) do |path|
+        @path = path
+        @gtfs = GTFS::Source.build(@path, {strict: false})
+        @gtfs.load_graph
+        yield self
       end
     end
   end
 
-  def self.parse_feed_and_operators(url, filename)
-    # load feed
-    gtfs = GTFS::Source.build(filename, {strict: false})
-    gtfs.load_graph
+  def parse_feed_and_operators
     # find visited stops
     stop_map = {}
     agency_visited_stops = {}
     feed_visited_stops = Set.new
-    gtfs.agencies.each do |agency|
+    @gtfs.agencies.each do |agency|
       visited_stops = Set.new
-      gtfs.children(agency).each do |route|
-        gtfs.children(route).each do |trip|
-          gtfs.children(trip).each do |stop|
+      @gtfs.children(agency).each do |route|
+        @gtfs.children(route).each do |trip|
+          @gtfs.children(trip).each do |stop|
             stop_map[stop] ||= Stop.from_gtfs(stop)
             visited_stops << stop_map[stop]
           end
@@ -64,15 +47,55 @@ def self.parse_feed_and_operators(url, filename)
       feed_visited_stops |= visited_stops
     end
     # feed
-    feed = Feed.from_gtfs(url, feed_visited_stops)
+    feed = Feed.from_gtfs(@url, feed_visited_stops)
     # operators
     operators = []
-    gtfs.agencies.each do |agency|
+    @gtfs.agencies.each do |agency|
       operator = Operator.from_gtfs(agency, agency_visited_stops[agency])
       operators << operator
       feed.operators_in_feed.new(gtfs_agency_id: agency.id, operator: operator, id: nil)
     end
     # done
     return [feed, operators]
   end
+
+  def self.download_to_tempfile(url, maxsize=nil)
+    fetch(url) do |response|
+      file = Tempfile.new('test.zip', Dir.tmpdir)
+      file.binmode
+      total = 0
+      begin
+        response.read_body do |chunk|
+          file.write(chunk)
+          total += chunk.size
+        end
+        raise IOError.new('Exceeds maximum file size') if (maxsize && total > maxsize)
+        file.close
+        yield file.path
+      ensure
+        file.close unless file.closed?
+        file.unlink
+      end
+    end
+  end
+
+  def self.fetch(url, limit=10, &block)
+    # http://ruby-doc.org/stdlib-2.2.3/libdoc/net/http/rdoc/Net/HTTP.html
+    # You should choose a better exception.
+    raise ArgumentError.new('Too many redirects') if limit == 0
+    url = URI.parse(url)
+    Net::HTTP.start(url.host, url.port) do |http|
+      http.request_get(url.request_uri) do |response|
+        case response
+        when Net::HTTPSuccess then
+          yield response
+        when Net::HTTPRedirection then
+          fetch(response['location'], limit-1, &block)
+        else
+          raise response.value
+        end
+      end
+    end
+  end
+
 end

app/workers/feed_info_worker.rb

@@ -5,27 +5,42 @@ class FeedInfoWorker
   sidekiq_options :retry => false
 
   def perform(url, cachekey)
-    error, feed, operators = nil, nil, nil
+    feed, operators = nil, nil
+    errors = []
+    response = {}
     begin
-      FeedInfo.download_to_tempfile(url, maxsize=100*1024*1024) do |filename|
-        feed, operators = FeedInfo.parse_feed_and_operators(url, filename)
+      feed_info = FeedInfo.new(url: url)
+      feed_info.open do |f|
+        feed, operators = f.parse_feed_and_operators
       end
+    rescue GTFS::InvalidSourceException => e
+      errors << {
+        exception: 'InvalidSourceException',
+        message: 'Invalid GTFS Feed'
+      }
+    rescue SocketError => e
+      errors << {
+        exception: 'SocketError',
+        message: 'Error connecting to host'
+      }
+    rescue Net::HTTPServerException => e
+      errors << {
+        exception: 'HTTPServerException',
+        message: e.to_s
+      }
     rescue StandardError => e
-      data = {
-        status: 'error',
-        url: url,
+      errors << {
         exception: e.class.name,
-        message: e.to_s
+        message: 'Could not download file'
       }
     else
-      data = {
-        status: 'complete',
-        url: url,
-        feed: FeedSerializer.new(feed).as_json,
-        operators: operators.map { |o| OperatorSerializer.new(o).as_json }
-      }
+      response[:feed] = FeedSerializer.new(feed).as_json
+      response[:operators] = operators.map { |o| OperatorSerializer.new(o).as_json }
     end
-    Rails.cache.write(cachekey, data, expires_in: FeedInfo::CACHE_EXPIRATION)
+    response[:status] = errors.size > 0 ? 'error' : 'complete'
+    response[:errors] = errors
+    response[:url] = url
+    Rails.cache.write(cachekey, response, expires_in: FeedInfo::CACHE_EXPIRATION)
   end
 end
 

spec/services/feed_info_spec.rb

@@ -5,7 +5,7 @@
   let (:url_404) { 'http://httpbin.org/status/404' }
   let (:url_binary) { 'http://httpbin.org/stream-bytes/1024?seed=0' }
 
-  context '#fetch' do
+  context '.fetch' do
     it 'returns a response' do
       VCR.use_cassette('feed_info_fetch') do
         response = {}
@@ -39,7 +39,7 @@
     end
   end
 
-  context '#download' do
+  context '.download' do
     it 'downloads to temp file' do
       VCR.use_cassette('feed_info_download') do
         data = {}
@@ -81,12 +81,44 @@
     end
   end
 
-  context '#parse' do
+  context '.new' do
+    it 'requires url or path' do
+      expect { FeedInfo.new }.to raise_error(ArgumentError)
+    end
+  end
+
+  context '.open' do
+    it 'downloads if path not provided' do
+      url = 'http://www.caltrain.com/Assets/GTFS/caltrain/GTFS-Caltrain-Devs.zip'
+      VCR.use_cassette('fetch_caltrain') do
+        expect { FeedInfo.new(url: url).open { |f| f } }.not_to raise_error
+      end
+    end
+
+    it 'fails with bad host' do
+      url = 'http://test.example.com/gtfs.zip'
+      VCR.use_cassette('fetch_bad_host') do
+        expect { FeedInfo.new(url: url).open { |f| f } }.to raise_error(SocketError)
+      end
+    end
+
+    it 'raises exception on bad gtfs' do
+      VCR.use_cassette('feed_info_download_binary') do
+        expect { FeedInfo.new(url: url_binary).open { |f| f } }.to raise_error(GTFS::InvalidSourceException)
+      end
+    end
+  end
+
+  context '.parse' do
     let (:url) { 'http://www.caltrain.com/Assets/GTFS/caltrain/GTFS-Caltrain-Devs.zip' }
     let (:path) { Rails.root.join('spec/support/example_gtfs_archives/f-9q9-caltrain.zip') }
+    let (:feed_info) { FeedInfo.new(url: url, path: path) }
 
     it 'parses feed' do
-      feed, operators = FeedInfo.parse_feed_and_operators(url, path)
+      feed, operators = nil, nil
+      feed_info.open do |feed_info|
+        feed, operators = feed_info.parse_feed_and_operators
+      end
       expect(feed.onestop_id).to eq('f-9q9-wwwcaltraincom')
       expect(feed.url).to eq(url)
       expect(feed.geometry).to be_truthy
@@ -96,7 +128,10 @@
     end
 
     it 'parses operators' do
-      feed, operators = FeedInfo.parse_feed_and_operators(url, path)
+      feed, operators = nil, nil
+      feed_info.open do |feed_info|
+        feed, operators = feed_info.parse_feed_and_operators
+      end
       expect(operators.size).to eq(1)
       operator = operators.first
       expect(operator.onestop_id).to eq('o-9q9-caltrain')

spec/workers/feed_info_worker_spec.rb

@@ -14,7 +14,7 @@
     expect(cachedata[:operators].first[:onestop_id]).to eq('o-9q9-caltrain')
   end
 
-  it 'fails with status message' do
+  it 'fails with 404' do
     url = 'http://www.bart.gov/this-is-a-bad-url.zip'
     cachekey = 'test'
     Rails.cache.delete(cachekey)
@@ -23,7 +23,32 @@
     end
     cachedata = Rails.cache.read(cachekey)
     expect(cachedata[:status]).to eq('error')
-    expect(cachedata[:exception]).to eq('Net::HTTPServerException')
-    expect(cachedata[:message]).to eq('404 "Not Found"')
+    expect(cachedata[:errors].first[:exception]).to eq('HTTPServerException')
+    expect(cachedata[:errors].first[:message]).to eq('404 "Not Found"')
+  end
+
+  it 'fails with bad host' do
+    url = 'http://test.example.com/gtfs.zip'
+    cachekey = 'test'
+    Rails.cache.delete(cachekey)
+    VCR.use_cassette('fetch_bad_host') do
+      FeedInfoWorker.new.perform(url, cachekey)
+    end
+    cachedata = Rails.cache.read(cachekey)
+    expect(cachedata[:status]).to eq('error')
+    expect(cachedata[:errors].first[:exception]).to eq('SocketError')
+  end
+
+  it 'fails with invalid gtfs' do
+    url = 'http://httpbin.org/stream-bytes/1024?seed=0'
+    cachekey = 'test'
+    Rails.cache.delete(cachekey)
+    VCR.use_cassette('feed_info_download_binary') do
+      FeedInfoWorker.new.perform(url, cachekey)
+    end
+    cachedata = Rails.cache.read(cachekey)
+    expect(cachedata[:status]).to eq('error')
+    expect(cachedata[:errors].first[:exception]).to eq('InvalidSourceException')
+    expect(cachedata[:errors].first[:message]).to eq('Invalid GTFS Feed')
   end
 end