deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mfoj...@redhat.com
Subject [PATCH core 1/2] Core: Improved url_for helpers
Date Fri, 31 Aug 2012 12:48:04 GMT
From: Michal Fojtik <mfojtik@redhat.com>

* Updated url_for helper code to latest version from github[1]
* Improved handling of matrix parameters
* Added 'url()' Sinatra helper to produce valid links when Deltacloud
  is 'mounted' in another Rack application.

Signed-off-by: Michal fojtik <mfojtik@redhat.com>
---
 server/Rakefile                                    |    1 +
 server/lib/cimi/helpers.rb                         |    2 +-
 server/lib/deltacloud/helpers.rb                   |    2 +-
 server/lib/deltacloud/helpers/url_helper.rb        |  183 ++++++++++++--------
 .../tests/helpers/sinatra/url_for_helper_test.rb   |   69 ++++++++
 5 files changed, 183 insertions(+), 74 deletions(-)
 create mode 100644 server/tests/helpers/sinatra/url_for_helper_test.rb

diff --git a/server/Rakefile b/server/Rakefile
index 80d45d3..c7cad4b 100644
--- a/server/Rakefile
+++ b/server/Rakefile
@@ -165,6 +165,7 @@ namespace :test do
     t.test_files = FileList[
       'tests/helpers/core_ext/*test.rb',        # Deltacloud extensions (core_ext) and other
helpers
       'tests/helpers/rack/*test.rb',            # Rack extensions Deltacloud use
+      'tests/helpers/sinatra/*test.rb',         # Sinatra helpers and extensions Deltacloud
use
       'tests/drivers/base/*test.rb',            # Deltacloud drivers API tests
       'tests/drivers/models/*test.rb',          # Deltacloud models tests
       'tests/deltacloud/*test.rb',              # Deltacloud internal API tests
diff --git a/server/lib/cimi/helpers.rb b/server/lib/cimi/helpers.rb
index 2dba3be..93910e4 100644
--- a/server/lib/cimi/helpers.rb
+++ b/server/lib/cimi/helpers.rb
@@ -50,12 +50,12 @@ module CIMI::Collections
 
     helpers Deltacloud::Helpers::Drivers
     helpers Sinatra::AuthHelper
-    helpers Sinatra::UrlForHelper
     helpers Rack::RespondTo::Helpers
     helpers Deltacloud::Helpers::Application
     helpers CIMIHelper
 
     register Rack::RespondTo
+    register Sinatra::UrlFor
 
     enable :xhtml
     enable :dump_errors
diff --git a/server/lib/deltacloud/helpers.rb b/server/lib/deltacloud/helpers.rb
index 6566c59..9ce14bf 100644
--- a/server/lib/deltacloud/helpers.rb
+++ b/server/lib/deltacloud/helpers.rb
@@ -29,11 +29,11 @@ module Deltacloud::Collections
 
     helpers Deltacloud::Helpers::Drivers
     helpers Sinatra::AuthHelper
-    helpers Sinatra::UrlForHelper
     helpers Rack::RespondTo::Helpers
     helpers Deltacloud::Helpers::Application
 
     register Rack::RespondTo
+    register Sinatra::UrlFor
 
     enable :xhtml
     enable :dump_errors
diff --git a/server/lib/deltacloud/helpers/url_helper.rb b/server/lib/deltacloud/helpers/url_helper.rb
index 1b084d4..ff5c910 100644
--- a/server/lib/deltacloud/helpers/url_helper.rb
+++ b/server/lib/deltacloud/helpers/url_helper.rb
@@ -25,91 +25,130 @@
 # USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 module Sinatra
-  module UrlForHelper
+  module UrlFor
 
     require 'uri'
 
-    def method_missing(name, *args)
-      if name.to_s =~ /^([\w\_]+)_url$/
-        if args.size > 0
-          t = $1
-          if t.match(/^(stop|reboot|start|attach|detach)_/)
-            action = $1
-            api_url_for(t.pluralize.split('_').last + '/' + args.first.to_s + '/' + action,
:full)
-          elsif t.match(/^(destroy|update)_/)
-            api_url_for(t.pluralize.split('_').last + '/' + args.first.to_s, :full)
-          else
-            api_url_for(t.pluralize, :full) + '/' + "#{args.first}"
+    # FIXME:
+    # This is the list of 'allowable' actions. Everything else is considered
+    # as a collection by the UrlFor:
+    #
+    ACTIONS = [
+      :start, :stop, :new, :run, :reboot, :attach, :detach, :register, :unregister
+    ]
+
+    def self.valid_action?(action)
+      ACTIONS.include?(action.to_sym)
+    end
+
+    def self.registered(app)
+      app.helpers UrlFor::Helper
+      app.class_eval do
+        def method_missing(name, *args)
+          action_list = Sinatra::UrlFor::ACTIONS.join('|')
+          return super unless name =~ %r[^((#{action_list})_)?([\w_]+)_url$]
+          action, resource, options = $2, $3, (args.first || {})
+
+          options = { :id => options } unless options.is_a? Hash
+
+          # instances_url => /instances
+          # instances_url( :format => :xml) => /instances?format=xml
+          #
+          if (action.nil? or (action && action=='create')) and options.empty?
+            return url_for('/%s' % resource, options.reject { |k, v| k == :id })
           end
-        else
-          api_url_for($1, :full)
+
+          # instance_url(:id => 'i-111') => /instances/i-111
+          # instance_url(:id => 'i-123', :format => 'xml') => /instances/i-123?format=xml
+          if action.nil? and options.has_key?(:id)
+            raise TypeError.new("The :id parameter cannot be nil (/#{resource}/nil)") if
options[:id].nil?
+            return url_for('/' + [resource, options[:id]].compact.join('/'),
+                    options.reject { |k, v| k == :id })
+          end
+
+          # stop_instance_url(:id => 'i-1234') => /instances/i-123/stop
+          #
+          # NOTE: The resource must be in singular!
+          #
+          if action and options.has_key?(:id)
+            action = nil if ['create', 'destroy'].include? action
+            return url_for('/' + [resource.pluralize, options[:id], action].compact.join('/'),
+                    options.reject { |k, v| k == :id })
+          end
+
+          # create_image_url => /images
+          #
+          if action and action == :create
+            return url_for('/' + resource.pluralize, options.reject { |k,v| l == :id })
+          end
+
+          super
         end
-      else
-        super
       end
     end
 
-    def api_url_for(url_fragment, mode=:path_only)
-      matrix_params = ''
-      if request.params['api']
-        matrix_params += ";provider=%s" % request.params['api']['provider'] if request.params['api']['provider']
-        matrix_params += ";driver=%s" % request.params['api']['driver'] if request.params['api']['driver']
-      end
-      url_fragment = "/#{url_fragment}" unless url_fragment =~ /^\// # There is no need to
prefix URI with '/'
-      if mode == :path_only
-        url_for "#{settings.root_url}#{matrix_params}#{url_fragment}", mode
-      else
-        url_for "#{matrix_params}#{url_fragment}", :full
-      end
-    end
+    module Helper
 
-    # Construct a link to +url_fragment+, which should be given relative to
-    # the base of this Sinatra app.  The mode should be either
-    # <code>:path_only</code>, which will generate an absolute path within
-    # the current domain (the default), or <code>:full</code>, which will
-    # include the site name and port number.  (The latter is typically
-    # necessary for links in RSS feeds.)  Example usage:
-    #
-    #   url_for "/"            # Returns "/myapp/"
-    #   url_for "/foo"         # Returns "/myapp/foo"
-    #   url_for "/foo", :full  # Returns "http://example.com/myapp/foo"
-    #--
-    # See README.rdoc for a list of some of the people who helped me clean
-    # up earlier versions of this code.
-    def url_for url_fragment, mode=:path_only
-      case mode
-      when :path_only
-        base = request.script_name.empty? ? Deltacloud.default_frontend.root_url : request.script_name
-      when :full
-        scheme = request.scheme
-        port = request.port
-        request_host = request.host
-        if request.env['HTTP_X_FORWARDED_FOR']
-          scheme = request.env['HTTP_X_FORWARDED_SCHEME'] || scheme
-          port = request.env['HTTP_X_FORWARDED_PORT']
-          request_host = request.env['HTTP_X_FORWARDED_HOST']
+      # Code originaly copied from https://github.com/emk/sinatra-url-for
+      # Copyright 2009 Eric Kidd
+      def url_for(url_fragment, mode=nil, options = nil)
+
+        if mode.is_a? Hash
+          options = mode
+          mode = nil
+        end
+
+        mode ||= :full
+
+        mode = mode.to_sym unless mode.is_a? Symbol
+        optstring = nil
+
+        # Deal with matrix parameters
+        #
+        if request.params['api'] and request.params['api'].is_a? Hash
+          matrix_params = ''
+          if request.params['api']['driver']
+            matrix_params += ';driver=%s' % request.params['api']['driver']
+          end
+          if request.params['api']['provider']
+            matrix_params += ';provider=%s' % request.params['api']['provider']
+          end
+        end
+
+        if options.is_a? Hash
+          optstring = '?' + options.map do |k,v|
+            next if k.nil?
+            "#{k}=#{URI.escape(v.to_s, /[^#{URI::PATTERN::UNRESERVED}]/)}"
+          end.compact.join('&') if !options.empty?
         end
-        if (port.nil? || port == "" ||
-            (scheme == 'http' && port.to_s == '80') ||
-            (scheme == 'https' && port.to_s == '443'))
-          port = ""
+
+        case mode
+        when :path_only
+          base = url request.script_name
+        when :full
+          scheme = request.scheme
+          if (scheme == 'http' && request.port == 80 ||
+              scheme == 'https' && request.port == 443)
+            port = ""
+          else
+            port = ":#{request.port}"
+          end
+          if matrix_params
+            uri_components = url_fragment.split('/')
+            if uri_components.size>1
+              uri_components[1] = uri_components[1] + matrix_params
+              url_fragment = uri_components.join('/')
+            end
+          end
+          base = url(request.script_name).gsub(/\/$/, '')
         else
-          port = ":#{port}"
+          raise TypeError, "Unknown url_for mode #{mode.inspect}"
         end
-        base = "#{scheme}://#{request_host}#{port}#{request.script_name.empty? ? settings.config.root_url
: request.script_name}"
-      else
-        raise TypeError, "Unknown url_for mode #{mode}"
-      end
-      uri_parser = URI.const_defined?(:Parser) ? URI::Parser.new : URI
-      url_escape = uri_parser.escape(url_fragment)
-      # Don't add the base fragment if url_for gets called more than once
-      # per url or the url_fragment passed in is an absolute url
-      if url_escape.match(/^#{base}/) or url_escape.match(/^http/)
-        url_escape
-      else
-        "#{base}#{url_escape}"
+        "#{base}#{url_fragment}#{optstring}"
       end
+
+      alias_method :api_url_for, :url_for
     end
-  end
 
+  end
 end
diff --git a/server/tests/helpers/sinatra/url_for_helper_test.rb b/server/tests/helpers/sinatra/url_for_helper_test.rb
new file mode 100644
index 0000000..159f657
--- /dev/null
+++ b/server/tests/helpers/sinatra/url_for_helper_test.rb
@@ -0,0 +1,69 @@
+require 'rubygems'
+require 'require_relative' if RUBY_VERSION < '1.9'
+
+require_relative '../../test_helper.rb'
+require_relative '../rack/common.rb'
+require_relative '../../../lib/deltacloud/helpers/url_helper.rb'
+
+class TestURLApp < Sinatra::Base
+  register Sinatra::UrlFor
+  use Rack::MatrixParams
+
+  get '/api/:id' do
+    case params[:id]
+      when '1' then tests_url
+      when '2' then tests_url(:id => '1')
+      when '3' then stop_test_url(:id => '1')
+      when '4' then tests_url(:id => '1', :format => :xml)
+      when '5' then tests_url(:id => nil)
+    end
+  end
+end
+
+describe Sinatra::UrlFor do
+
+  before do
+    def app; TestURLApp; end
+  end
+
+  it 'should return /tests for for test_url' do
+    must_return_for '1', 'http://example.org/tests'
+  end
+
+  it 'should return /tests/1 for test_url(:id => 1)' do
+    must_return_for '2', 'http://example.org/tests/1'
+  end
+
+  it 'should return /tests/1/stop for stop_test_url(:id => 1)' do
+    must_return_for '3', 'http://example.org/tests/1/stop'
+  end
+
+  it 'should return /tests/1?format=xml for stop_test_url(:id => 1, :format => :xml)'
do
+    must_return_for '4', 'http://example.org/tests/1?format=xml'
+  end
+
+  it 'should preserve the matrix parameters in URL' do
+    get '/api;driver=mock/1'
+    response_body.must_equal 'http://example.org/tests;driver=mock'
+    get '/api;driver=mock/2'
+    response_body.must_equal 'http://example.org/tests;driver=mock/1'
+    get '/api;driver=mock/3'
+    response_body.must_equal 'http://example.org/tests;driver=mock/1/stop'
+    get '/api;driver=mock/4'
+    response_body.must_equal 'http://example.org/tests;driver=mock/1?format=xml'
+  end
+
+  it 'should raise exception when :id is nil for test_url(:id => nil)' do
+    lambda { get '/api/5' }.must_raise TypeError
+  end
+
+  private
+
+  def must_return_for(url, value)
+    get '/api/%s' % url
+    status.must_equal 200
+    response_body.wont_be_empty
+    response_body.must_equal value
+  end
+
+end
-- 
1.7.10.2


Mime
View raw message