Move redirect support to Response.

Author: benubois

lib/feedkit.rb

@@ -8,6 +8,7 @@
 require "cgi"
 require "rchardet"
 require "addressable"
+require "ostruct"
 
 require "feedkit/core_ext/try"
 require "feedkit/feedjira_extension"
@@ -16,6 +17,7 @@
 require "feedkit/detect_encoding"
 require "feedkit/errors"
 require "feedkit/basic_auth"
+require "feedkit/redirect"
 require "feedkit/response"
 require "feedkit/request"
 require "feedkit/curl"

lib/feedkit/curl.rb

@@ -28,7 +28,7 @@ def download
       out, _, status = Open3.capture3(command % params)
       if status.success?
         response = CurlResponse.new(@parsed_url.url)
-        Response.new(tempfile: tempfile, response: response, parsed_url: @parsed_url)
+        Response.new(tempfile: tempfile, response: response, parsed_url: @parsed_url, redirects: [])
       else
         raise ServerError.new(HTTP::Response::Status.new(500).to_s, nil)
       end

lib/feedkit/redirect.rb

@@ -0,0 +1,17 @@
+module Feedkit
+  class Redirect
+    PERMANENT_REDIRECTS = [301, 308].to_set.freeze
+
+    attr_reader :status, :from, :to
+
+    def initialize(status:, from:, to:)
+      @status = status
+      @from = from
+      @to = to
+    end
+
+    def permanent?
+      PERMANENT_REDIRECTS.include?(@status)
+    end
+  end
+end

lib/feedkit/request.rb

@@ -23,6 +23,7 @@ def initialize(url, on_redirect: nil, auto_inflate: true, username: nil, passwor
       @user_agent    = user_agent
       @last_modified = last_modified
       @etag          = etag
+      @redirects     = []
     end
 
     def download
@@ -32,7 +33,7 @@ def download
 
       response = request
       if response.status.code == 304
-        Response.new(tempfile: Tempfile.new, response: response, parsed_url: @parsed_url)
+        Response.new(tempfile: Tempfile.new, response: response, parsed_url: @parsed_url, redirects: @redirects)
       else
         download_to_file(response)
       end
@@ -59,7 +60,7 @@ def download_to_file(response)
       tempfile.open # flush written content
       tempfile.rewind
 
-      Response.new(tempfile: tempfile, response: response, parsed_url: @parsed_url)
+      Response.new(tempfile: tempfile, response: response, parsed_url: @parsed_url, redirects: @redirects)
     rescue
       tempfile&.close
       raise
@@ -68,7 +69,7 @@ def download_to_file(response)
     def client
       http = HTTP
        .headers(headers)
-       .follow(max_hops: 4, on_redirect: @on_redirect)
+       .follow(max_hops: 4, on_redirect: on_redirect)
        .timeout(connect: 5, write: 5, read: 30)
        .encoding(Encoding::BINARY)
 
@@ -93,6 +94,12 @@ def basic_auth
       end
     end
 
+    def on_redirect
+      proc do |from, to|
+        @redirects.push Redirect.new(status: from.status.code, from: from.uri.to_s, to: to.uri.to_s)
+      end
+    end
+
     def request
       response = client.get(@parsed_url.url, ssl_context: ssl_context)
       response_error!(response) unless success?(response)

lib/feedkit/response.rb

@@ -4,13 +4,14 @@
 
 module Feedkit
   class Response
-    attr_reader :path
+    attr_reader :path, :redirects
 
-    def initialize(tempfile:, response:, parsed_url:)
+    def initialize(tempfile:, response:, parsed_url:, redirects:)
       @tempfile   = tempfile
       @path       = tempfile.path
       @response   = response
       @parsed_url = parsed_url
+      @redirects  = redirects
     end
 
     def body
@@ -50,7 +51,7 @@ def headers
     end
 
     def url
-      result = @response.uri.to_s
+      result = request_url
       if @parsed_url.username && @parsed_url.password
         parts = result.split("/")
         parts[2] = credentials.to_s + parts[2]
@@ -59,6 +60,14 @@ def url
       result
     end
 
+    def request_url
+      if !@redirects.empty? && @redirects.all?(&:permanent?)
+        @redirects.last.to
+      else
+        @parsed_url.url.to_s
+      end
+    end
+
     def credentials
       username = URI.encode_www_form_component(@parsed_url.username)
       password = URI.encode_www_form_component(@parsed_url.password)

test/feedkit/request_test.rb

@@ -125,12 +125,25 @@ def test_should_follow_redirects
     stub_request(:get, first_url).to_return(response)
     stub_request(:get, last_url)
 
-    on_redirect = proc do |_, to|
-      @location = to.uri.to_s
-    end
+    response = ::Feedkit::Request.download(first_url)
+    assert_equal last_url, response.url
+  end
+
+  def test_should_not_follow_temporary_redirects
+    first_url = "http://www.example.com"
+    last_url = "#{first_url}/final"
+
+    response = {
+      status: 302,
+      headers: {
+        "Location" => last_url
+      }
+    }
+    stub_request(:get, first_url).to_return(response)
+    stub_request(:get, last_url)
 
-    response = ::Feedkit::Request.download(first_url, on_redirect: on_redirect)
-    assert_equal last_url, @location
+    response = ::Feedkit::Request.download(first_url)
+    assert_equal first_url, response.url
   end
 
   def test_should_get_caching_headers