[BREAK BUILD] Upgrade URL rewriting to Routes (there is one test case in routes_test.rb that fails, and it depends on some refactoring that I agreed upon with Ulysses)

app/controllers/admin_controller.rb

@@ -33,7 +33,7 @@ class AdminController < ApplicationController
         @wiki.create_web(@params['name'], @params['address'])
         redirect_show('HomePage', @params['address'])
       else 
-        redirect_to :action => 'index'
+        redirect_to :controller => 'wiki', :action => 'index'
       end
     else
       # no form submitted -> render template
@@ -80,7 +80,7 @@ class AdminController < ApplicationController
     if wiki.authenticate(@params['system_password_orphaned'])
       wiki.remove_orphaned_pages(@web_name)
       flash[:info] = 'Orphaned pages removed'
-      redirect_to :action => 'list'
+      redirect_to :controller => 'wiki', :web => @web_name, :action => 'list'
     else
       flash[:error] = password_error(@params['system_password'])
       return_to_last_remembered

app/controllers/application.rb

@@ -1,12 +1,7 @@
-require 'url_rewriting_hack'
-
 # The filters added to this controller will be run for all controllers in the application.
 # Likewise will all the methods added be available for all controllers.
 class ApplicationController < ActionController::Base
 
-  # implements Instiki's legacy URLs
-  require 'url_rewriting_hack'
-
   before_filter :set_utf8_http_header, :connect_to_model
   after_filter :remember_location
 
@@ -88,7 +83,7 @@ class ApplicationController < ActionController::Base
   def remember_location
     if @response.headers['Status'] == '200 OK'
       unless @@REMEMBER_NOT.include? action_name or @request.method != :get
-        @session[:return_to] = url_for 
+        @session[:return_to] = @request.request_uri
         logger.debug("Session ##{session.object_id}: remembered URL '#{@session[:return_to]}'")
       end
     end

app/views/file/file.rhtml

@@ -1,10 +1,10 @@
 <%
   @title = "Upload #{@file_name}"
-  @hide_navigatio = false
+  @hide_navigation = false
 %>
 
 <p>
-<%= form_tag({}, {:multipart => true}) %>
+<%= form_tag({:controller => 'file', :web => @web_name, :action => 'file'}, {:multipart => true}) %>
   <p>
     File to upload:
     <br/>
@@ -14,9 +14,6 @@
     <input type="submit" value="Update" /> as 
     <input type="text" name="author" id="authorName" value="<%= @author %>" 
         onClick="this.value == 'AnonymousCoward' ? this.value = '' : true" />
-	<% if @page %>
-      | <a href="../file/">Cancel</a> <small>(unlocks page)</small>
-    <% end %>
   </p>
 <%= end_form_tag %>
 </p>

config/environment.rb

@@ -47,6 +47,7 @@ end
 require 'action_controller'
 require 'active_record_stub'
 require 'instiki_errors'
+require 'routes'
 
 unless defined? RAILS_DEFAULT_LOGGER
   RAILS_DEFAULT_LOGGER = Logger.new(STDERR)

config/routes.rb

@@ -0,0 +1,19 @@
+ActionController::Routing.draw do |map|
+  map.connect 'create_system', :controller => 'admin', :action => 'create_system'
+  map.connect 'create_web', :controller => 'admin', :action => 'create_web'
+  map.connect 'edit_web', :controller => 'admin', :action => 'edit_web'
+  map.connect 'remove_orphaned_pages', :controller => 'admin', :action => 'remove_orphaned_pages'
+
+  map.connect ':web/file/:id', :controller => 'file', :action => 'file'
+  map.connect ':web/pic/:id', :controller => 'file', :action => 'pic'
+  map.connect ':web/import/:id', :controller => 'file', :action => 'import'
+
+  map.connect 'login', :controller => 'wiki', :action => 'login'
+  map.connect ':web/login', :controller => 'wiki', :action => 'login'
+  map.connect 'web_list', :controller => 'wiki', :action => 'web_list'
+  map.connect ':web/web_list', :controller => 'wiki', :action => 'web_list'
+  map.connect ':web/:action/:id', :controller => 'wiki'
+  map.connect ':web/:action', :controller => 'wiki'
+  map.connect ':web', :controller => 'wiki', :action => 'index'
+  map.connect '', :controller => 'wiki', :action => 'index'
+end

libraries/url_rewriting_hack.rb

@@ -1,112 +0,0 @@
-# Below are some hacks to Rails internal classes that implement Instiki URLs scheme.
-# It is no doubt a bad practice to override internal implementation of anything.
-# When Rails implements some way to do it in the framework, this code should be replaced 
-# with something more legitimate.
-
-# In Instiki URLs are mapped to the ActionPack actions, possibly performed on a particular 
-# web (sub-wiki) and page within that web.
-#
-# 1. Controller is determined by action name (default is 'wiki')
-# 2. '/name1/' maps to action 'name1', unspecified web
-#    Example: http://localhost/create_system/
-# 3. Special case of above, URI '/wiki/' maps to action 'index', because Rails sets this address 
-#    when default controller name is specified as 'wiki', and an application root 
-#    (http://localhost:2500/)is requested.
-# 4. '/name1/name2/' maps to web 'name1', action 'name2'
-#    Example: http://localhost/mywiki/search/
-# 5. '/name1/name2/name3/' maps to web 'name1', action 'name2', 
-#    Example: http://localhost/mywiki/show/HomePage
-
-
-require 'dispatcher'
-
-# Overrides Rails DispatchServlet.parse_uri 
-class DispatchServlet
-
-  def self.parse_uri(path)
-    result = parse_path(path)
-    if result
-      result[:controller] = ActionMapper.map_to_controller(result[:action])
-      result
-    else
-      false
-    end
-  end
-
-  def self.parse_path(path)
-    ApplicationController.logger.debug "Parsing URI '#{path}'"
-    component = '([-_a-zA-Z0-9]+)'
-    page_name = '(.*)'
-    case path.sub(%r{^/(?:fcgi|mruby|cgi)/}, "/")
-      when '/wiki/'
-        { :web => nil, :controller => 'wiki', :action => 'index' }
-      when %r{^/#{component}/?$}
-        { :web => nil, :controller => 'wiki', :action => $1 }
-      when %r{^/#{component}/#{component}/?$}
-        { :web => $1, :controller => 'wiki', :action => $2 }
-      when %r{^/#{component}/#{component}/(.*)/?$}
-        { :web => $1, :controller => 'wiki', :action => $2, :id => drop_trailing_slash($3) }
-      else
-        false
-    end
-  end
-
-  def self.drop_trailing_slash(line) 
-    if line[-1] == ?/
-      line.chop
-    else
-      line
-    end
-  end
-  
-  class ActionMapper
-
-    @@action_to_controller_map = {
-      'create_system' => 'admin',
-      'create_web' => 'admin',
-      'edit_web' => 'admin',
-      'remove_orphaned_pages' => 'admin',
-      'file' => 'file',
-      'import' => 'file',
-      'pic' => 'file',
-    }
-    
-    def self.map_to_controller(action)
-      @@action_to_controller_map[action] || 'wiki'
-    end
-  
-  end
-
-end
-
-
-require 'action_controller/url_rewriter.rb'
-
-# Overrides parts of AP UrlRewriter to achieve the Instiki's legacy URL scheme
-module ActionController
-  class UrlRewriter
-
-    VALID_OPTIONS << :web unless VALID_OPTIONS.include? :web
-  
-    private  
-    
-    def resolve_aliases(options)
-      options[:controller_prefix] = options[:web] unless options[:web].nil?
-      options
-    end
-  
-    def controller_name(options, controller_prefix)
-      ensure_slash_suffix(options, :controller_prefix)
-  
-      controller_name = case options[:controller_prefix]
-        when String:  options[:controller_prefix]
-        when false : ""
-        when nil   : controller_prefix || ""
-      end
-      # In Instiki we don't need the controller name (there is only one comtroller, anyway)
-      # therefore the below line is commented out
-      # controller_name << (options[:controller] + "/") if options[:controller] 
-      return controller_name
-    end
-  end
-end

test/functional/admin_controller_test.rb

@@ -26,32 +26,32 @@ class AdminControllerTest < Test::Unit::TestCase
 
   def test_create_system_form_submitted
     ApplicationController.wiki = WikiServiceWithNoPersistence.new
-    assert !@controller.wiki.setup?
+    assert !ApplicationController.wiki.setup?
     
     process('create_system', 'password' => 'a_password', 'web_name' => 'My Wiki', 
         'web_address' => 'my_wiki')
       
     assert_redirected_to :web => 'my_wiki', :controller => 'wiki', :action => 'new', 
         :id => 'HomePage'
-    assert @controller.wiki.setup?
+    assert ApplicationController.wiki.setup?
-    assert_equal 'a_password', @controller.wiki.system[:password]
+    assert_equal 'a_password', ApplicationController.wiki.system[:password]
-    assert_equal 1, @controller.wiki.webs.size
+    assert_equal 1, ApplicationController.wiki.webs.size
-    new_web = @controller.wiki.webs['my_wiki']
+    new_web = ApplicationController.wiki.webs['my_wiki']
     assert_equal 'My Wiki', new_web.name
     assert_equal 'my_wiki', new_web.address
   end
 
   def test_create_system_form_submitted_and_wiki_already_initialized
-    wiki_before = @controller.wiki
+    wiki_before = ApplicationController.wiki
-    assert @controller.wiki.setup?
+    assert ApplicationController.wiki.setup?
 
     process 'create_system', 'password' => 'a_password', 'web_name' => 'My Wiki', 
         'web_address' => 'my_wiki'
 
     assert_redirected_to :web => 'wiki1', :action => 'show', :id => 'HomePage'
-    assert_equal wiki_before, @controller.wiki
+    assert_equal wiki_before, ApplicationController.wiki
     # and no new web should be created either
-    assert_equal 1, @controller.wiki.webs.size
+    assert_equal 1, ApplicationController.wiki.webs.size
     assert_flash_has :error
   end
 
@@ -200,19 +200,19 @@ class AdminControllerTest < Test::Unit::TestCase
 
     r = process('remove_orphaned_pages', 'web' => 'wiki1', 'system_password_orphaned' => 'pswd')
 
-    assert_redirected_to :action => 'list'
+    assert_redirected_to :controller => 'wiki', :web => 'wiki1', :action => 'list'
     assert_equal [@home, @oak], @web.select.sort,
         "Pages are not as expected: #{@web.select.sort.map {|p| p.name}.inspect}"
 
 
     # Oak is now orphan, second pass should remove it
-    r = process('remove_orphaned_pages', 'web' => 'wiki1', 'system_password' => 'pswd')
+    r = process('remove_orphaned_pages', 'web' => 'wiki1', 'system_password_orphaned' => 'pswd')
-    assert_redirected_to :action => 'list'
+    assert_redirected_to :controller => 'wiki', :web => 'wiki1', :action => 'list'
     assert_equal [@home], @web.select.sort,
         "Pages are not as expected: #{@web.select.sort.map {|p| p.name}.inspect}"
 
     # third pass does not destroy HomePage
-    r = process('remove_orphaned_pages', 'web' => 'wiki1', 'system_password' => 'pswd')
+    r = process('remove_orphaned_pages', 'web' => 'wiki1', 'system_password_orphaned' => 'pswd')
     assert_redirected_to :action => 'list'
     assert_equal [@home], @web.select.sort,
         "Pages are not as expected: #{@web.select.sort.map {|p| p.name}.inspect}"

test/functional/routes_test.rb

@@ -0,0 +1,53 @@
+#!/bin/env ruby -w
+
+require File.dirname(__FILE__) + '/../test_helper'
+
+require 'action_controller/routing'
+
+class RoutesTest < Test::Unit::TestCase
+
+  def test_parse_uri
+    assert_routing('', :controller => 'wiki', :action => 'index')
+    assert_routing('x', :controller => 'wiki', :action => 'index', :web => 'x')
+    assert_routing('x/y', :controller => 'wiki', :web => 'x', :action => 'y')
+    assert_routing('x/y/z', :controller => 'wiki', :web => 'x', :action => 'y', :id => 'z')
+    assert_recognizes({:web => 'x', :controller => 'wiki', :action => 'y'}, 'x/y/')
+    assert_recognizes({:web => 'x', :controller => 'wiki', :action => 'y', :id => 'z'}, 'x/y/z/')
+  end
+  
+  def test_parse_uri_interestng_cases
+    assert_routing('_veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery-long_web_/an_action/HomePage', 
+      :web => '_veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery-long_web_', 
+      :controller => 'wiki', 
+      :action => 'an_action', :id => 'HomePage'
+    )
+    assert_recognizes({:controller => 'wiki', :action => 'index'}, '///')
+  end
+  
+  def test_parse_uri_liberal_with_pagenames
+
+    assert_routing('web/show/$HOME_PAGE', 
+        :controller => 'wiki', :web => 'web', :action => 'show', :id => '$HOME_PAGE')
+      
+    assert_routing('web/show/HomePage?arg1=value1&arg2=value2', 
+        :controller => 'wiki', :web => 'web', :action => 'show', 
+        :id => 'HomePage?arg1=value1&arg2=value2')
+    
+    assert_routing('web/file/abc.zip', 
+        :web => 'web', :controller => 'file', :action => 'file', :id => 'abc.zip')
+    assert_routing('web/pic/abc.jpg', 
+        :web => 'web', :controller => 'file', :action => 'pic', :id => 'abc.jpg')
+    assert_routing('web/import', :web => 'web', :controller => 'file', :action => 'import')
+    # default option is wiki
+    assert_recognizes({:controller => 'wiki', :web => 'unknown_path', :action => 'index', }, 
+      'unknown_path')
+  end
+
+  def test_cases_broken_by_routes
+    assert_routing('web/show/HomePage/something_else', 
+        :controller => 'wiki', :web => 'web', :action => 'show', :id => 'HomePage/something_else')
+    assert_routing('web/show/Page+With+Spaces', 
+        :controller => 'wiki', :web => 'web', :action => 'show', :id => 'Page+With+Spaces')
+  end
+
+end

test/test_helper.rb

@@ -14,9 +14,9 @@ class Test::Unit::TestCase
 
   def setup_controller_test(controller_class = nil, host = nil)
     if controller_class
-      @controller = controller_class
+      @controller = controller_class.new
     elsif self.class.to_s =~ /^(\w+Controller)Test$/
-      @controller = Object::const_get($1)
+      @controller = Object::const_get($1).new
     else
       raise "Cannot derive the name of controller under test from class name #{self.class}"
     end

test/unit/url_rewriting_hack_test.rb

@@ -1,94 +0,0 @@
-#!/bin/env ruby -w
-
-require File.dirname(__FILE__) + '/../test_helper'
-require 'url_rewriting_hack'
-
-class UrlRewritingHackTest < Test::Unit::TestCase
-  
-  def test_parse_uri
-    assert_equal({:controller => 'wiki', :action => 'x', :web => nil}, 
-        DispatchServlet.parse_uri('/x/'))
-    assert_equal({:web => 'x', :controller => 'wiki', :action => 'y'}, 
-        DispatchServlet.parse_uri('/x/y'))
-    assert_equal({:web => 'x', :controller => 'wiki', :action => 'y'}, 
-        DispatchServlet.parse_uri('/x/y/'))
-    assert_equal({:web => 'x', :controller => 'wiki', :action => 'y', :id => 'z'}, 
-        DispatchServlet.parse_uri('/x/y/z'))
-    assert_equal({:web => 'x', :controller => 'wiki', :action => 'y', :id => 'z'}, 
-        DispatchServlet.parse_uri('/x/y/z/'))
-  end
-  
-  def test_parse_uri_approot
-    assert_equal({:controller => 'wiki', :action => 'index', :web => nil}, 
-        DispatchServlet.parse_uri('/wiki/'))
-  end
-
-  def test_parse_uri_interestng_cases
-
-    assert_equal({:web => '_veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery-long_web_', 
-          :controller => 'wiki', 
-          :action => 'an_action', :id => 'HomePage'
-        }, 
-        DispatchServlet.parse_uri(
-            '/_veeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeery-long_web_/an_action/HomePage')
-    )
-
-    assert_equal false, DispatchServlet.parse_uri('')
-    assert_equal false, DispatchServlet.parse_uri('//')
-    assert_equal false, DispatchServlet.parse_uri('web')
-  end
-  
-  def test_parse_uri_liberal_with_pagenames
-
-    assert_equal({:controller => 'wiki', :web => 'web', :action => 'show', :id => '$HOME_PAGE'}, 
-      DispatchServlet.parse_uri('/web/show/$HOME_PAGE'))
-      
-    assert_equal({:controller => 'wiki', :web => 'web', :action => 'show', 
-        :id => 'HomePage/something_else'}, 
-        DispatchServlet.parse_uri('/web/show/HomePage/something_else'))
-    
-    assert_equal({:controller => 'wiki', :web => 'web', :action => 'show', 
-        :id => 'HomePage?arg1=value1&arg2=value2'}, 
-        DispatchServlet.parse_uri('/web/show/HomePage?arg1=value1&arg2=value2'))
-    
-    assert_equal({:controller => 'wiki', :web => 'web', :action => 'show', 
-        :id => 'Page+With+Spaces'}, 
-        DispatchServlet.parse_uri('/web/show/Page+With+Spaces'))
-  end
-
-  def test_url_rewriting
-    request = ActionController::TestRequest.new
-    ur = ActionController::UrlRewriter.new(request, 'wiki', 'show')
-
-    assert_equal 'http://test.host/myweb/myaction',
-        ur.rewrite(:web => 'myweb', :controller => 'wiki', :action => 'myaction')
-
-    assert_equal 'http://test.host/myOtherWeb/',
-        ur.rewrite(:web => 'myOtherWeb', :controller => 'wiki')
-
-    assert_equal 'http://test.host/myaction',
-        ur.rewrite(:controller => 'wiki', :action => 'myaction')
-
-    assert_equal 'http://test.host/',
-        ur.rewrite(:controller => 'wiki')
-  end
-  
-  def test_controller_mapping
-    request = ActionController::TestRequest.new
-    ur = ActionController::UrlRewriter.new(request, 'wiki', 'show')
-
-    assert_equal 'http://test.host/file',
-        ur.rewrite(:controller => 'file', :action => 'file')
-    assert_equal 'http://test.host/pic/abc.jpg',
-        ur.rewrite(:controller => 'file', :action => 'pic', :id => 'abc.jpg')
-    assert_equal 'http://test.host/web/pic/abc.jpg',
-        ur.rewrite(:web => 'web', :controller => 'file', :action => 'pic', :id => 'abc.jpg')
-    assert_equal 'http://test.host/web/import',
-        ur.rewrite(:web => 'web', :controller => 'file', :action => 'import')
-
-    # default option is wiki
-    assert_equal 'http://test.host/unknown_action',
-        ur.rewrite(:controller => 'wiki', :action => 'unknown_action')
-  end
-
-end