You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltacloud.apache.org by Michal Fojtik <mf...@redhat.com> on 2012/08/31 14:52:26 UTC
Re: [PATCH core 1/2] Core: Improved url_for helpers
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 <mf...@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 <mf...@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
Re: [PATCH core 1/2] Core: Improved url_for helpers
Posted by David Lutterkort <lu...@redhat.com>.
On Fri, 2012-08-31 at 14:52 +0200, Michal Fojtik wrote:
> 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.
While you're doing this: I just spent a major amount of time tracking
down why in the CIMI FE (before applying your patches), when I get the
details for a machine[1], I get the following:
<operation href="http://localhost:3001/cimi/restart_machines/inst0" rel="http://www.dmtf.org/cimi/action/restart"/>
<operation href="http://localhost:3001/cimi/machines/inst0/stop" rel="http://www.dmtf.org/cimi/action/stop"/>
IOW, the restart operation has a bogus href. I finally tracked that down
to Sinatra::UrlForHelper and the special treatment it gives known
actions. Lessons learned:
* method_missing considered dangerous, since it makes it
impossible to find where something is defined; you have the
head-scratching situation that
context.respond_to?(:restart_machine_url) is false, but
context.restart_machine_url(42) actually does something (in
lib/cimi/models/machine.rb(convert_instance_actions)
* doing the special handling of actions UrlForHelper breaks the
separation between Rabbit and the app
This is a longwinded way of saying: could we have Rabbit generate these
helpers when actions/operations are defined rather than try and guess
the right thing in UrlForHelper ? Ideally, with a check since there's an
ambiguity: if the helper would be called a_x_y_url, it could either be
for the a action in collection x_y, or for the a_x action in the y
collection - just failing loudly when we'd try and define the a_x_y_url
helper a second time would be good enough.
David
[1] http://localhost:3001/cimi/machines/inst0 running deltacloudd -i mock -f cimi