Merge pull request #13 from tjdett/trailing-slashes

Fixing incorrect redirection handling

Author: cbeer

lib/oai/client.rb

@@ -82,16 +82,17 @@ def initialize(base_url, options={})
       @debug = options.fetch(:debug, false)
       @parser = options.fetch(:parser, 'rexml')
 
-      @follow_redirects = options.fetch(:redirects, true)
-      @http_client = options.fetch(:http, Faraday.new(@base))
-
-      if !options.key?(:http) and @follow_redirects
-
-        count = @folow_redirects if @folow_redirects.is_a? Fixnum
-        count ||= 5
-
-        require 'faraday_middleware'
-        @http_client.use FaradayMiddleware::FollowRedirects, :limit => count
+      @http_client = options.fetch(:http) do
+        Faraday.new(:url => @base) do |builder|
+          follow_redirects = options.fetch(:redirects, true)
+          if follow_redirects
+            count = follow_redirects.is_a?(Fixnum) ? follow_redirects : 5
+
+            require 'faraday_middleware'
+            builder.response :follow_redirects, :limit => count
+          end
+          builder.adapter :net_http
+        end
       end
 
       # load appropriate parser

test/client/helpers/provider.rb

@@ -8,61 +8,58 @@ class ComplexProvider < OAI::Provider::Base
   source_model ComplexModel.new(100)
 end
 
-class ProviderServer < WEBrick::HTTPServlet::AbstractServlet
-  @@server = nil
-  
-  def initialize(server)
-    super(server)
+class ProviderServer
+
+  attr_reader :consumed, :server
+
+  def initialize(port, mount_point)
+    @consumed = []
     @provider = ComplexProvider.new
+    @server = WEBrick::HTTPServer.new(
+      :BindAddress => '127.0.0.1',
+      :Logger => WEBrick::Log.new('/dev/null'),
+      :AccessLog => [],
+      :Port => port)
+    @server.mount_proc(mount_point, server_proc)
   end
-  
-  def do_GET(req, res)
-    begin
-      res.body = @provider.process_request(req.query)
-      res.status = 200
-      res['Content-Type'] = 'text/xml'
-    rescue => err
-      puts err
-      puts err.backtrace.join("\n")
-      res.body = err.backtrace.join("\n")
-      res.status = 500
-    end
+
+  def port
+    @server.config[:Port]
   end
-  
-  def self.start(port)
-    unless @@server
-      @@server = WEBrick::HTTPServer.new(
-        :BindAddress => '127.0.0.1', 
-        :Logger => WEBrick::Log.new('/dev/null'),
-        :AccessLog => [],
-        :Port => port)
-      @@server.mount("/oai", ProviderServer)
 
-      trap("INT") { @@server.shutdown }
-      @@thread = Thread.new { @@server.start }
-      puts "Starting Webrick/Provider on port[#{port}]"
-    end
+  def start
+    @thread = Thread.new { @server.start }
   end
-  
-  def self.stop
-    puts "Stopping Webrick/Provider"
-    if @@thread
-      @@thread.exit
-    end
+
+  def stop
+    @thread.exit if @thread
   end
-  
-  def self.wrap(port = 3333)
+
+  def self.wrap(port = 3333, mount_point='/oai')
+    server = self.new(port, mount_point)
     begin
-      start(port)
+      server.start
+      yield(server)
+    ensure
+      server.stop
+    end
+  end
 
-      # Wait for startup
-      sleep 2
-    
-      yield
+  protected
 
-    ensure
-      stop
+  def server_proc
+    Proc.new do |req, res|
+      begin
+        res.body = @provider.process_request(req.query)
+        res.status = 200
+        res['Content-Type'] = 'text/xml'
+      rescue => err
+        puts err
+        puts err.backtrace.join("\n")
+        res.body = err.backtrace.join("\n")
+        res.status = 500
+      end
     end
   end
-  
-end
+
+end

test/client/helpers/test_wrapper.rb

@@ -1,18 +1,6 @@
-module Test::Unit
-  class AutoRunner
-    alias_method :real_run, :run
-    
-    def run
-      ProviderServer.wrap { real_run }
-    end
-
-  end
-  
-end
-
-unless $provider_server_already_started
-  $provider_server_already_started = true
-  ProviderServer.start(3333)
-  sleep 2
+unless $provider_server
+  $provider_server = ProviderServer.new(3333, '/oai')
+  $provider_server.start
+  sleep 0.2
 end
 

test/client/tc_http_client.rb

@@ -1,12 +1,13 @@
 require 'test_helper'
+require 'webrick'
 
 class HttpClientTest < Test::Unit::TestCase
 
   def test_pluggable_http_client
     oai_response = <<-eos
     <Identify>
       <repositoryName>Mock OAI Provider</repositoryName>
-      <baseURL>http://nowhere.example.com</baseURL> 
+      <baseURL>http://nowhere.example.com</baseURL>
     </Identify>
 eos
 
@@ -20,7 +21,94 @@ def test_pluggable_http_client
 
     assert_kind_of OAI::IdentifyResponse, response
     assert_equal 'Mock OAI Provider [http://nowhere.example.com]', response.to_s
-    
+
+  end
+
+  def test_http_client_handles_trailing_slash_redirects
+    # First, test that this works when mocking out Faraday client
+    oai_response = <<-eos
+      <Identify>
+        <repositoryName>Mock OAI Provider</repositoryName>
+        <baseURL>http://nowhere.example.com</baseURL>
+      </Identify>
+    eos
+
+    stubs = TrailingSlashAwareStubs.new do |stub|
+      stub.get('/oai/?verb=Identify') { [200, {}, oai_response] }
+      stub.get('/oai?verb=Identify') {
+        [301, {
+          'Location' => 'http://localhost:3334/oai/?verb=Identify'
+        }, '']
+      }
+    end
+
+    faraday_stub = Faraday.new do |builder|
+      require 'faraday_middleware'
+      builder.use FaradayMiddleware::FollowRedirects
+      builder.adapter :test, stubs
+    end
+
+    client = OAI::Client.new 'http://localhost:3334/oai', :http => faraday_stub
+    response = client.identify
+
+    assert_kind_of OAI::IdentifyResponse, response
+    assert_equal 'Mock OAI Provider [http://nowhere.example.com]', response.to_s
+    assert_equal 2, stubs.consumed[:get].length
+    assert_equal stubs.consumed[:get].first.path, '/oai'
+    assert_equal stubs.consumed[:get].last.path, '/oai/'
+
+    # Now try it with a real server and default Faraday client
+    TrailingSlashProviderServer.wrap(3334) do |server|
+      client = OAI::Client.new "http://localhost:#{server.port}/oai"
+      response = client.identify
+
+      assert_kind_of OAI::IdentifyResponse, response
+      assert_equal 'Complex Provider [http://localhost]', response.to_s
+      assert_equal 2, server.consumed.length
+      assert_equal server.consumed.first.path, '/oai'
+      assert_equal server.consumed.last.path, '/oai/'
+    end
+  end
+
+  private
+
+  class TrailingSlashProviderServer < ProviderServer
+    def server_proc
+      Proc.new do |req, res|
+        @consumed << req
+        case req.path
+        when "/oai/"
+          begin
+            res.body = @provider.process_request(req.query)
+            res.status = 200
+            res['Content-Type'] = 'text/xml'
+          rescue => err
+            puts err
+            puts err.backtrace.join("\n")
+            res.body = err.backtrace.join("\n")
+            res.status = 500
+          end
+        else
+          res.body = ''
+          res.status = 301
+          res['Location'] = "http://localhost:#{port}/oai/?#{req.query_string}"
+        end
+        res
+      end
+    end
+  end
+
+  class TrailingSlashAwareStubs < Faraday::Adapter::Test::Stubs
+    attr_reader :consumed
+
+    # ensure leading, but not trailing slash
+    def normalize_path(path)
+      path = '/' + path if path.index('/') != 0
+      #path = path.sub('?', '/?')
+      #path = path + '/' unless $&
+      path.gsub('//', '/')
+    end
+
   end
 end