deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH core 1/4] Core: Make MatrixParams not overide PATH_INFO
Date Thu, 06 Sep 2012 22:51:11 GMT
On Thu, 2012-09-06 at 14:20 +0200, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>
> 
> * Overiding PATH_INFO with REQUEST_URI cause incorrect
>   incorrect mapping of application.
> 
> Signed-off-by: Michal fojtik <mfojtik@redhat.com>

ACK, some minor comments:

> ---
>  server/lib/deltacloud/core_ext/string.rb |    4 ++++
>  server/lib/sinatra/rack_matrix_params.rb |   20 +++++++++++++-------
>  2 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/server/lib/deltacloud/core_ext/string.rb b/server/lib/deltacloud/core_ext/string.rb
> index bc382ed..483fe33 100644
> --- a/server/lib/deltacloud/core_ext/string.rb
> +++ b/server/lib/deltacloud/core_ext/string.rb
> @@ -73,6 +73,10 @@ class String
>      "#{self[0..(length/2)]}#{end_string}"
>    end
>  
> +  def remove_matrix_params
> +    self.gsub(/;([^\/]*)/, '').gsub(/\?(.*)$/, '')
> +  end

Why do we also remove query params ? Also, this method is so specialised
that it might be better to leave it as an instance method in the
MatrixParams middleware.

> diff --git a/server/lib/sinatra/rack_matrix_params.rb b/server/lib/sinatra/rack_matrix_params.rb
> index e999f89..8a489d3 100644
> --- a/server/lib/sinatra/rack_matrix_params.rb
> +++ b/server/lib/sinatra/rack_matrix_params.rb
> @@ -36,15 +36,21 @@ module Rack
>      #
>      # All HTTP methods are supported, in case of POST they will be passed as a
>      # regular <form> parameters.
> -
>      def call(env)
> -      # Copy PATH_INFO to REQUEST_URI if Rack::Test
> -      env['REQUEST_URI'] = env['PATH_INFO'] if env['rack.test']
> -      env['REQUEST_PATH'] = env['PATH_INFO'] if env['rack.test']
> +
> +      # This ugly hack should fix the issue with Rack::Test where
> +      # these two variables are empty and Rack::Test will always
> +      # return 404.
> +      #
> +      if env['rack.test']
> +        env['REQUEST_URI'] = env['PATH_INFO']
> +        env['REQUEST_PATH'] = env['PATH_INFO']
> +      end

Strictly speaking, these two should be File::join(env['SCRIPT_NAME'],
env['PATH_INFO']) .. shouldn't be a big deal in the test environment.

>        # Split URI to components and then extract ;var=value pairs
> -      uri_components = env['REQUEST_URI'].split('/')
>        matrix_params = {}
> +      uri_components = (env['rack.test'] ? env['PATH_INFO'] : env['REQUEST_URI']).split('/')

There's no need for the conditional here: we set env['REQUEST_URI'] =
env['PATH_INFO'] above if we are running tests.

David



Mime
View raw message