deltacloud-dev mailing list archives

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

Just more clarification why this is needed:

If you try to run Deltacloud in OpenShift, our current
config.ru will use 'mount' in Passenger. Without Sinatra
'url()' helper all links are broken (they does not have /api
prefix). This patch should fix that as well.

  -- Michal


> 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
> 

Michal Fojtik
http://deltacloud.org
mfojtik@redhat.com




Mime
View raw message